Extract security header middleware#369
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extracts and centralizes security header behavior into a dedicated middleware module, hardens redirect “next” path validation, and adds regression tests for security headers and forwarded-HTTPS redirects.
Changes:
- Added
app/security_headers.pywith a reusable middleware that applies security headers, relaxes CSP for API docs, preserves forwarded-HTTPS redirect schemes, and normalizes HEAD semantics. - Refactored
app/main.pyto register the new middleware and strengthened_safe_next_pathby validating decoded forms. - Added
tests/test_security_headers.pyto validate header defaults, docs CSP, HEAD behavior, and forwarded-HTTPS redirects.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_security_headers.py | Adds regression tests covering security headers, HEAD behavior, and forwarded-HTTPS redirect scheme preservation. |
| app/security_headers.py | Introduces centralized security-headers middleware and redirect scheme preservation logic. |
| app/main.py | Removes inlined middleware/constants, registers new middleware, and strengthens next-path validation. |
| app/ledger/service.py | Simplifies SQLAlchemy execute result typing by removing unnecessary casts/imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if original_method == "HEAD": | ||
| headers = dict(response.headers) | ||
| headers["content-length"] = "0" | ||
| response = Response( | ||
| status_code=response.status_code, | ||
| headers=headers, | ||
| media_type=response.media_type, | ||
| ) |
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=320, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/320", | ||
| title="Security header middleware", | ||
| reward_mrwk="25", | ||
| acceptance="HEAD requests should keep GET route semantics without a body.", | ||
| ) | ||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) |
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| bounty = create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=321, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/321", | ||
| title="Forwarded HTTPS redirect", | ||
| reward_mrwk="25", | ||
| acceptance="Trailing slash redirects should not downgrade public HTTPS requests.", | ||
| ) | ||
| client = TestClient( | ||
| create_app(database_url=sqlite_url, webhook_secret="secret"), | ||
| base_url="http://mrwk.ltclab.site", | ||
| ) |
|
Fixed the failing CI on this PR by restoring the SQLAlchemy Validation after the fix:
|
MolhamHamwi
left a comment
MolhamHamwi
left a comment
There was a problem hiding this comment.
Reviewed head 36b63821 with no blockers.
Evidence checked:
- Inspected
app/security_headers.pyagainst the removed middleware block inapp/main.py; the extracted registration still rewritesHEADtoGET, restores the original method, reapplies the zero-length HEAD response, preserves forwarded-HTTPS redirects, and keeps the docs-specific CSP override before default security headers. - Checked
tests/test_security_headers.pycovers default headers, HEAD-as-GET response behavior, relaxed API docs CSP, and forwarded HTTPS redirect scheme preservation. - Verified GitHub's quality/readiness check is green.
Local validation:
.venv/bin/python -m pytest tests/test_security_headers.py -q→ 4 passed.venv/bin/python -m ruff check app/security_headers.py tests/test_security_headers.py→ passed.venv/bin/python -m mypy app/security_headers.py→ passed
CharlieLZ
left a comment
CharlieLZ
left a comment
There was a problem hiding this comment.
Requested changes on current head 36b6382.
Blocking issue: the new HEAD normalization rebuilds a fresh Response from dict(response.headers), which collapses repeated headers. That drops all but one Set-Cookie header on HEAD requests, even though the corresponding GET response preserves both cookies. This is a behavior regression for any route that sets or clears multiple cookies.
Minimal repro:
- Created a tiny FastAPI app with
register_security_headers_middleware(app). - Added a route that calls
response.set_cookie("a", "1")andresponse.set_cookie("b", "2"). GET /cookiesreturns both cookies:['a=1; Path=/; SameSite=lax', 'b=2; Path=/; SameSite=lax'].HEAD /cookiesreturns only the first cookie:['a=1; Path=/; SameSite=lax'], withcontent-length: 0.
The fix should preserve multi-value/raw headers when converting HEAD responses, then only force Content-Length: 0.
Validation:
/Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m pytest tests/test_security_headers.py tests/test_security.py::test_browser_responses_set_security_headers tests/test_security.py::test_api_docs_allow_external_assets_under_csp tests/test_api_mcp.py::test_trailing_slash_redirects_keep_forwarded_https_scheme tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 9 passed./Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m ruff check app/main.py app/ledger/service.py app/security_headers.py tests/test_security_headers.py-> passed./Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m ruff format --check app/main.py app/ledger/service.py app/security_headers.py tests/test_security_headers.py-> 4 files already formatted./Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m mypy app/main.py app/ledger/service.py app/security_headers.py-> success.git diff --check origin/main...HEAD-> clean.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
Refs #320
Summary
app/main.pyintoapp/security_headers.py.create_app()through a small module boundary, leaving route setup focused on application wiring.36b6382restores explicit ledgerCursorResult[Any]typing for rowcount checks after CI caught the removed casts.Complexity reduced
app/main.pyno longer owns cross-cutting response security behavior. CSP policy selection, strict default headers, HEAD normalization, and proxy-aware redirect rewriting now live together in a focused module with direct regression coverage.Tests
python -m pytest -q-> 339 passedpython -m pytest tests/test_security_headers.py tests/test_security.py::test_browser_responses_set_security_headers tests/test_security.py::test_api_docs_allow_external_assets_under_csp tests/test_api_mcp.py::test_trailing_slash_redirects_keep_forwarded_https_scheme tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 9 passedpython -m ruff format --check app/main.py app/ledger/service.py app/security_headers.py tests/test_security_headers.pypython -m ruff check app/main.py app/ledger/service.py app/security_headers.py tests/test_security_headers.pypython -m mypy app