feat(forward): add forward modes and the gateway-owned header sets - #185
Conversation
…meters Adds headers_deny / query_deny alongside headers_allow / query_allow, so each forward dimension resolves exactly one of three modes: a positive-list when *_allow is declared, a negative-list when *_deny is declared, and forward-all when neither is. The absent state now means forward-all rather than nothing-crosses. Absent and declared-empty are made distinguishable: ForwardConfig's four list components become @nullable and the canonical constructor no longer normalizes null to List.of(), so query_allow: [] stays a positive-list naming nothing while an omitted query_allow is the forward-all baseline. Absence is modelled as a @nullable type rather than a stored Optional, per ADR-0033. No new type is introduced, so ForwardConfig rides its existing ConfigModelReflection registration and the native image is unaffected. ForwardPolicyStage replaces copyAllowedHeaders / copyAllowedQuery with mode-dispatching copies. Negative-list and forward-all share one implementation, since forward-all is a negative-list denying nothing. The FORWARDING_HEADERS skip is retained on every mode, so the permissive baseline is not a way to smuggle a regenerated forwarding header through. Header-name matching is case-insensitive per RFC 9110; query-parameter matching stays case-sensitive on both modes, matching how the positive-list has always matched. A new ConfigValidator rule refuses a route declaring both lists for one dimension, naming the route and the dimension and collecting into the shared errors list without failing fast (ADR-0009). A declared-empty list counts as declared. The bound cannot move into the JSON Schema: additionalProperties: false bounds the forward key set, but a mutual exclusion between two of its optional keys is cross-cutting. endpoint.schema.json gains the two deny keys. gateway.schema.json is deliberately unchanged - it declares no forward object at any level, since forward is a per-route key and routes live only in endpoint files. WebSocketRelayStageTest is included although the outline's sweep did not reach it: its route fixtures configured their forward posture by OMISSION, so they inherited the default whose meaning this change inverts. Every fixture now declares its posture explicitly, and a matched control route pins the forward-all baseline on the relay path. Co-Authored-By: Claude <noreply@anthropic.com>
…Headers Generalises the one owner rather than declaring a second name list: ConnectionHeaders now carries REQUEST_STRIP (16 names) alongside the untouched RESPONSE_STRIP (10), each enumerated in full — no shared core, no delta list, no direction enum. This is what makes the forward-all baseline safe: without it a forward-all route would send the BFF's sealed __Host- session cookie to every upstream, defeating the pattern whose premise is that the backend never sees it. The two sets are independent enumerations. REQUEST_STRIP does currently contain every RESPONSE_STRIP name, but that is recorded as an observation about two hand-maintained lists, not as a derivation: a name added to one direction does not join the other. Both sets are pinned exhaustively by their own test expectation, so a one-directional addition fails that set's assertion rather than being silently propagated. The class javadoc justifies per name why the three names lifted from RESPONSE_STRIP are deliberate request-direction inclusions rather than inherited ones: proxy-connection (RFC 9113 8.2.2 connection-specific and, unusually, client-sendable), proxy-authorization (RFC 7230 6.1 hop-by-hop, a credential for the hop the gateway itself terminates), and proxy-authenticate (no legitimate request-direction use at all — withholding it costs one string and denies a smuggling vector). ForwardPolicyStage applies the strip on every mode, after the mode copy and before set_headers is merged, so the gate governs client input while an operator's static configuration is not subject to it. Two members carry behaviour beyond membership and are therefore decided in the application rather than in the set literal: - authorization is the single re-admittable member, honoured only when the route declares a positive-list naming it. Under a negative-list or forward-all there is no positive-list, so a permissive baseline can never leak an inbound credential. A re-admitted value is still overwritten by the mediated bearer, which runs last. - te crosses only when its value is exactly the trailers token; any other value negotiates a transfer coding for the terminated hop and is withheld. Co-Authored-By: Claude <noreply@anthropic.com>
…rder the pipeline The forward stage now admits a fixed set of protocol headers on its own, so an operator never has to enumerate HTTP mechanics to make a backend work. The set has two tiers: content negotiation and ranges cross unconditionally, while the five RFC 9110 13 conditional validators ride the route's not_modified toggle — the same flag the response direction reads before relaying ETag / Last-Modified, so the gateway never asks a question whose answer it then discards. A headers_deny entry naming one of these still wins: the set is applied only to names the mode copy did not already refuse. A positive-list is deliberately not consulted, because crossing without being named is the whole point of the set. The set is disjoint from ConnectionHeaders.REQUEST_STRIP by construction, which is what makes it safe to apply after the strip; a test now pins that invariant so a future addition to either list cannot quietly re-admit a gateway-owned name. The application order is pinned and encodes a precedence rule: the mode copy and strip, then the protocol set, then set_headers, then the regenerated forwarding headers, then the mediated bearer. Operator configuration beats client input; gateway-owned transport truth beats operator configuration. Keying the outbound map case-insensitively is load-bearing rather than cosmetic. Inbound field names arrive lower-cased while the set literal and set_headers use canonical spellings, so a case-sensitive map sent the client's content-type alongside the operator's Content-Type — a duplicate header, and the precedence rule silently not holding. The tests assert the cardinality of each field rather than looking one spelling up, because a get()-based assertion passes while the duplicate crosses. Co-Authored-By: Claude <noreply@anthropic.com>
Resolves the not_modified question by keeping the toggle and pinning what it actually governs. The request-direction membership was widened from two headers to five in the preceding commit; the production edit this deliverable declared was therefore already in the tree, and no substitute was invented for it. What was missing was the coverage, which is what this commit adds. The toggle spans two stages, so the coupling is asserted in one test rather than two that could drift apart: on a positive-list route each of the five validators is withheld while the flag is off and crosses once it is on, and in the same assertion ResponseStage still strips ETag / Last-Modified when off and relays them when on. Dropping either half now fails a test that names the other, which is the orphaning this pairing exists to catch. ResponseStageTest keeps the half it can see locally: each validator must genuinely FLIP with the flag, so pinning it in either direction fails there. The unconditional tier gets the matched control it lacked — those five cross under both states of the flag, so a change that accidentally gated the whole set behind the toggle fails here while the conditional test still passes. One boundary is pinned as characterization rather than endorsed. not_modified gates the protocol set's admission path, not the mode copy, so a forward-all route forwards a validator with the flag off — and the response half then strips the ETag answering it. The route forwards a precondition and discards the answer. Closing that means changing forward-all semantics, which is wider than widening this tier's membership, so it is recorded as behaviour and left for the ADRs. ResponseStage.java is confirmed untouched by this plan: the diff from the merge base is empty. Co-Authored-By: Claude <noreply@anthropic.com>
…and the ITs Deliverables 5 and 6 of the forward-mode plan, committed together because the per-deliverable chain-tail did not fire across their cross-deliverable depends_on edge and left both surfaces accumulated in the worktree. D5 — the never-forward set grows a fifth membership question. REQUEST_STRIP now also withholds the underscore spellings of the regenerated forwarding names (X_Forwarded_For and its kin) and the vendor client-IP aliases (X-Real-IP, X-Client-IP, True-Client-IP, CF-Connecting-IP). Under the old allow-list-only model these were dropped incidentally, because everything unlisted was; under the forward-all baseline they are ordinary client headers, so withholding them has to be stated. They live in REQUEST_STRIP rather than in the forwarding set because the gateway does not regenerate them and the resolver never queries them — filing them there would assert a regeneration relationship that does not exist. ADR-0036 and ADR-0037 record the three-mode policy and the per-direction header sets, and the documentation sweep re-points every page that taught deny-by-default as the forward stage's global posture. Deny-by-default is the URL layer's property and is unchanged; stage 5 filters within a route the request is already authorised for. D6 — the examples, the assertions and the upgrade note. The G5 positive control moves from Content-Type — which the gateway-understood protocol set now carries with no entry at all, so the assertion had stopped evidencing the allowlist it named — to X-Sheriff-Allowed, chosen to be both genuinely allow-listed and genuinely outside the protocol set and REQUEST_STRIP. Two new descriptor routes back the modes the suite could not otherwise reach: httpbin-deny-list (negative-list) and httpbin-forward-all (no forward block at all). DeclaredLimitBoundaryIT grows from one case to five — the positive list, the protocol set crossing unlisted, a deny entry outranking that set, the forward-all baseline bounded by the gateway-owned names, and the TE carve-out. Root README.adoc carries the upgrade note with its bound: the change alters what crosses within routes the operator had already exposed, never which routes are reachable. grpc.yaml keeps its two `te` entries, against the outline's instruction to drop them. The TE-trailers carve-out decides whether a COPIED header survives the request strip; it never copies one. Under forward-all the mode copy supplies te and the entry is redundant, but under a positive-list an unlisted name is never copied at all, so removing the entry would withhold TE: trailers from a gRPC upstream that requires it. The asymmetry is documented in that file and pinned by ForwardPolicyStageTest with named, omitted and forward-all legs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMQk1hwkFLiHa14TBeK6z1
The forward policy no longer computes a deny-by-default header set or an allow-listed query — it applies one of three modes. Five consumer-side javadoc sites in the edge package still described the abandoned model, contradicting the producer contract the same change rewrote. Route selection at stage 2 keeps its deny-by-default wording: that claim is still true, and relocating it there is the point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMQk1hwkFLiHa14TBeK6z1
… change Four edits: drop two diff-narrating doc paragraphs whose former/now framing loses its referent post-merge (the durable facts are preserved once each), replace a redundant second lower-casing in isWithheldFromUpstream with equalsIgnoreCase, and inline a single-caller test constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMQk1hwkFLiHa14TBeK6z1
…he whole equivalence class REQUEST_STRIP carried the vendor aliases in their hyphenated spelling only, so X_Real_IP, X_Client_IP, True_Client_IP and CF_Connecting_IP crossed untouched under forward-all — together with the mixed forwarding spellings, 22 spellings of gateway-owned provenance names reached the upstream, each folding onto the canonical variable for any CGI/FastCGI/WSGI/Rack backend. GW-04 already states the control as canonicalizing the whole equivalence class before stripping, and cites the Traefik fix-chain as the cautionary tale — while shipping an enumeration missing 22 members. Closed by a rule rather than a longer list: separatorFolded() lowercases via Locale.ROOT and folds _ to -, and both isRequestStripped and the forwarding-regeneration skip test the folded form. A name added to either set later is now covered across all spellings with no second edit. Also scopes the Cookie-is-absolute claim in the three normative docs: set_headers runs after the strip and can place any name. That is intended and test-pinned, but the docs asserted a flat absolute and never mentioned the escape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMQk1hwkFLiHa14TBeK6z1
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThe gateway now supports independent positive-list, negative-list, and forward-all policies for headers and query parameters. Configuration preserves absent versus empty lists, rejects conflicting modes, protects gateway-owned headers, and updates documentation and tests. ChangesForwarding policy
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/http/ConnectionHeaders.java (1)
157-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe five
x_forwarded_*literals mirrorForwardPolicyStage.FORWARDING_HEADERSand can drift.
isRequestStrippedfolds_to-before the second membership test.ForwardPolicyStage.isRegeneratedForwardingNamefolds the same way againstFORWARDING_HEADERS, which already holdsx-forwarded-for,x-forwarded-host,x-forwarded-proto,x-forwarded-port,x-forwarded-prefixandforwarded. Every underscore spelling therefore reaches the forwarding skip regardless of these five entries.The entries are a hand-maintained mirror of a set defined in another file. If a sixth regenerated forwarding name is added to
FORWARDING_HEADERS, this list does not follow. Consider removing the five entries and relying on the folded forwarding skip, or state in the Javadoc that they are redundant defence in depth. The class Javadoc already documents that the two directions are not derived from each other, so the current placement note does not cover this specific overlap.As per path instructions: "Treat a hardcoded list that must mirror a set defined elsewhere ... as a defect unless it is derived from that source at build or run time."
Source: Path instructions
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ResponseStageTest.java (1)
74-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
validatorFlipsWithTheTogglerepeats the two tests above it.
relaysValidatorsWhenEnabledalready asserts the enabled state forETagandLast-Modified.stripsValidatorsWhenDisabledalready asserts the disabled state for the same values. The new test asserts both again with no new input. Consider replacing the two earlier tests with this combined one, so the flip is stated once.api-sheriff/src/test/java/de/cuioss/sheriff/gateway/forward/ForwardPolicyStageTest.java (2)
463-478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
protocolSetDoesNotDuplicateTheModeCopysits in theStaticSetHeadersgroup.The test uses
forwardAll()and noset_headers. It asserts protocol-set behaviour. Move it into theProtocolHeaderSetgroup so the group name matches the assertion.
714-720: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
stagehelper sits between two nested test classes.
ProtocolHeaderSetends at line 712 andRequestStripstarts at line 722. The other helpers are grouped after line 991. Movestagenext toallow,denyandforwardAllso all fixtures stay in one block.api-sheriff/src/test/java/de/cuioss/sheriff/gateway/http/ConnectionHeadersTest.java (1)
118-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe size assertions repeat the exhaustive set expectations.
shouldCarryTheExactSetandshouldCarryTheExactRequestSetalready pin both sets element by element. The counts10and25therefore add no coverage, and every membership edit needs a second edit here. Keep the containment assertion and drop the two size assertions.integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/DeclaredLimitBoundaryIT.java (1)
270-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
UNLISTED_PROTOCOL_HEADERandDENIED_PROTOCOL_HEADERhold the same value.Both constants are
"Accept-Language". The two names carry route-specific intent, but a reader must compare the string literals to see that they are the same header. Consider one constant named for the header plus a comment that names both roles, or keep both and state the shared value in each Javadoc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f4e7599a-fb7e-4451-964c-0e35f5ae342f
📒 Files selected for processing (45)
README.adocapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/RouteTableBuilder.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/ForwardConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/ResolvedRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/RouteConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/forward/ForwardPolicyStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/forward/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/http/ConnectionHeaders.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/routing/RouteRuntime.javaapi-sheriff/src/main/resources/schema/endpoint.schema.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/RouteTableBuilderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ResponseStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/forward/ForwardPolicyStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/http/ConnectionHeadersTest.javadeployment/compose-sample/docker/sheriff-config/endpoints/demo-api.yamldoc/README.adocdoc/adr/0008-data-plane-transport.adocdoc/adr/0014-asset-serving-terminal-action.adocdoc/adr/0036-Forward_filtering_is_a_three-mode_policy_inside_an_authorised_route.adocdoc/adr/0037-The_gateway-owned_header_sets_are_enumerated_per_direction_and_bounded_by_knowledge.adocdoc/architecture.adocdoc/configuration.adocdoc/development/declared-limit-assertion-coverage.adocdoc/features-analysis.adocdoc/plan/04-request-pipeline.adocdoc/security-threat-model.adocdoc/user/compose-sample.adocdoc/user/protocol-routes.adocdoc/variants/01-base-gateway.adocdoc/variants/02-bff-session.adocdoc/variants/03-bff-cookie.adocdoc/variants/README.adocintegration-tests/src/main/docker/sheriff-config/endpoints/bff-session.yamlintegration-tests/src/main/docker/sheriff-config/endpoints/grpc.yamlintegration-tests/src/main/docker/sheriff-config/endpoints/httpbin.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/DeclaredLimitBoundaryIT.java
Triage dispositionsIn reply to comment_id:
|
…nse control Review triage over PR #185. Four sites still described the inbound Authorization rule as a flat absolute (endpoint.schema.json, doc/user/protocol-routes.adoc, doc/variants/01-base-gateway.adoc), or as superseded when it is retained verbatim (doc/plan/04-request-pipeline.adoc). The rule is asymmetric: cookie is absolute against client input under every mode, authorization is the single member re-admittable via a positive-list. GW-03 claimed the gateway re-applies CharacterValidationStage before writing any response. That class exists in two documentation files and no Java source — both response paths copy source headers behind the ConnectionHeaders name filter only, inspecting no characters. Replaced with what the code actually does rather than substituting an unverified claim about Vert.x's own header-write validation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMQk1hwkFLiHa14TBeK6z1
|
/review |
Pull request was closed
PR Reviewer Guide 🔍
|
1 participant
Implements #182 and #183 as one change. Targeted at the 0.1.1 cut.
Intent
Give operators three expressible postures for both headers and query parameters, and stop making them enumerate HTTP mechanics.
*_allow)*_deny)The two lists are mutually exclusive: a route declaring both for one dimension is refused at boot, not resolved at runtime.
Deny-by-default is retained — at the URL layer, where it belongs
Nothing here weakens the zero-trust posture; it relocates the claim to where it actually holds.
What landed
The three forward modes (
headers_deny/query_denyalongside the existing*_allow).ForwardConfig's list components became@Nullableand the canonical constructor stopped normalizingnulltoList.of(), so absent (forward-all) and declared empty (a list naming nothing) are finally distinguishable. A newConfigValidatorrule refuses both lists on one dimension.The gateway-owned never-forward set —
ConnectionHeaders.REQUEST_STRIPbeside the untouchedRESPONSE_STRIP, two named sets each enumerated in full rather than a shared core with deltas. The asymmetry is deliberate and documented:Cookieis absolute against client input under every mode (the BFF pattern's premise is that the backend never sees the sealed__Host-cookie), whileAuthorizationis the single member re-admittable via a positive-list, with the mediated bearer still winning becauseapplyMediatedBearerruns last.TEcrosses only as the exacttrailerstoken, which is what gRPC-over-HTTP/2 requires.The gateway-understood protocol header set — content negotiation, range requests, and the five conditional validators, crossing without being named.
User-Agentis deliberately out.A latent precedence bug fixed.
applyConditionalHeadersran afterputAll(setHeaders()), so anything applied there overrodeset_headers. Harmless while onlyIf-None-Matchsat there;Content-Typeis a header operators do set. The pinned order is now mode copy → strip → protocol set →set_headers→ regenerated forwarding → mediated bearer. The substance is not only the reorder: the outbound map became case-insensitive, so an operator'sContent-Typereplaces the client's lower-casedcontent-typeinstead of riding alongside it — previously both crossed.Two ADRs (0036, 0037) and the normative documentation re-scoped rather than demolished.
Security review found a real gap, and it is closed here
The finalize security audit found that
REQUEST_STRIPcarried the vendor aliases in their hyphenated spelling only. Under forward-all,X_Real_IP,X_Client_IP,True_Client_IP,CF_Connecting_IP— plus the mixed forwarding spellings — crossed untouched: 22 spellings of gateway-owned provenance names, each folding onto the same variable as the canonical name for any CGI/FastCGI/WSGI/Rack backend.GW-04 already stated the control as canonicalizing the whole equivalence class before stripping, citing the Traefik fix-chain as the cautionary tale, while shipping an enumeration missing 22 members. Closed by a rule, not a longer list:
separatorFolded()lowercases viaLocale.ROOTand folds_to-, and bothisRequestStrippedand the forwarding-regeneration skip test the folded form. A name added to either set later is now covered across every spelling with no second edit.Upgrade note
Every route on 0.1.0 with no
forwardblock — or with one declaring no*_allow— begins forwarding everything on 0.1.1. The bound is what makes it proportionate: it changes what crosses within routes the operator already chose to expose, never which routes are reachable, and credentials are withheld regardless. The mitigation is one line — declare a positive-list. Stated in the rootREADME.adocwhere an adopter will see it.Verification
verify -Ppre-commit) and fullverifygreen on the rebased tree.DeclaredLimitBoundaryITgrew from 1 to 11 cases. G5's positive control was broken by design —ALLOWED_HEADER = "Content-Type"is now carried by the protocol set, so the IT would have stayed green while no longer evidencing its claim. Re-pointed, and G5's row rewritten since "deny-by-default" is no longer what it asserts. New assertions cover a protocol header crossing unlisted, a*_denyentry beating it,Cookienever crossing under forward-all, andTEcrossing only astrailers.Deferred, recorded rather than silently dropped
Under forward-all and negative-list, a
not_modified: falseroute still carriesIf-None-Matchupstream via the mode copy whileResponseStagestrips the answeringETag— the toggle gates only the protocol set's admission path. Closing it means changing forward-all semantics, which is wider than this change. ADR-0037 records it as deferred, not rejected, and the boundary is pinned as explicit characterization rather than softened into a passing assertion.🤖 Generated with Claude Code
https://claude.ai/code/session_01MMQk1hwkFLiHa14TBeK6z1
Summary by CodeRabbit
New Features
Bug Fixes
Documentation