Subject: [quicwg/base-drafts] Clarify and split the "malformed" definition (#3352)

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Jan 2020 12:02:10 -0800

Lots of text, but it&#39;s basically all an import from RFC 7540 (which helps #3264, but doesn&#39;t totally resolve it).  This makes the definition of malformed requests entirely local and more precise than previously.

The odd item on the malformed list was &quot;extraneous frames.&quot;  We&#39;re already fairly permissive about what other frames can show up interleaved with the actual request, so this requirement moved to that section -- receipt of DATA and HEADERS frames outside of the prescribed order is now a connection error.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3352

-- Commit Summary --

  * Import more RFC7540 header text
  * Extraneous frames aren&#39;t malformed messages; they&#39;re worse
  * Missed a multi-paragraph
  * Missed some HTTP/2s

-- File Changes --

    M draft-ietf-quic-http.md (123)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3352.patch
https://github.com/quicwg/base-drafts/pull/3352.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Jan 2020 12:07:55 -0800

@MikeBishop pushed 1 commit.

141047ad7c6aaad2c41a3477592cac38b138d0ea  Invalid message sequences are also malformed

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352/files/9d1b8327658348845bce8ac685faee2459a3f19d..141047ad7c6aaad2c41a3477592cac38b138d0ea

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Jan 2020 12:09:33 -0800

f5d137d1c834e74074e77bf0834a2158fa33cbda  Invalid message sequences are also malformed

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352/files/eb9f21ae8e8258cad7ca4eccb917a99de72ad4be..f5d137d1c834e74074e77bf0834a2158fa33cbda

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Jan 2020 20:12:09 +0000 (UTC)

The Message Exchange section required that a client send precisely one request per stream, and that a server send precisely one final response (preceded by zero or more non-final responses), but how to deal with violations wasn't defined.  This seems to fall in the same spirit of malformed messages, so I added that to the definition of "malformed."

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#issuecomment-575326105

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 16 Jan 2020 15:51:07 -0800

kazuho approved this pull request.

Thank you for working on this. I read through all the changes, but could not find a single issue! :100: :+1:

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#pullrequestreview-344290663

---

From: Lucas Pardue <[email-masked]>
Date: Thu, 16 Jan 2020 16:29:29 -0800

LPardue commented on this pull request.

I think this is a good addition. I did however find some small room for improvement :99:

> @@ -410,10 +417,10 @@ considered invalid.
 
 An HTTP request/response exchange fully consumes a bidirectional QUIC stream.

I guess while we are changing so much text we could make it explicity and say

"An HTTP request/response exchange fully consumes a client-initiated bidirectional QUIC stream."

> @@ -450,15 +457,103 @@ field names MUST be converted to lowercase prior to their encoding.  A request
 or response containing uppercase header field names MUST be treated as
 malformed ({{malformed}}).
 
+Like HTTP/2, HTTP/3 does not use the Connection header field to indicate
+connection-specific header fields; in this protocol, connection- specific

```suggestion
connection-specific header fields; in this protocol, connection-specific
``` or is the space intended?

> @@ -520,9 +617,9 @@ permitted (e.g., idempotent actions like GET, PUT, or DELETE).
 ### Malformed Requests and Responses {#malformed}
 
 A malformed request or response is one that is an otherwise valid sequence of
-frames but is invalid due to the presence of extraneous frames, prohibited
-header fields, the absence of mandatory header fields, or the inclusion of
-uppercase header field names.
+frames but is invalid due to the presence of prohibited header fields, the
+absence of mandatory header fields, an invalid sequence of HTTP messages, or the
+inclusion of uppercase header field names.

You don't mention pseudo-header fields in this paragraph but I think it would help to do so (because earlier it says `Pseudo-header fields are not HTTP header fields.`).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#pullrequestreview-344293974

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 16 Jan 2020 17:39:05 -0800

kazuho commented on this pull request.

That's a keen observation. Now I wonder if this section talks just about client-initiated requests, or if it covers pushes too.

If it is the former, we need similar clarification on what is valid for a push. If it is the latter, this sentence should be adjusted in a way that the paragraph would cover pushes too.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#discussion_r367733681

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Jan 2020 20:59:20 -0800

MikeBishop commented on this pull request.

Good call.  Yes, this covers pushes, too -- it's a response message that follows this format.  (I don't see *why* you'd push with a non-final response in the sequence, but it's totally valid....)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#discussion_r367769603

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Jan 2020 21:02:56 -0800

