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.

Sign up for GitHub

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

Jump to bottom

c-headers: do not list PRI as a valid HTTP method #105

Closed
wants to merge 1 commit into from
Closed

c-headers: do not list PRI as a valid HTTP method #105

wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Apr 20, 2021

PRI gets special treatment (parse preamble and error) and even though it
is parsed and allowed after HTTP/1.x - it is not technically a method
that could appear in the request parsed with llhttp. Remove the method
from the list of HTTP methods in C headers to prevent confusion and
breakage of body-parser.

See: #103 (comment)

C header changes

diff --git a/tmp/llhttp-old.h b/tmp/llhttp-new.h
index fd1fdb9..0b06170 100644
--- a/tmp/llhttp-old.h
+++ b/tmp/llhttp-new.h
@@ -231,7 +231,6 @@ typedef enum llhttp_method llhttp_method_t;
   XX(31, LINK, LINK) \
   XX(32, UNLINK, UNLINK) \
   XX(33, SOURCE, SOURCE) \
-  XX(34, PRI, PRI) \
 
 
 #define RTSP_METHOD_MAP(XX) \

cc @nodejs/http

@indutny
c-headers: do not list PRI as a valid HTTP method
3ad507a 
PRI gets special treatment (parse preamble and error) and even though it
is parsed and allowed after `HTTP/1.x` - it is not technically a method
that could appear in the request parsed with llhttp. Remove the method
from the list of HTTP methods in C headers to prevent confusion and
breakage of `body-parser`.
@indutny indutny mentioned this pull request Apr 20, 2021
Closed
mcollina
mcollina approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

indutny added a commit that referenced this pull request Apr 22, 2021
@indutny
c-headers: do not list PRI as a valid HTTP method
3f85eb7 
PRI gets special treatment (parse preamble and error) and even though it
is parsed and allowed after `HTTP/1.x` - it is not technically a method
that could appear in the request parsed with llhttp. Remove the method
from the list of HTTP methods in C headers to prevent confusion and
breakage of `body-parser`.

PR-URL: #105
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@indutny
Copy link
Member Author

indutny commented Apr 22, 2021

Landed in 3f85eb7, thank you!

@indutny indutny closed this Apr 22, 2021
@indutny indutny deleted the fix/pri-listed branch April 22, 2021 16:33
@indutny
Copy link
Member Author

indutny commented Apr 22, 2021

Released in v6.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

Assignees
No one assigned
Labels
semver-patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@indutny @mcollina @BethGriggs