Cookie without Value Parsing Regression in v7.0.4

[LipingSun]

trafficstars

Support plan

• is this issue currently blocking your project? (yes/no): yes
• is this issue affecting a production system? (yes/no): yes

Context

• node version: v14
• module version with issue: 7.0.4
• last module version without issue: 7.0.3
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): hapi application
• any other relevant information: n/a

What are you trying to achieve or the steps to reproduce?

1. Send cookie without value in the mid of http cookie header, then the validation fails.

cookie: key1=value1; key2; key3=value3

2. It works if key2 is at the end of cookie header

cookie: key1=value1; key2

What was the result you got?

Server Error Log: Invalid cookie name: key2; key3

What result did you expect?

The exact same cookie header works in 7.0.3

[kanongil]

Your cookie is not valid according to the spec (= is required for each pair).

The parsing behaviour changed with #81.

Why do you need to parse invalid cookie strings?

[devinivy]

I believe these should both be failing, but in different ways:

• key1=value1; key2; key3=value3 fails with reason "Invalid cookie name"
• key1=value1; key2 fails with reason "Header contains unexpected syntax: key2"

https://runkit.com/devinivy/634f10aa85dc48000805df2c

[LipingSun]

We have ignoreErrors enabled.

Understand it's not valid according to cookie spec. But for our case, the invalid cookie is from root domain which polluted the cookie send to our subdomain.

This is a common situation in large enterprise, some team put invalid cookie on company1.com, then the cookie will be send to app1.company1.com. It's difficult to trace who put it there. But since browser accepts it, we have to enable ignoreErrors to avoid service intteruptions.

[kanongil]

Hmm… I'm not sure there is a good solution for this class of problem.

We could revise the parsing to account for this specific case, but any such a tweak could cause other non spec-compliant cookie strings to fail to parse in a worse way.

Though, it's possible this can be solved reasonably, by restarting the parsing after the ; on failed cookie names that contain ;. Ie. failed would still contain { name: "key2; key3", value: "value3", … }, but due to the restart, it would also add the key3: "value3" to the states.

Alternatively, I would suggest that you add a onRequest hook to your server, with some custom logic that can remove the non-compliant parts of any cookie header.

[devinivy]

Ahh sorry, I am still a little hung-up on making sure I understand the report. There are two ideas in the report:

1. On statehood 7.0.4 this cookie parses key1=value1; key2; key3=value3 but this one does not key1=value1; key2.

• However, in my tests I see them both parse with ignoreErrors: true and both fail with ignoreErrors: false.

2. On statehood 7.0.4 this cookie fails to parse key1=value1; key2; key3=value3 but on 7.0.3 it succeeds.

• However, In my tests I see the cookie fail parsing on both 7.0.4 and 7.0.3. The failures are different, though, and on 7.0.3 if ignoreErrors is true then you do get a value parsed-out for key3.

If we're in agreement about all that, I take this report to be about the fact that with ignoreErrors: true (since a parsing error occurs on both 7.0.4 and 7.0.3) that key1=value1; key2; key3=value3 parses to { key1: 'value1' } on 7.0.4 versus { key1: 'value1', key3: 'value3' } on 7.0.3. Does that sound right?

[kanongil]

If we're in agreement about all that, I take this report to be about the fact that with ignoreErrors: true (since a parsing error occurs on both 7.0.4 and 7.0.3) that key1=value1; key2; key3=value3 parses to { key1: 'value1' } on 7.0.4 versus { key1: 'value1', key3: 'value3' } on 7.0.3. Does that sound right?

That is my understanding. Essentially, with ignoreErrors: true and key1=value1; key2; key3=value3 it now fails to parse the key3=value3 entry. I assume it doesn't really matter which exact errors are returned.

[LipingSun]

You guys are on the right direction.

More importantly, in 7.0.4, key1=value1; key2; key3=value3 is parsed as { key1: 'value1', 'key2; key3': 'value3' }. Of cause, with ignoreErrors: true, the key2; key3 is ignored. However, it eventually fails at https://github.com/hapijs/statehood/blob/v7.0.4/lib/index.js#L257

[LipingSun]

Is there a timeline when this can be fixed?

[devinivy]

Unfortunately we can't promise timelines around most fixes. For critical issues where something is blatantly incorrect, crashing, or a matter of security, we do our best to resolve things promptly.

That said, I don't think this something that we can treat as a proper bug: we just aren't in a good position to make promises about how invalid cookies get parsed. There are multiple ways to try and interpret that invalid cookie, and statehood is successfully choosing one of them using a simple/maintainable parser that speaks directly to the cookie spec. However, if you want to take a look and contribute some changes to the parser, we'd be happy to review and I don't know about others, but I am personally open to landing a change as long as the parser remains pretty simple (and doesn't use regexes—the parser was rewritten because the regex used in it before lead to security issues).

Barring that, I think your best path here might be to pursue the recommendation gil tossed out there aboe:

Alternatively, I would suggest that you add a onRequest hook to your server, with some custom logic that can remove the non-compliant parts of any cookie header.

Hope this helps!

[LipingSun]

Happy to make a PR. Is there a contribution instruction I should follow?