honojs / hono

Web framework built on Web Standards
https://hono.dev
MIT License
18.69k stars 528 forks source link

Cookie not properly parsed. `validCookieNameRegEx` seems too strict. #3189

Open dimroth opened 1 month ago

dimroth commented 1 month ago

What version of Hono are you using?

4.5.1

What runtime/platform is your app running on?

Cloudflare workers

What steps can reproduce the bug?

Create a middleware with the following code

app.use(async (c, next) => {
  const allGetCookie = getCookie(c);
  console.log("getCookie", allGetCookie);
  console.log("allCookies", c.req.header("Cookie"));
  await next()
});

Send a curl request providing a cookie to your endpoint

curl --location --request POST 'http://localhost:8787/' \
--header 'Cookie: paraglide:lang=en;test=ok'

Your console should print

getCookie { test: 'OK' }
allCookies paraglide:lang=en;test=OK

What is the expected behavior?

getCookie should print {'paraglide:lang': 'en', test: 'OK'} Cookie names containing : should be accepted.

What do you see instead?

According to this popular SO topic, all alphanums plus !#$%&'()*+-./:<=>?@[]^_`{|}~ characters should be considered OK though not entirely complying with rfc6265.

The most popular cookie parsing libraries don't enforce cookie name checking (ex: cookie, cookie-js...) and unfortunately, some external libraries or services can sometime set exotic cookie names 😢 ...which makes the use of getCookie not ideal since it can omit a lot of the cookies we see in the real world.

Don't you think we could also skip the validation of cookie name or be less strict about it?

Additional information

If so, I'd be happy to submit a PR

yusukebe commented 1 month ago

Hi @dimroth Thank you for the issue.

@Jxck Hey, What do you think of this?

ryuapp commented 1 month ago

and unfortunately, some external libraries or services can sometime set exotic cookie names 😢 ...which makes the use of getCookie not ideal since it can omit a lot of the cookies we see in the real world.

I'm interested in exotic cookie names. Could you inform me of some examples? I would like to know if there are any libraries or services that actually use such names.

dimroth commented 1 month ago

Thank you for the feedback. @ryuapp, unfortunately, I could only experience it with @inlang/paraglide-sveltekit which will want to set a cookie named paraglide:lang. I'll edit the post if I find others in the meantime.

It makes sense to enforce a RFC 6265 compliant format for a cookie producing library but is it really necessary to check the validity of names for a consumer? (like I previously mentioned, most popular cookie libs don't)

Jxck commented 1 month ago

This is interesting topic. I'll check spec and code.

@dimroth

most popular cookie libs don't

Could you share some libs you mentioning here ?

dimroth commented 1 month ago

Here you go:

Jxck commented 1 month ago

I've checked the RFC specification.

Cookie parse() checks valid characters while parsing

https://github.com/honojs/hono/blob/1fafc7aec87f1afe68b37506df58a66acd9f29aa/src/utils/cookie.ts#L77

based on ABNF in RFC

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-11#section-4.1.1

this ABNF is for producing cookie from server. It means that, if Cookie from client was served as Set-Cookie by Hono, it's fine. But if hono receives cookie not served by hono, this problem happens.

Solution 1

according to "3.2.2.1. Programming Languages & Software Frameworks" in RFC6265bis

A programming language or software framework with support for cookies could reasonably be used to create an application that acts as a cookie producer, cookie consumer, or both. Because a developer may want to maximize their compatibility as either a producer or consumer, these languages or frameworks should strongly consider supporting both sets of requirements, Section 4 and Section 5, behind a compatibility mode toggle. This toggle should default to Section 4's requirements.

Doing so will reduce the chances that a developer's application can inadvertently create cookies that cannot be read by other servers.

The section above is what Hono should respect. And Section 5 requirement mentions relaxed algorithm (than ABNF) for Set-Cookie.

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-15#section-5.6 (tl;dr: split ";", "=", trim space etc)

This is for Set-Cookie but Set-Cookie is basically upper compatible with Cookie header. So I think Hono can be

Solution 2

hono parses received cookie as current implementation. And if application wanna violates ABNF, author should parse it by themself (e.g. use parser of same library) As reporter shown, c.req.header("Cookie") can be a workaround. There is no way of knowing how received cookie deviate from the specification, but this solution make hono free from such concern.

dimroth commented 1 month ago

@Jxck Thank you for your thorough investigation and spec reference. The point is well-made.

Additional considerations:

While workarounds exist, implementing Solution 1 would likely result in a more flexible, frictionless helper, potentially increasing framework adoption.