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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class ScriptFactory {
sourceWrapper = sourceWrapper.replacingOccurrences(of: "$<scriplet>", with: source)
sourceWrapper = sourceWrapper.replacingOccurrences(
of: "$<required_href>",
with: configuration.frameURL.domainURL.absoluteString
with: configuration.frameURL.windowOriginURL.absoluteString
)
// when `isMainFrame` is true, we still need to inject to all frames to handle `about:blank` first-party frames
resultingScript = WKUserScript(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,15 @@ extension URL {

return renderedString.bidiBaseDirection == .leftToRight
}

/// Matches what `window.origin` would return in javascript.
public var windowOriginURL: URL {
var components = URLComponents()
components.scheme = self.scheme
components.port = self.port
components.host = self.host
return components.url ?? self
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Brandon-T Brandon-T Jan 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Brandon-T Brandon-T Jan 24, 2025

Choose a reason for hiding this comment

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

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

image image

LGTM.

Copy link
Collaborator Author

@StephenHeaps StephenHeaps Jan 24, 2025

Choose a reason for hiding this comment

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

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

Added in a7628ea

}
}

extension InternalURL {
Expand Down
55 changes: 55 additions & 0 deletions ios/brave-ios/Tests/BraveSharedTests/URLExtensionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import Foundation
import Shared
import WebKit
import XCTest

class URLExtensionTests: XCTestCase {
Expand Down Expand Up @@ -135,4 +136,58 @@ class URLExtensionTests: XCTestCase {
embeddedURL
)
}

// Test that `windowOriginURL` returns the same value as `window.origin`.
@MainActor func testWindowOriginURL() async {
let testURLs = [
// multiple subdomains
(URL(string: "https://one.two.three.example.com")!, "https://one.two.three.example.com"),
// trailing slash
(URL(string: "https://example.com/")!, "https://example.com"),
// query
(URL(string: "https://www.example.com/?v=1234567")!, "https://www.example.com"),
// match
(URL(string: "https://www.example.com")!, "https://www.example.com"),
// punycode
(URL(string: "http://Дом.ru/")!, "http://xn--d1aqf.ru"),
// punycode
(URL(string: "http://Дoм.ru/")!, "http://xn--o-gtbz.ru"),
]

let webView = WKWebView()
for (value, expected) in testURLs {
do {
let expectation = XCTestExpectation(description: "didFinish")
let navigationDelegate = NavigationDelegate(didFinish: {
expectation.fulfill()
})
webView.navigationDelegate = navigationDelegate
webView.loadHTMLString("", baseURL: value)

// await load of html
await fulfillment(of: [expectation], timeout: 2)

guard let result = try await webView.evaluateJavaScript("window.origin") as? String else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

XCTFail("Expected a String result")
return
}
XCTAssertEqual(result, expected)
XCTAssertEqual(result, value.windowOriginURL.absoluteString)
} catch {
XCTFail("Expected a valid `window.origin`")
}
}
}
}

private class NavigationDelegate: NSObject, WKNavigationDelegate {
private var didFinish: () -> Void

init(didFinish: @escaping () -> Void) {
self.didFinish = didFinish
}

func webView(_ webView: WKWebView, didFinish navigation: WKNavigation!) {
didFinish()
}
}
Loading