Skip to content

Fix browser specific manifest generation #12533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

MakMuftic
Copy link

Fixes: #12532

Explanation:
I tracked a bug inside Metamasks code that creates browser-specific manifests. The idea here is to have _base.json manifest that holds manifest properties that are needed on all browsers and then also have browser-specific manifests such as chrome.json. Then, when manifest for specific browser distribution is created, these two manifests are merged into one. The problem occurs when you have array property such as permissions in both _base.json and chrome.json (browser-specific manifest). Below you can find examples of how this merge worked before, and how it works now.

OLD BEHAVIOUR

  /* _base.json */
  "permissions": [
    "storage",
    "unlimitedStorage",
    "clipboardWrite",
    "http://localhost:8545/",
    "https://*.infura.io/",
    "activeTab",
    "webRequest",
    "*://*.eth/",
    "notifications"
  ]
  
  /* chrome.json */
  "permissions": [
    "chrome-extension://*/*"
  ]
  
  /* results in merged permissions as shown below */
  /* problem is that new permissions from browser specific manifest ovveride base permission /*
  "permissions": [
    "chrome-extension://*/*",  /* notice that storage permission is missing */
    "unlimitedStorage",
    "clipboardWrite",
    "http://localhost:8545/",
    "https://*.infura.io/",
    "activeTab",
    "webRequest",
    "*://*.eth/",
    "notifications"
  ]
  

NEW BEHAVIOUR

  /* _base.json */
  "permissions": [
    "storage",
    "unlimitedStorage",
    "clipboardWrite",
    "http://localhost:8545/",
    "https://*.infura.io/",
    "activeTab",
    "webRequest",
    "*://*.eth/",
    "notifications"
  ]
  
  /* chrome.json */
  "permissions": [
    "chrome-extension://*/*"
  ]
  
  /* results in merged permissions as shown below */
  "permissions": [ 
    "storage"
    "unlimitedStorage",
    "clipboardWrite",
    "http://localhost:8545/",
    "https://*.infura.io/",
    "activeTab",
    "webRequest",
    "*://*.eth/",
    "notifications",
    "chrome-extension://*/*"
  ]
  

I have read the CLA Document and I hereby sign the CLA

@MakMuftic MakMuftic requested review from kumavis and a team as code owners October 29, 2021 13:59
@MakMuftic MakMuftic requested a review from brad-decker October 29, 2021 13:59
@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

jj-crypto
jj-crypto previously approved these changes Oct 30, 2021
@MakMuftic
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great catch!

@Gudahtt
Copy link
Member

Gudahtt commented Nov 24, 2021

Hey @MakMuftic, could you update this branch to resolve conflicts? Thanks!

@darkwing darkwing removed the request for review from brad-decker December 7, 2021 18:31
@kumavis
Copy link
Member

kumavis commented Dec 7, 2021

couldnt modify this pr so i resolved the conflict here #13007

@darkwing
Copy link
Contributor

darkwing commented Dec 7, 2021

Superceded by #13007

@darkwing darkwing closed this Dec 7, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix browser-specific manifest generation
5 participants