fix(uhttp): opt-in cache-key headers via CacheOption + WithCacheKeyHeaders - #1093
Conversation
…aders
CreateCacheKey only folded Accept, Content-Type, Cookie, and Range into
the HTTP response cache key. Any other request header was silently
ignored, so two GET requests differing only in an unlisted header (e.g.
Authorization) collided on the same cache entry and one could be served
the other's cached response.
CreateCacheKey now takes a trailing ...CacheOption -- additive, so every
existing zero-arg CreateCacheKey(req)/cache.Get(req)/cache.Set(req, resp)
call keeps compiling. CacheOption is an interface (not a concrete param)
so future dimensions (TTL, query-param keying, etc.) don't require
touching these signatures again.
WithCacheKeyHeaders(...) is a WrapperOption, set once at client
construction:
cli := uhttp.NewBaseHttpClient(httpClient, uhttp.WithCacheKeyHeaders("Authorization"))
BaseHttpClient stores the resulting CacheOption(s) and forwards them to
every Get/Set call itself makes, so Do's signature is completely
untouched -- no sibling method, no per-call option needed.
Values are read from req.Header at key-computation time rather than
supplied by the caller, so the key can never describe a token other than
the one actually sent. That also means the round-trip-clone fix in
BaseHttpClient.Do (round-tripping on req.Clone(req.Context()) instead of
req) matters again: http.Client.Do forks the *http.Request on every call
once Timeout > 0 (which uhttp.NewClient always sets), but that fork is
shallow, so Header stays the same map the caller passed in -- a
RoundTripper mutating it between the Get and Set calls (transport.go's
userAgentTripper does this for User-Agent) would otherwise make the Set
key diverge from every future Get key for an opted-in header.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // key (via WithCacheKeyHeaders) between the Get above and the Set | ||
| // below, and CreateCacheKey(req) would hash a different value for | ||
| // each -- a store that no future lookup can ever match. | ||
| resp, err = c.HttpClient.Do(req.Clone(req.Context())) // #nosec G704 -- this HTTP wrapper intentionally supports arbitrary connector-defined endpoints. |
There was a problem hiding this comment.
🟡 Suggestion (medium-high confidence): this clone also changes default-path cache behavior, not just the opt-in path, and the change should be called out. http.Client.send calls req.AddCookie on the caller's request before its internal fork (net/http/client.go), so for any wrapped client with a Jar, jar cookies previously landed on req.Header between Get and Set — and Cookie is in the default key set, so those clients effectively had a write-only cache (Set key ≠ every future Get key). After this change both keys omit jar cookies, so the cache starts hitting and two requests carrying different jar sessions now share one entry. That is the same collision class this PR is fixing for Authorization, arriving silently by default for jar-based connectors. Consider either folding c.HttpClient.Jar cookies into the key, or documenting this in the PR and the CreateCacheKey doc comment, plus a test asserting the caller's req.Header is no longer mutated by the transport.
| // don't collide in the cache. The value folded in is always read from | ||
| // req.Header at request time, so the key can never describe a value other | ||
| // than the one actually sent. | ||
| func WithCacheKeyHeaders(headers ...string) WrapperOption { |
There was a problem hiding this comment.
🟡 Suggestion (high confidence in the mechanism): the key is computed from req.Header before the round trip, so any header attached below Do — an oauth2.Transport/token RoundTripper, URL.User basic auth, or the cookie jar — is invisible to it. WithCacheKeyHeaders("Authorization") on a client whose token is injected by a transport therefore contributes nothing to the key and the requests still collide, with no error and no log line, while the caller believes they opted in (the clone above now guarantees that value can never be observed). Worth documenting explicitly here, and consider a debug/warn in Do when a named header is absent from req.Header.
| // default set of headers (Accept, Content-Type, Cookie, Range). Kept as an | ||
| // interface so future dimensions (TTL, query-param keying, etc.) can be | ||
| // added without changing CreateCacheKey's or icache's signatures again. | ||
| type CacheOption interface { |
There was a problem hiding this comment.
🟡 Suggestion: CacheOption is exported and now appears in exported signatures (CreateCacheKey, GoCache.Get/Set, DBCache.Get/Set), but its only method is unexported and the only implementation (cacheKeyHeadersOption) is unexported — WithCacheKeyHeaders returns a WrapperOption, not a CacheOption. No package outside uhttp can construct one, so those exported variadic parameters are unusable downstream. Consider exporting a constructor (e.g. func CacheKeyHeaders(headers ...string) CacheOption) and having WithCacheKeyHeaders wrap it.
| for _, h := range cfg.headers { | ||
| key := http.CanonicalHeaderKey(h) | ||
| for _, value := range req.Header[key] { | ||
| headerParts = append(headerParts, fmt.Sprintf("%s=%s", key, value)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (minor): names already covered by the default set, or repeated across options, are folded in twice — WithCacheKeyHeaders("Accept") yields Accept=...&Accept=... in cacheString. Keys stay internally consistent so nothing breaks, but it makes the key depend on redundant configuration. Dedupe against the default set and against already-appended names before appending.
| } | ||
|
|
||
| func (g *GoCache) Get(req *http.Request) (*http.Response, error) { | ||
| func (g *GoCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error) { |
There was a problem hiding this comment.
🟡 Suggestion: adding a variadic parameter to exported methods on exported types (GoCache.Get/Set, DBCache.Get/Set) keeps ordinary call sites compiling, but it does break downstream code that assigns these methods to a func(*http.Request) (*http.Response, error) value or that satisfies a locally-declared cache interface with the old signature. Per the repo's compatibility criteria this is worth a note in the PR description and a 0.x minor bump in pkg/sdk/version.go (currently unchanged at v0.24.1) rather than shipping silently as a patch.
General PR Review: fix(uhttp): opt-in cache-key headers via CacheOption + WithCacheKeyHeadersBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness. The new commit addresses three prior findings: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsNote: this run could not emit the machine-readable |
|
Fable: #1093 (cache-option-wrapper) — required changes
Keep: option-pattern API, values read from |
- Drop the unconditional req.Clone(...) round-trip in Do. It didn't help
caller-set headers, and for headers injected mid-flight (jar cookies --
Cookie is in the default key set -- or in-place RoundTrippers) it turned
a dead cache into cross-scope cache hits, reintroducing the CE-1056 bug
on the default path. Delete the test that pinned that behavior
(TestWrapper_Do_CachesDespiteRoundTripHeaderInjection).
- Export CacheKeyHeaders(...) CacheOption and have WithCacheKeyHeaders wrap
it, so CacheOption -- which already appears in exported signatures
(CreateCacheKey, GoCache/DBCache Get/Set) -- can be constructed outside
uhttp.
- Document on WithCacheKeyHeaders/CacheKeyHeaders that named headers must
be set on the request before Do; transport-injected headers are not
seen.
- Dedupe opted-in header names against the default set and each other, so
WithCacheKeyHeaders("Accept") no longer folds values in twice.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
|
|
||
| if req.Method == http.MethodGet && resp.StatusCode == http.StatusOK { | ||
| cacheErr := c.baseHttpCache.Set(req, resp) | ||
| cacheErr := c.baseHttpCache.Set(req, resp, c.cacheOptions...) |
There was a problem hiding this comment.
🟡 Suggestion (high confidence): this Set re-derives the cache key from req after the round trip, while the Get at line 477 derived it before. http.Client.send forks the Request struct shallowly once Timeout > 0, so the Header map stays the caller's — a RoundTripper that sets a header in place (this repo's own userAgentTripper, transport.go:139, does exactly that) mutates the very request Do re-keys on. If such a header is named in WithCacheKeyHeaders, the entry is stored under a key no future Get can produce, so the cache silently never hits — stronger than what the new doc says ("not seen by the cache lookup and will not be reflected in the key"): it is reflected, but only on the write side. Consider computing the key once before the round trip and reusing it for Set (or snapshotting the opted-in header values), and either way tightening the doc wording and adding a test pinning the documented behavior now that TestWrapper_Do_CachesDespiteRoundTripHeaderInjection was removed.
| } | ||
| seenHeaders[key] = true | ||
| for _, value := range req.Header[key] { | ||
| headerParts = append(headerParts, fmt.Sprintf("%s=%s", key, value)) |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): now that arbitrary caller-named headers are folded in, the "%s=%s" parts joined with & are no longer an injective encoding — a header value containing & or = can produce the same joined string as a different set of headers, i.e. a cache-key collision between two requests that should be distinct. Values here are typically opaque tokens/tenant IDs so this is unlikely in practice, but escaping each name and value (e.g. url.QueryEscape) before joining would make the encoding unambiguous.
3 participants
Summary
CreateCacheKeyonly foldedAccept,Content-Type,Cookie, andRangeinto the HTTP response cache key. Any other request header was silently ignored, so two GET requests differing only in an unlisted header (e.g.Authorization, a custom API-version or tenant header) collided on the same cache entry and one request could be served the other's cached response.CreateCacheKey(req *http.Request, opts ...CacheOption)-- a trailing variadic, so every existing zero-argCreateCacheKey(req)/cache.Get(req)/cache.Set(req, resp)call keeps compiling unchanged.CacheOptionis an interface rather than a concrete parameter, leaving room for future dimensions (TTL, query-param keying, etc.) without touching these signatures again.WithCacheKeyHeaders(...)is aWrapperOption, configured once at client construction and applied to every request that client makes -- not a per-call option:BaseHttpClientstores the resultingCacheOption(s) and forwards them to everyGet/Setcall itself makes.Do's signature is completely untouched -- no sibling method, no per-call option, every existing call site keeps working exactly as before.req.Headerat key-computation time, not supplied by the caller -- so the key can never describe a token that isn't the one actually sent on that request.req.Header(same source as the default set), the request-clone fix inBaseHttpClient.Domatters here:http.Client.Doforks the*http.Requeststruct on every call onceTimeout > 0(whichuhttp.NewClientalways sets), but that fork is shallow, soHeaderstays the same map the caller passed in -- aRoundTrippermutating it between the cache'sGetandSetcalls (userAgentTripperdoes this forUser-Agent) would otherwise make theSetkey diverge from every futureGetkey for an opted-in header. Fixed by round-tripping onreq.Clone(req.Context())instead ofreq.Alternatives considered
Two other API shapes for the same fix were explored and are open for comparison:
CallOptiononDoitself (Do(req, uhttp.WithCacheKeyHeaders(...), ...)), values still read fromreq.Header.DoWithCacheKeyHeaders(req, map[string]string, ...)sibling method, values supplied directly by the caller rather than read fromreq.Header(traded the request-clone fix away, at the cost of the key being able to describe a value that doesn't match what's on the wire).This PR's design keeps
Doand every existing call site fully untouched, matches values to what's actually sent, and leaves room to grow viaCacheOptionwithout further signature churn -- at the cost of a per-client rather than per-call configuration point.Test plan
pkg/uhttp/client_test.go: default set unaffected by headers outside it; aCacheOptionopts a header in (case-insensitively) without widening the key to everything else. All pre-existing zero-argCreateCacheKey/Get/Setcalls compile and pass unmodified.pkg/uhttp/wrapper_test.go:TestWrapper_WithCacheKeyHeaders_DistinguishesRequestsconfirms differentAuthorizationvalues no longer collide while the same value still hits cache;TestWrapper_Do_CachesDespiteRoundTripHeaderInjectionconfirms the request-clone fix is load-bearing -- fails without it, passes with it.go test ./pkg/uhttp/...andgo test ./...(whole module)golangci-lint run ./pkg/uhttp/...go build ./...