diff --git a/.changeset/app-dev-proxy-forward-preflight.md b/.changeset/app-dev-proxy-forward-preflight.md new file mode 100644 index 00000000000..7e7b9e907f8 --- /dev/null +++ b/.changeset/app-dev-proxy-forward-preflight.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Forward CORS preflight requests from the app dev proxy to your app, so credentialed requests work again diff --git a/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts b/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts index 7d26a2bf0c8..8ba2b20c2f0 100644 --- a/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts +++ b/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts @@ -8,6 +8,7 @@ import https from 'https' import net from 'net' const each = ['http', 'https'] as const +const authorizedOrigin = 'https://app.example' describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { const test = getTestReverseProxy(protocol) @@ -55,8 +56,25 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { }) }) - test('responds to CORS preflight OPTIONS with default headers', {retry: 2}, async ({setup}) => { + test('forwards the CORS preflight to the target and keeps its credentials decision', {retry: 2}, async ({setup}) => { const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/path1/test`, { + method: 'OPTIONS', + headers: { + Origin: authorizedOrigin, + 'Access-Control-Request-Method': 'POST', + 'Access-Control-Request-Headers': 'Content-Type', + }, + agent, + }) + expect(response.status).toBe(204) + expect(response.headers.get('access-control-allow-origin')).toBe(authorizedOrigin) + expect(response.headers.get('access-control-allow-credentials')).toBe('true') + expect(response.headers.get('access-control-allow-methods')).toBe('POST') + expect(response.headers.get('access-control-allow-headers')).toBe('Content-Type') + }) + + test('responds to the preflight when the target has no OPTIONS handler', {retry: 2}, async ({setup}) => { + const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/path2/test`, { method: 'OPTIONS', headers: { Origin: 'https://extensions.shopifycdn.com', @@ -70,10 +88,46 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { expect(response.headers.get('access-control-allow-methods')).toBe('GET') expect(response.headers.get('access-control-allow-headers')).toBe('Authorization') expect(response.headers.get('access-control-max-age')).toBe('86400') + expect(response.headers.get('access-control-allow-credentials')).toBeNull() }) - test('responds to CORS preflight OPTIONS with defaults when no request headers', {retry: 2}, async ({setup}) => { + test('keeps the target response when it does not authorize the origin', {retry: 2}, async ({setup}) => { const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/path1/test`, { + method: 'OPTIONS', + headers: {Origin: 'https://evil.example', 'Access-Control-Request-Method': 'POST'}, + agent, + }) + expect(response.status).toBe(204) + expect(response.headers.get('access-control-allow-origin')).toBeNull() + expect(response.headers.get('access-control-allow-credentials')).toBeNull() + }) + + for (const status of [404, 403]) { + test(`responds to the preflight when the target answers ${status}`, {retry: 2}, async ({setup}) => { + const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/path2/status-${status}`, { + method: 'OPTIONS', + headers: {Origin: 'https://evil.example', 'Access-Control-Request-Method': 'POST'}, + agent, + }) + expect(response.status).toBe(204) + expect(response.headers.get('access-control-allow-origin')).toBe('https://evil.example') + expect(response.headers.get('access-control-allow-credentials')).toBeNull() + }) + } + + test('responds to the preflight when the target is unreachable', {retry: 2}, async ({setup}) => { + const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/unreachable/test`, { + method: 'OPTIONS', + headers: {Origin: 'https://extensions.shopifycdn.com', 'Access-Control-Request-Method': 'GET'}, + agent, + }) + expect(response.status).toBe(204) + expect(response.headers.get('access-control-allow-origin')).toBe('https://extensions.shopifycdn.com') + expect(response.headers.get('access-control-allow-credentials')).toBeNull() + }) + + test('responds to the preflight with defaults when no request headers', {retry: 2}, async ({setup}) => { + const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/path2/test`, { method: 'OPTIONS', agent, }) @@ -81,6 +135,7 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { expect(response.headers.get('access-control-allow-origin')).toBe('*') expect(response.headers.get('access-control-allow-methods')).toBe('GET, POST, PUT, DELETE, PATCH, OPTIONS') expect(response.headers.get('access-control-allow-headers')).toBe('Content-Type, Authorization') + expect(response.headers.get('access-control-allow-credentials')).toBeNull() }) test('closes the server when aborted', async ({setup}) => { @@ -113,11 +168,31 @@ function getTestReverseProxy(protocol: 'http' | 'https') { // eslint-disable-next-line no-empty-pattern setup: async ({}, use) => { const targetServer1 = http.createServer((req, res) => { + if (req.method === 'OPTIONS') { + const origin = req.headers.origin + // Like a backend with an origin allowlist: only trusted origins get CORS headers. + res.writeHead( + 204, + origin === authorizedOrigin + ? { + 'Access-Control-Allow-Origin': origin, + 'Access-Control-Allow-Credentials': 'true', + 'Access-Control-Allow-Methods': req.headers['access-control-request-method'] ?? '', + 'Access-Control-Allow-Headers': req.headers['access-control-request-headers'] ?? '', + } + : {}, + ) + return res.end() + } res.writeHead(200, {'Content-Type': 'text/plain'}) res.end('Response from target server 1') }) const targetServer2 = http.createServer((req, res) => { + if (req.method === 'OPTIONS') { + res.writeHead(Number(/status-(\d+)/.exec(req.url ?? '')?.[1] ?? 405)) + return res.end() + } res.writeHead(200, {'Content-Type': 'text/plain'}) res.end('Response from target server 2') }) @@ -131,11 +206,18 @@ function getTestReverseProxy(protocol: 'http' | 'https') { const targetPort1 = (targetServer1.address() as net.AddressInfo).port const targetPort2 = (targetServer2.address() as net.AddressInfo).port + // A port that was free a moment ago and has nothing listening on it. + const closedServer = http.createServer() + await new Promise((resolve) => closedServer.listen(0, 'localhost', resolve)) + const closedPort = (closedServer.address() as net.AddressInfo).port + await new Promise((resolve) => closedServer.close(() => resolve())) + const abortController = new AbortController() const {server: proxyServer} = await getProxyingWebServer( { '/path1': `http://localhost:${targetPort1}`, '/path2': `http://localhost:${targetPort2}`, + '/unreachable': `http://localhost:${closedPort}`, default: `http://localhost:${targetPort1}`, }, abortController.signal, diff --git a/packages/app/src/cli/utilities/app/http-reverse-proxy.ts b/packages/app/src/cli/utilities/app/http-reverse-proxy.ts index a0380b05995..13bbe4ce8d3 100644 --- a/packages/app/src/cli/utilities/app/http-reverse-proxy.ts +++ b/packages/app/src/cli/utilities/app/http-reverse-proxy.ts @@ -26,6 +26,7 @@ export async function getProxyingWebServer( // to block the loading of the ESM module graph. const httpProxy = await import('http-proxy-node16') const proxy = httpProxy.default.createProxyServer() + proxy.on('proxyRes', handlePreflightResponse) const requestListener = getProxyServerRequestListener(rules, proxy, stdout) @@ -70,27 +71,14 @@ function getProxyServerRequestListener( return function (req, res) { const target = match(rules, req) if (target) { - // Handle CORS preflight requests directly - // The proxy does not forward OPTIONS reliably, so we respond here - // using the headers requested by the client. - if (req.method === 'OPTIONS') { - res.writeHead(204, { - 'Access-Control-Allow-Origin': req.headers.origin ?? '*', - 'Access-Control-Allow-Methods': - req.headers['access-control-request-method'] ?? 'GET, POST, PUT, DELETE, PATCH, OPTIONS', - 'Access-Control-Allow-Headers': - req.headers['access-control-request-headers'] ?? 'Content-Type, Authorization', - 'Access-Control-Max-Age': '86400', - }) - return res.end() - } - return proxy.web(req, res, {target}, (err) => { + return proxy.web(req, res, {target, selfHandleResponse: isPreflight(req)}, (err) => { useConcurrentOutputContext({outputPrefix: 'proxy', stripAnsi: false}, () => { const lastError = isAggregateError(err) ? err.errors[err.errors.length - 1] : undefined const error = lastError ?? err outputWarn(`Error forwarding web request: ${error.message}`, stdout) outputWarn(`└ Unreachable target "${target}" for path: "${req.url}"`, stdout) }) + if (isPreflight(req) && !res.headersSent) respondToPreflight(req, res) }) } @@ -105,6 +93,63 @@ ${outputToken.json(JSON.stringify(rules))} } } +// Headers that only apply to a single connection and must not be relayed when we write the response ourselves. +const HOP_BY_HOP_HEADERS = new Set([ + 'connection', + 'keep-alive', + 'proxy-authenticate', + 'proxy-authorization', + 'te', + 'trailer', + 'transfer-encoding', + 'upgrade', +]) + +function isPreflight(req: http.IncomingMessage) { + return req.method === 'OPTIONS' +} + +/** + * CORS preflights are answered by the target app, which is the only one that knows which origins to trust + * (including whether to allow credentials). + * + * Dev servers that don't implement OPTIONS (4xx/5xx) or can't be reached get a response from the proxy instead, + * reflecting the request. This also replaces a deliberate rejection from the target, which is acceptable for local + * development. Credentials are never granted here, as that would authorize any origin. + */ +function handlePreflightResponse( + targetResponse: http.IncomingMessage, + req: http.IncomingMessage, + res: http.ServerResponse, +) { + if (!isPreflight(req)) return + + const statusCode = targetResponse.statusCode ?? 500 + if (statusCode >= 400) { + targetResponse.resume() + respondToPreflight(req, res) + return + } + + const headers = Object.fromEntries( + Object.entries(targetResponse.headers).filter(([name]) => !HOP_BY_HOP_HEADERS.has(name)), + ) + res.writeHead(statusCode, headers) + targetResponse.on('error', () => res.destroy()) + targetResponse.pipe(res) +} + +function respondToPreflight(req: http.IncomingMessage, res: http.ServerResponse) { + res.writeHead(204, { + 'Access-Control-Allow-Origin': req.headers.origin ?? '*', + 'Access-Control-Allow-Methods': + req.headers['access-control-request-method'] ?? 'GET, POST, PUT, DELETE, PATCH, OPTIONS', + 'Access-Control-Allow-Headers': req.headers['access-control-request-headers'] ?? 'Content-Type, Authorization', + 'Access-Control-Max-Age': '86400', + }) + res.end() +} + function match(rules: {[key: string]: string}, req: http.IncomingMessage, websocket = false) { const path: string = req.url ?? '/'