Skip to content
New issue

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

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

Already on GitHub? # 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.

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

Successfully merging this pull request may close these issues.

4 participants