[ResponseCaching] Correctly handle Vary: * in delimited or multi-value headers (RFC 9111) - #69210
Conversation
- Comply with RFC 9111 § 4.1 by inspecting each delimited token within Vary headers. - Iterate StringValues directly with StringTokenizer to prevent string allocations on hot paths. - Add unit and integration tests covering multi-entry and delimited '*' headers. Fixes dotnet#69192
|
Thanks for your PR, @Sadik00789. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
@dotnet-policy-service agree |
| var tokenizer = new StringTokenizer(rawHeader, HeaderDelimiters); | ||
| foreach (var segment in tokenizer) | ||
| { | ||
| if (segment.Trim().Equals("*", StringComparison.Ordinal)) | ||
| { | ||
| context.Logger.ResponseWithVaryStarNotCacheable(); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Any reason to not use a span here? Something like:
var rawHeader = varyHeader[i].AsSpan();
if (rawHeader.Length == 0)
{
continue;
}
foreach (var segment in rawHeader.Split(','))
{
if (rawHeader[segment].Trim().SequenceEquals("*"))
{
context.Logger.ResponseWithVaryStarNotCacheable();
return false;
}
}There was a problem hiding this comment.
Updated to use Span.Split(',') and zero-allocation span checks, and cleaned up the delimiters array. Thanks for the suggestion!
Youssef1313
left a comment
There was a problem hiding this comment.
What about OutputCaching?
|
During the initial audit, I checked src/Middleware/OutputCaching/ and found that OutputCaching does not inspect context.Response.Headers.Vary when determining cache eligibility (IOutputCachePolicy / AllowCacheStorage). Cache vary behavior there is driven strictly by its own policy rules (CacheVaryByRules / OutputCacheKeyProvider). Did you want OutputCacheMiddleware / DefaultOutputCachePolicy to also inspect downstream response headers and refuse to cache if Vary: * is present, or should CacheVaryByRules handle * explicitly? If so, I'm happy to tackle that in this PR or a follow-up issue. |
…er.cs Co-authored-by: Jiri Cincura ↹ <jiri@cincura.net>
…er.cs Co-authored-by: Jiri Cincura ↹ <jiri@cincura.net>
|
Applied the suggested changes ( |
cincuranet
left a comment
There was a problem hiding this comment.
LGTM. @Youssef1313 do you want to give it a look?
Fixes #69192.
Per RFC 9111 § 4.1, a response containing
*anywhere as a member of theVaryheader field MUST NOT be used to satisfy a subsequent request. Previously,ResponseCachingPolicyProviderperformed an exact match onvaryHeader.Count == 1 && string.Equals(varyHeader, "*", ...)which failed when:varyHeader.Count > 1.Varyheader contained comma-delimited tokens (e.g.,*, Accept-Encoding).Changes
OutputCachingdetermines vary behavior via explicit cache key rules (IOutputCachePolicy,CacheVaryByRules) rather than readingcontext.Response.Headers.Vary. No changes required there.MemoryExtensions.Split(',')onrawHeader.AsSpan().StringValuesdirectly by index to avoidstring.Joinallocations.SequenceEqual("*")without heap allocations.IsResponseCacheable_VaryHeaderByStar_NotAllowedto a[Theory]covering single strings, multi-entryStringValues, and delimited values with spaces.*remain cacheable.ResponseCachingMiddlewareTestswith downstream appended headers to verify subsequent requests are not served from cache.Customer Impact
Prevents incorrectly caching responses that vary by
*when response compression or other middleware appends headers toVary.Regression?
No.
Risk
Low. Restricts caching behavior to strictly adhere to RFC 9111 without allocating heap strings.