search

jshttp / cookie

HTTP server cookie parsing and serialization
MIT License
1.36k stars 159 forks source link

fix: narrow the validation of cookies to match RFC6265 #167

Closed bewinsnw closed 1 month ago

bewinsnw commented 4 months ago

fixes #165. Previously, validation on serialization used the field-content regexp from RFC7230 to perform all checks. However, that is the regexp for the cookie header as a whole, but each individual part of a cookie has a tighter restriction.

In the bug report #165 I demonstrated that whitespace in names and values was invalid but allowed by the field-content pattern. In this code I started by adding tests for those two cases, then added the expressions derived from the RFC; the relevant portions of the RFC have been inlined as comments.

Removing the field-content regexp unearthed that as well as those two it was being used to validate domain names and paths. Paths are fairly unrestricted (being any ascii character except control characters and semicolon) but the code was wrong here too - it allowed all 8-bit characters not just 7-bit characters.

Domain names have a well recognised pattern, there are only a couple of gotchas: RFC6265 explicitly says RFC1123 applies, so the leading character of a label is allowed to be a digit; and while I wondered for a moment if this should allow things like [::1] the domain matching section of https://datatracker.ietf.org/doc/html/rfc6265#section-5.1.3 teaches that domain values are not ip addresses.

wesleytodd commented 4 months ago

Thanks, this VERY well structured from the comments to the new regular expressions, great work. CI is passing, but I want to wait a bit and see if anyone else on the project as opinions on this. My one review comment is not intended to block, so if you feel strongly about that form of regex please tell me.

wesleytodd commented 4 months ago

Hey, can we fix up the lint issues? After that I think we can land this and update in both express v4 and v5

bewinsnw commented 4 months ago

Hey, can we fix up the lint issues? After that I think we can land this and update in both express v4 and v5

🤦 that's what I get for doing those last two commits in the online editor. Fixed, thanks

bewinsnw commented 3 months ago

I think that coveralls just glitched out. I can see at https://coveralls.io/builds/68891763 that the coveralls run was successful, but it looks like it never reported back to github. Just needs a kick.