Compare If-None-Match the way RFC 9110 defines it - #62
Conversation
web-etag-match? was a single string equality between the whole
If-None-Match field value and the first value of the response's ETag
header, so three behaviours §13.1.2 requires never fired:
* never matched, so a client asking "304 me if this exists" always got
a full body; a list ("a", "b") was compared as one opaque string and
never matched anything, so a client holding several cached variants
re-downloaded every time; and the comparison was strong, so W/"x"
never matched "x" in either direction even though If-None-Match is
specified to use the weak function.
The list is now parsed per §8.8.3.2. The scan is quote-aware rather
than a String.split-by on commas because etagc admits %x2C, so a comma
inside a quoted tag is part of the tag and must not split it.
web-conditional-not-modified? never looked at the method, so a matching
If-None-Match on a PUT answered 304. §13.2.2 makes 304 a GET/HEAD-only
outcome; every other method now gets a bodyless 412, built fresh rather
than routed through web-not-modified, which is the §15.4.5 header-drop
path for 304s only. If-Modified-Since is likewise GET/HEAD-only
(§13.1.3), and preconditions are ignored altogether on a non-2xx
response (§13.2.1) — without that guard `*` would have turned every 404
into a 304.
If-Match, If-Unmodified-Since and If-Range are deliberately out of
scope: they need a precondition-evaluation order, and http's new
Precondition module is not in the pinned 0.4.2.
There was a problem hiding this comment.
Build & Tests
carp -x test/web.carp — 356 passed, 0 failed. carp -x test/websocket.carp — 155 passed, 0 failed. carp -x gendocs.carp leaves the working tree clean, so docs/ still reproduces from the branch. Exit codes read from the unpiped commands. CI green at 973dddf (this repo runs the one macOS leg), verified through check-runs at that exact SHA. Branch based on afa412c, still origin/main's head; one bot commit. CHANGELOG entry is under ## Unreleased / ### Fixed, and main still has that section open, so it is filed correctly.
Findings
1. The 412 throws away every header and every cookie the response had built up
web.carp:1939-1941 and web.carp:2088-2091. web-precondition-failed builds a response from nothing:
(Response.init 412 @"Precondition Failed" @"HTTP/1.1" [] {} @"")
but it replaces ranged, which is the response after web-run-after has run the after-hooks (web.carp:2085). CORS.after-hook adds Access-Control-Allow-Origin to every response, so on this path it is added and then discarded. Measured with a CORS after-hook installed and a handler that sets ETag, Location, X-Custom and a cookie:
code ACAO X-Custom Location cookies
GET no INM 200 true true false 1
GET INM matches 304 true true false 1
PUT no INM 201 true true true 1
PUT INM matches 412 false false false 0
The 304 row is the contrast: web-not-modified drops only the four representation headers §15.4.5 names and keeps everything else, cookies included. The 412 keeps nothing.
That makes this a regression on the exact request the PR is fixing. Before this branch, PUT … If-None-Match: "abc" got a 304 — the wrong status, but one that carried Access-Control-Allow-Origin. Now it gets the right status with no CORS header at all, so a browser client does not see a 412: it sees the fetch rejected by the CORS check, with no status to read. Same for a Set-Cookie the handler had already issued.
Building the 412 the way web-not-modified builds the 304 — from ranged, with the status replaced, the body emptied and the representation headers dropped — fixes it and keeps the two paths symmetric. Worth an assertion that a 412 carries a header an after-hook added, since none of the new tests install a hook.
2. The request side is now case-insensitive, the response side still is not
web.carp:1926: (Map.get-maybe (Response.headers resp) "ETag") is an exact-case lookup, and web-if-modified-since? does the same for Last-Modified. The If-None-Match side of the same function now goes through header-values-ci. A handler that sets Etag or etag gets no conditional handling:
GET, ETag: "abc", If-None-Match: "abc" -> 304
GET, etag: "abc", If-None-Match: "abc" -> 200
Pre-existing, and response header names are the handler's own so it is much less likely to bite than the request side — but the asymmetry is new, and header-values-ci is right there.
3. The list parser recovers where the field should be ignored
web-parse-etag-list skips a malformed member and keeps going, so a trailing garbage run does not invalidate the tags before it:
parse "a"junk => ["a"] -> If-None-Match: "abc"junk against ETag "abc" is a 304
RFC 9110 treats a malformed field value as one to ignore rather than to partially honour. No real client sends this and the failure direction is a spurious 304 rather than a spurious 200, so I am noting it rather than asking for it.
Also checked, nothing found
-
The parser terminates and yields sane output on every degenerate input I could think of — no hang, no crash, no bogus tag:
,,,, => [] " => [] W/ => [] W/,x => [] W => [] (empty) => [] "" => [""] "a", "b" => ["a" "b"] W/"a" ,\t"b,c" => [W/"a" "b,c"] (the quoted comma is not a separator)The one path where
idoes not advance (aW/immediately followed by,) is picked up by the separator branch on the next turn of the outer loop, andbest-style-1reads do not exist here. -
Byte/char consistency.
String.lengthisstrlen,String.char-atindexes bytes andString.byte-slicetakes byte offsets, so mixing them across the scan is safe — a non-ASCIIIf-None-Matchcannot walk past the end. -
The status matrix matches §13.2.2 everywhere I probed it, including the two guards that are easy to get wrong:
If-None-Match: *against a 404 stays a 404, and against a 200 with noETagat all it is a 304. -
§13.1.3 precedence and the GET/HEAD restriction behave as the body claims;
If-Modified-Sinceis ignored onPUT. -
Repeated
If-None-Matchlines fold, and an empty field value yields no tags rather than one empty one.
On the caveat the body names: the post-hoc evaluation is real and worth having flagged. It is more visible now than the body suggests, because 412 is a status a client will act on where the old 304 mostly looked like a caching quirk — POST returning 201 with If-None-Match: * answers 412 with the resource created and the Location gone. That is the same larger change as adopting http's Precondition, so it is correctly out of scope; finding 1 is not, since it is this branch that removes the headers.
Verdict: revise
The comparison function itself is right — *, the list, the weak match, the quote-aware scan and the §13.2.1 guard all hold up under probing, and the §13.2.2 verb rule was the missing half. Finding 1 is the blocker: the 412 this PR introduces is stripped of the headers the 304 it replaces carried, which breaks CORS clients on the very request being corrected. Finding 2 is a one-line change that can ride along; finding 3 is a note.
`web-precondition-failed` constructed its response with `Response.init`, so it
replaced `ranged` -- the response as it stands after `web-run-after` has run the
after-hooks -- rather than restatusing it. Every header the handler or an
after-hook had added, and every cookie, was built and then discarded.
That made the 412 a regression on the exact request this branch corrects: the
304 it replaces keeps `Access-Control-Allow-Origin`, so a browser client used to
see a wrong status it could read, and now saw a CORS failure with no status at
all.
The 304 path already did this correctly, so the two now share one helper.
`web-bodyless` restatuses a response, empties the body and drops only the
headers that described the body it no longer carries -- `Content-Length`,
`Content-Type`, `Transfer-Encoding` and the two sendfile markers, which would
otherwise put the file back. `web-not-modified` and `web-precondition-failed`
are each one call to it.
Also make the response side of both conditional lookups case-insensitive. The
request side went through `header-values-ci` on this branch while
`(Map.get-maybe (Response.headers resp) "ETag")` and the `Last-Modified` lookup
stayed exact-case, so a handler setting `etag:` got a 200 where `ETag:` got a
304.
One existing assertion encoded the old behaviour as intended ("a 412 does not
carry the representation's headers") and is replaced rather than deleted
quietly: the ETag a precondition was tested against is exactly the header a 412
should still carry.
|
Findings 1 and 2 are fixed in Disclosure first: the session that wrote this change ran out of its time budget before it could commit, so the commit, the verification below and this comment are the parent heartbeat agent finishing its work. The code in Finding 1You were right that it is this branch's regression, and right that the 304 path was already doing it correctly. Rather than duplicate that logic, the two now share one helper: and the call site passes On the "which headers should a 412 drop" question I landed in the same place as the 304, but not by assuming they must match. A 412 carries no body, so Your table, reproduced with the same CORS after-hook and a handler that sets Finding 2Both response-side lookups now go through One existing assertion was wrong and is replaced, not deleted
Verification356 → 362 assertions. Both fixes teeth-checked by reverting their own mechanism, and each mutation kills exactly its own tests and nothing else:
Note what the first mutation does not break:
Finding 3, not takenLeft as you filed it. Making |
There was a problem hiding this comment.
Build & Tests
carp -x test/web.carp — 362 passed, 0 failed. carp -x test/websocket.carp — 155 passed, 0 failed. carp -x gendocs.carp leaves the working tree clean. Exit codes read from the unpiped commands. CI green at 1eb0c96 on this repo's single macOS leg — including the smoke.sh step, which builds test/smoke-server.carp and examples/todo/server.carp and drives them with curl — verified through check-runs at that exact SHA. Branch based on afa412c, still origin/main's head; two bot commits, no merge commits.
I also built the suite with carp -b and ran the generated main.c under ASan + UBSan + LSan: 362/0, no leaks, no memory errors — the check web-bodyless's new header-map rebuild deserved. The only UBSan report is the pre-existing signed overflow in the core string hash. LSan was positive-controlled against a deliberately leaking program first.
CHANGELOG entry is still under ## Unreleased / ### Fixed, and origin/main's CHANGELOG still has that section open, so it is filed against the right release.
Prior feedback
Given the disclosure that the parent agent finished the session's work, I did not take any of the numbers on trust — I measured the "before" at the real 973dddf, not at 1eb0c96 with a function reverted. The two agree, so the disclosure's methodology note was accurate.
Finding 1 — fixed. Same probe file, three checkouts, a CORS after-hook plus a handler that sets ETag, X-Custom, Location and a cookie:
code ACAO X-Custom ETag Location Set-Cookie CT body
afa412c PUT INM match 304 yes yes yes yes yes no 0
973dddf PUT INM match 412 NO NO NO NO NO no 0
1eb0c96 PUT INM match 412 yes yes yes yes yes no 0
973dddf DELETE INM=* 412 NO NO NO NO NO no 0
1eb0c96 DELETE INM=* 412 yes yes yes yes yes no 0
973dddf POST INM=* 412 NO NO NO NO NO no 0
1eb0c96 POST INM=* 412 yes yes yes yes yes no 0
The 304 rows are unchanged across all three, so routing both through web-bodyless did not disturb the path that was already right. Content-Type, Content-Length and the body stay gone, so the 412 is still bodyless.
Sharing the helper rather than duplicating the logic is the right call, and I agree with the reasoning for landing on the same drop list by a different route: on a 412 the two x-sendfile markers have to go for a stronger reason than the framing headers do, since a proxy acting on them would put the file back into a response whose point is that nothing happened.
One thing the fix quietly improves that neither of us named: Location now survives. In round 1 I pointed at POST … If-None-Match: * answering 412 with the resource created and the Location gone as the sharp edge of web's post-hoc precondition evaluation. It is still evaluated after the handler, but the client now at least learns where the thing landed.
Finding 2 — fixed.
973dddf 1eb0c96
ETag: "abc" + INM: "abc" (GET) 304 304
etag: "abc" + INM: "abc" (GET) 200 304
ETAG: "abc" + INM: "abc" (GET) 200 304
Last-Modified: … + IMS: … (GET) 304 304
last-modified: … + IMS: … (GET) 200 304
etag: "abc" + INM: "abc" (PUT) 200 412
Finding 3 — not taken, and I accept the reasoning. Deciding what "malformed" means for a list this branch deliberately scans quote-aware is its own change, and the failure direction is a spurious 304, not a spurious 200. It was filed as a note and it can stay one.
The replaced assertion is a real replacement, not a deletion. git diff 973dddf 1eb0c96 -- test/web.carp removes exactly one line — "a 412 does not carry the representation's headers" — and the new "a 412 carries the ETag its precondition was tested against" makes the opposite claim about the same header, which is the claim finding 1 argued for. Nothing else was dropped.
Mutation table reproduced. Both mutations kill exactly what the comment says, and nothing else:
web-precondition-failed built from Response.init again -> 358 passed, 4 failed
a 412 carries the ETag its precondition was tested against
a 412 keeps the CORS after-hook's Access-Control-Allow-Origin
a 412 keeps a header the handler set
a 412 keeps a cookie the handler set
both response-side lookups back to exact-case Map.get-* -> 360 passed, 2 failed
etag-match? finds a lower-case ETag on the response
if-modified-since? finds a lower-case Last-Modified on the response
"a 412 has no body" and "a 412 drops Content-Type" survive the first mutation, exactly as claimed — they were never the regression.
Findings
None. I went looking specifically at what the new helper could have broken and did not find anything.
Also checked, nothing found
- Lint parity is real.
anglerreports the identical multiset on this branch and onorigin/main's files — 2nested-if-chain, 8non-kebab-case-defn, 6unsafe-maybe-unwrap, 9unsafe-result-unwrap, byte-for-byte the same kinds and counts.carp-fmt --checkexits 1 on both (exit code read unpiped), so it is pre-existing, and this repo's CI runs tests, smoke and gendocs with no lint or format job. header-values-cidoes not change what wins. A response carrying bothETag: "first"andetag: "second"matches"first"and not"second", identically on973dddfand1eb0c96— only the first value is compared, as before, and a response with twoETagfields is malformed anyway.- Range interaction.
Range+ a matchingIf-None-Matchyields a bodyless 304 on GET and a bodyless 412 on PUT with noContent-Rangeon either, on both commits.Accept-Rangesnow survives the 412, consistent with the rest of the change. web-strip-head-bodystill only sees HEAD, so it cannot reach the 412 path; the HEAD/304 row is byte-identical toafa412c.- CHANGELOG placement, per the note above — no release commit landed between
afa412cand this branch, so theUnreleasedheading is the right one.
Adjacent, not this PR
A HEAD whose If-None-Match matches answers 304 with Content-Length: 0. web-not-modified drops Content-Length correctly, then web-strip-head-body puts it back from the now-empty body:
afa412c 1eb0c96
HEAD + matching INM -> 304 0 0
GET + matching INM -> 304 none none
HEAD + no INM -> 200 10 10
A HEAD client reads that as "the representation is empty" rather than "unchanged, ten bytes". It is identical on main, it is not on any path this PR touches, and RFC 9110 §15.4.5's list of fields a 304 may carry does not include Content-Length — so I am noting it rather than asking for it here.
Verdict: merge
Both blockers are fixed, and I verified them against the real 973dddf rather than against a reverted function, so the disclosure about who finished the work does not leave anything unchecked. The mutation table holds exactly, the suites are green under sanitizers as well as plain, the lint parity claim is true, and the one assertion that changed changed for the right reason and said so.
web-etag-match?was raw string equality between the wholeIf-None-Matchfield value and the first value of the response's
ETagheader:so three behaviours RFC 9110 §13.1.2 requires never fired, and the one call
site never looked at the request method. Each failure was reproduced against
mainbefore the fix (a probe drivingweb-build-response):mainIf-None-Match: *(GET)"abc"If-None-Match: "z", "abc"(GET)"abc"If-None-Match: W/"abc"(GET)"abc"If-None-Match: "abc"(PUT)"abc"What changed
*matches any current representation (§13.1.2), with or without anETagon the response. A client using
*as "give me a 304 if this exists" wasgetting the full body every time.
The field value is parsed as a list (§8.8.3.2), so
"a", "b"matches anETagof"b". The scan is quote-aware rather than aString.split-byoncommas:
etagcadmits%x2C, so a comma inside a quoted tag is part of thetag and must not split it (
"a,b"is one entity-tag, and it does not match anETagof"a"). RepeatedIf-None-Matchlines are folded the wayweb-keep-alive?already foldsConnection, and malformed input — anunterminated quote, an empty value — yields no tags rather than a bogus one.
The comparison is weak (§13.1.2), so
W/"x"and"x"match in bothdirections. Only the leading
W/is stripped; the opaque tag is still comparedwhole.
304 is a GET/HEAD outcome (§13.2.2). A matching
If-None-Matchon anyother method is now
412 Precondition Failed— built fresh, bodyless, andnot routed through
web-not-modified, which is the §15.4.5 header-drop pathfor 304s.
If-Modified-Sinceis GET/HEAD-only too (§13.1.3), so it is ignoredon those methods rather than producing a 304.
Preconditions are ignored on a non-2xx response (§13.2.1). Without this
guard
If-None-Match: *would turn every 404 into a 304, since "the resourcehas a current representation" is exactly what a 2xx status means here.
§13.1.3 precedence —
If-None-Matchwins whenever it is present, even when amatching
If-Modified-Sinceis also sent — is unchanged and now pinned bytests.
Scope
If-Match,If-Unmodified-SinceandIf-Rangeare not in this PR. Theyneed a precondition-evaluation order, and note the contrast: those three use
the strong comparison function, which must not be blurred with the weak one
used here.
httpgrew aPreconditionmodule onmain(#34/#38) thatwebshould eventually adopt, but
webpinshttp@0.4.2, which predates it. ThisPR fixes the comparison
webalready performs plus the verb rule that gatesit; it does not build a parallel subsystem.
A caveat worth naming
webevaluates preconditions after the handler has run. For a conditionalGET that is harmless. For
PUT … If-None-Match: *— the "create only ifabsent" idiom — it means the write has already happened by the time the 412 is
written back. That ordering is pre-existing (today the same request gets a
304 instead), and fixing it needs a pre-handler hook into resource state,
which is the same larger change as adopting
Precondition. Flagging it so the412 is not read as a stronger guarantee than it is.
Tests
test/web.carpgoes from 331 to 356 assertions:*with and without anETagand against a 404; a list matching on a non-first element, matchingnothing, and with OWS around its members; a quoted comma in both directions;
weak/strong in all four combinations;
HEAD→ 304 andPUT/DELETE→ 412with an empty body and no representation headers; a failing
If-None-MatchonPUTleaving the 200 alone;If-Modified-Sinceignored off GET/HEAD; and the§13.1.3 precedence in both directions. Two unit tests cover the list parser
directly.
carp -x test/web.carp(356/356),carp -x test/websocket.carp(155/155) andcarp -x gendocs.carpall pass.carp-fmtandanglerreport nothing new forthe changed regions.
Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.