fix: prevent set from being forwarded to client#2686
Open
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors response header rule execution so Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2686 +/- ##
===========================================
+ Coverage 40.10% 63.04% +22.93%
===========================================
Files 975 245 -730
Lines 122862 26309 -96553
Branches 5535 0 -5535
===========================================
- Hits 49275 16586 -32689
+ Misses 71935 8384 -63551
+ Partials 1652 1339 -313
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx`:
- Line 67: The sentence describing the `set` operation overpromises reuse —
update the wording so it clarifies that only `Cache-Control` values stored via
the `set` operation are reused by the router, while other headers set this way
are neither forwarded to the client nor reused later; reference the operations
`set` and `propagate` and the header `Cache-Control` when adjusting the sentence
on that line.
In `@router/core/header_rule_engine.go`:
- Around line 460-464: The code checks rule.Name == cacheControlKey
case-sensitively, so canonicalize or perform a case-insensitive compare before
the Cache-Control special-case branch: replace the equality check with a
case-insensitive comparison (e.g., strings.EqualFold(rule.Name, cacheControlKey)
or compare strings.ToLower(rule.Name) to a lowercased cacheControlKey) in the
block that currently references rule.Name and cacheControlKey so
propagation.setCacheControl is correctly set when header names differ only by
case; keep the existing propagation.header.Set(rule.Name, rule.Value) behavior
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e257883-8e4d-4cbe-a0f7-e7a56d4e36b2
📒 Files selected for processing (6)
docs-website/router/proxy-capabilities/adjusting-cache-control.mdxdocs-website/router/proxy-capabilities/subgraph-response-header-operations.mdxrouter-tests/protocol/header_propagation_race_test.gorouter-tests/protocol/header_set_test.gorouter/core/header_rule_engine.gorouter/core/header_rule_engine_test.go
docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx
Outdated
Show resolved
Hide resolved
SkArchon
force-pushed
the
milinda/eng-8718-weird-response-header-forwarding-logic
branch
from
fcce914 to
874bce1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-website/router/proxy-capabilities/adjusting-cache-control.mdx`:
- Around line 157-165: The YAML example under cache_control_policy is invalid:
the list item is incorrectly indented under value and the subgraphs key is
missing; update the block for the cache_control_policy config (keys:
cache_control_policy, enabled, value) by adding a top-level subgraphs: key and
dedenting the list so the - name: "specific-subgraph" and its value:
"max-age=5400, public" are entries under subgraphs, producing valid YAML that
readers can copy/paste.
In `@router/core/header_rule_engine_test.go`:
- Around line 137-161: The test incorrectly expects subgraph-specific cache
rules to be appended to HeaderRules.AfterSubgraphResponse; update the assertions
in the AddCacheControlPolicyToRules tests so the global policy is asserted only
against result.AfterSubgraphResponse (e.g., check length 1 and Default ==
"max-age=300") and assert the subgraph-specific rule is checked via
result.Subgraphs["sg1"].Response (Default == "max-age=60"), referencing
AddCacheControlPolicyToRules, config.CacheControlPolicy and
HeaderRules.AfterSubgraphResponse/Subgraphs to locate the code to change.
In `@router/core/header_rule_engine.go`:
- Around line 470-475: The current HeaderRuleOperationSet branch writes
synthetic values directly into res.Header (res.Header.Set(rule.Name,
rule.Value)), which allows later propagate rules to see and export them;
instead, stop mutating res.Header for synthetic “set” operations and store these
values in a separate internal map on the response (e.g., res.syntheticSetHeaders
or similar) so they are available to downstream rule logic that needs them but
are not considered real response headers for propagation (also update
propagate/propagation handling to read only real res.Header for
propagation.header and consult the new synthetic map only where appropriate).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23accb33-dfce-4949-be4d-fed78b77036b
📒 Files selected for processing (8)
docs-website/router/proxy-capabilities/adjusting-cache-control.mdxdocs-website/router/proxy-capabilities/subgraph-response-header-operations.mdxrouter-tests/operations/singleflight_test.gorouter-tests/protocol/header_propagation_test.gorouter-tests/protocol/header_set_test.gorouter/core/header_rule_engine.gorouter/core/header_rule_engine_test.gorouter/pkg/config/config.go
✅ Files skipped from review due to trivial changes (1)
- docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- router-tests/operations/singleflight_test.go
- router-tests/protocol/header_set_test.go
- router-tests/protocol/header_propagation_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/core/header_rule_engine.go (1)
207-207: Simplify redundanthasResponseRulescheck.
len(rhrrs) > 0already includespostRulesviagetAllRules(), so the extra|| postRules.hasRules()is unnecessary.♻️ Suggested simplification
- hf.hasResponseRules = len(rhrrs) > 0 || postRules.hasRules() + hf.hasResponseRules = len(rhrrs) > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/header_rule_engine.go` at line 207, The assignment to hf.hasResponseRules is redundant because rhrrs (from getAllRules()) already includes postRules; replace the current condition "len(rhrrs) > 0 || postRules.hasRules()" with a single check "len(rhrrs) > 0" so hf.hasResponseRules is true only when the aggregated rule slice rhrrs is non-empty; update the code where hf.hasResponseRules is set (the line referencing hf.hasResponseRules, rhrrs, and postRules) to use only len(rhrrs) > 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/core/header_rule_engine.go`:
- Line 207: The assignment to hf.hasResponseRules is redundant because rhrrs
(from getAllRules()) already includes postRules; replace the current condition
"len(rhrrs) > 0 || postRules.hasRules()" with a single check "len(rhrrs) > 0" so
hf.hasResponseRules is true only when the aggregated rule slice rhrrs is
non-empty; update the code where hf.hasResponseRules is set (the line
referencing hf.hasResponseRules, rhrrs, and postRules) to use only len(rhrrs) >
0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2df74a91-36d6-4264-870b-fd02a63e951a
📒 Files selected for processing (5)
docs-website/router/proxy-capabilities/adjusting-cache-control.mdxrouter/core/header_rule_engine.gorouter/core/header_rule_engine_buildheader_test.gorouter/core/header_rule_engine_test.gorouter/core/router.go
✅ Files skipped from review due to trivial changes (1)
- docs-website/router/proxy-capabilities/adjusting-cache-control.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/header_rule_engine_test.go
SkArchon
requested review from
a team,
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
endigma
requested changes
Mar 30, 2026
| </Warning> | ||
|
|
||
| <Note> | ||
| Until router version VERSION, setting a `Cache-Control` header would do a hard override of whatever value would be computed by the `cache_control_policy` configuration. This has been changed since. |
Member
There was a problem hiding this comment.
VERSION? how do we chicken-egg this with the release now that docs are in tree?
Contributor
Author
There was a problem hiding this comment.
Do a release -> then update and merge since its only my pr ideally -> and then merge
Either way can open a new PR if the number is wrong worst case
Member
There was a problem hiding this comment.
You mean releasing from the PR branch?
| ``` | ||
|
|
||
| <Warning> | ||
| As such we recommend not using this method and simply sticking to using the `subgraph` configuration in `cache_control_policy`, as it makes things more explicit and clearer. |
Member
There was a problem hiding this comment.
Suggested change
| As such we recommend not using this method and simply sticking to using the `subgraph` configuration in `cache_control_policy`, as it makes things more explicit and clearer. | |
| We do not recommend using this method, simply sticking to using the `subgraph` configuration in `cache_control_policy` makes things more explicit and clearer. |
|
|
||
| <Note> | ||
| Go canonicalizes headers by default e.g. `x-my-header` to `X-My-Header.` Write your rule accordingly or use `(?i)``^X-Test-.*` flags to make your regex case insensitive. | ||
| The `set` value is **not** forwarded to the client response. |
Member
There was a problem hiding this comment.
Suggested change
| The `set` value is **not** forwarded to the client response. | |
| The `set` value is **not** forwarded to the client response unless it is propagated. |
Comment on lines
+112
to
+114
| <Warning> | ||
| You can use `set` to set a header value and then use a `propagate` rule to forward that same header to the client. However, we would caution against this, as the header will not be propagated if no subgraph was actually called. | ||
| </Warning> |
Member
There was a problem hiding this comment.
Can we merge all this into one block? It's kinda whiplash to read "is not forwarded**" -> "do xyz to forward it"
Comment on lines
+116
to
+144
| ### Configuration | ||
|
|
||
| * `name` - The name of the header to set | ||
| * `value` - The value to set for the header | ||
|
|
||
| ### Example: Setting Cache Control on a Subgraph Response | ||
|
|
||
| ```yaml | ||
| cache_control_policy: | ||
| enabled: true | ||
| value: "max-age=180, public" | ||
|
|
||
| headers: | ||
| subgraphs: | ||
| specific-subgraph: | ||
| response: | ||
| - op: "set" | ||
| name: "Cache-Control" | ||
| value: "max-age=5400" | ||
| ``` | ||
|
|
||
| In this example, when a request hits `specific-subgraph`, the `Cache-Control: max-age=5400` value is injected into the subgraph response. The restrictive cache control algorithm then processes this value alongside other subgraph responses to compute the final `Cache-Control` header sent to the client. This is equivalent to doing | ||
|
|
||
| ``` | ||
| cache_control_policy: | ||
| enabled: true | ||
| value: "max-age=180, public" | ||
|
|
||
| ``` No newline at end of file |
Member
There was a problem hiding this comment.
If we don't want people to do this, perhaps we should not include so many searchable examples of doing it in the docs
Comment on lines
+1069
to
+1072
| require.Equal(t, "", res.Response.Header.Get("X-Header-A"), | ||
| "response %d: set response headers should not be forwarded to the client", i) | ||
| require.Equal(t, "", res.Response.Header.Get("X-Header-B"), | ||
| "response %d: set response headers should not be forwarded to the client", i) |
|
|
||
| * `default` - Fallback to this value when the `named`, `matching` or `rename` header could not be found. | ||
|
|
||
| * `set` - Sets a header on the request forward to the subgraph. You must set the following values: |
Member
There was a problem hiding this comment.
Does the page still flow properly after this change? We still have set?
| * `name` - The name of the header to set | ||
| * `value` - The value to set for the header | ||
|
|
||
| ### Example: Setting Cache Control on a Subgraph Response |
Member
There was a problem hiding this comment.
Perhaps we can replace this with a use case set is actually ideal for? We can feature config like this elsewhere as a footgun example but I think showing an example then a warning to not do it is bad practice
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
2 participants
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.
This PR does the following on the response side
"set" is now not propagated to the client, instead the goal of set is to add a header to the response from the subgraph, so it looks like the set header "came from the subgraph directly".
Since "set" is now acts like a header that came directly from the subgraph, if users want to they "technically" can add a propagate op to send this header to the client. Though this is still not recommended (mentioned in the docs), as the same limitations of how set was before applies. (Limitations such as if no subgraph runs nothing will get propagated).
Previously if a user uses "set" to set a Cache-Control header, this would hard override any values set from the cache control configuration without looking at what is the most restrictive configration. This has been removed. Since "set" now acts like a header sent directly from the subgraph, it would behave the same way as it would for an actual Cache Control header that was sent from a subgraph.
Why these changes?
For example
The header would be sent to the client ONLY IF the products subgraph was called. Likewise when using
headers.allfor the same, in case of errors the client header is not set. This PR's goal is to realign set to mean "set (or add) a header to the subgraph response headers".If user's still want to do this they can use the following alternative (currently only supports expressions).
Previously a user was allowed to use the "set" header to do a hard override on the Cache-Control header. Usually we would only pick the most restrictive cache control header, however upon usage of set, whatever was provided would be overridden. After discussing internally we decided to not allow users a hard override for Cache-Control headers, as it could be a potential security issue (i.e. :- When a subgraph wants to expire its data, it's not expired because the "set" override set a higher ttl, and we don't use the lower ttl because of the "set" override for the header)
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.