Skip to content
Open
Show file tree
Hide file tree
Changes from 9 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
134 changes: 132 additions & 2 deletions packages/php-wasm/web-service-worker/src/fetch-with-cors-proxy.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,35 @@
import { afterEach, describe, expect, it, vi } from 'vitest';
import { fetchWithCorsProxy } from './fetch-with-cors-proxy';
import {
afterEach,
beforeAll,
beforeEach,
describe,
expect,
it,
vi,
} from 'vitest';
import { fetchWithCorsProxy, __testing } from './fetch-with-cors-proxy';
import { FirewallInterferenceError } from './firewall-interference-error';

describe('fetchWithCorsProxy', () => {
beforeAll(async () => {
// Pre-warm the one-time ReadableStream body feature detection
// cache so its internal fetch() call doesn't consume mock
// responses set up by individual tests.
const tempMock = vi
.spyOn(globalThis, 'fetch')
.mockResolvedValue(new Response(''));
try {
await fetchWithCorsProxy(
'https://warmup.invalid/',
undefined,
'https://proxy.test/?url='
);
} catch {
// Expected — we only need the detection side-effect.
}
tempMock.mockRestore();
});

afterEach(() => {
vi.restoreAllMocks();
});
Expand Down Expand Up @@ -307,4 +334,107 @@ describe('fetchWithCorsProxy', () => {
expect(await new Response(proxyRequest.body).text()).toBe('form data');
expect(await response.text()).toBe('proxied');
});

describe('non-streaming fallback (Safari)', () => {
beforeEach(() => {
__testing.resetStreamBodySupported();
});

/**
* Helper that sets up a fetch mock whose first call (the
* ReadableStream body feature-detection probe) always rejects,
* forcing the non-streaming/buffering code path.
*/
function mockFetchWithoutStreamingSupport(
...responses: Array<Response | Error>
) {
const mock = vi.spyOn(globalThis, 'fetch');
// Detection probe — reject to simulate Safari.
mock.mockRejectedValueOnce(
new TypeError('ReadableStream uploading is not supported')
);
for (const r of responses) {
if (r instanceof Error) {
mock.mockRejectedValueOnce(r);
} else {
mock.mockResolvedValueOnce(r);
}
}
return mock;
}

it('buffers the POST body as ArrayBuffer for the direct fetch', async () => {
const fetchMock = mockFetchWithoutStreamingSupport(
new Response('ok')
);

const request = new Request('https://example.com/api', {
method: 'POST',
body: 'buffered payload',
});

await fetchWithCorsProxy(
request,
undefined,
'https://proxy.test/?url='
);

// detection + direct fetch
expect(fetchMock).toHaveBeenCalledTimes(2);
const directReq = fetchMock.mock.calls[1][0] as Request;
expect(directReq.url).toBe('https://example.com/api');
expect(await new Response(directReq.body).text()).toBe(
'buffered payload'
);
});

it('preserves the buffered body through to the CORS proxy fallback', async () => {
const corsProxyHeaders = new Headers();
corsProxyHeaders.set('X-Playground-Cors-Proxy', 'true');

const fetchMock = mockFetchWithoutStreamingSupport(
new Error('CORS'),
new Response('proxied', { headers: corsProxyHeaders })
);

const request = new Request('https://example.com/upload', {
method: 'POST',
body: 'safari payload',
});

const response = await fetchWithCorsProxy(
request,
undefined,
'https://proxy.test/?url='
);

// detection + direct + proxy
expect(fetchMock).toHaveBeenCalledTimes(3);
const proxyReq = fetchMock.mock.calls[2][0] as Request;
expect(proxyReq.url).toBe(
'https://proxy.test/?url=https://example.com/upload'
);
expect(await new Response(proxyReq.body).text()).toBe(
'safari payload'
);
expect(await response.text()).toBe('proxied');
});

it('handles GET requests with no body in non-streaming mode', async () => {
const fetchMock = mockFetchWithoutStreamingSupport(
new Response('ok')
);

await fetchWithCorsProxy(
'https://example.com/page',
undefined,
'https://proxy.test/?url='
);

// detection + direct fetch
expect(fetchMock).toHaveBeenCalledTimes(2);
const directReq = fetchMock.mock.calls[1][0] as Request;
expect(directReq.url).toBe('https://example.com/page');
});
});
});
110 changes: 82 additions & 28 deletions packages/php-wasm/web-service-worker/src/fetch-with-cors-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,37 @@ import { FirewallInterferenceError } from './firewall-interference-error';

