fix: resolve relative URLs in link click handler#8
Conversation
the click handler only caught links starting with /browse/ or http(s)://. relative links added by JavaScript after the HTMLRewriter ran (like /page or relative.html) fell through and navigated to the proxy origin, causing 404s. now the handler uses resolveForProxy() for all links, which resolves relative URLs against the target origin. also adds mailto: and tel: to the resolveForProxy skip list, and handles // protocol-relative URLs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the injected in-page click interception logic to resolve all clicked link URLs through the same proxy resolution function used by the fetch/XHR patches, fixing cases where dynamically-added relative links would otherwise navigate to the proxy origin and 404.
Changes:
- Update the iframe click handler to route navigation through
resolveForProxy()rather than only intercepting/browse/andhttp(s)://links. - Extend
resolveForProxy()to skipmailto:/tel:and attempt to support protocol-relative (//...) URLs. - Add an integration test asserting the rewritten HTML contains the updated click handler/resolution logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/uppercase-script.ts | Adjusts click interception to use resolveForProxy() and expands scheme handling/skip list. |
| src/rewriter.test.ts | Adds a test to assert the updated click handler/resolution logic is present in rewritten HTML. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (url.startsWith('http://') || url.startsWith('https://') || url.startsWith('//')) { | ||
| return PROXY_PREFIX + url; | ||
| } |
There was a problem hiding this comment.
resolveForProxy() treats protocol-relative URLs (//example.com/...) as already-absolute and returns /browse/ + url. This produces paths like /browse///example.com, which handleProxy() then turns into https:////example.com (invalid) and will fail to fetch. To match server-side new URL() behavior (see resolveAndProxy()), resolve //... against the current target URL’s protocol (e.g., via new URL(url, targetUrl).href) before prefixing, or normalize // to https:// (or the target’s protocol) before proxying.
| function resolveForProxy(url) { | ||
| if (!url || url.startsWith(PROXY_PREFIX) || url.startsWith('data:') || url.startsWith('blob:') || url.startsWith('javascript:') || url.startsWith('#')) { | ||
| if (!url || url.startsWith(PROXY_PREFIX) || url.startsWith('data:') || url.startsWith('blob:') || url.startsWith('javascript:') || url.startsWith('#') || url.startsWith('mailto:') || url.startsWith('tel:')) { | ||
| return url; |
There was a problem hiding this comment.
Adding tel: to the client-side skip list helps for dynamically-added links, but links rewritten by the HTMLRewriter will still be proxied because resolveAndProxy() doesn’t exclude tel: (it rewrites all a[href]). That means tel: links present in original HTML will still become /browse/tel:... and break on click. Consider also excluding tel: in the server-side resolveAndProxy() so static tel: links are not rewritten.
| // the click handler should use resolveForProxy instead of only matching http:// urls | ||
| expect(html).toContain("resolveForProxy(href)"); | ||
| // should handle mailto: and tel: links without proxying | ||
| expect(html).toContain("mailto:"); | ||
| expect(html).toContain("tel:"); |
There was a problem hiding this comment.
This test asserts on raw substrings in the generated HTML (e.g., resolveForProxy(href)), which is brittle to formatting/minification changes and doesn’t actually validate that relative clicks navigate correctly. If possible, make the assertion more structural (e.g., regex that tolerates whitespace) and/or add a focused unit test around the URL resolution behavior used by the click handler (relative, protocol-relative, mailto/tel).
| // the click handler should use resolveForProxy instead of only matching http:// urls | |
| expect(html).toContain("resolveForProxy(href)"); | |
| // should handle mailto: and tel: links without proxying | |
| expect(html).toContain("mailto:"); | |
| expect(html).toContain("tel:"); | |
| // Find the script block that references resolveForProxy and assert on its contents | |
| const scriptMatch = html.match(/<script[^>]*>([\s\S]*?resolveForProxy[\s\S]*?)<\/script>/); | |
| expect(scriptMatch).not.toBeNull(); | |
| const scriptContent = scriptMatch![1]; | |
| // the click handler should use resolveForProxy with href (allowing for whitespace/minification) | |
| expect(scriptContent).toMatch(/resolveForProxy\s*\(\s*href\s*\)/); | |
| // should handle mailto: and tel: links without proxying, within the same script | |
| expect(scriptContent).toMatch(/mailto:/); | |
| expect(scriptContent).toMatch(/tel:/); |
Summary
/browse/orhttp(s)://. Relative links added by JavaScript after the HTMLRewriter ran (like/pageorrelative.html) fell through and navigated to the proxy origin, causing 404s.resolveForProxy()for all link clicks — same resolution logic as fetch/XHR patchesmailto:andtel:to the skip list, and handles//protocol-relative URLsTest plan
🤖 Generated with Claude Code