-
Notifications
You must be signed in to change notification settings - Fork 859
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
[C129] [Android] Fix for brave scheme pages #25602
Conversation
Chromium change: https://source.chromium.org/chromium/chromium/src/+/4d2cab773589a430dfae515994c3e6cad0eb0270 [M129] Disallow VerbatimMatches to open non-navigable URLs by default. This change prevents non-navigable URLs from being opened upon paste, refine, autocomplete etc., effectively disallowing accidental execution of inline javascript: blocks. The non-navigable (e.g. executable) URIs will be effectively pushed down on the suggestions list, making them still available, but moving forward these will require an explicit user action to be invoked (i.e. the user now has to intentionally tap these suggestions to initiate the corresponding action). The change removes redundant test that relies on inline page injection. This is already covered by another test: http://shortn/_NG1M484b41 (cherry picked from commit 04938340e1a93e5e5588badd5e01600dd3356d52) Bug: b/360642942
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
|
||
#include "components/omnibox/browser/verbatim_match.h" | ||
|
||
#include "content/public/common/url_constants.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this include should also be under #if BUILDFLAG(IS_ANDROID)
, but they didn't do it right upstream, so we can probably just keep it the same for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will fail on iOS if we keep it like this, actually, but we can see what CI says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS passed, as @mkarolin mentioned they don't guard it in the upstream as well, so should be ok for us too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It passed iOS because of 97aab4f, but we will remove that fix at some point (hopefully, brave/brave-browser#41014) and then we would have to update the override here as well.
Verification PASSED on
|
Uplift of #25602 (squashed) to beta
Uplift of #25602 (squashed) to release
Chromium change:
https://source.chromium.org/chromium/chromium/src/+/4d2cab773589a430dfae515994c3e6cad0eb0270
[M129] Disallow VerbatimMatches to open non-navigable URLs by default.
This change prevents non-navigable URLs from being opened upon paste, refine, autocomplete etc., effectively disallowing accidental execution of inline javascript: blocks.
The non-navigable (e.g. executable) URIs will be effectively pushed down on the suggestions list, making them still available, but moving forward these will require an explicit user action to be invoked (i.e. the user now has to intentionally tap these suggestions to initiate the corresponding action).
The change removes redundant test that relies on inline page injection. This is already covered by another test: http://shortn/_NG1M484b41
(cherry picked from commit 04938340e1a93e5e5588badd5e01600dd3356d52)
Bug: b/360642942
Resolves brave/brave-browser#41069
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: