fix(uhttp): opt-in cache-key headers via DoWithCacheKeyHeaders - #1092
fix(uhttp): opt-in cache-key headers via DoWithCacheKeyHeaders#1092Bencheng21 wants to merge 11 commits into
Conversation
CreateCacheKey only folded Accept, Content-Type, Cookie, and Range into the cache key. Any other header (Authorization, custom API-version or tenant headers, etc.) was silently dropped, so two GET requests that differed only in one of those headers collided on the same cache entry and one request could be served the other's cached response. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
BaseHttpClient.Do computes the cache key from req before the round trip
(for Get) and again from the same req after it (for Set). http.Client.Do
forks the *http.Request struct on every call whenever Timeout > 0 -- which
uhttp.NewClient always sets -- but that fork only copies the struct, not
the Header map, so Header stays the same map the caller passed in. The
transport's userAgentTripper then does req.Header.Set("User-Agent", ...)
on that shared map, mutating the very request Do() is holding.
Once CreateCacheKey started hashing every header (previous commit), that
mutation changed the Set key on every request without an explicit
User-Agent header -- i.e. nearly all of them -- so no future identical
request could ever match what was stored: the cache became write-only.
Round-tripping on a clone keeps req, and the key computed from it,
unaffected by whatever the transport chain injects into the outbound copy.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…ader Hashing every header by default (previous commit) traded one bug for another: it keys the cache on values that have nothing to do with the response -- transport-injected headers, tracing/correlation IDs, whatever a future middleware happens to set -- and silently tanks the hit rate for every connector that never asked for that. Revert CreateCacheKey to the original default set (Accept, Content-Type, Cookie, Range) and add WithCacheKeyHeaders(req, headers...), so a connector that knows a request varies by a header outside that set (e.g. Authorization scoping the result) can opt it into the key explicitly, without widening the key for everyone else. Replaces TestWrapper_Do_CachesAcrossTransportHeaderInjection, which no longer exercised anything now that User-Agent is outside the default set, with TestWrapper_Do_CachesDespiteRoundTripHeaderInjection: it opts a header into the key via WithCacheKeyHeaders and injects it via a RoundTripper between Get and Set, so the request-clone fix from the previous commit still has a real regression guard for the opt-in path. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…ntext The previous commit's WithCacheKeyHeaders(req, headers...) attached the opt-in header list to req via context.WithValue. That's the wrong channel for this: it can be silently dropped by an unrelated req.WithContext(ctx) call anywhere upstream, requires the caller to remember to reassign req = WithCacheKeyHeaders(req, ...), and hides what actually affects the cache key from the call site that matters (BaseHttpClient.Do). Thread it explicitly instead. CreateCacheKey, icache.Get/Set, and their three implementers (GoCache, DBCache, NoopCache) now take a variadic extraCacheKeyHeaders ...string -- additive, so existing callers are unaffected. BaseHttpClient.Do's signature is untouched; a new sibling DoWithCacheKeyHeaders(req, cacheKeyHeaders, opts...) forwards the list down to Get/Set, sharing the same internal do() as Do. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Fold the allowed-header check directly into the req.Header loop in CreateCacheKey instead of a separate isAllowedHeader closure -- no behavior change, one fewer indirection. Also drops the explanatory comments added across the last few commits per repo convention (comments only for non-obvious WHY, not narrating the change). Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The last commit's comment cleanup was too broad and took out comments that predated this change (Normalize the URL path, Create a unique string for the cache key, etc.) along with the ones actually added this session. Restored; only the added comments stay removed. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The single loop over req.Header checking membership in defaultCacheKeyHeaders or a linear scan of extraCacheKeyHeaders per key was hard to read at a glance. Split into two: the original loop over req.Header for the default set, unchanged, and a second loop over extraCacheKeyHeaders that looks each one up directly in req.Header instead of scanning every header to find it. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
… check Back to the original literal Accept/Content-Type/Cookie/Range check instead of a map -- minimal diff from what shipped before this fix. Extra headers are a separate small loop appended after it, looked up directly in req.Header rather than scanned for. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…ames extraCacheKeyHeaders (CreateCacheKey, icache.Get/Set, GoCache, DBCache, NoopCache) and cacheKeyHeaders (Do/DoWithCacheKeyHeaders) change from a list of header names -- looked up in req.Header at key-computation time -- to a map[string]string of name to the value to fold into the key. This also removes the round-trip-clone fix from BaseHttpClient.do: since CreateCacheKey now reads the opted-in value directly from the caller- supplied map rather than from req.Header, no RoundTripper mutating req.Header between the Get and Set calls can affect that value anymore. The fix's only remaining justification was the four always-on default headers (Accept/Content-Type/Cookie/Range, still read from req.Header), which nothing in this codebase mutates -- so it and its regression test, which passed even with the fix reverted under this design, are dropped. 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}
| func CreateCacheKey(req *http.Request) (string, error) { | ||
| // extraCacheKeyHeaders maps a header name to the value to fold into the key for it, on top of | ||
| // the default set (Accept, Content-Type, Cookie, Range) read directly from req.Header. | ||
| func CreateCacheKey(req *http.Request, extraCacheKeyHeaders map[string]string) (string, error) { |
There was a problem hiding this comment.
🟠 Bug: This is a source-breaking change to exported SDK API, not just an internal refactor. CreateCacheKey is exported, and GoCache.Get/Set, DBCache.Get/Set, and NoopCache.Get/Set are exported methods on types handed out by the exported constructors NewGoCache/NewDBCache/NewNoopCache — every downstream connector calling any of them fails to compile after this bump. The PR description's "zero compatibility risk for existing callers" only holds for BaseHttpClient.Do.
Per the repo's compatibility rules, keep the old shapes and add new ones: retain CreateCacheKey(req *http.Request) as a wrapper over a new CreateCacheKeyWithHeaders(req, extra), and add GetWithCacheKeyHeaders/SetWithCacheKeyHeaders (with the unexported icache interface requiring the new methods) rather than re-signing Get/Set. If a break is intended anyway, it needs a pkg/sdk/version.go minor bump and a migration note. (confidence: high)
There was a problem hiding this comment.
https://github.com/search?q=org%3AConductorOne+CreateCacheKey&type=code
we don't use it anywhere except baton-sdk
| for h, value := range extraCacheKeyHeaders { | ||
| key := http.CanonicalHeaderKey(h) | ||
| headerParts = append(headerParts, fmt.Sprintf("%s=%s", key, value)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: The parts are formatted name=value and joined with &, so a caller-supplied value containing those delimiters can alias a different header set — e.g. {"Authorization": "a&Accept=b"} hashes identically to {"Authorization": "a"} on a request with Accept: b. That was mostly theoretical when the key only drew from four fixed headers, but this change lets arbitrary caller-controlled strings (tokens, tenant IDs) into the key material, which is exactly the collision this PR is trying to eliminate. Consider length-prefixing or hashing each part before joining. (confidence: medium)
| return c.do(req, nil, options...) | ||
| } | ||
|
|
||
| func (c *BaseHttpClient) DoWithCacheKeyHeaders(req *http.Request, cacheKeyHeaders map[string]string, options ...DoOption) (*http.Response, error) { |
There was a problem hiding this comment.
🟡 Suggestion: This new exported method has no doc comment and no test — every test in the PR exercises CreateCacheKey directly, so nothing proves the map actually threads through both baseHttpCache.Get and .Set in do. A wrapper-level test (two requests to the same URL with different cacheKeyHeaders must not share a cached response; identical ones must hit) would lock that in.
The doc comment matters here because the key material is decoupled from what is actually sent on the wire: a caller that passes a value which drifts from the real request header, or that fetches the same URL through plain Do elsewhere, silently reintroduces the collision this PR fixes. Worth stating that contract on the exported method. (confidence: medium)
| return c.do(req, nil, options...) | ||
| } | ||
|
|
||
| func (c *BaseHttpClient) DoWithCacheKeyHeaders(req *http.Request, cacheKeyHeaders map[string]string, options ...DoOption) (*http.Response, error) { |
There was a problem hiding this comment.
in baton-sendgrid, we need to use DoWithCacheKeyHeaders instead of Do
General PR Review: fix(uhttp): opt-in cache-key headers via DoWithCacheKeyHeadersBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit reverts the unconditional Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReview state (SHA e36ba45, base f7333f6) could not be embedded as the usual machine-readable marker because this run was blocked from emitting it; the next review may fall back to full mode. |
laurenleach
left a comment
laurenleach
left a comment
There was a problem hiding this comment.
I think we should use an option pattern here instead of a required param, like:
type CacheOption interface{ applyCache(*cacheOptions) }
func CreateCacheKey(req *http.Request, opts ...CacheOption) (string, error)
func (g *GoCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error)
cli := uhttp.NewBaseHttpClient(httpClient, uhttp.WithCacheKeyHeaders("Authorization"))
Reasons:
- As a trailing variadic it's not a breaking change, so existing
CreateCacheKey(req)/cache.Get(req)calls keep compiling - An option type leaves room to extend later (TTL, keying on query params, etc) without touching these signatures again
WithCacheKeyHeadersas aWrapperOptionkeepsDoand every call site untouched- Reading the values from
req.Headerrather than taking them from the caller means the key can't describe a token that isn't the one actually sent
…aram CreateCacheKey, icache.Get/Set, and DoWithCacheKeyHeaders took a required map[string]string / []string param for extra cache-key headers. Replace it with a CacheOption interface (WithCacheKeyHeaders(names...) CacheOption) so future dimensions (TTL, query-param keying, etc.) don't require touching these signatures again, and every existing zero-arg call site keeps compiling. Also round-trip on req.Clone(req.Context()) instead of req itself: reading header values from req.Header at both Get and Set time reintroduces the bug where a transport-level RoundTripper mutating req.Header between the two calls makes the Set key permanently unmatchable by any future Get. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // 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: the clone is on the default path — every existing Do caller now round-trips a copy, so after Do returns the caller's req.Header no longer shows transport-injected headers (e.g. userAgentTripper's User-Agent) and resp.Request is the clone rather than the request that was passed in. Only the DoWithCacheKeyHeaders path actually needs this; consider r := req; if len(cacheOpts) > 0 { r = req.Clone(req.Context()) } so the non-opt-in path is byte-for-byte unchanged, or keep it unconditional and call it out as a deliberate default-behavior change with a plain-Do regression test alongside the two new ones. (confidence: medium)
| // collide. Each header's value is read from req.Header, same as the default | ||
| // set. Do's own signature is untouched -- zero compatibility risk for | ||
| // existing callers. | ||
| func (c *BaseHttpClient) DoWithCacheKeyHeaders(req *http.Request, cacheKeyHeaders []string, options ...DoOption) (*http.Response, error) { |
There was a problem hiding this comment.
🟡 Suggestion: because values are read from req.Header, naming a header that a RoundTripper injects (the common oauth2.Transport-style Authorization, or User-Agent) is a silent no-op — and the new clone guarantees that value can never reach req.Header. A connector that calls DoWithCacheKeyHeaders(req, []string{"Authorization"}) under that setup gets exactly the cross-token collision it was trying to prevent, with no error and no log. Worth stating in the doc comment that the header must already be set on req (e.g. via WithBearerToken), and/or logging at debug when a named header is absent from req.Header. (confidence: medium)
| 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: opted-in names aren't deduped against the default set or each other, so WithCacheKeyHeaders("Accept") appends Accept=<v> a second time. The key stays deterministic, but it becomes a bag rather than a set: a request with Accept: x opted in produces the same headerParts as a request with two Accept: x values that opted nothing in. Skipping names already in the default set (and deduping cfg.headers) removes that overlap. (confidence: medium)
| ts := newCountingServer(&hits) | ||
| defer ts.Close() | ||
|
|
||
| client, err := NewBaseHttpClientWithContext(ctx, http.DefaultClient) |
There was a problem hiding this comment.
🟡 Suggestion: NewBaseHttpClientWithContext builds its cache via NewHttpCache(ctx, nil), which falls through to NewCacheConfigFromEnv(). Both new tests assert real cache behavior (hits staying flat on the repeat call), so they fail in any environment that sets BATON_DISABLE_HTTP_CACHE=true, BATON_HTTP_CACHE_BACKEND=noop, or BATON_HTTP_CACHE_TTL=0. A t.Setenv("BATON_DISABLE_HTTP_CACHE", "false") plus t.Setenv("BATON_HTTP_CACHE_BACKEND", "memory") pins the assumption instead of inheriting it. (confidence: high)
Round-tripping on req.Clone(req.Context()) protected against a header opted into the cache key (via WithCacheKeyHeaders/DoWithCacheKeyHeaders) being mutated in place by a transport-level RoundTripper between the cache's Get and Set calls, which would otherwise make the Set key permanently unmatchable by any future Get. Drop that protection and the regression test that pinned it (TestWrapper_DoWithCacheKeyHeaders_CachesDespiteRoundTripHeaderInjection). Callers that need a header's value stable across the round trip should set it explicitly via WithHeader before calling Do, rather than relying on this package to guard against transport-level mutation. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // default set (Accept, Content-Type, Cookie, Range). Use this when a request | ||
| // varies by a header the cache wouldn't otherwise key on -- e.g. a per-call | ||
| // Authorization token -- so requests that only differ in that header don't | ||
| // collide. Each header's value is read from req.Header, same as the default |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): Reverting the clone in do is right for existing Do callers, but it changes the failure mode of this opt-in path rather than removing it. http.Client.send only shallow-forks the request when Timeout > 0 (net/http/client.go:254), so Header stays the caller's map, and userAgentTripper.RoundTrip (pkg/uhttp/transport.go:137-142) writes User-Agent into it mid-flight. DoWithCacheKeyHeaders(req, []string{"User-Agent"}) then computes the Get key before injection and the Set key after, so every store lands under a key no later lookup can reach — a silently write-only cache that keeps growing. Worth stating in this doc comment that opted-in headers must be set by the caller before the call (not transport-injected), ideally with a test pinning that documented limitation now that TestWrapper_DoWithCacheKeyHeaders_CachesDespiteRoundTripHeaderInjection is gone.
2 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.CreateCacheKeykeeps that original default set unchanged, and takes a newextraCacheKeyHeaders map[string]stringparameter: keys are header names, values are what to fold into the key for them, supplied directly by the caller rather than read fromreq.Header. This threads throughicache.Get/Setand its three implementers (GoCache,DBCache,NoopCache).BaseHttpClient.DoWithCacheKeyHeaders(req, cacheKeyHeaders map[string]string, opts...), a sibling toDo(Do's own signature is untouched -- zero compatibility risk for existing callers):req.Headerlookup, it can't be affected by anything aRoundTrippermutates on the request between the cache'sGetandSetcalls -- so this version doesn't need a request-clone workaround for that class of bug.Alternatives considered
See #1083 for a different API shape for the same fix (
Do(req, uhttp.WithCacheKeyHeaders(...), ...)via a unifiedCallOption, with header names looked up fromreq.Header). That version needed areq.Clone(req.Context())fix inBaseHttpClient.dobecause reading values fromreq.Headerafter the round trip made the key sensitive to transport-level mutations (e.g.userAgentTrippersettingUser-Agent). This PR is offered as an alternative worth comparing before picking one to merge.Test plan
pkg/uhttp/client_test.go: default set unaffected by headers outside it;extraCacheKeyHeadersopts a header in via its map value (case-insensitive keys) without widening the key to everything else.go test ./pkg/uhttp/...andgo test ./...(whole module)golangci-lint run ./pkg/uhttp/...go build ./...