Skip to content
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

Match Chromium behavior for extension loading #8392

Merged
merged 3 commits into from
Apr 7, 2021

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Mar 30, 2021

Fixes brave/brave-browser#15024

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Verify you CAN NOT install on macOS or Windows

  1. New profile (macOS or Windows); launch Brave
  2. Download chromium-cleaner
  3. Visit brave://extensions
  4. Drag the chromium-cleaner.crx extension from desktop onto the chrome://extensions page
  5. Verify extension shows in brave://extensions as disabled (shows warning; you can't enable). Should look like this:
    image
  6. Enable Developer Mode in top right of extensions page
  7. Try steps 4 + 5 again; should fail again

Verify you CAN install on Linux

  1. New profile (Linux); launch Brave
    1. Download chromium-cleaner
  2. Visit brave://extensions
  3. Drag the chromium-cleaner.crx extension from desktop onto the chrome://extensions page
  4. Verify extension shows in brave://extensions as disabled (shows warning; you can't enable). Should look like this:
    image
  5. Enable Developer Mode in top right of extensions page
  6. Try steps 4 + 5 again; this time it should work

@bsclifton bsclifton requested a review from a team as a code owner March 30, 2021 20:12
@bsclifton bsclifton self-assigned this Mar 30, 2021
@bsclifton
Copy link
Member Author

Whoops - for Android, I need to fix GN dependency for test/BUILD.gn. Android build is trying to add the dep which doesn't work, resulting in:

13:44:00  ERROR at //chrome/browser/extensions/BUILD.gn:17:1: Assertion failed.
13:44:00  assert(enable_extensions)
13:44:00  ^-----
13:44:00  See //testing/test.gni:155:25: which caused the file to be included.
13:44:00                deps += [ _dep ]
13:44:00                          ^---

@bsclifton
Copy link
Member Author

bsclifton commented Apr 2, 2021

@brave/patch-reviewers @fmarier can I get a review on this please? 😄 Test plan added!

test/BUILD.gn Outdated Show resolved Hide resolved
@bsclifton bsclifton force-pushed the bsc-extension-verify-parity branch from 89fa5f5 to f8e79a3 Compare April 6, 2021 19:54
@bsclifton bsclifton force-pushed the bsc-extension-verify-parity branch from f8e79a3 to b6080fc Compare April 6, 2021 21:46
@bsclifton
Copy link
Member Author

Ready for re-review, @mkarolin 😄

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

chromium_src changes LGTM, leaving security implications for @fmarier

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

I've not looked too deeply into the implementation of these checks in Chromium, but +1 on matching the Chrome behavior.

@bsclifton
Copy link
Member Author

OK great - CI looks good; there was one lint error with a test that I'll fix and commit. Ready to merge 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants