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

Feature: Session Only Cookies #1752

Merged
merged 7 commits into from
Feb 7, 2022

Conversation

abhi12299
Copy link
Contributor

Allows setting cookies that last for just the browser session by using SessionOnly property in fiber.Cookie which ignores Max-Age and Expires headers if set to true.
Now you can set a session only cookie like so:

c.Cookie(&fiber.Cookie{
    //...
    SessionOnly: true,
    // ...
})

This should close #1610.

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 6, 2022

  • don´t forget the documentation repository

@ReneWerner87
Copy link
Member

hmm with the feature i actually only meant the csrf middleware

thought you could set the cookie to session without this switch by setting the expires to 0

but this does not work with the csrf middleware, because there the default is used

we need the check there but a check to take the default value in case of doubt

@abhi12299
Copy link
Contributor Author

abhi12299 commented Feb 6, 2022

don´t forget the documentation repository

Wanted to update after your review.

thought you could set the cookie to session without this switch by setting the expires to 0

Oh that is actually right.. So should I revert it from there?

we need the check there but a check to take the default value in case of doubt

Didn't get you.. do you want me to keep the SessionOnly key in middleware Config struct? If we remove this key, then in order to put Expires to 0 in the csrf middleware, we need to compare the cfg.Expiration to some special value (maybe like -1 or 0) and set Expires to 0, but that is probably not the right way. Or alternatively, we could keep SessionOnly and set Expires to 0 that way. What do you suggest?

@ReneWerner87
Copy link
Member

Or alternatively, we could keep SessionOnly and set Expires to 0 that way. What do you suggest?

Yeah, keep the config settings and do it in this way

@abhi12299
Copy link
Contributor Author

abhi12299 commented Feb 7, 2022

@ReneWerner87 I read about how cookies can be made session only, and I found that Expires and Max-Age have to be omitted. Setting Expires to 0 will make browser delete the cookie immediately, which you can read about here and here. So there is one observation:

thought you could set the cookie to session without this switch by setting the expires to 0

For a cookie to be session only, we cannot just set expiration to 0 which means we have to keep a flag in ctx.Cookie function.
Also in case of the csrf middleware, we have to omit Expiration when SessionOnly is true.

What are your thoughts on this? I will proceed further once we decide what needs to be done.

@ReneWerner87
Copy link
Member

if int(cfg.Expiration.Seconds()) <= 0 {

The line I meant, for that we would need the setting

We need to know that the user differs from the default

@abhi12299
Copy link
Contributor Author

if int(cfg.Expiration.Seconds()) <= 0 {

The line I meant, for that we would need the setting

We need to know that the user differs from the default

@ReneWerner87 understood. I will use the setting to bypass setting expiration altogether.
By the way, did you read through the reference links that I gave above? I think we might need a similar setting for ctx.Cookie struct as well.

@ReneWerner87
Copy link
Member

ok perfect

#1752 (comment)

only this task is missing

@ReneWerner87 ReneWerner87 merged commit 68fcd8c into gofiber:master Feb 7, 2022
@welcome
Copy link

welcome bot commented Feb 7, 2022

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

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

Successfully merging this pull request may close these issues.

🤗 why it's not possible to set cookie for CSRF middleware expire date to session?
3 participants