search

honojs / hono

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

Setting cookie with Max-Age > 400 days throws #2762

Open hmnd opened 5 months ago

hmnd commented 5 months ago

What version of Hono are you using?

4.3.9

What runtime/platform is your app running on?

Cloudflare Workers

What steps can reproduce the bug?

Set cookie with Max-Age > 400 days.

What is the expected behavior?

Cookie is set without throwing an error.

What do you see instead?

An error is thrown.

Additional information

This has caused an issue with supabase's auth library and could affect similar usage of cookies in other libs/projects.

Given the RFC is targeted primarily at User Agents (The user agent MUST limit the maximum value of the Max-Age attribute), I don't believe it's wise to force this onto users of Hono.

EdamAme-x commented 5 months ago

can you tell me details of error?

hmnd commented 5 months ago

@EdamAme-x here: https://github.com/honojs/hono/blob/main/src%2Futils%2Fcookie.ts#L152-L158

EdamAme-x commented 5 months ago

Thanks @hmnd . Understood. I think we can choose to remove the "Max-Age" restriction, since specifying more than 400 days does not result in an explicit error.

EdamAme-x commented 5 months ago

I ready just now @hmnd

ryuapp commented 5 months ago

Is there any good in being able to set max-age to more than 400 days?

Chrome will automatically change the max-age to 400 days if it exceeds 400 days. In other words, it behaves differently than the value set in the web framework. So I don't think restrictions should be removed if there is no benefit.

The restriction was added in the following PR: https://github.com/honojs/hono/pull/2314

Related: https://developers.google.com/privacy-sandbox/blog/progress-in-the-privacy-sandbox-2022-04#additional_cookie_updates

hmnd commented 5 months ago

@ryuapp I think the more important question is, is there any benefit to throwing when a max age > 400 is set? If you review my earlier comments, you can see this has caused me grief trying to use Hono with Supabase Auth: https://github.com/supabase/auth-helpers/issues/441#issuecomment-2046645189

EdamAme-x commented 5 months ago

hi @ryuapp . I don't see the need to throw an error if it is automatically 400 days, as I think it is.

ryuapp commented 5 months ago

@ryuapp I think the more important question is, is there any benefit to throwing when a max age > 400 is set? If you review my earlier comments, you can see this has caused me grief trying to use Hono with Supabase Auth: https://github.com/supabase/auth-helpers/issues/441#issuecomment-2046645189

Thank you for your comment. As mentioned by the contributor in #2314, the restriction guides developers to best practice. The reason for the restriction is that there is no point in exceeding 400 days. This is because RFC6265bis is about to specify that even if max-age exceeds 400 days, user-agent MUST limit to within 400 days. So I agree with the restriction and have heard benefits of setting them for 400 days or more.

Of course, I agree that it's tiresome because of the restrictions. It's a balance with DX. Do you have sample code? That way we will know the problem better.

EdamAme-x commented 5 months ago

I agree. @ryuapp But, if the cause of this problem is hono-side. I think we will should create options of max-age limit of cookie. Do you show me your code? @hmnd

hmnd commented 5 months ago

@ryuapp @EdamAme-x I'm not doing anything special, just using @supabase/ssr with SvelteKit, as described here. You can see the cookie options that package uses here.

I don't understand how that's relevant to the issue at hand, though. The question is whether Hono should be interfering with the cookies that users set.

IMO Hono should not be interfering here, as it breaks usage of similar packages that set a max-age that the user cannot modify. As I previously mentioned, the RFC is also only relevant to user agents, not web servers.

Is there a good reason for Hono to be doing this?

EdamAme-x commented 5 months ago

The only solution seems to be to remove the limits or add an option. As I said, it's not an explicit error (this is as warning), so it seems to me that the limits can be removed. What do you think? @ryuapp I think removal is necessary for compatibility with many libraries... If there is a solution that doesn't have to be removed, and if it's cool, that's the best I think.

fzn0x commented 5 months ago

I think we should only apply the limits in the user agent

yusukebe commented 5 months ago

Hi @hmnd Thanks for the issue.

IMO Hono should not be interfering here, as it breaks usage of similar packages that set a max-age that the user cannot modify. As I previously mentioned, the RFC is also only relevant to user agents, not web servers.

Is there a good reason for Hono to be doing this?

@Jxck @usualoma Do you have any thoughts?

Jxck commented 5 months ago

RFC is also only relevant to user agents, not web servers.

Yes it mentions "User Agent" but Max-Age is a attribute for Set-Cookie, not Cookie. and Set-Cookie is sent by Server, not by User Agent.

Why don't you file a bug not to hono but to the implementation which ignores RFC?

ryuapp commented 5 months ago

I looked at default settings of famous auth libraries such as Auth.js, and found that cookie's max-age was set to 7 days, 30 days, 1 year, etc. So this problem only concerns some libraries like @supabase/auth-helpers, not many. Also, the max-age can be set in the auth library, so this problem is not critical. If many other libraries are affected, it'sad, we can also consider removing them again.