const CORS_PROXY_HEADER = 'X-Playground-Cors-Proxy';

let streamBodySupported: boolean | undefined;

/** @internal Test-only utilities — not part of the public API. */
export const __testing = {
resetStreamBodySupported(): void {
streamBodySupported = undefined;
},
};

async function supportsReadableStreamBody(): Promise<boolean> {
if (streamBodySupported !== undefined) {
return streamBodySupported;
}
try {
const stream = new ReadableStream({
start(controller) {
controller.close();
},
});
await fetch('data:a/a,', {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason we need to use a meaningless MIME type here? If so, let's note why in a comment. If not, maybe we can remove the type altogether. The data URL spec says it is totally optional, and fetch('data:,') works fine in Firefox, Chrome, and Safari for me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are right. I didn't realise mime type was optional. Good to know. Tested 👍

method: 'POST',
body: stream,
duplex: 'half',
} as RequestInit);
streamBodySupported = true;
} catch {
streamBodySupported = false;
}
return streamBodySupported;
}

export async function fetchWithCorsProxy(
input: RequestInfo,
init?: RequestInit,
Expand Down Expand Up @@ -54,17 +85,44 @@ export async function fetchWithCorsProxy(
return await fetch(requestObject);
}

// Tee the request to avoid consuming the request body stream on the initial
// fetch() so that we can retry through the cors proxy.
const [directRequest, corsProxyRequest] = await teeRequest(requestObject);
const useStreaming = await supportsReadableStreamBody();

let directRequest: Request;
let corsProxyBody: ArrayBuffer | ReadableStream<Uint8Array> | null;

if (useStreaming) {
const [teedDirect, teedProxy] = await teeRequest(requestObject);
directRequest = teedDirect;
corsProxyBody = teedProxy.body;
} else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason we shouldn't move this if/else into the teeRequest() utility?

If we solve this problem in the utility function, it will be solved wherever teeRequest() is used. teeRequest() is not currently used elsewhere in this repo, but I think the workaround belongs there, unless there is a reason not to move it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Very good point! I can refactor it in a follow up PR so that the fix is not still waiting to land. Sounds good?

/**
* Buffer the request body so it can be reused across the direct
* fetch attempt and the CORS proxy fallback. Safari does not
* support ReadableStream as a fetch() request body
* ("ReadableStream uploading is not supported").
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wherever we note this, let's:

This way we can feel comfortable removing the workaround when it is no longer needed.

*/
let bufferedBody: ArrayBuffer | null = null;
if (requestObject.body) {
bufferedBody = await new Response(requestObject.body).arrayBuffer();
}
if (bufferedBody !== null) {
directRequest = await cloneRequest(requestObject, {
body: bufferedBody,
});
} else {
// No body to buffer; reuse the original request without overriding `body`.
directRequest = requestObject;
}
corsProxyBody = bufferedBody;
}

