ldraney/pal-e-api
Archived
1
0
Fork 0
Code Issues 15 Pull requests 1 Projects Releases Packages Wiki Activity Actions

fix: add CORS middleware for cross-origin browser requests #257

Closed
forgejo_admin wants to merge 1 commit from 256-cors-middleware into main
pull from: 256-cors-middleware
merge into: ldraney:main
Conversation 2 Commits 1 Files changed 3 +83
forgejo_admin commented 2026-04-12 15:41:04 +00:00
Contributor
Copy link

Summary

The API had zero CORS headers, causing every cross-origin fetch from pal-e-production.tail5b443a.ts.net to be blocked by the browser's same-origin policy. This adds CORSMiddleware to FastAPI before router includes, with env-var-driven origins.

Changes

  • src/pal_e_docs/config.py -- added cors_origins setting (env var PALDOCS_CORS_ORIGINS, comma-separated string, defaults to https://pal-e-production.tail5b443a.ts.net,http://localhost:5173)
  • src/pal_e_docs/main.py -- added CORSMiddleware with allow_credentials=True, all standard methods, wildcard headers, parsed from settings.cors_origins
  • tests/test_cors.py -- 7 tests covering allowed/disallowed origins, preflight OPTIONS, credentials header, and no-origin passthrough

Design notes

  • allow_credentials=True is set because the frontend attaches a Keycloak Bearer token via Authorization header (confirmed in src/lib/api-client.ts). The frontend does NOT use credentials: 'include' on fetch, but allow_credentials is required for the browser to expose response headers when an Authorization header is present.
  • The env var follows the existing PALDOCS_ prefix convention (not PAL_E_DOCS_ as suggested in the issue), consistent with all other settings in config.py.
  • Non-browser callers (curl, SDK) are unaffected -- CORS headers only appear when an Origin header is present in the request.

Test Plan

  • 692 tests pass (685 existing + 7 new CORS tests)
  • ruff format and ruff check clean
  • Deploy via existing Woodpecker CI pipeline, then verify curl -D- -H "Origin: https://pal-e-production.tail5b443a.ts.net" https://pal-e-docs.tail5b443a.ts.net/healthz returns access-control-allow-origin header
  • Verify https://pal-e-production.tail5b443a.ts.net/notes/arch-westside-emails renders without CORS errors

Review Checklist

  • ruff format and ruff check pass
  • All 692 tests pass (685 existing + 7 new)
  • No secrets or credentials in diff
  • Env var follows existing PALDOCS_ prefix convention
  • Middleware added before router includes per FastAPI best practice
  • Disallowed origins confirmed to receive no CORS headers

Related Notes

  • Downstream: forgejo_admin/pal-e-platform#278 (hostname swap will need PALDOCS_CORS_ORIGINS update)

Related

Closes #256

## Summary The API had zero CORS headers, causing every cross-origin fetch from `pal-e-production.tail5b443a.ts.net` to be blocked by the browser's same-origin policy. This adds `CORSMiddleware` to FastAPI before router includes, with env-var-driven origins. ## Changes - `src/pal_e_docs/config.py` -- added `cors_origins` setting (env var `PALDOCS_CORS_ORIGINS`, comma-separated string, defaults to `https://pal-e-production.tail5b443a.ts.net,http://localhost:5173`) - `src/pal_e_docs/main.py` -- added `CORSMiddleware` with `allow_credentials=True`, all standard methods, wildcard headers, parsed from `settings.cors_origins` - `tests/test_cors.py` -- 7 tests covering allowed/disallowed origins, preflight OPTIONS, credentials header, and no-origin passthrough ## Design notes - `allow_credentials=True` is set because the frontend attaches a Keycloak Bearer token via `Authorization` header (confirmed in `src/lib/api-client.ts`). The frontend does NOT use `credentials: 'include'` on fetch, but `allow_credentials` is required for the browser to expose response headers when an `Authorization` header is present. - The env var follows the existing `PALDOCS_` prefix convention (not `PAL_E_DOCS_` as suggested in the issue), consistent with all other settings in `config.py`. - Non-browser callers (curl, SDK) are unaffected -- CORS headers only appear when an `Origin` header is present in the request. ## Test Plan - 692 tests pass (685 existing + 7 new CORS tests) - `ruff format` and `ruff check` clean - Deploy via existing Woodpecker CI pipeline, then verify `curl -D- -H "Origin: https://pal-e-production.tail5b443a.ts.net" https://pal-e-docs.tail5b443a.ts.net/healthz` returns `access-control-allow-origin` header - Verify https://pal-e-production.tail5b443a.ts.net/notes/arch-westside-emails renders without CORS errors ## Review Checklist - [x] `ruff format` and `ruff check` pass - [x] All 692 tests pass (685 existing + 7 new) - [x] No secrets or credentials in diff - [x] Env var follows existing `PALDOCS_` prefix convention - [x] Middleware added before router includes per FastAPI best practice - [x] Disallowed origins confirmed to receive no CORS headers ## Related Notes - Downstream: `forgejo_admin/pal-e-platform#278` (hostname swap will need `PALDOCS_CORS_ORIGINS` update) ## Related Closes #256 
The API had zero CORS headers, causing every cross-origin fetch from
pal-e-production.tail5b443a.ts.net to be blocked by same-origin policy.
Adds CORSMiddleware before router includes with env-var-driven origins
(PALDOCS_CORS_ORIGINS, comma-separated).

Closes #256

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-04-12 15:42:00 +00:00
Author
Contributor
Copy link

QA Review -- PR #257

Acceptance Criteria Check (from #256)

  • CORSMiddleware added to main.py before router includes -- line 52, before include_router calls at line 60+
  • allow_origins configurable via env var -- PALDOCS_CORS_ORIGINS (comma-separated), follows existing PALDOCS_ prefix convention
  • Default origin list includes https://pal-e-production.tail5b443a.ts.net and http://localhost:5173
  • allow_methods includes GET, POST, PATCH, PUT, DELETE, OPTIONS
  • allow_headers="*"
  • allow_credentials=True -- confirmed frontend uses Bearer token via Authorization header (not credentials: 'include')
  • New test file tests/test_cors.py with 7 tests covering: allowed origins, disallowed origins, preflight OPTIONS, all methods, credentials header, no-origin passthrough
  • No regression -- 692 tests pass (685 existing + 7 new)
  • ruff format and ruff check clean
  • No secrets or credentials in diff
  • No Tofu/kustomize changes needed -- deploys via existing CI pipeline

Code Quality

  • Middleware placement is correct (after FastAPI() instantiation, before router includes)
  • Origin parsing handles whitespace and empty strings via list comprehension with strip() and if o.strip()
  • Config uses str type with comma-split at usage site -- simple and appropriate for a flat env var
  • Tests use the shared conftest.py fixtures properly and cover both positive and negative cases
  • Test file defines its own client fixture (duplicates conftest.py) -- minor but acceptable since it keeps the test self-contained

Nits

None.

VERDICT: APPROVED

## QA Review -- PR #257 ### Acceptance Criteria Check (from #256) - [x] `CORSMiddleware` added to `main.py` before router includes -- line 52, before `include_router` calls at line 60+ - [x] `allow_origins` configurable via env var -- `PALDOCS_CORS_ORIGINS` (comma-separated), follows existing `PALDOCS_` prefix convention - [x] Default origin list includes `https://pal-e-production.tail5b443a.ts.net` and `http://localhost:5173` - [x] `allow_methods` includes GET, POST, PATCH, PUT, DELETE, OPTIONS - [x] `allow_headers="*"` - [x] `allow_credentials=True` -- confirmed frontend uses Bearer token via Authorization header (not `credentials: 'include'`) - [x] New test file `tests/test_cors.py` with 7 tests covering: allowed origins, disallowed origins, preflight OPTIONS, all methods, credentials header, no-origin passthrough - [x] No regression -- 692 tests pass (685 existing + 7 new) - [x] `ruff format` and `ruff check` clean - [x] No secrets or credentials in diff - [x] No Tofu/kustomize changes needed -- deploys via existing CI pipeline ### Code Quality - Middleware placement is correct (after `FastAPI()` instantiation, before router includes) - Origin parsing handles whitespace and empty strings via list comprehension with `strip()` and `if o.strip()` - Config uses `str` type with comma-split at usage site -- simple and appropriate for a flat env var - Tests use the shared `conftest.py` fixtures properly and cover both positive and negative cases - Test file defines its own `client` fixture (duplicates `conftest.py`) -- minor but acceptable since it keeps the test self-contained ### Nits None. ### VERDICT: APPROVED
forgejo_admin commented 2026-04-12 15:43:21 +00:00
Author
Contributor
Copy link

PR #257 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / Pydantic Settings / CORSMiddleware

config.py -- cors_origins field added as a comma-separated string with sensible defaults (https://pal-e-production.tail5b443a.ts.net,http://localhost:5173). The env var resolves to PALDOCS_CORS_ORIGINS via the existing env_prefix = "PALDOCS_" convention. This is correct -- the PR body explains the deviation from the issue's suggested PAL_E_DOCS_CORS_ORIGINS name, and the existing config confirms PALDOCS_ is the established prefix. Good call.

main.py -- CORSMiddleware is added BEFORE all app.include_router() calls, which is the correct FastAPI pattern (middleware must wrap routes to intercept preflight OPTIONS before routing). The origin parsing ([o.strip() for o in settings.cors_origins.split(",") if o.strip()]) correctly handles whitespace and empty segments.

CORS configuration specifics:

  • allow_credentials=True -- PR body documents the justification (Keycloak Bearer token via Authorization header). Acceptable.
  • allow_methods=["GET", "POST", "PATCH", "PUT", "DELETE", "OPTIONS"] -- Covers all standard REST methods. Correct.
  • allow_headers=["*"] -- Wildcard headers is standard for APIs that accept Authorization headers. Acceptable.
  • No wildcard * in origins -- good, explicit allowlist only.

OWASP CORS check: Origins are allowlisted (not wildcarded), credentials are scoped to known origins, no reflected-origin pattern. Clean.

test_cors.py -- 7 tests covering:

  1. Allowed production origin returns correct header
  2. Allowed localhost origin returns correct header
  3. Disallowed origin gets NO CORS headers (not just a different value -- absent entirely)
  4. Preflight OPTIONS returns allow-origin
  5. Preflight includes all configured methods
  6. Credentials header present for allowed origin
  7. No-origin request (non-browser) gets no CORS headers

Test coverage is thorough. Both happy path and negative cases are covered. The disallowed-origin test correctly asserts header absence, not just value mismatch.

BLOCKERS

None.

NITS

  1. Test isolation: The test file imports app directly from main.py, which means it uses the module-level settings singleton with default cors_origins. This works because the defaults match what the tests assert against, but if someone changes the default in config.py, all 7 tests break silently. Consider a comment in the test file noting this coupling, or using monkeypatch on the settings object. Non-blocking -- the current approach is simple and correct for the current state.

  2. Preflight max-age: No max_age parameter is set on the middleware, so browsers will re-send preflight requests on every cross-origin call. Setting max_age=600 (10 minutes) would reduce preflight traffic. Minor optimization, not a correctness issue.

SOP COMPLIANCE

  • Branch named after issue: 256-cors-middleware follows {issue-number}-{kebab-case-purpose}
  • PR body follows template: Summary, Changes, Design notes, Test Plan, Review Checklist, Related
  • Related references issue: "Closes #256" present
  • No secrets committed: diff contains no credentials, tokens, or .env files
  • Scope is tight: exactly 3 files touched (config.py, main.py, test_cors.py), 83 additions, 0 deletions
  • No unrelated changes: models.py untouched, no scope creep
  • Test plan includes both automated (692 tests) and manual verification steps

PROCESS OBSERVATIONS

  • Clean single-purpose PR. Low change failure risk -- middleware is additive (no existing behavior modified).
  • Downstream dependency noted in PR body (platform #278 for hostname swap). Good traceability.
  • 7 new tests for 14 lines of production code is excellent coverage density.

VERDICT: APPROVED

## PR #257 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / Pydantic Settings / CORSMiddleware **config.py** -- `cors_origins` field added as a comma-separated string with sensible defaults (`https://pal-e-production.tail5b443a.ts.net,http://localhost:5173`). The env var resolves to `PALDOCS_CORS_ORIGINS` via the existing `env_prefix = "PALDOCS_"` convention. This is correct -- the PR body explains the deviation from the issue's suggested `PAL_E_DOCS_CORS_ORIGINS` name, and the existing config confirms `PALDOCS_` is the established prefix. Good call. **main.py** -- CORSMiddleware is added BEFORE all `app.include_router()` calls, which is the correct FastAPI pattern (middleware must wrap routes to intercept preflight OPTIONS before routing). The origin parsing (`[o.strip() for o in settings.cors_origins.split(",") if o.strip()]`) correctly handles whitespace and empty segments. **CORS configuration specifics**: - `allow_credentials=True` -- PR body documents the justification (Keycloak Bearer token via Authorization header). Acceptable. - `allow_methods=["GET", "POST", "PATCH", "PUT", "DELETE", "OPTIONS"]` -- Covers all standard REST methods. Correct. - `allow_headers=["*"]` -- Wildcard headers is standard for APIs that accept Authorization headers. Acceptable. - No wildcard `*` in origins -- good, explicit allowlist only. **OWASP CORS check**: Origins are allowlisted (not wildcarded), credentials are scoped to known origins, no reflected-origin pattern. Clean. **test_cors.py** -- 7 tests covering: 1. Allowed production origin returns correct header 2. Allowed localhost origin returns correct header 3. Disallowed origin gets NO CORS headers (not just a different value -- absent entirely) 4. Preflight OPTIONS returns allow-origin 5. Preflight includes all configured methods 6. Credentials header present for allowed origin 7. No-origin request (non-browser) gets no CORS headers Test coverage is thorough. Both happy path and negative cases are covered. The disallowed-origin test correctly asserts header absence, not just value mismatch. ### BLOCKERS None. ### NITS 1. **Test isolation**: The test file imports `app` directly from `main.py`, which means it uses the module-level `settings` singleton with default `cors_origins`. This works because the defaults match what the tests assert against, but if someone changes the default in config.py, all 7 tests break silently. Consider a comment in the test file noting this coupling, or using `monkeypatch` on the settings object. Non-blocking -- the current approach is simple and correct for the current state. 2. **Preflight max-age**: No `max_age` parameter is set on the middleware, so browsers will re-send preflight requests on every cross-origin call. Setting `max_age=600` (10 minutes) would reduce preflight traffic. Minor optimization, not a correctness issue. ### SOP COMPLIANCE - [x] Branch named after issue: `256-cors-middleware` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Design notes, Test Plan, Review Checklist, Related - [x] Related references issue: "Closes #256" present - [x] No secrets committed: diff contains no credentials, tokens, or .env files - [x] Scope is tight: exactly 3 files touched (config.py, main.py, test_cors.py), 83 additions, 0 deletions - [x] No unrelated changes: models.py untouched, no scope creep - [x] Test plan includes both automated (692 tests) and manual verification steps ### PROCESS OBSERVATIONS - Clean single-purpose PR. Low change failure risk -- middleware is additive (no existing behavior modified). - Downstream dependency noted in PR body (platform #278 for hostname swap). Good traceability. - 7 new tests for 14 lines of production code is excellent coverage density. ### VERDICT: APPROVED
forgejo_admin closed this pull request 2026-04-12 16:52:21 +00:00
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ldraney/pal-e-api!257
No description provided.