Parse cookie pairs without a regex

[devinivy]

Using a simple parser rather than a regex is a more direct, durable, and performant solution to collecting cookie name-value pairs. I think that writing this logic out also makes it a bit clearer how we treat bad/invalid cookies. In order to confirm that the behavior remains identical to the regex-based parser, I added tests for a couple additional edge-cases.

[kanongil]

Just a few quick comments.

I like that this logic allows value pairs inside a double-quoted string to be found, like
a="b; c=d; e=". Yours will find a="b, c=d, e=", while the regex just finds a=b; c=d; e=.

Did you verify that it is actually faster?

[devinivy]

Thanks for the review @kanongil.

I agree with all your notes re: trim() and the ; SP delimiter, but these are more substantial behavioral changes to statehood than I'm hoping to make now. Our existing parser is "loose" in certain ways that I was trying to maintain, which is one reason it doesn't look identical to the parsers you'd find in express, fastify, or tough-cookie. I also have some questions about just how close to the spec we should keep— for example, it seems common in parsers across the node ecosystem to ignore a trailing semi-colon, which wouldn't jibe with the ; SP parsing. These are good conversations that we should have, but I am hoping to defer that to future work, and just maintain the behavior of the existing parser within reason as part of this work.

I did confirm it was faster, too! Here's a quick peek at the time for 100 random runs (x is a run with a given random cookie string, y is time).

Expand graph:

│                             •                                                                    
│                                                                                                  
│                                                                           •                      
│                                                                       •                          
│                                                                               •                  
│                                                                                                  
│                                                                                                  
│                                                                                                  
│                                                                                                  
│                                                                                                  
│                                                                                                  
│                                                                                •                 
│     •                                                                                            
│ •                                                                      •                         
│                     •                          ••                •         ••                    
│•     •     •                                                                                     
│                                                                                                  
│  •        •       •    •     ••         •   •                                                  • 
│   •   •         •     •        •  •  •   •                                                       
│    •   •      •      •    •           ••          •                      •                •     •
│         ••  •• • •      ••      •• ••     •  •                 •             •                   •
│                    •       •               •       • •  ••  • •     •              • •           
│                                               •  •  •     •  •  • •     •       •   • ••     ••  
│                                                       ••   •       •             ••     ••  •    
│                                                                      •                     •     
│                                                                                                  
│                                                                                                  
│                                                                                                  
│                                                                                                  
│*        *                                                                                        
│                                                                                                  
│          *                *                                                                      
│ *                          *                                                                     
│  ***   *  *    *                                                                                 
│     ***     *           **          *           *                   *                            
│            * ** ********    ********   * **   ** **  *****  * *      **  **    * * *  **  * **** 
│                                      ** *  ***     **     ** * *****   **  **** * * **  ** *    **
│                                                                                                  
┼──────────────────────────────────────────────────────────────────────────────────────────────────▶

[kanongil]

Loose parsing is probably for the best.

The main behavioural change is the mentioned exposing of new value-pairs with mis-matched '"', which I would consider a bug fix. Maybe you can add a test for when they match as well? Eg. for a="; b=2; "; c=3. Here the regex returns a=; b=2; & c=3, while your logic would return something like a=", b=2 & "; c=3.

[devinivy]

Sounds good, I will take a look at that and at minimum add a test 👍

[devinivy]

Thank you to @watson, @jportner, and AlanBugz in conjunction with Kibana's bug bounty program for disclosing a susceptibility to ReDoS in statehood's cookie parser, now fixed by this patch and available on npm.