try {
return await fetch(directRequest);
} catch {
// If the developer has explicitly allowed the request to pass the
// credentials headers with the X-Cors-Proxy-Allowed-Request-Headers header,
// then let's include those credentials in the fetch() request.
const headers = new Headers(corsProxyRequest.headers);
const headers = new Headers(directRequest.headers);
const corsProxyAllowedHeaders =
headers.get('x-cors-proxy-allowed-request-headers')?.split(',') ||
[];
Expand All @@ -87,38 +145,34 @@ export async function fetchWithCorsProxy(
}

/**
* Buffer the cors proxy request body into an ArrayBuffer if talking to a `http://` URL.
*
* Streaming request bodies don't work with the local dev server, which uses http://
* as a protocol. However, with a streamed request body, Chrome silently upgrades a
* HTTP/1.1 request to HTTP/2. However, our HTTP/1.1-only local dev server still replies
* with a HTTP/1.1 response. Chrome then treats the request as failed with an
* ERR_ALPN_NEGOTIATION_FAILED error.
* When using streaming bodies, buffer into an ArrayBuffer if the
* CORS proxy uses `http:`. Streaming request bodies cause Chrome
* to silently upgrade HTTP/1.1 to HTTP/2, but an HTTP/1.1-only
* server replies with HTTP/1.1, triggering ERR_ALPN_NEGOTIATION_FAILED.
*
* Inferring the HTTP version from the URL protocol is unreliable and will fail
* if the CORS proxy is hosted on a `https://` URL that speaks HTTP < 2. This is
* a recognized limitation of the CORS proxy feature. If you host it on an `https://` URL,
* make sure to use HTTP/2.
*
* See: https://developer.chrome.com/docs/capabilities/web-apis/fetch-streaming-requests
* @see https://developer.chrome.com/docs/capabilities/web-apis/fetch-streaming-requests
*/

// In development, corsProxyUrl may be /cors-proxy/. We need to resolve the absolute URL
// to access the protocol.
const rootUrl = new URL(import.meta.url);
rootUrl.pathname = '';
rootUrl.search = '';
rootUrl.hash = '';
const corsProxyUrlObj = new URL(corsProxyUrl, rootUrl.toString());

let body: ArrayBuffer | ReadableStream<Uint8Array> | null =
corsProxyRequest.body;
if (body && new URL(corsProxyUrlObj).protocol === 'http:') {
body = await new Response(body).arrayBuffer();
let body = corsProxyBody;
if (useStreaming && body) {
// In development, corsProxyUrl may be /cors-proxy/. We need to resolve the absolute URL
// to access the protocol.
const rootUrl = new URL(import.meta.url);
rootUrl.pathname = '';
rootUrl.search = '';
rootUrl.hash = '';
const corsProxyUrlObj = new URL(corsProxyUrl, rootUrl.toString());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to clear all the fields on the rootUrl? Wouldn't the URL() constructor be able to make sense of the root URL on its own?

For example, in the browser dev tools console, new URL('a', new URL('http://happycode.net/?stuff#huzzah')).href evaluates to 'http://happycode.net/a'.

I think this can be as simple as:

if (
	useStreaming &&
	body &&
	new URL(corsProxyUrlObj, import.meta.url).protocol === 'http:'
) {

I know the rootUrl changes are pre-existing code that was moved. But I think we can either simplify while we're moving around what I think is unnecessary code, or maybe we should just leave the code alone except for adding useStreaming to the original if.

There may be something I'm missing, but that is my read here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, seems alright to do. I have made the change.

if (corsProxyUrlObj.protocol === 'http:') {
body = await new Response(body).arrayBuffer();
}
}

const newRequest = await cloneRequest(corsProxyRequest, {
url: `${corsProxyUrl}${requestObject.url}`,
const newRequest = await cloneRequest(directRequest, {
url: `${corsProxyUrl}${directRequest.url}`,
headers,
body,
...(requestIntendsToPassCredentials && { credentials: 'include' }),
Expand All @@ -132,7 +186,7 @@ export async function fetchWithCorsProxy(
// came from a network firewall rather than the actual CORS proxy.
if (!response.headers.has(CORS_PROXY_HEADER)) {
throw new FirewallInterferenceError(
requestObject.url,
directRequest.url,
response.status,
response.statusText
);
Expand Down
Loading