fix(api): warn at the FORCE_HTTPS redirect itself (TRANSPORT-001) (#3047) - #3067
Open
bdunncompany wants to merge 1 commit into
Open
fix(api): warn at the FORCE_HTTPS redirect itself (TRANSPORT-001) (#3047)#3067bdunncompany wants to merge 1 commit into
bdunncompany wants to merge 1 commit into
Conversation
Closes #3047. #2988 put the `[proxy-trust]` warning in `trustsForwardedHeadersFrom`, which only fires when the peer is NOT trusted. That leaves the variant this issue describes completely silent: a *trusted* proxy that never sends `X-Forwarded-Proto`. The trust gate passes, `effectiveRequestScheme` finds no scheme header to honor and falls back to the internal plain-HTTP proxy->API hop, and under FORCE_HTTPS every non-health route 308s while `/health` (exempt) keeps returning 200 — reached from a perfectly correct TRUSTED_PROXY_CIDRS. Per the issue, the signal belongs at the redirect rather than the trust gate: one line at the point of user-visible impact, instead of N warnings that each have to anticipate a different cause. This covers the untrusted-peer and TRUST_PROXY_HEADERS=false cases #2988 already handles as well. The warning names what an operator needs to disambiguate the causes without reading the source: - `peer` — the immediate TCP peer, so it can be compared to TRUSTED_PROXY_CIDRS - `trustedProxy` — false means the CIDR list does not cover this peer, or TRUST_PROXY_HEADERS is off - `xForwardedProto` — `(absent)` with `trustedProxy=true` is precisely the variant no `clientIp.ts` warning can reach - `canonicalHost` — distinguishes the 400 branch from the 308 It also states the symptom (fleet-wide 308s while /health stays 200), because that combination is what makes the fault look like anything other than a proxy misconfiguration. Warned on the 400 branch too: an unrecognized Host is the same misconfiguration seen through a different lens, and was equally silent. Suppression is per peer on a 15-minute window with a bounded map, matching `clientIp.ts` — a redirect storm is exactly the situation here, so an unsuppressed log would flood. Not addressed (carried in #3047 and left for a maintainer call): suggestion 1 (boot-time cross-check of FORCE_HTTPS against the proxy-trust settings) and suggestion 4 (a `/health` trusted-peer field). Both change boot or health-check semantics, which felt like your call rather than mine. ## Local verification - `security.test.ts`: 31 pass, 7 new covering the redirect warn, the `(absent)` variant, the 400 branch, per-peer suppression, health-path exemption, and FORCE_HTTPS off - confirmed the tests catch it: removing the call fails 4 of them - full `@breeze/api` suite: 1235 files, 19452 tests passed, 0 failures - `tsc --noEmit` and eslint clean
Deploying breeze with
|
| Latest commit: |
9f64359
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://53556990.breeze-9te.pages.dev |
| Branch Preview URL: | https://fix-3047-log-at-redirect.breeze-9te.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3047. Picking this up since it was unassigned and is the other half of the outage I reported in #2987 — happy to hand it back if you'd rather take it.
The gap #2988 left
trustsForwardedHeadersFromonly warns when the peer is not trusted. The variant in this issue stays completely silent: a trusted proxy that never sendsX-Forwarded-Proto. The trust gate passes,effectiveRequestSchemefinds no scheme header and falls back to the internal plain-HTTP proxy→API hop, and underFORCE_HTTPSevery non-health route 308s while/health(exempt) keeps returning200— all from a perfectly correctTRUSTED_PROXY_CIDRS.Per your framing in the issue, the signal belongs at the redirect rather than the trust gate: one line at the point of user-visible impact instead of N warnings that each have to anticipate a different cause. This also covers the untrusted-peer and
TRUST_PROXY_HEADERS=falsecases #2988 already handles.What the warning says
Enough to disambiguate the causes without reading the source:
peerTRUSTED_PROXY_CIDRStrustedProxyfalse= CIDR list doesn't cover this peer, orTRUST_PROXY_HEADERSis offxForwardedProto(absent)withtrustedProxy=trueis exactly the variant noclientIp.tswarning can reachcanonicalHostIt also states the symptom — fleet-wide 308s while
/healthstays 200 — because that combination is what makes this look like anything except a proxy misconfiguration. In the original incident every signal an operator normally checks stayed green for ~50 minutes.Also warns on the 400 branch. An unrecognized
Hostis the same misconfiguration through a different lens and was equally silent.Suppression is per peer on a 15-minute window with a bounded map, matching
clientIp.ts. A redirect storm is precisely the situation here, so an unsuppressed log would flood.Deliberately not included
Suggestion 1 (boot-time cross-check of
FORCE_HTTPSagainst the proxy-trust settings) and suggestion 4 (a/healthtrusted-peer field). Both change boot or health-check semantics, which reads like your call rather than mine — happy to do either on your say-so. Suggestion 3 (release-note flag) is independent of code.Local verification
security.test.ts: 31 pass, 7 new — the redirect warn, the(absent)variant, the 400 branch, per-peer suppression, health-path exemption, andFORCE_HTTPSoff.@breeze/apisuite: 1235 files, 19452 tests passed, 0 failures.tsc --noEmitand eslint clean on both changed files.