Re-reading with fresh eyes, I see things to fix tomorrow.

Artifact of copying from 7540.

> -those defined in {{!HTTP2}}.  The restrictions on the use of pseudo-header
-fields in Section 8.1.2 of {{!HTTP2}} also apply to HTTP/3.  Messages which
-are considered malformed under these restrictions are handled as described in
-{{malformed}}.
+the status code for the response.
+
+Pseudo-header fields are not HTTP header fields.  Endpoints MUST NOT generate
+pseudo-header fields other than those defined in this document, except as
+negotiated via an extension; see {{extensions}}.
+
+Pseudo-header fields are only valid in the context in which they are defined.
+Pseudo-header fields defined for requests MUST NOT appear in responses;
+pseudo-header fields defined for responses MUST NOT appear in requests.
+Pseudo-header fields MUST NOT appear in trailers.  Endpoints MUST treat a
+request or response that contains undefined or invalid pseudo-header fields as
+malformed ({{malformed}}).

This probably needs to be forcibly rewrapped -- obviously the editor's auto-wrapping was confused by the spacing.

> +
+  ":path":
+
+  : Contains the path and query parts of the target URI (the "path-absolute"
+    production and optionally a '?' character followed by the "query"
+    production (see Sections 3.3 and 3.4 of [RFC3986]).  A request in asterisk
+    form includes the value '*' for the ":path" pseudo-header field.
+
+  : This pseudo-header field MUST NOT be empty for "http" or "https"
+    URIs; "http" or "https" URIs that do not contain a path component
+    MUST include a value of '/'.  The exception to this rule is an
+    OPTIONS request for an "http" or "https" URI that does not include
+    a path component; these MUST include a ":path" pseudo-header field
+    with a value of '*' (see [RFC7230], Section 5.3.4).
+
+All HTTP/3 requests MUST include exactly one valid value for the ":method",

Arguably, this language from 7540 permits it to also contain any number of invalid values.  Worth rephrasing.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#pullrequestreview-344364325

---

From: Mike Bishop <[email-masked]>
Date: Fri, 17 Jan 2020 10:41:27 -0800

After reading the whole section, I think that while the section intends to talk about both push and requests, this paragraph is specific to requests.  I've added more discussion of push streams above, but I think I'm going to leave this particular paragraph alone, modulo fixing @LPardue's initial suggestion.

Nope, it actually does wrap this way by sheer coincidence.  (Or @martinthomson was playing a really long prank in RFC 7540.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#discussion_r368075798

---

From: Mike Bishop <[email-masked]>
Date: Fri, 17 Jan 2020 10:42:16 -0800

0eb5cf31f0a4d7522d57c758901f0408fbef7fc5  Review feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352/files/f5d137d1c834e74074e77bf0834a2158fa33cbda..0eb5cf31f0a4d7522d57c758901f0408fbef7fc5

---

From: Martin Thomson <[email-masked]>
Date: Sun, 19 Jan 2020 21:53:13 -0800

martinthomson approved this pull request.

This is lots of words, but I can't really fault them too much.

>  A server MAY send one or more PUSH_PROMISE (see {{frame-push-promise}}) or
 DUPLICATE_PUSH (see {{frame-duplicate-push}}) frames before, after, or
 interleaved with the frames of a response message. These PUSH_PROMISE and
 DUPLICATE_PUSH frames are not part of the response; see {{server-push}} for
-more details.
+more details.  These frames are not permitted in pushed responses.

```suggestion
more details.  These frames MUST NOT be sent in pushed responses.
```
or

```suggestion
more details.  A pushed response is malformed if it includes PUSH_PROMISE or DUPLICATE_PUSH frames.
```

> +    create a Host header field if one is not present in a request by
+    copying the value of the ":authority" pseudo-header field.
+
+  ":path":
+

I think that we should use "Section X.X of [RFCblah]" as the one form of external section references.

+    a path component; these MUST include a ":path" pseudo-header field
+    with a value of '*' (see [RFC7230], Section 5.3.4).
+
+All HTTP/3 requests MUST include exactly one value for the ":method", ":scheme",
+and ":path" pseudo-header fields, unless it is a CONNECT request ({{connect}}).
+An HTTP request that omits mandatory pseudo-header fields or contains invalid
+values for those fields is malformed ({{malformed}}).
+
+HTTP/3 does not define a way to carry the version identifier that is included in
+the HTTP/1.1 request line.
+
+For responses, a single ":status" pseudo-header field is defined that carries
+the HTTP status code field (see [RFC7231], Section 6).  This pseudo-header field
+MUST be included in all responses; otherwise, the response is malformed (Section
+8.1.2.6).

Fix this section reference.

> @@ -520,9 +622,13 @@ permitted (e.g., idempotent actions like GET, PUT, or DELETE).
 
 A malformed request or response is one that is an otherwise valid sequence of
-frames but is invalid due to the presence of extraneous frames, prohibited
-header fields, the absence of mandatory header fields, or the inclusion of
-uppercase header field names.
+frames but is invalid due to:
+
+- the presence of prohibited header fields or pseudo-header fields
+- the absence of mandatory pseudo-header fields
+- invalid values for pseudo-header fields

If this is intended to be exhaustive, then you need to include ordering of pseudo-header fields and header fields.

And maybe the inclusion of a non-Final status code after a final status code.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#pullrequestreview-345089338

---

From: Mike Bishop <[email-masked]>
Date: Mon, 20 Jan 2020 10:13:45 -0800

> If this is intended to be exhaustive, then you need to include ordering of pseudo-header fields and header fields.

Good call.

> And maybe the inclusion of a non-Final status code after a final status code.

This would be one instance of an invalid sequence of messages.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#discussion_r368676385

---

From: Mike Bishop <[email-masked]>
Date: Mon, 20 Jan 2020 10:20:45 -0800

Fixing the ones in this imported text, but there are enough other instances of that pattern to handle separately.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#discussion_r368678394

---

From: Mike Bishop <[email-masked]>
Date: Mon, 20 Jan 2020 10:22:20 -0800

6f91507da1ecb893106d1eb3a31618b103bb0362  Martin's feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352/files/0eb5cf31f0a4d7522d57c758901f0408fbef7fc5..6f91507da1ecb893106d1eb3a31618b103bb0362

---

From: Mike Bishop <[email-masked]>
Date: Mon, 20 Jan 2020 10:34:04 -0800

Fixed, but I'm going to reverse that.  I like Kazuho's logic that misuse of a frame is a connection error, because the peer is the only one who could have generated it.  I'm going to make these H3_FRAME_UNEXPECTED instead.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#discussion_r368682179

---

From: Mike Bishop <[email-masked]>
Date: Mon, 20 Jan 2020 10:35:27 -0800

c1bff0ab3bb20ec8bcb999acbb2df0b9baec9756  Framing errors are connection-fatal

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352/files/6f91507da1ecb893106d1eb3a31618b103bb0362..c1bff0ab3bb20ec8bcb999acbb2df0b9baec9756

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jan 2020 17:44:37 -0800

ianswett approved this pull request.

LG, minus Martin's suggestion

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3352#pullrequestreview-346303548