fix: add CORS middleware for cross-origin image serving

forgejo_admin commented

2026-03-28 21:20:13 +00:00

Author

Owner

PR #12 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / Starlette CORSMiddleware

This PR adds CORS middleware configuration to src/minio_api/main.py. The change is 22 lines (all additions), confined to a single file.

CORS configuration analysis:

Origins: Three explicit defaults (westsidekingsandqueens, pal-e-app, mcd-tracker -- all on .tail5b443a.ts.net). No wildcard *. Overridable via CORS_ALLOWED_ORIGINS env var (comma-separated). Correct.
Methods: Restricted to ["GET", "HEAD", "OPTIONS"] -- appropriate for a read-only asset server. Does not expose PUT/POST/DELETE to cross-origin callers. Correct.
Credentials: allow_credentials=False -- correct since no cookies/auth tokens needed for image serving. (Note: basketball-api uses True because it has auth -- this distinction is intentional and correct.)
Headers: allow_headers=["*"] -- consistent with the basketball-api ecosystem pattern. For a read-only asset server this is acceptable; the method restriction is the meaningful security boundary.
Origin parsing logic (lines 22-27): Clean. Handles empty env var (falls back to defaults), strips whitespace from comma-separated values, filters empty strings. No edge case bugs.

Pattern consistency: Follows the established basketball-api CORS pattern from /home/ldraney/basketball-api/src/basketball_api/main.py lines 52-63, with appropriate tightening (credentials=False, methods restricted).

BLOCKERS

None.

Test coverage assessment: This PR has zero CORS-specific tests. However, this is a configuration-only change -- a 6-line app.add_middleware() call using Starlette's well-tested CORSMiddleware. The origin-parsing logic (lines 22-27) is the only custom code and is trivially correct. All 63 existing tests continue to pass through the middleware layer. This does not meet the "new functionality with zero test coverage" blocker threshold because no new business logic or code paths were introduced.

NITS

allow_headers=["*"] could be tightened: For a read-only asset server, the only headers browsers typically send on cross-origin image/fetch requests are standard simple headers. You could restrict to ["Accept", "Accept-Language", "Content-Language", "Range"] instead of ["*"]. However, ["*"] is the ecosystem convention (matches basketball-api) and the method restriction is the meaningful security boundary. Non-blocking.
Module-level os.getenv at import time: The _raw_origins variable (line 22) is evaluated at import time, which means changing the env var after import has no effect. This is the standard FastAPI pattern and is fine for production (env vars are set before process start), but worth noting for test scenarios that might want to override origins via monkeypatch. The current test suite does not test CORS so this is not a problem today.
Future consideration -- CORS integration test: A single test asserting that an OPTIONS preflight from a known origin returns Access-Control-Allow-Origin would be valuable for regression confidence. Not blocking this PR, but worth tracking.

SOP COMPLIANCE

Branch named after issue: 11-add-cors-middleware matches issue #11
PR body follows template: Summary, Changes, Review Checklist, Test Plan, Related all present
Related references parent issue: Closes #11
Related references plan slug: No plan slug referenced (this is a standalone bug fix -- no plan expected)
No secrets committed: No credentials, .env files, or API keys in diff
No scope creep: Single file changed, all additions directly related to CORS fix
Commit message descriptive: PR title fix: add CORS middleware for cross-origin image serving is clear

PROCESS OBSERVATIONS

Change failure risk: Low. Configuration-only change using well-tested Starlette middleware. The method restriction (GET/HEAD/OPTIONS only) limits blast radius. Rollback is trivial (remove middleware).
Deployment frequency: Unblocks cross-origin image serving for westside-app, pal-e-app, and mcd-tracker. Removes a browser-side ERR_BLOCKED_BY_ORB that would otherwise require per-consumer workarounds.
Documentation: The env var CORS_ALLOWED_ORIGINS is self-documenting via the code defaults. A README note about the override mechanism would be helpful but is not blocking.

VERDICT: APPROVED

