dotnet / dotnet/aspnetcore

Harden DistributedCacheTagHelper key serialization to be injective (CacheTagKey)

Open
#67,775 0 comments 0 reactions 0 assignees View on GitHub
area-mvc feature-tag-helpers
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 10h
Merged PRs (30d)
281

Description

## Summary

`CacheTagKey.GenerateKey()` serializes the cache-key state by concatenating components with an unescaped `||` token separator. The serialization is not injective: two different structured key states (different sets of vary-by name/value pairs) can produce the same serialized string. For the `DistributedCacheTagHelper` path this serialized string is hashed and used as the distributed storage key, so distinct logical key states can collapse onto a single distributed cache entry.

This is filed as a defense-in-depth hardening item. The request is to make the distributed key serialization injective so distinct key states always map to distinct storage keys.

## What is wrong

* `CacheTagKey.GenerateKey()` builds the key as text using the constant `CacheKeyTokenSeparator = "||"` for both intra-pair and inter-pair boundaries, with no escaping or length-prefixing of component values. Because the same token separates a name from its value and one pair from the next, the mapping from structured state to string is ambiguous (not injective).
* `DistributedCacheTagHelperService` derives the distributed storage key from `GenerateHashedKey()` (SHA-256 over `GenerateKey()`), and also embeds the serialized key alongside the cached payload as a validation check. Both the hash input and the embedded validation bytes are the ambiguous serialization, so neither detects two distinct states that serialize identically.
* The in-memory `CacheTagHelper` path is not affected: it keys `IMemoryCache` by the structured `CacheTagKey` object, whose `Equals`/`GetHashCode` compare the individual components. Only the distributed path relies on the flattened string.

## Why it matters (defense in depth)

* Correctness: independent of any adversary, an application that varies a distributed-cached fragment by values that can contain the separator sequence can get incorrect cache reuse (one logical variant served for another). This is a latent caching-correctness bug.
* Hardening: the distributed cache key is an isolation boundary between logical fragments. An injective key generator guarantees that distinct vary states cannot share a storage entry, strengthening fragment isolation as a matter of robustness rather than relying on applications to avoid particular characters in vary values.
* The existing embedded-serialized-key validation was designed to catch different strings that share a hash digest; it cannot catch two states that share the same pre-hash serialization. Making the serialization injective closes that gap at the source.

## Affected code

* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:26-33 - `CacheKeyTokenSeparator` and vary-section name constants.
* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:113-167 - `GenerateKey()` string assembly (scalars, collections, user, culture).
* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:169-183 - `GenerateHashedKey()` (SHA-256 over the generated string).
* src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs:292-320 - `AddStringCollection()` collection serialization (the ambiguous name/value framing).
* src/Mvc/Mvc.TagHelpers/src/Cache/DistributedCacheTagHelperService.cs:65-117 - lookup, render-on-miss, and value encoding using the serialized/hashed key.
* src/Mvc/Mvc.TagHelpers/src/Cache/DistributedCacheTagHelperService.cs:164-202 - embedded serialized-key encode/validate on the cached value.

Parity note: the relevant source is byte-identical across `origin/main` and `origin/release/10.0`, `release/9.0`, `release/8.0`, so the same fix applies to servicing branches. Introduced by commit `341430eae508912d3a868bf636b86ec04155378e` ("Introducing CacheTagKey").

## Recommended fix

Selected approach: replace the delimiter-based text serialization with a single versioned, injective canonical encoding, and use the exact same canonical bytes for (a) the SHA-256 storage key and (b) the embedded validation bytes.

Design points:

* Frame every component with an explicit length (or an unambiguous binary layout) so no value can be confused with a separator or with an adjacent component. Encode collections with an explicit element count, and frame each name and each value independently.
* Preserve the current structured identity exactly: base key/prefix, `vary-by`, the four ordered collections (cookie, header, query, route), `vary-by-user` (username) and `vary-by-culture` (culture + UI culture). Keep expiration fields out of the content key, matching current behavior.
* Preserve existing normalization: a missing selected value maps to empty string today (`ExtractCollection`), and header/query `StringValues` are already collapsed to a single string before the pair is built. Keep both behaviors; only the framing changes.
* Distinguish null vs empty consistently (for example a null collection vs a present-but-empty collection), so the encoding matches the structured object identity rather than reintroducing hidden aliases.
* Version and namespace the output: prefix the distributed storage key with an explicit format version. Increment the version so pre-fix (v1) entries are not read by post-fix code.

Public surface / compatibility:

* `GenerateKey()` and `GenerateHashedKey()` are public. Prefer keeping their signatures; `GenerateKey()` may return a versioned printable representation of the canonical bytes rather than the legacy grammar. No `PublicAPI.Shipped.txt` change is expected if signatures are unchanged.
* Cache invalidation: changing the key format makes existing v1 distributed entries unreachable. They should expire under their existing policy. Do not implement a v1 read-fallback or dual-write, since either would reinstate the ambiguous mapping on a v2 miss. During a rolling deployment, patched and unpatched instances will use separate namespaces (temporary duplicate entries and cold misses); call this out in release notes.

Alternatives considered:

* Escaping the separator: requires a complete, permanently stable escape grammar for the escape char, `||`, the collection parentheses, and any future component; a single missed context reintroduces the ambiguity. Rejected.
* Rejecting values that contain the separator: breaks legitimate vary values and turns valid requests into failures or cache bypasses; it filters symptoms rather than fixing the serialization. Rejected.
* Textual length-prefixing: viable and injective, but adds decimal-parsing and character-vs-byte-length edge cases with no operational benefit over a binary canonical form (the final storage key is already a digest). Not selected.

## Acceptance criteria

- [ ] For any two `CacheTagKey` states that are unequal by `Equals`, `GenerateKey()` and `GenerateHashedKey()` produce different results (injectivity), including when values contain the separator or collection punctuation.
- [ ] The distributed storage key, the hash input, and the embedded validation bytes all derive from one canonical encoding.
- [ ] The in-memory `CacheTagHelper` behavior is unchanged.
- [ ] Deterministic format vectors lock the encoding: ASCII, non-ASCII, empty, null-vs-empty, each collection boundary, each pair boundary, user, and culture components.
- [ ] A regression test proves two previously colliding vary states resolve to distinct distributed entries and render independently.
- [ ] A test confirms v2 code does not read a pre-populated v1 (legacy) storage entry.
- [ ] The same encoding is applied on main and backported unchanged to release/8.0, 9.0, 10.0; release-note text covers one-time cache misses and expiration of unreachable legacy entries.

## References

* Existing tests to extend: src/Mvc/Mvc.TagHelpers/test/CacheTagKeyTest.cs and src/Mvc/Mvc.TagHelpers/test/DistributedCacheTagHelperTest.cs.
* Detailed design and remediation notes were produced during investigation and can be shared with the implementer on request.

Contributor guide

Open the contributing guide

Research direction

Start with CacheTagKey.cs, especially GenerateKey(), GenerateHashedKey(), and AddStringCollection(), then trace how DistributedCacheTagHelperService stores and validates keys. Extend CacheTagKeyTest.cs and DistributedCacheTagHelperTest.cs with deterministic encoding, collision, and legacy-entry cases. Done means distinct CacheTagKey states remain distinct through hashing and distributed lookup, while in-memory behavior is unchanged and release-note requirements are covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.