whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.12k stars 332 forks source link

"get, decode, and split a header value" algorithm appears to choke on malformed values #1778

Closed ricea closed 2 months ago

ricea commented 2 months ago

What is the issue with the Fetch Standard?

To reproduce:

  1. Run the "get, decode, and split a header value" algorithm on malformed input like "a""b"

Expected behaviour:

Algorithm returns something, or possibly signals an error.

Actual behaviour:

Algorithm hits an assert.

As far as I can tell, there's nothing in the callers to this algorithm that guarantees the input is well-formed.

My interpretation of the spec steps inside the loop when applied to the input "a""b":

  1. 0 characters are appended to temporaryValue.
  2. collecting an HTTP quoted string results in a being appended to temporaryValue. postion now points to the third ".
  3. Whitespace is trimmed from temporaryValue. It is still a.
  4. a is appended to values.
  5. temporaryValue is cleared.
  6. We do not return.
  7. code point at position is ", not ,, so the assert fails.

If an implementer ignores the assert failure I think they will get the output «a, b », which is a behaviour.

Based on code inspection I believe Chromium will reject this particular input, but something like name="a""b" will be treated as name="a\"\"b", which is also a behaviour.

annevk commented 2 months ago

I have the feeling you skipped step 5.2.2? At least the way I read https://fetch.spec.whatwg.org/#header-value-get-decode-and-split is that once you see the first " and collect "a" (appending a to temporaryValue), position is not past the end of input, so you go back to the start of the while loop. Then you append nothing to temporaryValue as you're looking at " so then you collect "b" (appending b to temporaryValue). And only then do you start trimming whitespace.

I also double checked with the changes we recently made in #1769 and I don't think they regressed something, so if there is some kind of error here (and it does seem like we don't match browsers and maybe ought to do something with this), it's a novel one.

ricea commented 2 months ago

You're right, I missed the continue. Sorry about that.