diff --git a/work/tech-design/opaque-url-fragment-detection.md b/work/tech-design/opaque-url-fragment-detection.md index 56944000..ce8fa263 100644 --- a/work/tech-design/opaque-url-fragment-detection.md +++ b/work/tech-design/opaque-url-fragment-detection.md @@ -8,7 +8,8 @@ tags: - webkit - url - crash -status: draft +status: in-progress +chosen-approach: '_web_originalDataAsString via URLComponents(webKitUrl:)' asana: >- https://app.asana.com/1/137249556945/project/1202406491309510/task/1215403820548743 --- @@ -18,125 +19,71 @@ asana: >- --- -## Background & Requirements +## Background -The DuckDuckGo browser uses `WKWebView` for all page rendering. Every navigation goes through the `decidePolicyFor` delegate chain managed by `DistributedNavigationDelegate` (BrowserServicesKit). A central piece of that chain is classifying each `WKNavigationAction` as a **same-document navigation** (fragment-only change within the current page, e.g. `#section`) via: +Every navigation through `WKWebView` goes through `DistributedNavigationDelegate`, which classifies each `WKNavigationAction` as a **same-document navigation** (fragment-only change, e.g. `#section`) via `isSameDocumentNavigation`. Misclassification corrupts the navigation state machine. -```swift -// WKNavigationActionExtension.swift:184 -public var isSameDocumentNavigation: Bool { - switch navigationType { - case .linkActivated, .other: - return !isRedirect && newURL.hasFragment && currentURL.equals(newURL, by: .sameDocument) - case .backForward: - return (newURL.hasFragment || currentURL.hasFragment) && currentURL.equals(newURL, by: .sameDocument) - ... - } -} -``` +`isSameDocumentNavigation` relies on `hasFragment` and `.sameDocument` URL equality, both of which must correctly extract the fragment — which is where the bug lives. -`isSameDocumentNavigation` drives the `DistributedNavigationDelegate` state machine: same-document navigations reuse the existing `Navigation` object and trigger client-redirect handling rather than starting a fresh navigation, while non-same-document navigations allocate a new `Navigation`. Misclassification corrupts navigation state. +### Two URL parsers in WebKit -`hasFragment` and the `.sameDocument` equality both depend on extracting the URL fragment—which is where the bug lives. +| Parser | Used where | Fragment in opaque URLs (`data:`, `about:`, `blob:`) | +|--------|-----------|------------------------------------------------------| +| **WTF::URLParser** | WebKit internals, navigation decisions | ✅ Correctly split at `#` | +| **NSURL / CFURL** | Foundation APIs, Swift `URL` | ❌ Encodes `#` as `%23`; `.fragment` returns `nil` | -### Two URL parsers coexist in WebKit on Apple platforms - -| Parser | Used where | Standard | Fragment in opaque URLs (`data:`, `about:`, `blob:`) | -|--------|-----------|----------|------------------------------------------------------| -| **WTF::URLParser** | WebKit rendering, WKWebView navigation decisions | WHATWG URL | ✅ Correctly split at `#` | -| **NSURL / CFURL** | Foundation APIs, Swift `URL` | Old RFC 1808/2396 | ❌ Encodes `#` as `%23`, returns `nil` for `.fragment` | - -When WebKit bridges its internal `WTF::URL` to Objective-C it calls: +When WebKit bridges a `WTF::URL` to Objective-C it calls: ```c CFURLCreateAbsoluteURLWithBytes(alloc, rawBytes, len, kCFStringEncodingUTF8, nil, true) ``` -The raw bytes at this point still contain the literal `#`. CFURL re-parses them with its old-RFC parser, which does **not** recognise `#` as a fragment delimiter in opaque-scheme URLs. It encodes `#` → `%23` and stores the whole thing as the opaque path. - -Result for `data:text/html,hello#anchor`: +The raw bytes contain the literal `#`, but CFURL's old-RFC parser does not recognise `#` as a fragment delimiter in opaque-scheme URLs — it encodes it as `%23` and stores the whole thing as the opaque path. Result for `data:text/html,hello#anchor`: | Property | WTF::URL | NSURL / Swift URL | |----------|----------|-------------------| -| `.path` | `text/html,hello` | `""` (empty) | -| `.query` | nil | nil | -| `.fragment` | `"anchor"` | nil | +| `.fragment` | `"anchor"` | `nil` | | `.absoluteString` | `data:text/html,hello#anchor` | `data:text/html,hello%23anchor` | --- -## Problem Statement +## Problem -A pathological URL—a multi-MB `data:` URL or attacker-crafted href—reaches `WKNavigationAction.isSameDocumentNavigation` on the main thread. The call chain: - -1. `WebKit::WebPageProxy::decidePolicyForNavigationAction` dispatches to the main-thread delegate. -2. `DistributedNavigationDelegate` reads `wkNavigationAction.isSameDocumentNavigation`. -3. `isSameDocumentNavigation` calls `newURL.absoluteString.hashedSuffix` (and for `.backForward`, also `currentURL.absoluteString.hashedSuffix`). -4. `hashedSuffix` → `hashedSuffixRange` → `String.firstIndex(of: "#")` (`StringExtension.swift:67`) iterates grapheme-by-grapheme over `absoluteString`. -5. For a multi-MB `data:` URL this is O(n) UTF-8 grapheme decoding on the **main thread**, per navigation decision. -6. Main thread blocked >10 s under thermal-state `serious` at 100% CPU → RunningBoard scene-update watchdog kills the app: **SIGKILL, terminationReason `0x8BADF00D`**. +A multi-MB `data:` URL reaches `isSameDocumentNavigation` on the main thread. The old `hashedSuffix` implementation called `String.firstIndex(of: "#")`, iterating grapheme-by-grapheme — O(n) per navigation decision on the **main thread**. For a large enough URL this blocks the thread >10 s → RunningBoard watchdog SIGKILL (`0x8BADF00D`). The fix must: -1. Make `hashedSuffixRange` / `isSameDocumentNavigation` fast for arbitrarily large URLs (no O(n) grapheme scan). +1. Make fragment detection fast (no O(n) grapheme scan). 2. Correctly detect fragments in opaque URLs where NSURL has already encoded `#` as `%23`. --- -## Why Standard APIs Can't Help: The Information-Loss Chain +## Information-Loss Chain -Fragment information is **irreversibly lost** between WebKit's internal representation and what Swift code receives in `decidePolicyFor`: +Fragment information is irreversibly lost between WebKit and Swift: ``` -WTF::URL (WHATWG-compliant; m_string = "data:…#anchor", m_queryEnd before '#') - ↓ URL::createNSURL() → createCFURL() - ↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor") ← '#' still visible here - ↓ CFURL re-parses with old-RFC parser; encodes '#' → '%23' -NSURL (.absoluteString = "…%23anchor", .fragment = nil, .path = "") - ↓ [NSMutableURLRequest initWithURL: url().createNSURL()] (ResourceRequestCocoa.mm) -NSURLRequest (same NSURL) +WTF::URL (m_string = "data:…#anchor") + ↓ createCFURL() + ↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor") ← '#' visible here + ↓ CFURL re-parses; '#' → '%23' +NSURL (.absoluteString = "…%23anchor", .fragment = nil) ↓ WKNavigationAction.request.URL ← what Swift sees -WKNavigationAction._originalURL ← same path, same loss (createNSURL() again) ``` -Every Foundation/Swift API is downstream of the encoding step: - -- **`URL.fragment` / `.path` / `.query`** — return `nil` / `""` for all opaque URLs. -- **`URLComponents(url:)` / `(string:)` and `NSURLComponents` equivalents** — either use NSURL's already-broken cached parse tree, or re-parse `absoluteString` where `#` is already `%23`. RFC 3986 treats `%23` as a literal character, not a delimiter. Fragment stays nil in all four variants. -- **`WKNavigationActionPrivate.h`** (`_originalURL`, `_isRedirect`, `_hitTestResult`, …) — all URL-returning properties go through `WTF::URL::createNSURL()` → same `createCFURL()` path. No private API exposes the raw WTF string. - -There is **no Foundation or WebKit API**, public or private, that recovers the original `#` once the NSURL has been constructed. +Every downstream Foundation API (`URL.fragment`, `URLComponents`, `NSURLComponents`) operates on the already-broken NSURL — they all return `nil` for the fragment. No public or private WebKit API exposes the raw `WTF::URL` string … except one (see Chosen Approach). --- -## Approaches Tried +## Approaches Considered -### 1. Scan `absoluteString` for `%23` +### 1. Scan `absoluteString` for `%23` ❌ -Since NSURL encodes the original `#` as `%23` in `absoluteString`, scanning for `%23` as a fragment delimiter recovers the fragment in many cases. Implemented and partly working. +Since NSURL encodes `#` as `%23`, scanning for it recovers the fragment in many cases. Partly working, but fundamentally ambiguous: `%23` is also valid payload in `data:` URLs (SVG fill colors, anchor hrefs, CSS). The two cases produce identical `absoluteString` and cannot be distinguished from the string alone. A size guard avoids scanning large payloads but causes incorrect equality results. -**Fundamental problem:** `%23` is also valid payload data in `data:` URLs: -- SVG fill colors: `data:image/svg+xml,` — `%23` is `#` in the SVG -- HTML anchor hrefs: `data:text/html,` -- CSS: `data:text/css,color:%23333` +### 2. `CFURLCreateAbsoluteURLWithBytes` hook -Both cases produce identical `absoluteString`. They are **indistinguishable from the string alone**. A 4 KB size guard was added to avoid scanning large `data:` URLs, but that makes two different large data: URLs compare as equal — semantically wrong. - -### 2. Private API / `WKNavigationAction._originalURL` - -See information-loss chain above. No private API bypasses it; all paths go through `createCFURL()`. - ---- - -## Recommended Approach: `CFURLCreateAbsoluteURLWithBytes` Interpose - -### Key Insight - -WebKit calls `CFURLCreateAbsoluteURLWithBytes()` at the **exact** moment it converts a `WTF::URL` to a `CFURL`/`NSURL`. The raw byte buffer at that call site still contains the literal `#`, before CFURL encodes it. Intercepting this C function gives unambiguous access to the original bytes. - -### Mechanism: `DYLD_INTERPOSE` - -The `__DATA,__interpose` Mach-O section replaces any C function in the same dylib at load time, without method swizzling or private SPI: +WebKit calls `CFURLCreateAbsoluteURLWithBytes()` at the exact moment it converts a `WTF::URL` to a `CFURL`. The raw byte buffer at that call site still contains the literal `#`. Intercepting this C function via the Mach-O `__DATA,__interpose` section (without method swizzling) gives unambiguous access to the original bytes — scan for `0x23` in a simple C loop, store the byte range as an associated object on the bridged `NSURL`. ```c #define DYLD_INTERPOSE(_replacement, _replacee) \ @@ -147,87 +94,92 @@ The `__DATA,__interpose` Mach-O section replaces any C function in the same dyli { (const void *)&(_replacement), (const void *)&(_replacee) } ``` -### Implementation +Callers read `NSURL.ddg_fragmentByteRange` for the unambiguous `#` position. Could be limited to `#ifdef DEBUG` so there is zero production overhead. -Files: -- `CFURLCreateAbsoluteURLWithBytesInterpose.m` — DEBUG-only (`#ifdef DEBUG`) -- `NSURL+FragmentByteRange.h` / `.m` — category exposing the stored range +**Downside:** adds Objective-C interpose files (`CFURLCreateAbsoluteURLWithBytesInterpose.m`, `NSURL+FragmentByteRange.h/.m`), couples to a private CFURL entry point, and requires the associated-object write to happen before any Swift code reads the URL — fragile under future WebKit changes. -**Interpose logic:** +### 2a. `dyld_interpose` at the dyld level ⚠️ (option, not chosen) -1. Call the real `CFURLCreateAbsoluteURLWithBytes` to get the `CFURLRef`. -2. For opaque URLs (scheme present, no `//` authority), scan the raw byte buffer for `'#'` (byte `0x23`) with a simple C loop—O(n) but done once at NSURL construction, not per navigation decision. -3. If found, store the byte range as an `NSValue` associated object on the bridged `NSURL` via `objc_setAssociatedObject`. -4. Callers read `NSURL.ddg_fragmentByteRange` for the unambiguous `#` position. - -```objc -if (relativeURLBytes[i] == '#') { - NSRange fragmentRange = NSMakeRange(i, length - i); - NSURL *nsurl = (__bridge NSURL *)result; - objc_setAssociatedObject(nsurl, &kFragmentByteRangeKey, - [NSValue valueWithRange:fragmentRange], - OBJC_ASSOCIATION_RETAIN_NONATOMIC); -} -``` - -### Why This Is Correct - -- Bytes are scanned **before** CFURL encodes `#` → `%23` — no ambiguity with payload `%23`. -- Associated object is set on the **exact NSURL instance** for that WTF::URL. -- `ddg_fragmentByteRange.location` matches WTF::URL's internal `m_queryEnd + 1`. -- Works for all opaque schemes: `data:`, `about:`, `blob:`, `javascript:`. -- DEBUG-only: zero production overhead; interpose section absent from release builds. - -### Complete Call Flow with Interpose - -``` -WTF::URL (m_string = "data:…#anchor", m_queryEnd before '#') - ↓ URL::createCFURL() - ↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor") - ↑↑ INTERPOSED: scan rawBytes → store NSRange on result NSURL - ↓ NSURL (.absoluteString = "…%23anchor", .fragment = nil) - ↑ .ddg_fragmentByteRange → {location=N} — unambiguous '#' position -Swift URLExtension.opaqueComponents: - DEBUG → use ddg_fragmentByteRange (exact) - release → NSString.range(of: "#") fallback (unambiguous for non-data: schemes) -``` +Same concept as above but applied via `DYLD_INSERT_LIBRARIES` + `DYLD_INTERPOSE` at the dynamic linker level, outside the process binary entirely. More powerful (intercepts calls from any dylib), but more invasive, not available in App Store binaries, and harder to confine to DEBUG. --- -## Notes +## Chosen Approach: Private WebKit URL String (`_web_originalDataAsString`) -[1] The `%23` ambiguity only affects `data:` or `about:` URLs. For `blob:`, `javascript:` the opaque path never legitimately contains `%23` as encoded payload. +NSURL instances that originate from WebKit (via `createNSURL()`) carry a private property `_web_originalDataAsString` which holds the original `WTF::URL` byte string — **before** CFURL re-encoded `#` as `%23`. Parsing that string with Swift's native `URLComponents(string:)` gives correct path/query/fragment decomposition for all opaque URLs. -[2] The primary safety fix is the O(n) crash: replace `String.firstIndex(of: "#")` with a byte-level scan and short-circuit on `data:` scheme or length threshold. The interpose is the accuracy improvement layered on top for DEBUG validation. +```swift +// URLComponentsExtension.swift +extension URLComponents { + /// Parses a WebKit-origin URL using the original WTF::URL string. + /// Falls back to URLComponents(url:) for non-WebKit or hierarchical URLs. + init?(webKitUrl: URL) { + guard webKitUrl.isOpaque, + let originalString = (webKitUrl as NSURL).value(forKey: "_web_originalDataAsString") as? String, + let swiftNativeURLComponents = URLComponents(string: originalString) else { + self.init(url: webKitUrl, resolvingAgainstBaseURL: false) + return + } + self = swiftNativeURLComponents + } +} +``` -[3] WebKit bug for NSURL not handling `#` in opaque URLs: [bugs.webkit.org/68089](https://bugs.webkit.org/show_bug.cgi?id=68089). Chromium fixed this in M72 (2019). WebKit's WTF::URLParser is correct; only the Foundation/CFURL bridge is broken. +```swift +// URLExtension.swift +public var originalWebKitString: String? { + (self as NSURL).value(forKey: "_web_originalDataAsString") as? String +} + +public var hasFragment: Bool { + guard let components = URLComponents(webKitUrl: self), + let fragment = components.fragment else { return false } + return !fragment.isEmpty +} + +public func equals(_ other: URL, by components: EqualityComponents) -> Bool { + guard let selfParsed = URLComponents(webKitUrl: self), + let otherParsed = URLComponents(webKitUrl: other) else { return false } + // component-by-component comparison … +} +``` + +### Why this is correct + +- `_web_originalDataAsString` holds the `WTF::URL`'s `m_string` verbatim — `#` is still a literal character, not `%23`. +- Swift's `URLComponents(string:)` is WHATWG-aligned and correctly splits at `#`, so `.fragment` returns the real fragment. +- No ambiguity with `%23` payload: we parse the original string, not the NSURL's re-encoded `absoluteString`. +- The property is absent on non-WebKit URLs (`URL(string:)`-created values) — the initialiser falls back to `URLComponents(url:)`, which is correct for hierarchical URLs. +- Pure Swift/Obj-C KVC access; no interpose, no associated objects, no extra files. + +### Call flow + +``` +WTF::URL (m_string = "data:…#anchor") + ↓ createNSURL() +NSURL (.absoluteString = "…%23anchor", ._web_originalDataAsString = "data:…#anchor") + ↓ +URLComponents(webKitUrl: url) + → value(forKey: "_web_originalDataAsString") → "data:…#anchor" + → URLComponents(string: "data:…#anchor") + → .fragment = "anchor" ✅ +``` --- ## Testing - **Unit tests** (`URLExtensionTests.swift`): - - `equals(by: .sameDocument)` and `equals(by: .fuzzyIdentity)` on opaque URLs with fragment: `about:blank#section`, `data:text/html,…#anchor` - - Opaque URLs with query AND fragment: `data:text/html,…?key=value#anchor` - - Large (20 MB) `data:` URL performance test: comparison must complete in < 0.1 s - - Diagnostic timing test (`tempOpaqueNSURLComponentTimings`) printing per-operation durations for all Foundation URL APIs on a 20 MB `data:` URL with query and fragment - -- **Manual test** (DEBUG build): verify `ddg_fragmentByteRange` is set on `WKNavigationAction.request.URL` when navigating to a `data:` URL with a `#` fragment via `WKWebView`. - -- **Regression**: navigation tests in `DistributedNavigationDelegateTests.swift` and `NavigationTestHelpers.swift` updated to use `.equals(by: .fuzzyIdentity)` in place of removed `.matches()` API. + - `hasFragment` and `equals(by: .sameDocument / .fuzzyIdentity)` on `about:blank#section`, `data:text/html,…#anchor`, opaque URLs with query AND fragment. + - Large (20 MB) `data:` URL: comparison must complete in < 0.1 s. + - Diagnostic timing test (`tempOpaqueNSURLComponentTimings`) for all Foundation URL APIs on a 20 MB `data:` URL. +- **Regression**: navigation tests in `DistributedNavigationDelegateTests.swift` updated to use `.equals(by: .fuzzyIdentity)` in place of the removed `.matches()` API. --- -## Additional Considerations +## Notes -### Privacy -- No user data collected or transmitted. All processing is local and in-process. - -### Security -- No extra data is stored or passed anywhere -- The approach injects into the existing API and stores a helper pointer -### Site Breakage -- The O(n) fix prevents crashes on large URLs. Fragment-detection correctness improves navigation state machine accuracy but does not change whether a navigation is allowed or blocked. - -### Operational -- No new infrastructure, network calls, or persisted state. The crash was a SIGKILL watchdog kill visible in Sentry; the fix eliminates the `0x8BADF00D` terminations for this call chain. +- **WebKit bug:** [bugs.webkit.org/68089](https://bugs.webkit.org/show_bug.cgi?id=68089). Chromium fixed `#` handling in M72 (2019). `WTF::URLParser` is correct; only the CFURL bridge is broken. +- **Primary safety fix** is the O(n) crash: `isSameDocumentNavigation` now uses `URLComponents(webKitUrl:)` which is constant-time relative to URL length (KVC + a single string parse), replacing the old grapheme-scan loop. +- **`%23` ambiguity** only affects `data:` / `about:`. For `blob:` and `javascript:` the opaque path never legitimately contains `%23` as encoded payload. +- No user data collected or transmitted; all processing is local and in-process.