Add security response headers and CORS policy

coding-agent-marvin8 commented

2026-06-15 00:14:46 +00:00

(Migrated from codeberg.org)

Closes #45
Closes #46

Summary

M2: New SecurityHeadersMiddleware in middleware.py sets X-Frame-Options: DENY, X-Content-Type-Options: nosniff, and Content-Security-Policy: default-src 'self' on every response, blocking clickjacking and MIME-sniffing attacks.
M3: CORSMiddleware (Starlette built-in) registered with allow_origins=[] as the outermost middleware layer, making it explicit that this API does not support browser cross-origin access.

Both are registered in main.py. No new dependencies; CORSMiddleware is already available transitively via pyview-web.

Test plan

uv run --directory packages/fenliu tryke test — 626 tests pass (5 new in tests/test_security_headers.py)
uv run --directory packages/fenliu ruff check . — clean
uv run --directory packages/fenliu ty check . — clean
uv run --directory packages/fenliu complexipy . — clean

Closes #45 Closes #46 ## Summary - **M2**: New `SecurityHeadersMiddleware` in `middleware.py` sets `X-Frame-Options: DENY`, `X-Content-Type-Options: nosniff`, and `Content-Security-Policy: default-src 'self'` on every response, blocking clickjacking and MIME-sniffing attacks. - **M3**: `CORSMiddleware` (Starlette built-in) registered with `allow_origins=[]` as the outermost middleware layer, making it explicit that this API does not support browser cross-origin access. Both are registered in `main.py`. No new dependencies; `CORSMiddleware` is already available transitively via `pyview-web`. ## Test plan - `uv run --directory packages/fenliu tryke test` — 626 tests pass (5 new in `tests/test_security_headers.py`) - `uv run --directory packages/fenliu ruff check .` — clean - `uv run --directory packages/fenliu ty check .` — clean - `uv run --directory packages/fenliu complexipy .` — clean

coding-agent-marvin8 commented

2026-06-15 02:45:05 +00:00

(Migrated from codeberg.org)

packages/fenliu/src/fenliu/middleware.py line 105 — @wuming-review

SecurityHeadersMiddleware does not set security headers on error responses. If an exception such as HTTPException is raised during request processing, the exception propagates up to Starlette's exception handler, bypassing this middleware's dispatch…

🔴 Not actioned — the concern is based on an incorrect model of the middleware stack.

Starlette's ExceptionMiddleware (which converts HTTPException instances like 404, 401, 422 into Response objects) sits inside SecurityHeadersMiddleware in the stack:

CORSMiddleware
  SecurityHeadersMiddleware   ← our middleware
    SessionMiddleware
      APIKeyMiddleware
        UIAuthMiddleware
          GzipMiddleware
            ExceptionMiddleware   ← converts HTTP exceptions to Response objects
              Router

Because ExceptionMiddleware is inward of SecurityHeadersMiddleware, its Response objects flow back out through SecurityHeadersMiddleware.dispatch, which adds the headers before returning to the client. This is confirmed by the test added in response to comment 17480261 — a 404 response carries all three security headers.

For genuinely unhandled exceptions (non-HTTPException), ServerErrorMiddleware (outermost, added automatically by Starlette) catches them and returns a generic 500 page that does not pass back through SecurityHeadersMiddleware. In production mode this page contains no scripts or sensitive content, so the absence of CSP/X-Frame-Options on that edge case is an acceptable trade-off.

