Conversation
|
Note: benchmarks show no performance degradation whatsoever, which is expected since the condition is triggered in the same place where an error would be triggered otherwise. |
|
Another note: I've upgraded the |
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
Would the idea be to also address other points listed under https://github.com/creationix/http-parser-js/blob/master/tests/README.md using this flag, i.e. to extend the lenient mode to cover more technically invalid but real-world data?
|
The ones there that cover different content-length edge cases are too dangerous to implement here. They'll open up a way for a request smuggling. I think it might make sense to land it as it is now, and introduce more support depending on whether an issue will be filed in node.js repo or not. In each case we'll have to decide together whether such leniency would be safe to add. |
|
cc @bnoordhuis |
| } else if (parser->flags & F_LENIENT) { | ||
| parser->flags ^= F_LENIENT; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shorter/branchless way of doing the same (not that it really matters but, you know, it looks neat):
parser->flags ^= F_LENIENT & parser->flags;
parser->flags ^= F_LENIENT * !!enabled;There was a problem hiding this comment.
Good point. While we are here, am I right that &= ~F_LENIENT would not work reliably here? (out of curiosity)
There was a problem hiding this comment.
It does actually but two statements with a common prefix is aesthetically pleasing to me. :-)
There was a problem hiding this comment.
I decided to go with ~F_LENIENT as it is less tricky and explicit.
indutny
force-pushed
the
feature/lenient
branch
from
d3ae731 to
60fb151
Compare
Introduce `llhttp_set_lenient` API method for enabling/disabling lenient parsing mode for header value. With lenient parsing on - no token check would be performed on the header value. This mode was originally introduced to http-parser in nodejs/http-parser@e2e467b9 in order to provide a fallback mode for less compliant clients/servers.
indutny
force-pushed
the
feature/lenient
branch
from
60fb151 to
7b58fae
Compare
|
Landed in 95321e2, thank you everyone! |
Introduce `llhttp_set_lenient` API method for enabling/disabling lenient parsing mode for header value. With lenient parsing on - no token check would be performed on the header value. This mode was originally introduced to http-parser in nodejs/http-parser@e2e467b9 in order to provide a fallback mode for less compliant clients/servers. PR-URL: #33 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Introduce `llhttp_set_lenient` API method for enabling/disabling lenient parsing mode for header value. With lenient parsing on - no token check would be performed on the header value. This mode was originally introduced to http-parser in nodejs/http-parser@e2e467b9 in order to provide a fallback mode for less compliant clients/servers. PR-URL: nodejs/llhttp#33 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
3 participants
Introduce
llhttp_set_lenientAPI method for enabling/disabling lenientparsing mode for header value. With lenient parsing on - no token check
would be performed on the header value.
This mode was originally introduced to http-parser in
nodejs/http-parser@e2e467b in
order to provide a fallback mode for less compliant clients/servers.
See: nodejs/node#30222 (comment)
cc @addaleax @nodejs/http