Fix CORS preflight handling in the app dev reverse proxy

.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

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,17 +88,54 @@ 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,
     })
     expect(response.status).toBe(204)
     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<void>((resolve) => closedServer.listen(0, 'localhost', resolve))
+      const closedPort = (closedServer.address() as net.AddressInfo).port
+      await new Promise<void>((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,

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 ?? '/'