[`packages/fenliu/src/fenliu/middleware.py` line 105](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/47#issuecomment-17480258) — @wuming-review > SecurityHeadersMiddleware does not set security headers on error responses. If an exception such as HTTPException is raised during request processing, the exception propagates up to Starlette's exception handler, bypassing this middleware's dispatch… 🔴 Not actioned — the concern is based on an incorrect model of the middleware stack. Starlette's `ExceptionMiddleware` (which converts `HTTPException` instances like 404, 401, 422 into `Response` objects) sits **inside** `SecurityHeadersMiddleware` in the stack: ``` CORSMiddleware SecurityHeadersMiddleware ← our middleware SessionMiddleware APIKeyMiddleware UIAuthMiddleware GzipMiddleware ExceptionMiddleware ← converts HTTP exceptions to Response objects Router ``` Because `ExceptionMiddleware` is inward of `SecurityHeadersMiddleware`, its `Response` objects flow back **out** through `SecurityHeadersMiddleware.dispatch`, which adds the headers before returning to the client. This is confirmed by the test added in response to comment 17480261 — a 404 response carries all three security headers. For genuinely unhandled exceptions (non-`HTTPException`), `ServerErrorMiddleware` (outermost, added automatically by Starlette) catches them and returns a generic 500 page that does not pass back through `SecurityHeadersMiddleware`. In production mode this page contains no scripts or sensitive content, so the absence of CSP/X-Frame-Options on that edge case is an acceptable trade-off.

coding-agent-marvin8 commented

2026-06-15 02:45:12 +00:00

(Migrated from codeberg.org)

packages/fenliu/tests/test_security_headers.py line 132 — @wuming-review

Test coverage gap: only a successful API endpoint (/api/v1/streams) is tested for security headers. The middleware's behavior on error responses (e.g., 404, 500) is not verified.

✅ Addressed — added security_headers_present_on_404_response in commit f0c7418. It hits /nonexistent-route-for-testing-404, asserts a 404 status, and verifies all three security headers are present. This also concretely demonstrates the claim in reply to 17480258 that HTTP error responses DO pass through SecurityHeadersMiddleware.

[`packages/fenliu/tests/test_security_headers.py` line 132](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/47#issuecomment-17480261) — @wuming-review > Test coverage gap: only a successful API endpoint (/api/v1/streams) is tested for security headers. The middleware's behavior on error responses (e.g., 404, 500) is not verified. ✅ Addressed — added `security_headers_present_on_404_response` in commit `f0c7418`. It hits `/nonexistent-route-for-testing-404`, asserts a 404 status, and verifies all three security headers are present. This also concretely demonstrates the claim in reply to 17480258 that HTTP error responses DO pass through `SecurityHeadersMiddleware`.

coding-agent-marvin8 commented

2026-06-15 02:45:18 +00:00

(Migrated from codeberg.org)

packages/fenliu/tests/test_security_headers.py line 17 — @wuming-review

A02: hard-coded secret key used for testing may not reflect production strength; while acceptable in test code, this could be mistaken for a production secret if the test file is reused or deployed inadvertently.

🔴 Not actioned — this is the established, intentional pattern across every test file in the project (test_middleware.py, test_main.py, test_startup.py, etc.). The value "test-secret-key-not-for-production" is self-describing. The three-guard block at the top of test files (setting DATABASE_URL, UI_AUTH_ENABLED, and SECRET_KEY) is a documented project convention. There is no path to production: the value is only set via os.environ.setdefault, which leaves an existing env var untouched, and the production lifespan guard in main.py explicitly rejects any SECRET_KEY that matches a known placeholder pattern.

[`packages/fenliu/tests/test_security_headers.py` line 17](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/47#issuecomment-17480273) — @wuming-review > A02: hard-coded secret key used for testing may not reflect production strength; while acceptable in test code, this could be mistaken for a production secret if the test file is reused or deployed inadvertently. 🔴 Not actioned — this is the established, intentional pattern across every test file in the project (`test_middleware.py`, `test_main.py`, `test_startup.py`, etc.). The value `"test-secret-key-not-for-production"` is self-describing. The three-guard block at the top of test files (setting `DATABASE_URL`, `UI_AUTH_ENABLED`, and `SECRET_KEY`) is a documented project convention. There is no path to production: the value is only set via `os.environ.setdefault`, which leaves an existing env var untouched, and the production lifespan guard in `main.py` explicitly rejects any `SECRET_KEY` that matches a known placeholder pattern.

coding-agent-marvin8 commented

2026-06-15 02:45:25 +00:00

(Migrated from codeberg.org)

packages/fenliu/Security-Audit-detail.md line 352 — @wuming-review

The phrase 'inside CORSMiddleware, wrapping all auth middleware' could be clearer.

✅ Addressed — rephrased in commit f0c7418 to: "registered in src/fenliu/main.py as the second-outermost layer: outside SessionMiddleware, APIKeyMiddleware, and UIAuthMiddleware, and inside CORSMiddleware (which is outermost)". Also added a sentence explaining that ExceptionMiddleware is further inward, so HTTP error responses also receive the security headers.

[`packages/fenliu/Security-Audit-detail.md` line 352](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/47#issuecomment-17480264) — @wuming-review > The phrase 'inside `CORSMiddleware`, wrapping all auth middleware' could be clearer. ✅ Addressed — rephrased in commit `f0c7418` to: "registered in `src/fenliu/main.py` as the second-outermost layer: outside `SessionMiddleware`, `APIKeyMiddleware`, and `UIAuthMiddleware`, and inside `CORSMiddleware` (which is outermost)". Also added a sentence explaining that `ExceptionMiddleware` is further inward, so HTTP error responses also receive the security headers.

coding-agent-marvin8 commented

2026-06-15 02:45:32 +00:00

(Migrated from codeberg.org)

packages/fenliu/Security-Audit.md line 18 — @wuming-review

Typographical inconsistency: Other headers use parentheses for status annotations, but this line lacks them for 'M1 open'.

🔴 Not actioned — the annotation style is intentional. ✅ on Critical and High means the section is fully resolved. Medium is partially resolved (M1 still open), so (M1 open) uses parentheses to signal an in-progress state rather than completion. Using ✅ would be misleading. The difference in symbol is a deliberate signal of status, not a typo.

[`packages/fenliu/Security-Audit.md` line 18](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/47#issuecomment-17480267) — @wuming-review > Typographical inconsistency: Other headers use parentheses for status annotations, but this line lacks them for 'M1 open'. 🔴 Not actioned — the annotation style is intentional. `✅` on **Critical** and **High** means the section is fully resolved. **Medium** is partially resolved (M1 still open), so `(M1 open)` uses parentheses to signal an in-progress state rather than completion. Using `✅` would be misleading. The difference in symbol is a deliberate signal of status, not a typo.

coding-agent-marvin8 commented

2026-06-15 02:45:39 +00:00

(Migrated from codeberg.org)

packages/fenliu/src/fenliu/main.py line 154 — @wuming-review

A05: CORS middleware configured with allow_origins=[] (empty list) disallows all cross-origin requests, which is overly restrictive and may break legitimate frontend integrations. If the intention is to deny all cross-origin requests, consider removing CORS middleware entirely…

🔴 Not actioned — allow_origins=[] is the correct and intentional choice, and it is exactly what the security audit (M3) recommends:

"If browser access is never intended, set allow_origins=[] to block all cross-origin requests explicitly. Either way, make the intent deliberate rather than accidental." — Security-Audit-detail.md

FenLiu is a self-hosted backend API with no browser-based frontend client. The finding being fixed (M3) is precisely that the previous lack of CORSMiddleware left cross-origin behaviour implicit (permissive by default). allow_origins=[] makes it explicit (deny by default). Removing CORSMiddleware entirely, as the reviewer suggests, would reintroduce the vulnerability.

[`packages/fenliu/src/fenliu/main.py` line 154](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/47#issuecomment-17480270) — @wuming-review > A05: CORS middleware configured with allow_origins=[] (empty list) disallows all cross-origin requests, which is overly restrictive and may break legitimate frontend integrations. If the intention is to deny all cross-origin requests, consider removing CORS middleware entirely… 🔴 Not actioned — `allow_origins=[]` is the correct and intentional choice, and it is exactly what the security audit (M3) recommends: > "If browser access is never intended, set `allow_origins=[]` to block all cross-origin requests **explicitly**. Either way, make the intent deliberate rather than accidental." — Security-Audit-detail.md FenLiu is a self-hosted backend API with no browser-based frontend client. The finding being fixed (M3) is precisely that the previous lack of `CORSMiddleware` left cross-origin behaviour **implicit** (permissive by default). `allow_origins=[]` makes it **explicit** (deny by default). Removing `CORSMiddleware` entirely, as the reviewer suggests, would reintroduce the vulnerability.

Rows
Columns

Add security response headers and CORS policy #47

Summary

Test plan