fix: CORS headers missing on photo static files #208

Merged

forgejo_admin merged 1 commit from 207-fix-cors-headers-missing-on-photo-static into main

2026-03-28 17:06:01 +00:00

forgejo_admin commented

2026-03-28 17:02:00 +00:00

Owner

Summary

StaticFiles mounted inside the lifespan context manager does not go through FastAPI's middleware stack
This caused /uploads/photos/* responses to lack Access-Control-Allow-Origin headers, triggering Chrome ORB blocks when westside-app loaded player photos cross-origin
Moved the mount to module level so CORSMiddleware wraps static file responses

Changes

src/basketball_api/main.py: Moved app.mount("/uploads/photos", StaticFiles(...)) from inside the lifespan async context manager to module level, after CORSMiddleware is registered. Added os.makedirs at module level since StaticFiles requires the directory to exist at mount time. Kept the lifespan os.makedirs as a belt-and-suspenders safeguard.

Test Plan

Tests pass locally (614 passed)
Manual verification: curl -I https://basketball-api.tail5b443a.ts.net/uploads/photos/<any-file> includes access-control-allow-origin header
Verify westside-app coach dashboard loads player photos without Chrome ORB errors
No regressions — full test suite green

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #207
westside-app — the project whose coach dashboard is blocked by missing CORS headers

## Summary - StaticFiles mounted inside the lifespan context manager does not go through FastAPI's middleware stack - This caused `/uploads/photos/*` responses to lack `Access-Control-Allow-Origin` headers, triggering Chrome ORB blocks when westside-app loaded player photos cross-origin - Moved the mount to module level so CORSMiddleware wraps static file responses ## Changes - `src/basketball_api/main.py`: Moved `app.mount("/uploads/photos", StaticFiles(...))` from inside the `lifespan` async context manager to module level, after `CORSMiddleware` is registered. Added `os.makedirs` at module level since `StaticFiles` requires the directory to exist at mount time. Kept the lifespan `os.makedirs` as a belt-and-suspenders safeguard. ## Test Plan - [x] Tests pass locally (614 passed) - [ ] Manual verification: `curl -I https://basketball-api.tail5b443a.ts.net/uploads/photos/<any-file>` includes `access-control-allow-origin` header - [ ] Verify westside-app coach dashboard loads player photos without Chrome ORB errors - [x] No regressions — full test suite green ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #207 - `westside-app` — the project whose coach dashboard is blocked by missing CORS headers

forgejo_admin added 1 commit

2026-03-28 17:02:00 +00:00

fix: move StaticFiles mount to module level so CORS headers apply to photos

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

38c78a7467

StaticFiles mounted inside the lifespan context manager does not inherit
the parent app's CORSMiddleware. This caused /uploads/photos/* responses
to lack Access-Control-Allow-Origin headers, triggering Chrome ORB blocks
when westside-app loaded player photos cross-origin.

Move the mount to module level after CORSMiddleware is registered so the
middleware stack wraps static file responses. The upload directory is now
created both at module level (StaticFiles requires it at mount time) and
in lifespan (belt-and-suspenders for runtime).

Closes #207

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-28 17:02:47 +00:00

Author

Owner

QA Review -- PR #208

Scope Check

1 file changed (src/basketball_api/main.py), 9 additions, 4 deletions
Change is tightly scoped to the issue: move StaticFiles mount out of lifespan so CORS middleware applies
No unrelated changes, no secrets

Correctness

Root cause is correct. FastAPI's ASGI middleware stack (CORSMiddleware) only wraps routes/mounts registered on the app at startup. Mounts added dynamically inside the lifespan context manager are not wrapped by middleware, so responses from /uploads/photos/* lacked Access-Control-Allow-Origin headers.
Fix is correct. Moving app.mount(...) to module level (after app.add_middleware(CORSMiddleware, ...)) ensures the middleware stack wraps static file responses.
os.makedirs(settings.upload_dir, exist_ok=True) at module level is necessary because StaticFiles validates the directory exists at mount time. The duplicate call in lifespan is idempotent (exist_ok=True) and harmless.

Testing

614 tests pass, no regressions
Manual verification items in test plan are appropriate (curl for header, browser check for ORB)

Nits

None. The comment block above the mount is clear and explains the "why."

VERDICT: APPROVE

## QA Review -- PR #208 ### Scope Check - 1 file changed (`src/basketball_api/main.py`), 9 additions, 4 deletions - Change is tightly scoped to the issue: move StaticFiles mount out of lifespan so CORS middleware applies - No unrelated changes, no secrets ### Correctness - **Root cause is correct.** FastAPI's ASGI middleware stack (CORSMiddleware) only wraps routes/mounts registered on the app at startup. Mounts added dynamically inside the `lifespan` context manager are not wrapped by middleware, so responses from `/uploads/photos/*` lacked `Access-Control-Allow-Origin` headers. - **Fix is correct.** Moving `app.mount(...)` to module level (after `app.add_middleware(CORSMiddleware, ...)`) ensures the middleware stack wraps static file responses. - `os.makedirs(settings.upload_dir, exist_ok=True)` at module level is necessary because `StaticFiles` validates the directory exists at mount time. The duplicate call in lifespan is idempotent (`exist_ok=True`) and harmless. ### Testing - 614 tests pass, no regressions - Manual verification items in test plan are appropriate (curl for header, browser check for ORB) ### Nits - None. The comment block above the mount is clear and explains the "why." ### VERDICT: APPROVE

forgejo_admin commented

2026-03-28 17:04:30 +00:00

Author

Owner

PR #208 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic Settings

Analysis of the fix:
The root cause is correct. FastAPI's StaticFiles mounted inside a lifespan async context manager does not pass through the middleware stack -- CORSMiddleware never wraps those responses. Moving app.mount("/uploads/photos", ...) to module level (after CORSMiddleware is registered) is the canonical fix.

The module-level os.makedirs(settings.upload_dir, exist_ok=True) at line 96 is necessary because StaticFiles validates directory existence at mount time. The duplicate call inside lifespan (line 45) serves as a startup safeguard. Both use exist_ok=True, so there is no conflict. The "belt-and-suspenders" rationale documented in the PR body and code comments is sound.

Placement: The mount is placed after all include_router calls and before if __name__, which is correct -- app.mount for static files should come last so it does not shadow API routes.

BLOCKERS

None.

This is a 1-file, 9-addition/4-deletion bugfix that moves existing code from lifespan scope to module scope. No new functionality is introduced -- the static file mount and directory creation existed before. The change is a code relocation, not a feature addition, so the "new functionality with zero test coverage" blocker does not apply.

NITS

No automated CORS verification test. There are zero tests in the suite that assert access-control-allow-origin headers on /uploads/photos/* responses. The PR test plan covers this via manual curl -I, which is acceptable for a bugfix. However, a TestClient test that sends a request with an Origin header and asserts the CORS response header would prevent regression. Worth a follow-up ticket.
Hardcoded CORS origins (lines 54-59 in main.py) are a pre-existing concern, not introduced by this PR. Out of scope, but worth noting for a future cleanup ticket to move them to settings.

SOP COMPLIANCE

Branch named after issue (207-fix-cors-headers-missing-on-photo-static references #207)
PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
Related section references parent issue (Closes #207)
No secrets committed (single file change, clean diff)
No unnecessary file changes (1 file, tightly scoped)
Commit messages are descriptive
Related section references plan slug -- N/A (standalone bugfix, no plan)

PROCESS OBSERVATIONS

Change failure risk: Low. This is a code relocation with a well-understood mechanism. The full test suite (614 tests) passes.
Deployment frequency: Positive. Small, focused fix unblocks westside-app coach dashboard photo loading.
Follow-up recommended: Automated CORS regression test and CORS origins externalization to config. These are separate tickets, not blockers for this merge.

VERDICT: APPROVED

## PR #208 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic Settings **Analysis of the fix:** The root cause is correct. FastAPI's `StaticFiles` mounted inside a `lifespan` async context manager does not pass through the middleware stack -- `CORSMiddleware` never wraps those responses. Moving `app.mount("/uploads/photos", ...)` to module level (after `CORSMiddleware` is registered) is the canonical fix. The module-level `os.makedirs(settings.upload_dir, exist_ok=True)` at line 96 is necessary because `StaticFiles` validates directory existence at mount time. The duplicate call inside `lifespan` (line 45) serves as a startup safeguard. Both use `exist_ok=True`, so there is no conflict. The "belt-and-suspenders" rationale documented in the PR body and code comments is sound. **Placement:** The mount is placed after all `include_router` calls and before `if __name__`, which is correct -- `app.mount` for static files should come last so it does not shadow API routes. ### BLOCKERS None. This is a 1-file, 9-addition/4-deletion bugfix that moves existing code from lifespan scope to module scope. No new functionality is introduced -- the static file mount and directory creation existed before. The change is a code relocation, not a feature addition, so the "new functionality with zero test coverage" blocker does not apply. ### NITS 1. **No automated CORS verification test.** There are zero tests in the suite that assert `access-control-allow-origin` headers on `/uploads/photos/*` responses. The PR test plan covers this via manual `curl -I`, which is acceptable for a bugfix. However, a `TestClient` test that sends a request with an `Origin` header and asserts the CORS response header would prevent regression. Worth a follow-up ticket. 2. **Hardcoded CORS origins** (lines 54-59 in `main.py`) are a pre-existing concern, not introduced by this PR. Out of scope, but worth noting for a future cleanup ticket to move them to `settings`. ### SOP COMPLIANCE - [x] Branch named after issue (`207-fix-cors-headers-missing-on-photo-static` references #207) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related section references parent issue (`Closes #207`) - [x] No secrets committed (single file change, clean diff) - [x] No unnecessary file changes (1 file, tightly scoped) - [x] Commit messages are descriptive - [ ] Related section references plan slug -- N/A (standalone bugfix, no plan) ### PROCESS OBSERVATIONS - **Change failure risk: Low.** This is a code relocation with a well-understood mechanism. The full test suite (614 tests) passes. - **Deployment frequency: Positive.** Small, focused fix unblocks westside-app coach dashboard photo loading. - **Follow-up recommended:** Automated CORS regression test and CORS origins externalization to config. These are separate tickets, not blockers for this merge. ### VERDICT: APPROVED