Harden CacheTagHelper vary-by-user: cache key can collide for distinct authenticated principals without a stable Identity.Name
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 276
Description
## Summary
`CacheTagKey` derives the `vary-by-user` component of the cache key from `HttpContext.User?.Identity?.Name`, a value that is nullable and carries no framework-level uniqueness guarantee. As a result, two distinct authenticated principals can produce an equal `CacheTagKey`, which breaks the per-user isolation property that `vary-by-user` exists to provide.
## What is wrong
* Broken invariant: `vary-by-user="true"` is intended to guarantee that a cached fragment produced while one principal is signed in is never returned to a different principal. The current key derivation does not establish that invariant, because the only per-user input is `Identity.Name`, which may be `null` or shared across distinct accounts depending on how the application configures authentication.
* Where it originates: `CacheTagKey` captures `Identity.Name` verbatim, then uses it in structured equality, in `GetHashCode`, and in the generated key string. When that value is `null`/empty for an authenticated principal, the user component of the key degrades to an empty token that is indistinguishable from the anonymous case and identical for every such principal.
* Both consumers are affected: `CacheTagHelper` uses `CacheTagKey` as an `IMemoryCache` key, and `DistributedCacheTagHelper` uses the generated string as the distributed cache key.
* The current behavior is intentional and documented (`vary-by-user` is documented as keying on `@User.Identity.Name`), and the empty-suffix case is locked by existing tests. This is therefore a hardening request rather than a defect report: the framework should not silently collapse distinct authenticated principals into a shared cache bucket when it cannot obtain a stable user key.
## Why it matters (defense in depth)
* Correctness: `Equals`/`GetHashCode` on `CacheTagKey` claim that two keys represent the same cache state. For an authenticated-but-nameless principal that claim is not true, so a cache entry is shared between states that are not equivalent.
* Hardening: `vary-by-user` is an isolation primitive. Applications reasonably read it as "this fragment is scoped to the signed-in user." An isolation primitive that silently degrades to no isolation when its single input is absent is a weak default. Failing closed (or falling back to another stable identifier) removes an entire class of misconfiguration from the application's responsibility.
* The documented, standard configuration (ASP.NET Core Identity) sets the name claim to the unique `UserName`, so this does not affect default Identity-based applications. The gap is limited to applications whose authentication does not populate a unique `ClaimTypes.Name` - for example custom cookie or JWT setups that identify users by `sub`/`NameIdentifier` only.
## Affected code
* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:98-101 - `_username` is assigned directly from `httpContext.User?.Identity?.Name` with no authenticated/nameless distinction
* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:144-151 - `GenerateKey()` appends the `VaryByUser` token followed by the possibly empty `_username`
* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:208-209 - `Equals` compares `_username` ordinally; equal-or-both-null names compare as equal
* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:251 - `GetHashCode` includes `_username`, so colliding keys also hash equally
* src/Mvc/Mvc.TagHelpers/src/CacheTagHelper.cs:69-77 - uses `CacheTagKey` as the `IMemoryCache` lookup key
* src/Mvc/Mvc.TagHelpers/src/DistributedCacheTagHelper.cs:61 - constructs `CacheTagKey` for the distributed cache path
* src/Mvc/Mvc.TagHelpers/test/CacheTagKeyTest.cs:328-360 - existing tests that pin the current empty-suffix behavior and will need to be revisited
* src/Identity/Extensions.Core/src/UserClaimsPrincipalFactory.cs:79 - reference point showing the standard Identity path sets the name claim to the unique `UserName`
## Recommended fix
Selected approach: keep `Identity.Name` as the primary user key so existing keys are byte-for-byte unchanged for principals that have a name, and only change behavior in the case that is currently unsafe. When `VaryByUser` is `true` and `User.Identity.IsAuthenticated` is `true` but `Identity.Name` is null or empty, fall back to a stable claim (`ClaimTypes.NameIdentifier`, then `sub`) for the user component. If no stable identifier is available either, treat the entry as non-shareable: bypass the cache for that request (render the child content and do not store or serve a shared entry) rather than joining the empty bucket. The unauthenticated case keeps its current empty user component, so anonymous fragment caching is unaffected.
Alternatives considered:
* Always key on `NameIdentifier`/`sub` instead of `Identity.Name`. Rejected: it contradicts the documented contract, changes the generated key for every application, and invalidates every existing distributed cache entry on upgrade.
* Throw when `VaryByUser` is set and no stable user key is available. Rejected: this turns a working (if under-isolated) page into a runtime failure during a patch or minor upgrade, which is too aggressive for a hardening change.
* Documentation-only change noting that `vary-by-user` requires a unique, non-null `Identity.Name`. Worth doing regardless, but it leaves the weak default in place.
* Introduce an opt-in switch (`AppContext` flag or option) for the stricter behavior. Reasonable fallback if the behavior change is judged too risky for a servicing branch; the stricter behavior should still become the default in the next major.
Compatibility, migration, and versioning:
* `CacheTagKey.GenerateKey()` and `GenerateHashedKey()` are public API; their output for named principals must not change. Confining the change to the authenticated-and-nameless branch preserves that.
* For `DistributedCacheTagHelper`, any key-shape change invalidates existing entries in shared stores. The proposed scoping avoids invalidating entries for named principals; entries previously stored under the empty bucket by nameless authenticated principals simply stop being served, which is the desired outcome.
* Target `main` only. Because the current behavior is documented and unit-tested, this is not a servicing candidate; do not backport unless the area owners decide otherwise.
* Behavior change is observable (a fragment that was previously cached for nameless authenticated principals is no longer shared), so it needs a breaking-change note in the release notes.
## Acceptance criteria
* [ ] Two distinct authenticated principals that both lack a `ClaimTypes.Name` value never resolve to the same `CacheTagKey` for the same cache element, and never receive each other's cached fragment.
* [ ] The generated key and hashed key are unchanged for principals that have a non-empty `Identity.Name`.
* [ ] The unauthenticated/anonymous path retains its existing key shape and continues to share a single cache entry.
* [ ] `Equals` and `GetHashCode` agree with the new key derivation in all four cases: anonymous, authenticated with name, authenticated without name but with a stable claim, authenticated with no usable identifier.
* [ ] Tests cover both `CacheTagHelper` (in-memory) and `DistributedCacheTagHelper` (generated string key) paths.
* [ ] `src/Mvc/Mvc.TagHelpers/test/CacheTagKeyTest.cs` is updated so that the empty user component is asserted only for genuinely unauthenticated principals.
* [ ] Compatibility and migration expectations are documented, including the note that `vary-by-user` isolates by `Identity.Name` and that applications should ensure it is a stable, unique, non-null per-account value.
Contributor guide
Research direction
Start with src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs, then compare its consumers in CacheTagHelper.cs and DistributedCacheTagHelper.cs. Run src/Mvc/Mvc.TagHelpers/test/CacheTagKeyTest.cs and add coverage for authenticated and anonymous principals, verifying key equality, in-memory behavior, and distributed key generation against the acceptance criteria.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend, performance
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100