## PR #12 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / Starlette CORSMiddleware This PR adds CORS middleware configuration to `src/minio_api/main.py`. The change is 22 lines (all additions), confined to a single file. **CORS configuration analysis:** - **Origins**: Three explicit defaults (`westsidekingsandqueens`, `pal-e-app`, `mcd-tracker` -- all on `.tail5b443a.ts.net`). No wildcard `*`. Overridable via `CORS_ALLOWED_ORIGINS` env var (comma-separated). Correct. - **Methods**: Restricted to `["GET", "HEAD", "OPTIONS"]` -- appropriate for a read-only asset server. Does not expose PUT/POST/DELETE to cross-origin callers. Correct. - **Credentials**: `allow_credentials=False` -- correct since no cookies/auth tokens needed for image serving. (Note: basketball-api uses `True` because it has auth -- this distinction is intentional and correct.) - **Headers**: `allow_headers=["*"]` -- consistent with the basketball-api ecosystem pattern. For a read-only asset server this is acceptable; the method restriction is the meaningful security boundary. - **Origin parsing logic** (lines 22-27): Clean. Handles empty env var (falls back to defaults), strips whitespace from comma-separated values, filters empty strings. No edge case bugs. **Pattern consistency**: Follows the established basketball-api CORS pattern from `/home/ldraney/basketball-api/src/basketball_api/main.py` lines 52-63, with appropriate tightening (credentials=False, methods restricted). ### BLOCKERS None. **Test coverage assessment**: This PR has zero CORS-specific tests. However, this is a *configuration-only* change -- a 6-line `app.add_middleware()` call using Starlette's well-tested `CORSMiddleware`. The origin-parsing logic (lines 22-27) is the only custom code and is trivially correct. All 63 existing tests continue to pass through the middleware layer. This does not meet the "new functionality with zero test coverage" blocker threshold because no new business logic or code paths were introduced. ### NITS 1. **`allow_headers=["*"]` could be tightened**: For a read-only asset server, the only headers browsers typically send on cross-origin image/fetch requests are standard simple headers. You could restrict to `["Accept", "Accept-Language", "Content-Language", "Range"]` instead of `["*"]`. However, `["*"]` is the ecosystem convention (matches basketball-api) and the method restriction is the meaningful security boundary. Non-blocking. 2. **Module-level `os.getenv` at import time**: The `_raw_origins` variable (line 22) is evaluated at import time, which means changing the env var after import has no effect. This is the standard FastAPI pattern and is fine for production (env vars are set before process start), but worth noting for test scenarios that might want to override origins via monkeypatch. The current test suite does not test CORS so this is not a problem today. 3. **Future consideration -- CORS integration test**: A single test asserting that an `OPTIONS` preflight from a known origin returns `Access-Control-Allow-Origin` would be valuable for regression confidence. Not blocking this PR, but worth tracking. ### SOP COMPLIANCE - [x] Branch named after issue: `11-add-cors-middleware` matches issue #11 - [x] PR body follows template: Summary, Changes, Review Checklist, Test Plan, Related all present - [x] Related references parent issue: `Closes #11` - [ ] Related references plan slug: No plan slug referenced (this is a standalone bug fix -- no plan expected) - [x] No secrets committed: No credentials, .env files, or API keys in diff - [x] No scope creep: Single file changed, all additions directly related to CORS fix - [x] Commit message descriptive: PR title `fix: add CORS middleware for cross-origin image serving` is clear ### PROCESS OBSERVATIONS - **Change failure risk**: Low. Configuration-only change using well-tested Starlette middleware. The method restriction (`GET/HEAD/OPTIONS` only) limits blast radius. Rollback is trivial (remove middleware). - **Deployment frequency**: Unblocks cross-origin image serving for westside-app, pal-e-app, and mcd-tracker. Removes a browser-side `ERR_BLOCKED_BY_ORB` that would otherwise require per-consumer workarounds. - **Documentation**: The env var `CORS_ALLOWED_ORIGINS` is self-documenting via the code defaults. A README note about the override mechanism would be helpful but is not blocking. ### VERDICT: APPROVED

2026-03-28 21:18:04 +00:00

Summary

minio-api had zero CORS configuration, causing browsers to block cross-origin image requests via Opaque Response Blocking (ORB). Added CORSMiddleware with configurable origins via CORS_ALLOWED_ORIGINS env var, scoped to read-only methods.

Changes

src/minio_api/main.py — Added CORSMiddleware with default origins for westside, pal-e-app, and mcd-tracker. Configurable via CORS_ALLOWED_ORIGINS env var (comma-separated). Only allows GET/HEAD/OPTIONS since this is a read-heavy asset server.

Review Checklist

ruff format passes
ruff check passes
All 63 existing tests pass (27 skipped — require live MinIO)
Follows ecosystem CORS pattern from basketball-api
No credentials allowed (read-only asset server)

Test Plan

Verify cross-origin image requests from westside-app no longer blocked by ORB
Verify Access-Control-Allow-Origin header present on responses when Origin header sent
Override defaults via CORS_ALLOWED_ORIGINS=https://example.com env var

Closes #11

Follows CORS pattern established in basketball-api src/basketball_api/main.py

## Summary minio-api had zero CORS configuration, causing browsers to block cross-origin image requests via Opaque Response Blocking (ORB). Added `CORSMiddleware` with configurable origins via `CORS_ALLOWED_ORIGINS` env var, scoped to read-only methods. ## Changes - `src/minio_api/main.py` — Added `CORSMiddleware` with default origins for westside, pal-e-app, and mcd-tracker. Configurable via `CORS_ALLOWED_ORIGINS` env var (comma-separated). Only allows GET/HEAD/OPTIONS since this is a read-heavy asset server. ## Review Checklist - [x] ruff format passes - [x] ruff check passes - [x] All 63 existing tests pass (27 skipped — require live MinIO) - [x] Follows ecosystem CORS pattern from basketball-api - [x] No credentials allowed (read-only asset server) ## Test Plan - Verify cross-origin image requests from westside-app no longer blocked by ORB - Verify `Access-Control-Allow-Origin` header present on responses when `Origin` header sent - Override defaults via `CORS_ALLOWED_ORIGINS=https://example.com` env var ## Related - Closes #11 ## Related Notes - Follows CORS pattern established in basketball-api `src/basketball_api/main.py`

forgejo_admin added 1 commit

fix: add CORS middleware for cross-origin image serving a82ee353a6

minio-api had zero CORS configuration, causing browsers to block
cross-origin image requests via ORB. Added CORSMiddleware with
configurable origins via CORS_ALLOWED_ORIGINS env var. Only allows
GET/HEAD/OPTIONS since this is a read-heavy asset server.

Closes #11

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

forgejo_admin merged commit 3ad8765c9d into main

2026-03-28 21:20:34 +00:00

forgejo_admin deleted branch 11-add-cors-middleware

forgejo_admin referenced this pull request from a commit

fix: add CORS middleware for cross-origin image serving (#12)

Rows
Columns

fix: add CORS middleware for cross-origin image serving #12