fix(api): disable credentials when CORS origin is wildcard#98
Open
0xghost42 wants to merge 1 commit into
Open
Conversation
create_app() emitted Access-Control-Allow-Origin: * together with Access-Control-Allow-Credentials: true whenever ALLOWED_ORIGINS was unset (the documented development default). Per the CORS specification and every modern browser, the wildcard origin is invalid when credentials are allowed — the preflight fails with 'Credential is not supported if the CORS header Access-Control-Allow-Origin is *', so any cross-origin caller carrying cookies or an Authorization header could never reach the API. Resolve the env var first, then decide: - explicit ALLOWED_ORIGINS -> keep allow_credentials=True - unset / empty / whitespace -> use ['*'] with allow_credentials=False Cross-origin callers that need credentialed CORS must now set ALLOWED_ORIGINS to a concrete origin list, which is the only spec-valid configuration for that combination anyway. Same-origin and no-cookie cross-origin callers see no behavior change. Regression coverage in tests/integration/test_cors_config.py: asserts the unset / empty / explicit cases produce the expected (allow_origins, allow_credentials) tuple on the registered CORSMiddleware entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
create_app()registered the CORS middleware withallow_origins=['*']together withallow_credentials=TruewheneverALLOWED_ORIGINSwas unset — the documented "Allow all in development" default atsrc/roma_dspy/api/main.py:155-169.That combination is invalid per the CORS specification and is rejected by every modern browser:
Starlette / FastAPI's
CORSMiddlewareemits both headers as-configured, so the browser preflight fails with:Net effect: any cross-origin browser caller that needs cookies or an
Authorizationheader could not reach the API on a default ROMA install.Fix
Resolve
ALLOWED_ORIGINSfirst, then decide both fields together:ALLOWED_ORIGINS(one or more origins) → keepallow_credentials=True.['*']withallow_credentials=False.Cross-origin callers that need credentialed CORS now have to set
ALLOWED_ORIGINSto a concrete origin list, which is the only spec-valid shape for that combination anyway. Same-origin callers and no-cookie cross-origin callers see no behavior change.A comment above the new block explains the spec constraint so the next reader does not re-introduce the wildcard-plus-credentials shape.
Real behavior proof
Authorizationheaders could not preflight against a default ROMA install — browsers rejectAccess-Control-Allow-Origin: *wheneverAccess-Control-Allow-Credentials: trueis set, exactly the combinationcreate_app()emitted by default.create_app(). The middleware kwargs are inspected directly offapp.user_middleware(no Starlette monkey-patching, no mocked Pydantic settings), so the test verifies what the real registeredCORSMiddlewareinstance will send on the wire.tests/integration/test_cors_config.py; it exercises three realcreate_app()calls under threeALLOWED_ORIGINSenv states and asserts the registered middleware's(allow_origins, allow_credentials)tuple for each.ALLOWED_ORIGINSunset the middleware now resolves toallow_origins=['*'], allow_credentials=False(spec-valid wildcard); withALLOWED_ORIGINS="https://app.example.com, https://admin.example.com"it resolves to those two origins withallow_credentials=True(spec-valid credentialed CORS); with a whitespace-only env value (" , ") it falls back to the wildcard-without-credentials shape.ROMAinstall no longer trips theAccess-Control-Allow-Credentials/Access-Control-Allow-Originmismatch — the response is now spec-valid in both the dev (wildcard, no credentials) and prod (allowlist, credentials) shapes. Operators that previously had to setALLOWED_ORIGINSpurely to make CORS work get the wildcard back; operators that intentionally configured an allowlist see no behavior change.OPTIONSrequest, because the spec-violation lives in the response-header tuple the middleware will emit, not in any runtime branch.Notes
This is a behavior change for one narrow case: callers that today rely on the default
ALLOWED_ORIGINS=unsetto send credentials cross-origin. That case was already broken on the wire (browsers refused the preflight), so the fix surfaces the requirement to setALLOWED_ORIGINSrather than silently letting the misconfiguration ship. Same-origin callers and no-cookie cross-origin callers are unaffected.