Fix query result cache leaking across security contexts: include full SecurityClaims in cache key#9704
Open
nishantmonu51 wants to merge 2 commits into
Open
Fix query result cache leaking across security contexts: include full SecurityClaims in cache key#9704nishantmonu51 wants to merge 2 commits into
SecurityClaims in cache key#9704nishantmonu51 wants to merge 2 commits into
Conversation
The resolver query cache key only hashed the claims' user attributes, so requests differing only in additional security rules (e.g. locked filters on magic auth tokens), permissions, or skipped security checks produced identical keys and could receive each other's cached results. Hash the full marshaled SecurityClaims instead, matching the security engine's own computeCacheKey.
nishantmonu51
changed the title
SecurityClaims\ in cache keySecurityClaims in cache key
Collaborator
Author
|
@codex: review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
pjain1
reviewed
Jul 20, 2026
| // Hash the full security claims, not just the user attributes: | ||
| // permissions, additional rules (e.g. locked filters on magic auth tokens) and skipped checks | ||
| // all change the resolved security policy, and results must not be shared across them. | ||
| claimsJSON, err := json.Marshal(opts.Claims) |
Member
There was a problem hiding this comment.
nit: not sure about the improvement but should we skip hashing userID (present in Claims) as it will prevent cross user cache result sharing if everything else is same?
Member
There was a problem hiding this comment.
May be its fine as it is as user attributes have email which would be unique anyways, might help in embed use cases a little.
The user ID does not affect the resolved security policy: built-in rules derive user identity from the "id" and "email" user attributes, which remain part of the key. Excluding it lets requests that resolve to the same policy share cached results, which helps embed use cases where the user ID varies per session. Addresses review feedback from @pjain1.
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.
runtime.Resolveonly hashedClaims.UserAttributes, so requests that differed only inAdditionalRules(e.g. locked filters on magic auth tokens),Permissions, orSkipChecksproduced identical cache keys and could receive each other's cached results.SecurityClaimsis hashed into the key, matching what the security engine'scomputeCacheKeyalready does.Resolvecovers all resolvers (metrics,metrics_sql, annotations, unions); the legacyruntime.Querypath already included the full claims in its key and was not affected.metrics_security.yamlthat run the same query back-to-back with identical (empty) user attributes but different security contexts; without the fix, three of the four cases fail by returning another context's cached rows.Steps to reproduce (before this fix):
country, and an explore dashboard on it.country = 'US', URL B locked tocountry = 'EU'. Both are anonymous, so both carry empty user attributes; the locked filters travel asAdditionalRules.skip_nested_security: trueand one without; call the skipping one first, then the enforcing one with identical user attributes — the second returns the unfiltered cached result.Checklist: