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

Handle extra spaces in status line #216

Closed

Conversation

emontnemery
Copy link
Contributor

Handle extra spaces in status line

Background:
llhttp currently does not handle extra spaces in the status line correctly

This snippet:

HTTP/1.1  200  OK

Is parsed as status=0, v=1/1, reason = ' OK', which is clearly not correct.

This PR lets the extra spaces slide, which the recipient MAY do: https://www.rfc-editor.org/rfc/rfc9112.html#name-status-line

Another option would be to reject the status line if it's not exactly to spec:

status-line = HTTP-version SP status-code SP [ reason-phrase ]

@ShogunPanda
Copy link
Contributor

Hi and thanks for this.
At the moment llhttp implements HTTP/1.1 described in RFC 7230, not the one in RFC 9110 and 9112.
For this reason I'm leaning towards rejecting this for now.

@mcollina @ronag Opinions?

@mcollina
Copy link
Member

I think we might want to start a 9910-9112 branch and put there all changes like this one, once we are satisfied, we can ship it as a major.

@ShogunPanda
Copy link
Contributor

I wonder if, rather than trying to migrate the current RFC to the new one we could start fresh. Otherwise the parallel release line will be a mess to manage.

@mcollina
Copy link
Member

I don't think we should release the new version until we are ready. I'm not sure if I would recommend implementing yet another http 1.1 parser.

@ShogunPanda
Copy link
Contributor

I agree. It's a monster thing to do. But the problem is that at the moment llhttp is mostly black magic as llparse is loosely documented and @indutny is absent.

@ronag
Copy link
Member

ronag commented Jan 17, 2023

I'm adding this to the TSC agenda to discuss the state of maintenance on llhttp.

@ShogunPanda
Copy link
Contributor

@ronag Thanks, we definitely need this.

@emontnemery
Copy link
Contributor Author

At the moment llhttp implements HTTP/1.1 described in RFC 7230, not the one in RFC 9110 and 9112.
For this reason I'm leaning towards rejecting this for now.

That makes sense I guess.

RFC7230 says that a status line is:

status-line = HTTP-version SP status-code SP reason-phrase CRLF

Would a PR which rejects a status line which does not conform to that be accepted?

@ShogunPanda
Copy link
Contributor

@emontnemery Yes, definitely.

@emontnemery
Copy link
Contributor Author

OK, a stricter parsing is implemented in #217

@ShogunPanda
Copy link
Contributor

This has been superseded by #217.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants