Extract HTTP response safety helpers#371
Conversation
📝 WalkthroughWalkthroughThis PR refactors HTTP middleware logic into dedicated helpers, strengthens redirect validation, and simplifies type casting. New ChangesHTTP Middleware Refactoring and Security Hardening
Type Simplification in Ledger Service
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 HTTP security/redirect + HEAD-handling behavior into a dedicated middleware module, updates the FastAPI app to use it, and adds test coverage for the new middleware utilities.
Changes:
- Added
app/http_middleware.pywith response hardening (security headers, forwarded-HTTPS redirect preservation) and HEAD-as-GET routing utilities. - Updated
app/main.pyto call the new middleware helpers and tightened_safe_next_pathvalidation by checking the URL-decoded value. - Added
tests/test_http_middleware.pyto cover CSP differences for docs, forwarded proto redirects, and HEAD response behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_http_middleware.py | Adds tests validating CSP selection, forwarded-HTTPS redirect rewriting, and HEAD handling. |
| app/main.py | Removes inline middleware logic in favor of new helpers; strengthens next path sanitization. |
| app/ledger/service.py | Simplifies SQLAlchemy execute(update(...)) typing around rowcount checks. |
| app/http_middleware.py | Introduces centralized HTTP middleware utilities for security headers, redirects, and HEAD handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def apply_http_response_safety(request: Request, response: Response) -> Response: | ||
| preserve_forwarded_https_redirect(request, response) | ||
| for name, value in security_headers_for_path(request.url.path).items(): | ||
| response.headers.setdefault(name, value) | ||
| return response |
| claimed: Any = session.execute( | ||
| update(Bounty) | ||
| .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) | ||
| .values( | ||
| awards_paid=Bounty.awards_paid + 1, | ||
| status=case( | ||
| (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), | ||
| else_="open", | ||
| ), | ||
| ) | ||
| ) | ||
| if claimed.rowcount != 1: |
| host: str = "mrwk.ltclab.site", | ||
| headers: dict[str, str] | None = None, | ||
| ) -> Request: | ||
| request_headers = {"host": host} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/http_middleware.py`:
- Around line 53-62: The function response_for_original_method is collapsing
multi-value headers by doing headers = dict(response.headers), which loses
repeated keys like Set-Cookie; change it to construct the HEAD Response using
response.raw_headers (or otherwise preserve all header tuples) instead of
dict(response.headers), and ensure you copy response.status_code, media_type,
and any other metadata (e.g., content-type) from the original Response; keep
setting content-length to "0" but add/replace that single header without
de-duplicating existing headers so multiple Set-Cookie headers are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 581b075f-aac5-400f-ae0e-91d38ac19f94
📒 Files selected for processing (4)
app/http_middleware.pyapp/ledger/service.pyapp/main.pytests/test_http_middleware.py
| def response_for_original_method(response: Response, original_method: str) -> Response: | ||
| if original_method != "HEAD": | ||
| return response | ||
| headers = dict(response.headers) | ||
| headers["content-length"] = "0" | ||
| return Response( | ||
| status_code=response.status_code, | ||
| headers=headers, | ||
| media_type=response.media_type, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Show the HEAD normalization implementation that rebuilds headers via dict()
rg -n -C3 "def response_for_original_method|headers = dict\\(response\\.headers\\)" app/http_middleware.py
# 2) Show routes creating multiple cookie headers that should be preserved
rg -n -C3 "set_cookie\\(|delete_cookie\\(" app/main.pyRepository: ramimbo/mergework
Length of output: 2280
Preserve multi-value headers when normalizing HEAD responses.
In app/http_middleware.py (response_for_original_method, lines 53-62), headers = dict(response.headers) can collapse repeated header keys; since app/main.py sets/deletes multiple cookies via set_cookie(...) / delete_cookie(...), the resulting Set-Cookie headers can be dropped or altered for HEAD requests. Build the HEAD response without de-duplicating headers (e.g., preserve from response.raw_headers) and copy necessary response metadata.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/http_middleware.py` around lines 53 - 62, The function
response_for_original_method is collapsing multi-value headers by doing headers
= dict(response.headers), which loses repeated keys like Set-Cookie; change it
to construct the HEAD Response using response.raw_headers (or otherwise preserve
all header tuples) instead of dict(response.headers), and ensure you copy
response.status_code, media_type, and any other metadata (e.g., content-type)
from the original Response; keep setting content-length to "0" but add/replace
that single header without de-duplicating existing headers so multiple
Set-Cookie headers are preserved.
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers from my review. I inspected app/http_middleware.py, app/main.py, app/ledger/service.py, and tests/test_http_middleware.py on head ef99f84.
What I checked:
- The middleware extraction keeps the HEAD-as-GET routing behavior and restores the original request method before returning a zero-length HEAD response.
- Same-host forwarded-HTTPS 307/308 redirects still get rewritten from
httptohttps, while external locations are left alone. - API docs/redoc paths still receive the relaxed docs CSP, and normal pages keep the stricter default security headers.
- The
pay_bounty()/close_bounty()typing cleanup does not change the guarded SQL update predicates or post-updaterowcountchecks.
Validation run locally on this PR head:
./.venv/bin/python -m pytest tests/test_http_middleware.py tests/test_security.py tests/test_docs_public_urls.py -q-> 74 passed./.venv/bin/python -m pytest -q-> 339 passed./.venv/bin/python -m ruff check app/http_middleware.py app/main.py app/ledger/service.py tests/test_http_middleware.py-> passed./.venv/bin/python -m ruff format --check app/http_middleware.py app/main.py app/ledger/service.py tests/test_http_middleware.py-> passed./.venv/bin/python -m mypy app-> passedgit diff --check origin/main...HEAD-> passed
ayskobtw-lil
left a comment
ayskobtw-lil
left a comment
There was a problem hiding this comment.
Thanks for the extraction. I found one blocker in the HEAD response wrapper.
response_for_original_method() rebuilds HEAD responses with headers = dict(response.headers). Starlette keeps repeated headers such as Set-Cookie in raw_headers, but converting to a dict collapses them, so a HEAD request for any route that sets multiple cookies will silently drop all but one cookie.
Repro on current head ef99f84:
original raw set-cookie count 2
head raw headers [(b'content-length', b'0'), (b'set-cookie', b'first=1; Path=/; SameSite=lax')]
head set-cookie count 1
The fix should preserve response.raw_headers when constructing the empty HEAD response, replacing/adding only the single content-length: 0 header rather than round-tripping through dict(response.headers).
|
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
What changed:
app.mainintoapp/http_middleware.py, covering security headers, docs CSP selection, HEAD-as-GET routing, and forwarded-HTTPS redirect preservation.app.mainfocused on app wiring by delegating the middleware work throughrun_with_head_as_get()andapply_http_response_safety().nextvalues, and follow-upef99f84keeps explicit SQLAlchemy rowcount typing compatible with strict mypy.Validation:
python -m pytest -q-> 339 passed.python -m pytest tests/test_http_middleware.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_security.py::test_regular_pages_keep_strict_csp tests/test_api_mcp.py::test_head_requests_match_get_routes_without_body tests/test_api_mcp.py::test_trailing_slash_redirects_keep_forwarded_https_scheme -q-> 10 passed.python -m ruff format --check .-> 50 files already formatted.python -m ruff check .-> All checks passed.uv run --python 3.12 mypy app-> Success.python scripts/docs_smoke.py-> docs smoke ok.No private keys, seed material, secrets, deployment credentials, private vulnerability details, payout credentials, or MRWK price claims are included.