Fix HttpServerConnection ETag check to use If-None-Match - #3033
Merged
Conversation
sendResponseHeaders() was checking the If-Match request header to decide whether to return 304 Not Modified for a cached GET/HEAD, but If-Match is for optimistic-concurrency writes (RFC 7232 3.1), not cache revalidation. Real browsers send If-None-Match when revalidating a cached resource, so this code path never actually returned 304 for normal traffic - HTTP_HEADER_IF_NONE_MATCH wasn't even defined, so the header got stored as a custom field and request.headers.contains(HTTP_HEADER_IF_MATCH) was always false for it. Added an If-None-Match entry to the header field map and switched the 304 check to use it, matching the conditional GET semantics in RFC 7232.
PR Summary
|
slaff
approved these changes
Jul 20, 2026
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
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.
The ETag caching in
HttpServerConnection::sendResponseHeaders()returns 304 Not Modified when the If-Match request header matches the response ETag. That's the wrong header for this - If-Match is for optimistic-concurrency checks on writes (RFC 7232 §3.1), not for cache revalidation. The header a normal browser sends when revalidating a cached GET is If-None-Match, and that one isn't handled at all.It goes a bit deeper than just checking the wrong constant, too:
HttpHeaderFieldName/HTTP_HEADER_FIELDNAME_MAPin HttpHeaderFields.h never had an entry for If-None-Match in the first place. Without an entry in the table, an incomingIf-None-Matchheader falls through to the custom-field path inHttpHeaderFields::fromString()/findOrCreate()and gets stored under its own dynamically-allocated field id, completely separate fromHTTP_HEADER_IF_MATCH. Sorequest.headers.contains(HTTP_HEADER_IF_MATCH)was always false for a request that only carries If-None-Match, and the 304 path could basically never be reached by a real client - only by something that deliberately sends If-Match on a GET, which isn't standard browser behavior.Fix: add an
IF_NONE_MATCHentry to the header field map, and switch the check in HttpServerConnection.cpp to use it instead of IF_MATCH.I confirmed the logic by porting the field-table lookup and the 304 condition (unchanged) into a small standalone program and running both the old and new versions: with the current code, a request carrying only If-None-Match never triggers 304; with the fix it does, and a bare If-Match no longer incorrectly triggers 304 on a GET. I didn't spin up the full Sming Host build for this (submodules like FlashString aren't initialized in my checkout), so I can't point to an in-tree test run, but the header-matching code path itself is unchanged from upstream in the repro.