Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error code for illegal field composition #4336

Closed
martinthomson opened this issue Nov 5, 2020 · 12 comments · Fixed by #4346
Closed

Error code for illegal field composition #4336

martinthomson opened this issue Nov 5, 2020 · 12 comments · Fixed by #4346
Assignees
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus. ietf-lc An issue that was raised during IETF Last Call.

Comments

@martinthomson
Copy link
Member

(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
Copy link
Member

LPardue commented Nov 5, 2020

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
Copy link
Member Author

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.

@larseggert larseggert added the ietf-lc An issue that was raised during IETF Last Call. label Nov 6, 2020
@MikeBishop
Copy link
Contributor

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:

  • Malformed HTTP messages
  • Push whose request headers change across multiple promises
  • As a substitute for more specific codes

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
Copy link
Member Author

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.

@martinthomson martinthomson added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Nov 10, 2020
@MikeBishop
Copy link
Contributor

@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
Copy link
Member Author

That might be useful, yes.

@LPardue LPardue added the design An issue that affects the design of the protocol; resolution requires consensus. label Nov 10, 2020
@LPardue
Copy link
Member

LPardue commented Nov 10, 2020

This is a design change, albeit a marginal one.

@LPardue
Copy link
Member

LPardue commented Dec 11, 2020

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

@MikeBishop
Copy link
Contributor

Keeping open, as requested.

@MikeBishop MikeBishop reopened this Dec 14, 2020
@MikeBishop MikeBishop added call-issued An issue that the Chairs have issued a Consensus call for. and removed proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. labels Dec 14, 2020
@LPardue
Copy link
Member

LPardue commented Jan 8, 2021

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

@LPardue LPardue closed this as completed Jan 8, 2021
@LPardue LPardue removed the call-issued An issue that the Chairs have issued a Consensus call for. label Jan 8, 2021
@MikeBishop
Copy link
Contributor

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
Copy link
Member

LPardue commented Jan 13, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus. ietf-lc An issue that was raised during IETF Last Call.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants