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)

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

…prove unit test speed.

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

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

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


nit: private access control

Fixed in b998fd1

github-actions · 2025-01-23T20:39:40Z

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

+        // await load of html
+        await fulfillment(of: [expectation], timeout: 1)
+
+        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

Used in a unit test only to fetch window.origin value returned from WKWebView / WebKit and verify against our helper.

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

StephenHeaps added 2 commits January 22, 2025 16:48

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

a5cc1cf

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

Unit test for new URL.windowOriginURL.

f8e2844

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

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

fb742f2

…prove unit test speed.

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

review(kylehickinson): unit test helper access control.

b998fd1

github-actions bot reviewed Jan 23, 2025

View reviewed changes

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

Add punycode tests to unit test. Increase timeout.

a7628ea

github-actions bot reviewed Jan 24, 2025

View reviewed changes

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

github-actions bot 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

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

Are you sure you want to change the base?

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

github-actions bot 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

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