Patrick McManus [:mcmanus] Reporter
	Description • 9 years ago

name : part1\n
           part2\n
           part3

(the newlines and spaces were part of the header value encoded in h2). Firefox accepted this because we basically parse values with the h1 parser which accepts line folding,

There are two arguments against this - the first is that line folding is a piece of h1 specific syntax and h2 has different syntax (even though it inherits semantics). 

"Besides which, RFC 7540 says that the HEADERS frame carries headers as described in RFC 7230 section 3.2, which says in 3.2.4:

   Historically, HTTP header field values could be extended over

   multiple lines by preceding each extra line with at least one space

   or horizontal tab (obs-fold).  This specification deprecates such

   line folding except within the message/http media type

   (Section 8.3.1).  A sender MUST NOT generate a message that includes

   line folding (i.e., that has any field-value that contains a match to

   the obs-fold rule) unless the message is intended for packaging

   within the message/http media type."

	Patrick McManus [:mcmanus] Reporter
	Comment 1 • 9 years ago

nick, I assigned this to you as it might be best done near the hpack code.. let me know if that's cray cray.

	Patrick McManus [:mcmanus] Reporter
	Comment 2 • 9 years ago

edge already rejects this, and chrome is expected to make a corresponding change

	u408661 Assignee
	Comment 3 • 9 years ago

Yeah, I agree this makes the most sense in the hpack code. Will have a patch up soon-ish.

	u408661 Assignee
	Comment 5 • 9 years ago

Note - this patch also includes a bunch of "goto fail"-style fixes that I noticed while I was in the HPACK code :)

	u408661 Assignee
	Comment 6 • 9 years ago

So... based on the latest reading of the wg list (and 7230) it looks like we've already got the appropriate behavior here. Gotta love spec lawyering :)

Patrick, I'll leave it to you to make the decision here, since you're module owner (and more versed in spec-land than I am), but my $0.02 is to leave our code as is (and I can make another bug to fix the goto-fail stuff, since I've already got a patch for it.)

	Patrick McManus [:mcmanus] Reporter
	Comment 7 • 9 years ago

based on https://lists.w3.org/Archives/Public/ietf-http-wg/2015JulSep/0254.html

I think we still want this.. deferring a bit until more clear :)

	Patrick McManus [:mcmanus] Reporter
	Comment 8 • 9 years ago

let's do this with rst_stream/protocol_error

	u408661 Assignee
	Comment 9 • 9 years ago

(In reply to Patrick McManus [:mcmanus] from comment #8)
> let's do this with rst_stream/protocol_error

OK, yeah, based on the new latest wg reading, that seems right.

That makes the current patch wrong, as we currently treat any error return from the compressor as a GOAWAY, so we'll need to fix that, as well.

	Patrick McManus [:mcmanus] Reporter
	Comment 10 • 9 years ago

Comment on attachment 8651919 [details] [diff] [review]
patch

Review of attachment 8651919 [details] [diff] [review]:
-----------------------------------------------------------------

can you confirm with a trace that we are just resetting the stream and not the session? (I've read the comments too :) - just making sure).

thanks!

::: netwerk/protocol/http/Http2Compression.cpp
@@ +824,5 @@
>      return rv;
> +  }
> +
> +  int32_t newline = 0;
> +  while ((newline = value.FindChar('\n', newline)) != -1) {

add a comment about what we are enforcing

	u408661 Assignee
	Comment 11 • 9 years ago

This is just a patch I came up with (that I'd actually been meaning to come up with for a while) that makes debugging server-side issues on the h2 (and spdy) test server easier by allowing the user to run it independent of the xpcshell process

	u408661 Assignee
	Comment 12 • 9 years ago

Here's the patch to disallow folded headers, along with some 'goto-fail' type fixes (from the original version of the patch). This also fixes the situations where we could get into compression state mismatches with the server, as well as some whitespace funkiness (tabs!) that crept into the tests.

	Patrick McManus [:mcmanus] Reporter
	Comment 13 • 9 years ago

Comment on attachment 8664626 [details] [diff] [review]
helper patch used during debugging

Review of attachment 8664626 [details] [diff] [review]:
-----------------------------------------------------------------

should probably put this bug number on the checkin.. for tracking the review if nothing else :)

	Patrick McManus [:mcmanus] Reporter
	Comment 14 • 9 years ago

Comment on attachment 8664629 [details] [diff] [review]
Disallow folded headers in h2

Review of attachment 8664629 [details] [diff] [review]:
-----------------------------------------------------------------

thanks man - this was surprsingly hairy

	u408661 Assignee
	Comment 15 • 9 years ago

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cb61b645368

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/03aee3398396
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6059530b031

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/03aee3398396
https://hg.mozilla.org/mozilla-central/rev/d6059530b031

	Kohei Yoshino [:kohei]
	Comment 18 • 9 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/line-breaks-in-http2-headers-are-no-longer-allowed/

	Jean-Yves Perrier [:teoli]
	Comment 19 • 9 years ago

Updated: https://developer.mozilla.org/en-US/Firefox/Releases/44#HTTP