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

status-line ABNF and SP reason-phrase #197

Closed
mnot opened this issue Feb 12, 2019 · 14 comments

Comments

Projects
None yet
5 participants
@mnot
Copy link
Member

commented Feb 12, 2019

The ABNF for status-line is currently:

status-line = HTTP-version SP status-code SP reason-phrase CRLF
reason-phrase  = *( HTAB / SP / VCHAR / obs-text )

I.e., reason-phrase can be empty, but the SP before it is still required.

it appears that some parsers generate an error if this SP is not present, while others do not. Suggest instead:

status-line = HTTP-version SP status-code [ SP reason-phrase ] CRLF
reason-phrase  = 1*( HTAB / SP / VCHAR / obs-text )

?

@mnot mnot added the h1-messaging label Feb 12, 2019

@reschke

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Hm.

So we have parsers that behave correctly, and some which do not. By changing the ABNF, we would break the conforming parsers, and "heal" the non-conforming ones.

Or am I missing something?

@mnot

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

It'd be most accurate to do something like this:

status-line = HTTP-version SP status-code ( ( SP reason-phrase ) / BWS ) CRLF
reason-phrase  = 1*( HTAB / SP / VCHAR / obs-text )

... to make it clear that you shouldn't generate WS there, but you should parse it; that would get us to better behaviour all around. If we could come up with a better way to specify it in ABNF, that'd be even better; e.g.,

status-line = HTTP-version SP status-code [ SP reason-phrase ] BWS CRLF
reason-phrase  = 1*( HTAB / SP / VCHAR / obs-text )

... since I'd strongly suspect that implementations are going to strip the WS at the end anyway.

@reschke

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

to make it clear that you shouldn't generate WS there, but you should parse it;

Disagreed. This would still change a "MUST generate" to a "SHOULD NOT generate".

This is a normative change, and it's not clear to me why we're considering it.

@mnot

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

This would still change a "MUST generate" to a "SHOULD NOT generate".

I don't read the last one that way; it's saying you MUST generate it if there's actually a reason-phrase, and that you SHOULD NOT generate WS at the end, or if you omit the reason-phrase.

Do we really think that the current ABNF's requirement to generate a SP there reflects conscious intent?

This is a normative change, and it's not clear to me why we're considering it.

Because after thirty+ years, it's pretty clear that specifying protocol elements in ABNF doesn't promote interop?

Defining interop in these corner cases can be important, especially in weird cases where servers behave differently; it creates friction against changing servers. Yes, this is seen in the wild.

@reschke

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

I don't read the last one that way; it's saying you MUST generate it if there's actually a reason-phrase, and that you SHOULD NOT generate WS at the end, or if you omit the reason-phrase.

Yes. That's what I disagree with.

Do we really think that the current ABNF's requirement to generate a SP there reflects conscious intent?

I don't know.

Because after thirty+ years, it's pretty clear that specifying protocol elements in ABNF doesn't promote interop?

You lost me. Some people do not read the ABNF, yes, and get things wrong. Why does this come up right here right now?

Defining interop in these corner cases can be important, especially in weird cases where servers behave differently; it creates friction against changing servers. Yes, this is seen in the wild.

Yes, but in this case, the change breaks existing conforming implementations.

I know this is now seen in the wild, but it seems to be a consequence of removing the status-line from HTTP/2 (which was a bad idea in the first place, given the amount of confusion it has caused).

Are we now making normative changes to the HTTP/1.1 message format because of that?

@mnot

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

I don't see the connection to H2 at all; rather, I'm seeing much older implementations who are being looser with the requirement to place an SP there.

@reschke

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Well, the problem comes up because people are stopping to use reason phrases, and thus stumble over the ABNF oddity.

@wtarreau

This comment has been minimized.

Copy link

commented Feb 12, 2019

@mnot

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

I can live with the MAY ignore -- although it again devalues the ABNF. shrug

@wtarreau

This comment has been minimized.

Copy link

commented Feb 12, 2019

@kazuho

This comment has been minimized.

Copy link

commented Feb 18, 2019

I agree with @reschke that the definition should better be stable.

If we change the requirements, the fear would be that we might start seeing servers actively omitting the whitespace. That would cause interoperability issues with existing clients that have done the correct thing.

I am fine with adding something like: a parser MAY ignore the absence of the whitespace.

@royfielding

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I agree with @reschke that the SP being required was intentional for 7231 specifically because we believed it to be better for interop than not sending the SP. It's weird to read, sure, but not worse than bell bottoms.

@reschke

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

FWIW, I would agree with replacing "reason phrase is required but can be empty" with "reason phrase is optional". But let's leave the SP alone :-)

@mnot

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

Revised proposal:

  1. Change ABNF to make reason-phrase optional, but require at least one character when present
  2. Add prose to note that even when the reason-phrase is not present, the SP is still required.

@reschke reschke self-assigned this Mar 25, 2019

reschke added a commit that referenced this issue Mar 26, 2019

Change reason-phrase to be optional but non-empty (instead of requrin…
…g it and allowing it to be empty). Also add a note about what the absence of reason-phrase means in practice (#197)

@reschke reschke closed this Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.