Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d435c18f1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| weight = qval; | ||
| } | ||
|
|
||
| *identity_refused = !identity_allowed; |
There was a problem hiding this comment.
For Accept-Encoding: gzip, identity;q=0.5, *;q=0, identity is explicitly acceptable, but nxt_http_comp_compressor_lookup_enabled() aliases * to identity and the parser permanently clears identity_allowed when it encounters the wildcard. Consequently this new output flag is true and the static path discards an otherwise valid range, returning the entire compressed file instead of the requested 206. Track an explicit identity value separately so it takes precedence over the wildcard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 401bbeefd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
All three P2s reproduced before acting on them. All three are real; one was a regression this branch introduced. Fixed in P2-1 — explicit identity vs the wildcard: a regression I introducedConfirmed.
Before this branch the flag went nowhere and only the 406 path read it, so the bug was latent; adding the range check is what gave the wildcard the power to veto a range the client had explicitly allowed. What the wildcard said and what an explicit token said are now tracked separately, and the explicit one wins — which is also the RFC's more-specific-wins reading. P2-3 — case-insensitive tokens: real, and pre-existingConfirmed. Fixed with Both cases are now asserted — the q=0.5 one with its P2-2 — below
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5e37f2bc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| identity_allowed = identity_named_ok; | ||
| } | ||
|
|
||
| *identity_refused = !identity_allowed; |
There was a problem hiding this comment.
For Accept-Encoding: gzip, identity;Q=0, the Q parameter is valid because HTTP ABNF literals are case-insensitive, but the parser only searches for lowercase ;q=. It consequently treats identity;Q=0 as an unknown coding, leaves this refusal flag false, and the static path still returns a 206 containing identity bytes. Parse the weight parameter case-insensitively so every valid identity refusal reaches the new range check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70ac6cac1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f921631f25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The HEAD disparity is real; the attribution is not. Measured, and filed as #391. It happens with no Range at allSame build,
So the GET/HEAD parity this comment says the branch breaks is already broken on master for any compressible file, and the range path is not involved in reaching it. Cause, and why it is not this branchThe compression block is guarded by What the branch does change for HEAD is which of two already-identity-metadata answers is given: Neither carries the coding. The branch neither creates nor deepens the disparity — and for what it is worth the 200 is the more honest of the two, since the 206 was offering a client a byte range of a representation it had refused. Filed rather than folded in#391, with the measurement and a note that the fix is to run negotiation for HEAD so it reports what the GET would. Kept out of this PR for the same reason as #390: it is a pre-existing defect in a different code path, and fixing GET/HEAD parity properly means deciding whether HEAD should compress-to-measure or declare the identity representation — which is a change with its own blast radius, not a rider on a range fix. The issue also carries the caution this comment deserves credit for implying: a HEAD test can pass for the wrong reason because the body is empty either way, so the assertion has to be on the headers and has to be checked against an unfixed build. Review-freshness note, for anyone reading the threadSix of the seven inline comments here are earlier findings carried forward, not repeats of live ones. |
f921631 to
3800e7f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
A byte range is served as identity -- coding a slice would compress the
wrong bytes, and re-deriving which coded bytes correspond to an identity
range is not something most codings support. So a request carrying both
"Range" and "Accept-Encoding: identity;q=0" was answered with a 206 of the
file's own bytes: exactly what the client said it would not take.
Reproduced before the fix, gzip configured for text/css:
Accept-Encoding: gzip, identity;q=0
Range: bytes=0-9
-> 206, no Content-Encoding, body "body{color"
Such a request is still serveable, because it named a coding Unit has.
Drop the Range rather than the request and answer the full 200 in that
coding. Ignoring a Range is already how this function answers a malformed
one, a multi-range one and an If-Range mismatch -- RFC 9110 Sect. 14.2
lets a server ignore Range -- so this adds no new shape of response.
A 406 is the other defensible reading, and it is what a strict reading of
Sect. 12.5.3 suggests, but it refuses a request that can be satisfied: a
client asking for the first ten bytes of a file Unit can send in gzip is
better served the file than an error. nginx serves the range as identity
and never reaches this question; the ecosystem is split, and this picks
the answer that delivers bytes.
The refusal has to be carried, not re-derived. select_compressor()
collapsed it into -1, which only says "nothing acceptable" -- a request
refusing identity while accepting gzip selects gzip and never reaches
that. It now reports identity_refused separately, so the range path can
ask.
Where no coding is applied after all -- a body below the compressor's
"min_length", or a media type outside "types" -- the full 200 sent here is
identity, which the client refused just as firmly as it refused the 206.
That is the separate defect #390 reports, and the commit closing it answers
such a request 406 before the Range is looked at, so this drop never
reaches one.
Two cases either side are unchanged and asserted: "Accept-Encoding: gzip"
with a Range is still a 206 of identity bytes, which is deliberate; and a
request that accepts nothing is still 406, with or without a Range.
The test decompresses the body and compares it to the file rather than
trusting Content-Encoding. With the new branch disabled it fails with
206 == 200.
Closes: #355
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both files, same diff, appended to the end of the unreleased stanza. "ver=1.36.2" still counts 2 in docs/changes.xml and the XML parses. The stanza is named 1.36.2 because that is what it is named today; if the next release ships as 1.37.0 the stanza is renamed once and this entry travels with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
case-insensitively Two review findings, both reproduced before acting on them and both real. The first is a regression this branch introduced. An explicitly named coding beats the wildcard. "*" is looked up as identity, so "gzip, identity;q=0.5, *;q=0" cleared identity_allowed on the wildcard and never restored it on the explicit token -- the client refused everything it did not name, then named identity as acceptable, and Unit read that as a refusal. Before this branch the flag went nowhere and only the 406 path saw it; with the range check added, the wildcard began vetoing ranges the client could take. Measured: 206 before, 200 with the whole file gzipped after. What the wildcard said and what an explicit token said are now tracked apart, and the explicit one wins. Content codings are tokens, and tokens are case-insensitive (Sect. 8.4.1), but the lookup used nxt_strstr_eq(). "Identity;q=0" therefore matched nothing, the refusal was dropped, and the request got the 206 of identity bytes this branch exists to prevent. nxt_strcasestr_eq() now, which also makes "GZIP" select gzip. That half is pre-existing and reaches the 406 path too. Both are asserted: the q=0.5 case keeps its 206 with the right Content-Range and body, and the mixed-case case gets the gzip 200. 85 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more from review, both reproduced against the branch head before
acting on them.
The weight parameter is "OWS ';' OWS ('q' / 'Q') '=' qvalue" (Sect.
12.4.2), and an ABNF literal is case-insensitive besides, but the parse
was strstr(tkn, ";q="). So "identity;Q=0" carried no weight Unit could
see, fell through as an unknown coding, and the refusal never registered:
measured, that request with a Range got a 206 of exactly the identity
bytes it had refused. Found with a small scan for ';' followed by q or Q.
"*" is a tchar, so "*foo" is a legal coding name -- one Unit does not
have. nxt_http_comp_compressor_lookup_enabled() matched the wildcard on
the first byte alone, so "*foo" was resolved to identity; with the
explicit-token tracking added earlier in this branch it then counted as a
named identity refusal and discarded a range the client could have taken.
Measured: 200 with the whole file gzipped where a 206 was due. The
wildcard is now the whole one-character token.
The second is a regression this branch introduced -- before it the flag
was only read by the 406 path, where an unknown coding resolving to
identity happened not to matter. Both are asserted now, the "*foo" case
with its Content-Range and body rather than its status alone.
85 passed, 1 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OWS is SP or HTAB (Sect. 5.6.3), and it is legal on either side of the weight's semicolon, but the cleanup pass removed only the space. So "identity;<HTAB>q=0" and "identity<HTAB>;q=0" reached the weight scan with the tab still in them, matched no coding Unit knows, and read as unknown elements -- taking their refusal with them. Measured: both spellings with a Range got a 206 of exactly the identity bytes the client had refused, while the SP spelling of the same header was handled correctly. Fixed where the spaces were already going, so both positions are covered by one change rather than the weight scan learning about tabs. All three spellings are asserted alongside the plain one. 85 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ot at all
Three findings from an adversarial review, all reproduced first.
A second Accept-Encoding field was never read. Sect. 5.3 says a list-valued
field may arrive as several lines and must be treated as one value joined by
commas, but the value came from a variable query, and nxt_http_var_header()
answers with the first matching field and stops. So this reached the parser
as "gzip" alone:
Accept-Encoding: gzip
Accept-Encoding: identity;q=0
Range: bytes=0-9
-> 206, no Content-Encoding, body "body{color"
which is the defect this branch exists to fix, arriving through another
door. In the other order the gzip the client would have accepted was never
seen and the request drew a 406. Both measured. Now collected by walking
the request's fields and joining; a single field, which is every ordinary
request, still points at the field and copies nothing.
Ignoring the Range was not all or nothing. A satisfiable range from a
client that refused identity was dropped, but an unsatisfiable one still
answered 416 with "Content-Range: bytes */7500" -- the size of exactly the
representation the client had refused. The refusal is now tested before the
416 branch rather than after it.
And the comment justifying the drop was wrong about why. It said slicing a
coded response would compress the wrong bytes or need coded offsets most
codings cannot give. That describes slice-then-code; Unit codes the whole
file into a temp file and then applies file_pos/file_end to it, so a 206 of
coded bytes would be perfectly buildable. The real reason is that a coded
range is never resumable and costs a whole file to answer: a coded
representation carries a weak entity-tag, nxt_http_static_range() compares
If-Range strongly, so a resumed coded range always falls back to the full
response -- while Unit compresses the whole file to hand back ten bytes of
it. Not "a splice nothing can detect": the weak tag makes that splice
impossible rather than undetectable.
The Accept-Encoding tstr goes with the variable query that used it.
nxt_http_comp_conf_t no longer holds accept_encoding_query and
nxt_http_comp_compression_init() no longer compiles
"$header_accept_encoding"; the field walk above replaced both.
Proven rather than assumed: with the collector returning only the first
field again, the repeated-field case fails. 85 passed, 1 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The qvalue was read with strtod(qptr + 3, NULL), with no look at where the
conversion stopped. strtod() returns 0 when it consumes nothing at all, and
a zero weight here is an explicit refusal, so a weight Unit could not parse
was read as one. Measured against the branch head, gzip configured for
text/css and a 7500-byte file:
Accept-Encoding: identity;q=abc -> 406
Accept-Encoding: identity;q= -> 406
Accept-Encoding: identity;q=0x0 -> 406
Accept-Encoding: identity;q=0e0 -> 406
Each of those clients refused nothing. "q=0x0" and "q=0e0" are strtod()
being looser than the grammar rather than stopping early: hexadecimal and
an exponent are not qvalues.
"q=nan" was the other half, and worse. A NaN compares false against both
bounds, so "qval < 0.0 || qval > 1.0" let it through, and false against the
running best weight, so the element was then selected:
Accept-Encoding: gzip;q=nan -> 200, Content-Encoding: gzip
A qvalue is "( '0' [ '.' 0*3DIGIT ] ) / ( '1' [ '.' 0*3( '0' ) ] )" (Sect.
12.4.2). Check that shape before converting, and require the end of the
element or a further parameter after it. A NaN, a hexadecimal and an
exponent are all rejected by the shape, so strtod() is left only input it
cannot misread.
An element whose weight does not parse is ignored, exactly like an element
naming a coding this build does not have. Reading it as q=0 is the other
defensible answer -- nginx reads a bad quantity as zero -- but a zero is a
refusal here, and a typo should not turn into a 406.
The digit count in the fraction is deliberately not enforced. Rejecting
"identity;q=0.0000" would read a client's refusal as an acceptance, which is
the wrong direction to be strict in; it still refuses.
86 passed, 1 skipped. Reverting the shape check fails the new test on
"identity;q=" alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both files, same entry, appended to the end of the unreleased stanza. The XML parses and "ver=1.36.2" still counts 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3800e7f to
832f5a7
Compare
|
Rebased onto master ( What the rebase reconciled. Master merged the #167 fix ( Two defects found in review and fixed. A file below Behaviour, measured against master (
RFC 9110 Sect. 12.5.3 withdraws the "send without content coding" fallback when identity is explicitly unacceptable, and Sect. 12.1 then permits either honouring the header with 406 or disregarding it. 406 is the honouring branch and is what Unit already answers for Worth a reviewer's attention: the 406 reaches application responses too, not only static ( Validation: debug and release build with 0 warnings; all 9 commits build standalone; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 832f5a73b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (idx == -1) { | ||
| return NXT_HTTP_NOT_ACCEPTABLE; |
There was a problem hiding this comment.
For an application response with a known nonzero Content-Length and no compression configuration, Accept-Encoding: identity;q=0 now reaches this return. Unlike the static caller, nxt_router_response_ready_handler() treats every non-NXT_OK result at src/nxt_router.c:5548-5550 as a generic failure, whose fail path emits 503 at line 5628, so the newly intended 406 is never sent for application responses; the caller needs to handle NXT_HTTP_NOT_ACCEPTABLE separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 43abe751. nxt_router_response_ready_handler() now carries the status nxt_http_comp_check_acceptable() returned, so a negotiation failure answers 406 instead of the fail: path's 503 (src/nxt_router.c:5352,5358,5562,5643).
Worth noting the hole predates this PR: at eb1767c9 an identity;q=0 against a compressor-configured response of a matching media type already returned NXT_HTTP_NOT_ACCEPTABLE into the same goto fail. This PR only widened it to the NULL-config and the types/min_length cases. Measured with a PHP app before and after: 503 → 406 in all three shapes.
832f5a7 to
43abe75
Compare
|
Correction to my earlier comment, which said "the 406 reaches application responses too". That was wrong at the time: until Also in Validation on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43abe7510b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
43abe75 to
4040148
Compare
A client sending "identity;q=0" will not take the response's own bytes, and
with a compressor configured it was told the request was serveable on the
strength of a coding that then never ran. What it got was identity. Two
ways in, both reproduced with gzip configured for text/css, "min_length"
10:
GET /tiny.css (2 bytes)
Accept-Encoding: gzip, identity;q=0
-> 200, no Content-Encoding, body "ok"
GET /raw (no media type)
Accept-Encoding: gzip, identity;q=0
-> 200, no Content-Encoding, body "raw"
nxt_http_comp_check_acceptable() asked whether any coding was acceptable,
and nothing afterwards asked whether the one it picked would be applied.
"min_length" and the "types" rule are both decided after that point, and
both fall back to identity. The fallback is exactly what this client
refused, so there is no representation left: the answer is 406.
RFC 9110 Sect. 12.5.3 makes identity the assumed-acceptable coding only
while the field does not say otherwise; an explicit "identity;q=0" says
otherwise, and Sect. 12.1 then lets a server either honour the header or
disregard it. Serving the identity bytes is therefore permitted, not a
violation -- but Unit already answers 406 to "identity;q=0" alone, and
answering the same request differently because a coding was named and then
not applied is the inconsistency, not the 406.
The Accept-Encoding parse moves ahead of everything that can rule
compression out, including the "no compression configured" exit. Identity
is a representation every response has, so a refusal has to be read even
with no compressors: otherwise a static server with compression off hands a
range of the file's own bytes to a client that refused them, which is the
same defect from the other side. That means select_compressor() has to run
against a NULL configuration, so identity is recognised from the static type
table, and the wildcard from its own token, before the enabled array is
searched -- and the search itself returns "unknown" for a NULL configuration.
Behaviour, identity refused throughout:
an eligible coding, with a Range 200 in that coding, no range
below "min_length" 406
media type outside "types" 406
no compressors configured 406
nothing acceptable at all 406
A client that accepts identity is unaffected in every one of those: the
refusal gates each new branch.
This also settles a question left open by the range work: a response no
coding is applied to used to keep its range, on the reasoning that identity
was what it would get either way. That reasoning ends here, because the
request is no longer serveable at all, so the gate that carried it is gone
and the case is 406 with a Range and without.
The same answer now reaches application responses. Every non-NXT_OK result
of nxt_http_comp_check_acceptable() was a generic failure in
nxt_router_response_ready_handler(), whose "fail:" path answers 503, so a
negotiation failure was reported as the server being unable to serve anybody.
That is older than this commit -- at eb1767c an "identity;q=0" against a
compressor-configured response of a matching media type already returned
NXT_HTTP_NOT_ACCEPTABLE into the same "goto fail" -- and the branches above
would have widened it to every application response. The caller now carries
the status the check returned, and answers 406.
Selection accounts for "min_length" as well. It is a per-compressor option,
so the highest-weight coding being below its own threshold says nothing about
the next one: with gzip at "min_length" 1000 and deflate at 0, a two-byte
response is serveable as deflate. Choosing gzip on its weight alone sent
that response as identity, and answered 406 to a client that refused identity
although deflate could satisfy it. nxt_http_comp_select_compressor() now
skips a coding below its own minimum, which is also where the "min_length"
406 above comes from: a coding that cannot be applied is simply not selected,
so nxt_http_comp_check_acceptable() no longer tests the minimum separately.
The wildcard has to be able to select those codings. Sect. 12.5.3: "*"
"matches any available content coding not explicitly listed in the field", so
it stands for every enabled coding the client did not name -- and for
identity -- at its own weight. Reading it as identity alone left it unable
to select a compressor at all, which the 406 above turns from a quirk into a
refusal:
GET /big.css (7500 bytes, text/css)
Accept-Encoding: identity;q=0, *;q=1
-> 406, with gzip enabled, applicable and asked for
"*" is now applied after the field is read, so a named coding is never
matched by it and keeps its own weight whatever its place in the field:
"identity;q=0, *;q=0.5, gzip;q=0.1" selects gzip at 0.1 where gzip is the
only compressor, and deflate at 0.5 where there is one to take the
wildcard's weight. The comparison against the running best is strict, so a
named coding also wins a tie: "gzip, *" now sends gzip where it used to send
identity. Among the codings the wildcard does stand for, identity is taken
first, so a bare "*" is unchanged and still sends the response's own bytes.
A coding below its "min_length" is skipped here exactly as it is above -- the
wildcard must not offer what cannot be applied -- and with no compressors
configured identity is all it can ever mean, which is why
"identity;q=0, *;q=1" is still a 406 there.
A status that describes no representation is exempt from all of this. A
1xx, a 204 and a 304 have nothing to negotiate over, so there is nothing the
client could have refused and no 406 to give. The cheap length exits at the
top of the check do not cover them: nxt_http_response_content_length() stores
an application's Content-Length field without setting content_length_n, which
stays -1, so an application answering 204 or 304 with that field reached the
compressor selection and came back 406. The status is tested, never the
method: a HEAD carries no body but still describes the representation the
equivalent GET would return (RFC 9110 Sect. 9.3.2), so it is negotiated like
that GET, which is why nxt_http_request_is_bodyless_final() is the wrong
question here and nxt_http_status_no_representation() is factored out beside
it. On the static path r->status is 200 at the check, ahead of precondition
evaluation, so this changes nothing there and a 406 still outranks the 304 or
412 a validator would give.
Measured: 101 passed, 4 skipped across test_static.py,
test_static_compression.py and test_php_compression.py -- the three files
that send an Accept-Encoding. Each new branch was reverted in turn and fails
its own test: the "min_length" 406 with 200 == 406, the media-type 406 with
200 == 406, both the early "no compression" exit and the identity token read
only from the enabled array with 206 == 406, the caller's status with
503 == 406, and the selection's eligibility test with a 200 carrying no
Content-Encoding where deflate was expected. The bodyless exemption was
reverted three ways: dropped outright gives 406 == 204, narrowed to 204 alone
gives 406 == 304, and replaced by nxt_http_request_is_bodyless_final() gives
200 == 406 on the HEAD. The wildcard was reverted four ways: back to
"identity or wildcard" gives 406 == 200 on the positive case and on the
application path, dropping its "min_length" skip serves a below-minimum
coding, a non-strict tie hands "gzip, *" back to identity, and dropping the
"named" set lets "*" outbid a coding the field listed.
Closes: #390
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed in A named coding is never matched by the wildcard and keeps its own weight, so The wildcard honours Worth recording that |
4040148 to
fc0b32e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc0b32e850
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (idx == -1) { | ||
| return NXT_HTTP_NOT_ACCEPTABLE; |
There was a problem hiding this comment.
When Accept-Encoding leaves no acceptable coding, this return now occurs before vary_accept_encoding is set, so the generated 406 lacks Vary: Accept-Encoding. This regresses the previous compressor-enabled path, which set the flag before selecting a coding; a cache configured to store error responses can consequently reuse this 406 for a later client that accepts identity or gzip. Set/merge the variation marker before any result that depends on the request's Accept-Encoding value.
Useful? React with 👍 / 👎.
1 participant
Closes #355.
The defect
A byte range is served as identity — coding a slice would compress the wrong
bytes, and re-deriving which coded bytes correspond to an identity range is
not something most codings support. So a request carrying both a
RangeandAccept-Encoding: identity;q=0was answered with a 206 of the file's ownbytes: precisely what the client said it would not take.
Reproduced on master before touching anything, gzip configured for
text/css:The fix, and why not 406
Such a request is still serveable: it named a coding Unit has. So drop the
Range, not the request, and answer the full 200 in that coding.
Ignoring a Range is already how this function answers a malformed one, a
multi-range one, and an
If-Rangemismatch — RFC 9110 Sect. 14.2 lets aserver ignore
Range— so this adds no new shape of response, and thefunction's own doc comment already lists "serve the full 200" as an outcome.
A 406 is the other defensible reading and is what a strict reading of
Sect. 12.5.3 suggests. I did not take it: it refuses a request that can be
satisfied. A client asking for the first ten bytes of a file Unit can send in
gzip is better served the file than an error. nginx serves the range as
identity and never reaches the question; the ecosystem is split, and this
picks the answer that delivers bytes.
This is the judgement call in the PR. If you would rather have the 406,
the change is one line and the tests invert cleanly.
The refusal has to be carried, not re-derived
nxt_http_comp_select_compressor()collapsed the refusal into-1, whichonly ever means "nothing acceptable at all". A request that refuses identity
while accepting gzip selects gzip and never reaches that return, so the
information was gone by the time the range path needed it. It now reports
identity_refusedseparately.Verification
gzip, identity;q=0+ Rangegzip+ Rangeidentity;q=0, *;q=0± Rangegzip, identity;q=0, no RangeThe test decompresses the body and compares it to the file rather than
trusting
Content-Encoding. With the new branch disabled it failsassert 206 == 200, checked by disabling it and rebuilding.Both neighbouring cases are asserted so they cannot drift: a plain
gzipRange keeps its 206 of identity bytes, which is deliberate, and an
unacceptable request stays 406 with or without a Range.
83 passed, 1 skipped across the static and compression suites.
🤖 Generated with Claude Code
https://claude.ai/code/session_015j5NXxjo8hdYc99A8Db7tG