Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
hmnd opened this issue May 22, 2024 · 15 comments
Open

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

hmnd opened this issue May 22, 2024 · 15 comments

Comments

@hmnd
Copy link

hmnd commented May 22, 2024

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.

@hmnd hmnd added the bug label May 22, 2024
@EdamAme-x
Copy link
Contributor

can you tell me details of error?

@hmnd
Copy link
Author

hmnd commented May 25, 2024

@EdamAme-x
Copy link
Contributor

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
Copy link
Contributor

EdamAme-x commented May 26, 2024

I ready just now @hmnd

@ryuapp
Copy link
Contributor

ryuapp commented May 27, 2024

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: #2314

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

@hmnd
Copy link
Author

hmnd commented May 27, 2024

@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: supabase/auth-helpers#441 (comment)

@EdamAme-x
Copy link
Contributor

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

@ryuapp
Copy link
Contributor

ryuapp commented May 28, 2024

@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: supabase/auth-helpers#441 (comment)

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
Copy link
Contributor

EdamAme-x commented May 28, 2024

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
Copy link
Author

hmnd commented May 28, 2024

@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
Copy link
Contributor

EdamAme-x commented May 28, 2024

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
Copy link
Contributor

fzn0x commented May 28, 2024

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

@yusukebe
Copy link
Member

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
Copy link
Contributor

Jxck commented May 28, 2024

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
Copy link
Contributor

ryuapp commented May 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants