| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(privacy): Issues with content filtering in local frames on iOS #26622
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
Conversation
StephenHeaps
added
CI/skip-android
StephenHeaps
force-pushed
the
privacy/ios-local-frames
branch
5 times, most recently
from
1497fce to
c3bca5f
Compare
|
This all looks great to me. My only comment is that it looks like Not a blocking issue though. This looks great. Thank you! Also, just to check my understanding, the other local frame issues like scriptlets will be in a diff PR? |
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.
lgtm. Added some non-blocking comments too
@pes10k Yes I can remove |
|
Gotcha. +1 for removing the unneeded If this PR will also cover the scriptlet case, then I suggest adding a test for that too, since that'll touch some different code paths too. |
| const blockingCache = new Map(); | ||
| const sendMessage = $((resourceURL) => { | ||
| // when location.href does not match origin, we include origin in console | ||
| const originDisplay = window.location.href !== window.origin ? ` (${window.origin})` : ``; |
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.
Why do you need (${window.origin})? You can just use window.origin no? Likewise do we need empty back-ticks instead of ''?
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.
This is just in the log to try and be a bit more helpful / descriptive when the request comes from an about:blank iframe it will report as Brave prevented frame displaying about:blank (brave.com) from loading a resource from brave.com/some-resource.js (if we only displayed window.origin it wouldn't be clear it came from the iframe as only brave.com would be listed).
I can replace the empty backticks with ''.
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.
Just to note that there will be many cases where window.location.href !== window.origin that are irrelevant to local frames. window.location.href is the full URL of the current frame, while window.origin is the security origin. For this github page, the former is https://github.com/brave/brave-core/pull/26622, and the latter is https://github.com/.
If this check is intended to just check "is this an about:blank" frame, you might want something like window.location.origin !== window.origin
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.
Yeah the intention was to provide more info for all cases - not just about:blank - as the request(s) are now blocked against window.origin not window.location.href. I can just log window.origin here and leave out the window.location.href from the console info log if this is causing confusion, the intention was just to be a bit more descriptive (otherwise the log wouldn't indicate if the request was blocked on the main frame or from an iframe).
ios/brave-ios/Tests/ClientTests/Resources/scripts/cosmetic-filter-tests.js
Show resolved
Hide resolved
|
I haven't looked at the code yet, but I think websites can still bypass all the overrides we apply by doing this https://github.com/brave/internal/issues/914 |
|
I built this PR and tested my PoC from https://github.com/brave/internal/issues/914. It's still possible to bypass JS injects: |
...nd/UserContent/UserScripts/Scripts_Dynamic/Scripts/DomainSpecific/Paged/FrameCheckWrapper.js
Show resolved
Hide resolved
StephenHeaps
force-pushed
the
privacy/ios-local-frames
branch
from
c3bca5f to
708cb69
Compare
StephenHeaps
force-pushed
the
privacy/ios-local-frames
branch
from
708cb69 to
d3a3df9
Compare
|
Released in v1.75.71 |
Resolves brave/brave-browser#40649
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Add custom filter rules:
Cosmetic Filtering:
This should be hiddentext is hidden / not displayed on the main frame or iframeRequest Blocking:
This should be blockedis not displayed on the main frame or iframeScriptlets:
First party bodyhas value of 1First local frame bodyhas a value of 1