Skip to content

fix: resolve relative URLs in link click handler #8

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.

Sign up for GitHub

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

Merged
jergason merged 1 commit into from
Mar 23, 2026
Merged

fix: resolve relative URLs in link click handler #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions src/rewriter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,17 @@ describe("HTMLRewriter integration", () => {
expect(allProxied).toBe(true);
});

it("click handler uses resolveForProxy for link navigation", async () => {
const resp = await worker.fetch("/browse/https://httpbin.org/html");
if (resp.status !== 200) return;
const html = await resp.text();
// 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:");
Comment on lines +146 to +150
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
// 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:/);

Copilot uses AI. Check for mistakes.
});

it("forwards POST body and content-type header", async () => {
const resp = await worker.fetch("/browse/https://httpbin.org/post", {
method: "POST",
Expand Down
24 changes: 10 additions & 14 deletions src/uppercase-script.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,32 +96,28 @@ export const uppercaseScript = `
if (!link) return;
var href = link.getAttribute('href');
if (!href) return;
// let proxy-rewritten links work naturally in the iframe
// already proxied — just tell parent the real URL
if (href.startsWith(PROXY_PREFIX)) {
// extract the real URL and tell the parent to update the URL bar
var realUrl = href.slice(PROXY_PREFIX.length);
try {
window.top.postMessage({ type: 'navigate', url: realUrl }, '*');
} catch(ex) {}
try { window.top.postMessage({ type: 'navigate', url: realUrl }, '*'); } catch(ex) {}
return;
}
// absolute external link not yet rewritten
if (href.startsWith('http://') || href.startsWith('https://')) {
// resolve via the same logic as fetch/XHR
var resolved = resolveForProxy(href);
if (resolved !== href) {
e.preventDefault();
var proxied = PROXY_PREFIX + href;
try {
window.top.postMessage({ type: 'navigate', url: href }, '*');
} catch(ex) {}
window.location.href = proxied;
var navigateUrl = resolved.startsWith(PROXY_PREFIX) ? resolved.slice(PROXY_PREFIX.length) : href;
try { window.top.postMessage({ type: 'navigate', url: navigateUrl }, '*'); } catch(ex) {}
window.location.href = resolved;
}
}, true);

// resolve any URL to a proxied URL
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;
Comment on lines 116 to 118
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
if (url.startsWith('http://') || url.startsWith('https://')) {
if (url.startsWith('http://') || url.startsWith('https://') || url.startsWith('//')) {
return PROXY_PREFIX + url;
}
Comment on lines +120 to 122
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
// relative URL — resolve against the target origin
Expand Down
Loading