Stop fabricating 200 for every HEAD at the origins - #1578
Conversation
location / short-circuited HEAD with a bare return 200, so a HEAD never reached the app and reported a live page whatever the real status was. Reproduced against both origins before the change: GET /@good-karma/points answered 307 while HEAD answered 200. Link checkers, uptime probes and crawlers were all told the wrong thing. The 'always' flags are the other half of the same fix rather than a separate tidy-up. Those statuses were unreachable via HEAD while every response was a fabricated 200, which is in add_header's default status list, so deleting the block on its own would have quietly dropped HSTS from HEAD on 404, 429 and 5xx. 'always' on its own fixes nothing either: an add_header nested inside the if suppresses inheritance regardless. X-Cache-Status keeps its default status list on purpose. It is a cache diagnostic, not something we owe an error response. Applied to both boxes and reloaded in one window before committing, so the tracked copies match what is running. Verified direct to each origin, since through Cloudflare the bug is invisible: the edge normalises the request and already answered 307 with the headers.
PR Summary by QodoFix origin HEAD responses and preserve security headers on non-2xx
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughBoth origin vhosts now apply frontend security and CORS headers on all responses. They also remove special ChangesOrigin vhost behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized configuration change makes HEAD requests return the origin's real status and preserves security headers on error responses; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR makes direct-origin HEAD requests follow the real frontend proxy path rather than returning an unconditional 200, while preserving security headers across error statuses.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue identified. Both active origin configurations consistently remove the fabricated HEAD response and preserve the intended security headers when the upstream returns redirects or errors.
|
| Filename | Overview |
|---|---|
| infra/origin/eu.ecency.com.conf | Correctly delegates HEAD requests to the EU frontend upstream and retains security headers on all response statuses. |
| infra/origin/us.ecency.com.conf | Mirrors the EU origin fix, preserving equivalent HEAD and security-header behavior across origins. |
Reviews (1): Last reviewed commit: "Stop fabricating 200 for every HEAD at t..." | Re-trigger Greptile
Code Review by Qodo
1. Duplicate CORS header
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Code Review by Qodo
1. HEAD converted to GET
|
|
On the second finding, HEAD converted to GET: correct, and intended.
It does mean a HEAD on an uncached path now costs a render where it previously cost nothing, because it previously returned a fabricated 200 without reaching the app. That is the price of the answer being true, and it lands on link checkers and uptime probes rather than on readers. Worth watching, not worth trading correctness for. Verified after the change, direct to each origin: and real pages still answer 200 on both methods on both boxes. |
#1575 is the issue describing the fabricated 200; #1578 is the PR that removed it. The line read as though the issue did the work, which sends anyone following it to a report rather than a diff. Cites both now, since each answers a different question: the PR carries the change and the before/after measurements, the issue carries the reproduction. The main README already said #1578, so the two agree again.
1 participant
Closes #1575.
location /short-circuited HEAD with a barereturn 200, so a HEAD request never reached the app and reported a live page whatever the real status would have been.Reproduced against both origins before the change:
and after:
The
alwaysflags are the same fix, not a tidy-upWhile every HEAD was a fabricated 200, the security headers applied because 200 is in
add_header's default status list. Deleting the block alone would therefore have made HEAD reach real 404/429/5xx responses with the headers gone — a security regression that looks correct in a 200/307 spot check.alwayson its own fixes nothing either, since anadd_headernested inside theifsuppresses inheritance regardless of it. Both halves ship together.Verified after the change:
X-Cache-Statusdeliberately keeps its default status list: it is a cache diagnostic, not something we owe an error response.Behaviour change worth stating
Error responses now carry CORS and HSTS where they previously did not. That is intended, and matches what
X-Origin-Regionalready did.Verification notes
Both boxes were edited and reloaded in one window before this commit, so the tracked copies match what is running; one origin fixed and the other not would be the same bypass-by-routing failure the README's newsletter example describes.
Probing through
ecency.comis a false negative for this bug — the edge normalises the request and already returned 307 with the headers. It only reproduces direct to the origin.origin-config-auditpasses unchanged: 0 findings, 34 self-test cases.Summary by CodeRabbit
HEADrequests.