Skip to content

Fix CORS credentials on app-dev reverse-proxy preflight - #8320

Open
trippyogi wants to merge 2 commits into
Shopify:mainfrom
trippyogi:fix/8259-cors-credentials-include
Open

trippyogi wants to merge 2 commits into
Shopify:mainfrom
trippyogi:fix/8259-cors-credentials-include

Conversation

@trippyogi

@trippyogi trippyogi commented Aug 12, 2026 •

Copy link
Copy Markdown

WHY are these changes introduced?

Fixes #8259

After #7164, the app-dev reverse proxy began answering CORS OPTIONS itself instead of forwarding them to the backend. That synthetic 204 included Access-Control-Allow-Origin / methods / headers, but not Access-Control-Allow-Credentials. Credentialed cross-origin requests that require preflight therefore fail before reaching the backend.

WHAT is this pull request doing?

When the preflight request has an Origin, include Access-Control-Allow-Credentials: true on the proxy's OPTIONS response (still reflecting the concrete origin, never pairing credentials with *).

  • Unit coverage for Origin + credentials and no-Origin (credentials absent)
  • Patch changeset for @shopify/app

How to test your changes?

pnpm --filter @shopify/app exec vitest run src/cli/utilities/app/http-reverse-proxy.test.ts

Expect 12/12. The OPTIONS cases assert:

  • with Origin → Access-Control-Allow-Credentials: true
  • without Origin → credentials header absent

Optional manual check: run shopify app dev with a frontend request that actually triggers preflight (e.g. POST + Content-Type: application/json + credentials: include). On main the OPTIONS preflight fails CORS; on this branch it succeeds (backend must still send credentials CORS on the actual response).

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

After Shopify#7164, app-dev answers OPTIONS itself and omitted Access-Control-Allow-Credentials, so credentialed cross-origin fetches failed. When Origin is present, include Allow-Credentials on the preflight response.
@trippyogi
trippyogi marked this pull request as ready for review August 12, 2026 22:20
@trippyogi
trippyogi requested a review from a team as a code owner August 12, 2026 22:20
@Svector-anu

Copy link
Copy Markdown

Thanks for fixing the credentialed-preflight regression from #7164 — flagging a security concern with the current approach before this merges.

Reflecting the request's Origin header and pairing it with Access-Control-Allow-Credentials: true is not safe just because it's not the literal * wildcard. Per the Fetch/CORS spec, browsers only forbid Access-Control-Allow-Origin: * together with credentials — reflecting any origin verbatim (i.e. ACAO: <origin> where <origin> is copied unchecked from the request) is functionally equivalent to * from a security standpoint once credentials are allowed, because every origin satisfies "the response's origin equals the reflected value." Any website an app-dev user's browser visits could send a credentialed cross-origin request to their currently-running local reverse proxy (reachable either at localhost:<port> in --use-localhost mode, or at the dev tunnel URL) and have it succeed the CORS check, since the proxy will always echo back whatever Origin the attacker's page sends.

Concretely: getProxyServerRequestListener's OPTIONS branch (packages/app/src/cli/utilities/app/http-reverse-proxy.ts) has no allowlist — req.headers.origin ?? '*' accepts every origin unconditionally. Adding Access-Control-Allow-Credentials: true on top of that (this PR) turns a previously low-risk "any origin can send simple, non-credentialed requests" proxy into "any origin can send credentialed requests and read the response," for whatever session/auth the target dev server relies on.

Suggested fix: validate Origin against an actual allowlist before echoing it and before setting the credentials header — e.g. the known dev tunnel host, localhost/127.0.0.1 on the app's own dev ports, and/or Shopify admin origins — and fall back to omitting Access-Control-Allow-Origin/-Credentials entirely (not *) when the origin doesn't match. Happy to be wrong here if there's an existing constraint elsewhere in the dev-session setup that already restricts which origins can reach this proxy (e.g. a random per-session tunnel subdomain that's treated as the actual secret) — but that's not enforced in this file today, so I'd rather flag it than assume it.

@trippyogi

Copy link
Copy Markdown
Author

@Svector-anu Thanks for flagging this. You’re right that reflecting an arbitrary Origin while allowing credentials is not safe.

I changed the approach rather than adding a caller-origin allowlist to the CLI. The reverse proxy now forwards OPTIONS to the configured target, so the backend makes the CORS decision for both the preflight and the actual response. The proxy no longer reflects the incoming origin or grants credential access itself.

I verified that the current proxy forwards OPTIONS over both HTTP and HTTPS. The tests cover an authorized credentialed request plus unrelated, null, malformed, wrong-scheme, wrong-port, and no-Origin requests. A browser check also confirmed that the trusted request sends its session cookie while a foreign origin is stopped at preflight before its POST reaches the backend.

Against current main, the focused proxy tests pass 14/14, the full @shopify/app suite passes with 2,833 tests and 2 existing skips, and lint, type-check, build, and formatting all pass.

This does mean the target application must handle preflight requests, but it keeps the CORS policy in the component that actually knows which origins are trusted. Does this address the concern you raised?

@Dylan-GJ

Dylan-GJ commented Sep 18, 2026 •

Copy link
Copy Markdown

@Svector-anu Thanks for flagging this. You’re right that reflecting an arbitrary Origin while allowing credentials is not safe.

I changed the approach rather than adding a caller-origin allowlist to the CLI. The reverse proxy now forwards OPTIONS to the configured target, so the backend makes the CORS decision for both the preflight and the actual response. The proxy no longer reflects the incoming origin or grants credential access itself.

I verified that the current proxy forwards OPTIONS over both HTTP and HTTPS. The tests cover an authorized credentialed request plus unrelated, null, malformed, wrong-scheme, wrong-port, and no-Origin requests. A browser check also confirmed that the trusted request sends its session cookie while a foreign origin is stopped at preflight before its POST reaches the backend.

Against current main, the focused proxy tests pass 14/14, the full @shopify/app suite passes with 2,833 tests and 2 existing skips, and lint, type-check, build, and formatting all pass.

This does mean the target application must handle preflight requests, but it keeps the CORS policy in the component that actually knows which origins are trusted. Does this address the concern you raised?

The only issue I see on my end is that it completely reverses the change from this PR: #7164

Maybe to allow the fix from that previous PR, we could add a new CLI argument like --respond-to-preflight-request or --forward-preflight-request to enable or disable the previous code.

@Dylan-GJ

Dylan-GJ commented Oct 6, 2026

Copy link
Copy Markdown

@trippyogi thanks for working on this and for the careful follow-up on the security concern.

I opened #8803, which takes a slightly different route to fix #8259 and I'd like to explain why.

Forwarding every OPTIONS request, as this PR does, is the right call for the credentials question: the target app is the only one that knows which origins to trust. The downside is that it removes what #7164 added, so backends that don't answer preflights (the cases in #7324 and #5660) would get the CORS error back.

#8803 keeps your forwarding approach and adds a narrow safety net:

  • OPTIONS requests are forwarded to the target, and its response is relayed as is (credentials included).
  • Only when the target answers 4xx/5xx or can't be reached does the proxy respond itself, with the same response as before and never with Access-Control-Allow-Credentials.

Credentialed requests keep working through the target's own CORS config, and servers without an OPTIONS handler keep working as they do today.

Since #8803 supersedes this PR, I suggest closing #8320 once it merges, unless you'd rather rework this one. Your tests were a useful reference for the credentialed and untrusted-origin cases.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CORS error on shopify app dev on the cli reverse proxy with credentials: "include"

3 participants