search

quicwg / base-drafts

Internet-Drafts that make up the base QUIC specification
https://quicwg.org
1.63k stars 205 forks source link

Error code for illegal field composition #4336

Closed martinthomson closed 3 years ago

martinthomson commented 4 years ago

(I couldn't find anything on this, but I'm prepared to be corrected. Also, it's late, so if this is closed, I'm totally OK with that.)

We're just in the process of doing full validation of pseudo-header fields (I know, seems late, what are you gonna do about that). And we realized that there is no error code for indicating this sort of error. That is, if pseudo-fields are out of order, or a request contains :status, or there are uppercase characters.

Section 4.1.1.1 contains lots of MUSTs, but it seems to rely entirely on the definition of "malformed" which leads to H3_GENERAL_PROTOCOL_ERROR. This is fine, but not especially helpful in isolating problems.

Could we define an error code for this specific case?

LPardue commented 4 years ago

H2 just says malformed is treated as PROTOCOL_ERROR. So H3 is providing parity.

There might be an argument for more fidelity but I'm not convinced it would help much. It could require more contortions in the text - do you have different error for different types of malformed?

martinthomson commented 4 years ago

Internally, we use a different error code. Of course, all internal error codes map to the same code for sending in RESET_STREAM/CONNECTION_CLOSE.

MikeBishop commented 4 years ago

I'm inclined not to do much, if anything, here. They all trace back to "malformed," as you note, which leads to GENERAL_PROTOCOL_ERROR. The instances in which we use GPE are:

I wouldn't be opposed to splitting the first two off into a new / two new codes, because that would be consistent with our principle of indicating the general area where the failure occurred. I would push back on having separate error codes for specific ways the message could be malformed, because that's pretty expansive rabbit hole.

martinthomson commented 4 years ago

OK, I'm satisfied with that response. I think that we'll just have to signal extra information in the text field. Happy to close this.

MikeBishop commented 4 years ago

@martinthomson, do you think an error code for "malformed message" would be useful, if we don't go into the various types of malformed?

martinthomson commented 4 years ago

That might be useful, yes.

LPardue commented 4 years ago

This is a design change, albeit a marginal one.

LPardue commented 3 years ago

@MikeBishop let's merge the associated PR but keep this issue open and marked as "call-issued"

MikeBishop commented 3 years ago

Keeping open, as requested.

LPardue commented 3 years ago

Closing this now that the IESG review of draft 33 has concluded.

MikeBishop commented 3 years ago

In fairness, I question whether the IESG review of the HTTP drafts has concluded. But as this is not an IESG comment, I'm content to close it. People have had time to object.

LPardue commented 3 years ago

Yes I agree Mike. This was due to me being over eager with the close button. The issue may not have strictly been raised in the period that the IESG were directed to review the doc, and they have yet to ballot on draft 33. But either way there was no push back from the WG on this change since draft 33 was published.