fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison #27320

StephenHeaps · 2025-01-22T21:49:11Z

Some scriptlets are failing to execute due to the comparison in FrameCheckWrapper.js failing due to the requiredHref being stripped of m, mobile, and www subdomains.

Resolves brave/brave-browser#43454

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
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 wiki
- npm run presubmit wiki, 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Play some videos Shields up on YouTube
Verify no ads are displayed or metadata shown (0:00/0:06 / 0:00:0:15 timestamps shown before video plays)

kylehickinson · 2025-01-23T19:58:14Z

ios/brave-ios/Tests/BraveSharedTests/URLExtensionTests.swift

+  }
+}
+
+class NavigationDelegate: NSObject, WKNavigationDelegate {


nit: private access control

Fixed in b998fd1

ios/brave-ios/Tests/BraveSharedTests/URLExtensionTests.swift

Brandon-T · 2025-01-23T20:49:35Z

ios/brave-ios/Sources/BraveShared/Extensions/URLExtensions.swift

+    components.scheme = self.scheme
+    components.port = self.port
+    components.host = self.host
+    return components.url ?? self


Doesn't URL.origin.url aka URLOrigin(url: self).url already do this?

Similar. I would need to drop trailing slash but I believe with that either could be used.

For this: http://Дом.com what does this return as the origin in both JS and URLComponents?
If it returns the same, then it's all good.

It seems to work fine in both. URLComponents correctly punycodes the domain.

LGTM.

http://Дом.ru/ will output http://xn--d1aqf.ru
http://Дoм.ru/ will output http://xn--o-gtbz.ru

Added in a7628ea

github-actions · 2025-01-24T17:46:04Z

ios/brave-ios/Tests/BraveSharedTests/URLExtensionTests.swift

+        // await load of html
+        await fulfillment(of: [expectation], timeout: 2)
+
+        guard let result = try await webView.evaluateJavaScript("window.origin") as? String else {


_{reported by reviewdog 🐶}
[semgrep] evaluateJavaScript usages should be vet by the security-team.

References:
- https://github.com/brave/brave-browser/wiki/Security-reviews (point 13)

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-execute-script-ios.yaml

Cc @stoletheminerals @bridiver

github-actions · 2025-01-27T18:11:52Z

Chromium major version is behind target branch (132.0.6834.83 vs 133.0.6943.27). Please rebase.

…`domainURL` is stripping `m`, `mobile`, and `www` subdomains which can cause our check to miss incorrectly.

…prove unit test speed.

brave-builds · 2025-01-30T17:04:16Z

Released in v1.77.3

Uni-verse · 2025-01-30T22:13:00Z

Verified on iPhone 12 running iOS 18.2 using version 1.77 (3)

Verified test plan mentioned in fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison #27320 (comment)
- Ensured ads are not shown for new users.
- Ensured there are no ads when scrubbing the progress bar throughout video.
- Ensured that ads do not show up when using normal and aggressive ad-block settings.
- Ensured that existing users do not get ads on Youtube after upgrading app (1.77.2 -> 1.77.3).

example	example

…arison (uplift to 1.76.x) (#27419) Uplift of #27320 (squashed) to beta

…arison (uplift to 1.75.x) (#27420) Uplift of #27320 (squashed) to release

StephenHeaps added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 labels Jan 22, 2025

StephenHeaps self-assigned this Jan 22, 2025

StephenHeaps marked this pull request as ready for review January 23, 2025 15:47

StephenHeaps requested a review from a team as a code owner January 23, 2025 15:47

kylehickinson approved these changes Jan 23, 2025

View reviewed changes

github-actions bot reviewed Jan 23, 2025

View reviewed changes

ios/brave-ios/Tests/BraveSharedTests/URLExtensionTests.swift Show resolved Hide resolved

github-actions bot added the needs-security-review label Jan 23, 2025

github-actions bot assigned bridiver and stoletheminerals Jan 23, 2025

Brandon-T requested changes Jan 23, 2025

View reviewed changes

Brandon-T approved these changes Jan 24, 2025

View reviewed changes

github-actions bot reviewed Jan 24, 2025

View reviewed changes

github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Jan 27, 2025

StephenHeaps added 5 commits January 27, 2025 13:47

Fix url passed to FrameCheckWrapper.js to match window.origin. The …

2e1baed

…`domainURL` is stripping `m`, `mobile`, and `www` subdomains which can cause our check to miss incorrectly.

Unit test for new URL.windowOriginURL.

5f993ed

Wait for didFinish instead of sleeping when loading HTML string to im…

1d9ce11

…prove unit test speed.

review(kylehickinson): unit test helper access control.

65edd86

Add punycode tests to unit test. Increase timeout.

b109763

StephenHeaps force-pushed the bugfix/frame-check-wrapper-youtube branch from a7628ea to b109763 Compare January 27, 2025 18:57

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Jan 27, 2025

thypon removed the needs-security-review label Jan 28, 2025

StephenHeaps merged commit c9fc5bd into master Jan 30, 2025
18 checks passed

StephenHeaps deleted the bugfix/frame-check-wrapper-youtube branch January 30, 2025 16:16

github-actions bot added this to the 1.77.x - Nightly milestone Jan 30, 2025

brave-builds added a commit that referenced this pull request Jan 30, 2025

Uplift of #27320 (squashed) to beta

0823d1a

brave-builds added a commit that referenced this pull request Jan 30, 2025

Uplift of #27320 (squashed) to release

5c8e579

This was referenced Jan 30, 2025

fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison (uplift to 1.76.x) #27419

Merged

fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison (uplift to 1.75.x) #27420

Merged

StephenHeaps pushed a commit that referenced this pull request Jan 30, 2025

Uplift of #27320 (squashed) to release

4586604

kjozwiak pushed a commit that referenced this pull request Jan 31, 2025

fix(privacy): FrameCheckWrapper.js given incorrect value for url comp…

f2dce12

…arison (uplift to 1.76.x) (#27419) Uplift of #27320 (squashed) to beta

kjozwiak pushed a commit that referenced this pull request Jan 31, 2025

fix(privacy): FrameCheckWrapper.js given incorrect value for url comp…

ddd5898

…arison (uplift to 1.75.x) (#27420) Uplift of #27320 (squashed) to release

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison #27320

fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison #27320

StephenHeaps commented Jan 22, 2025 •

edited

Loading

kylehickinson Jan 23, 2025

StephenHeaps Jan 23, 2025

Brandon-T Jan 23, 2025

StephenHeaps Jan 23, 2025

Brandon-T Jan 24, 2025 •

edited

Loading

Brandon-T Jan 24, 2025 •

edited

Loading

StephenHeaps Jan 24, 2025 •

edited

Loading

github-actions bot Jan 24, 2025

github-actions bot commented Jan 27, 2025

brave-builds commented Jan 30, 2025

Uni-verse commented Jan 30, 2025

fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison #27320

fix(privacy): FrameCheckWrapper.js given incorrect value for url comparison #27320

Conversation

StephenHeaps commented Jan 22, 2025 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

kylehickinson Jan 23, 2025

Choose a reason for hiding this comment

StephenHeaps Jan 23, 2025

Choose a reason for hiding this comment

Brandon-T Jan 23, 2025

Choose a reason for hiding this comment

StephenHeaps Jan 23, 2025

Choose a reason for hiding this comment

Brandon-T Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Brandon-T Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

StephenHeaps Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2025

brave-builds commented Jan 30, 2025

Uni-verse commented Jan 30, 2025

StephenHeaps commented Jan 22, 2025 •

edited

Loading

Brandon-T Jan 24, 2025 •

edited

Loading

Brandon-T Jan 24, 2025 •

edited

Loading

StephenHeaps Jan 24, 2025 •

edited

Loading