Skip to content

fix: resolve relative URLs in client-side fetch/XHR patches #5

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 client-side fetch/XHR patches #5

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
8 changes: 8 additions & 0 deletions src/rewriter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,14 @@ describe("HTMLRewriter integration", () => {
expect(resp.headers.get("access-control-allow-origin")).toBe("*");
});

it("injects resolveForProxy for relative URL handling", async () => {
const resp = await worker.fetch("/browse/https://httpbin.org/html");
if (resp.status !== 200) return;
const html = await resp.text();
expect(html).toContain("function resolveForProxy");
expect(html).toContain("new URL(url, targetUrl)");
});

it("rewrites data-src attributes for lazy-loaded images", async () => {
const resp = await worker.fetch("/browse/https://www.wired.com");
if (resp.status !== 200) return;
Expand Down
32 changes: 26 additions & 6 deletions src/uppercase-script.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,22 +116,42 @@ export const uppercaseScript = `
}
}, 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('#')) {
return url;
}
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() claims to handle already-proxied /browse/ paths, but it only checks url.startsWith(PROXY_PREFIX). If application code constructs an absolute proxied URL (e.g. new URL('/api', location.href).toString() on a proxied page -> https://<proxy-origin>/browse/https://<target>/api), this function treats it as an absolute https:// URL and prefixes again, producing a broken double-proxy URL. Consider detecting proxy-origin absolute URLs whose pathname starts with PROXY_PREFIX and returning them unchanged (or normalized to a relative /browse/...), before the http(s) branch.

Suggested change
}
}
// handle absolute URLs that already point to this origin with a /browse/ path
try {
var absolute = new URL(url, window.location.href);
if (absolute.origin === window.location.origin && absolute.pathname.startsWith(PROXY_PREFIX)) {
return url;
}
} catch(e) {}

Copilot uses AI. Check for mistakes.
if (url.startsWith('http://') || url.startsWith('https://')) {
return PROXY_PREFIX + url;
}
// relative URL — resolve against the target origin
var path = window.location.pathname;
if (path.startsWith(PROXY_PREFIX)) {
var targetUrl = path.slice(PROXY_PREFIX.length) + window.location.search;
try {
var resolved = new URL(url, targetUrl).href;
return PROXY_PREFIX + resolved;
} catch(e) {}
}
return url;
Comment on lines +120 to +136
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() only special-cases a few non-network schemes (data:, blob:, javascript:). For fetch/XHR, other schemes like mailto:, tel:, chrome-extension: etc. should also be left untouched; currently they can be treated as "relative" and end up rewritten into /browse/<scheme>:..., which is guaranteed to fail. Consider a generic scheme check (e.g. detect ^[a-zA-Z][a-zA-Z0-9+.-]*: and only proxy http:/https:) to avoid rewriting unsupported schemes.

Copilot uses AI. Check for mistakes.
}

// patch fetch
var origFetch = window.fetch;
window.fetch = function(input, init) {
if (typeof input === 'string' && (input.startsWith('http://') || input.startsWith('https://'))) {
input = PROXY_PREFIX + input;
} else if (input instanceof Request && (input.url.startsWith('http://') || input.url.startsWith('https://')) && !input.url.includes(PROXY_PREFIX)) {
input = new Request(PROXY_PREFIX + input.url, input);
if (typeof input === 'string') {
input = resolveForProxy(input);
} else if (input instanceof Request && !input.url.includes(PROXY_PREFIX)) {
input = new Request(resolveForProxy(input.url), input);
Comment on lines +144 to +145
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.

The Request branch uses !input.url.includes(PROXY_PREFIX) to decide whether to rewrite. This can incorrectly skip proxying for legitimate target URLs that merely contain /browse/ in their path (e.g. https://example.com/browse/assets.js). Prefer checking whether the URL is already proxied by parsing it and verifying the proxy-origin + pathname.startsWith(PROXY_PREFIX) instead of substring matching.

Copilot uses AI. Check for mistakes.
}
return origFetch.call(this, input, init);
};

// patch XHR
var origOpen = XMLHttpRequest.prototype.open;
XMLHttpRequest.prototype.open = function(method, url) {
if (typeof url === 'string' && (url.startsWith('http://') || url.startsWith('https://')) && !url.includes(PROXY_PREFIX)) {
url = PROXY_PREFIX + url;
if (typeof url === 'string') {
url = resolveForProxy(url);
}
return origOpen.apply(this, [method, url, ...Array.prototype.slice.call(arguments, 2)]);
};
Expand Down
Loading