Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Automatically set cookie
secure
attribute, remove COOKIE_SECURE option #26953Automatically set cookie
secure
attribute, remove COOKIE_SECURE option #26953Changes from 2 commits
c9cd3bf
b58bec7
ecebb8f
f302954
c625510
ac5dc18
bcea3bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure this usage is correct, but it appears to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "next" call is right, but the Sessioner is not designed to be used like this. eg: It starts a GC goroutine for every
session.Sessioner()
call.The sessioner expects there should be only one instance for all requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is suboptimal, I think the only other option is to let sessioner accept a
func
for theSecure
attribute as well, or add aautoSecure
option. Or look for alternative sessioners that are more flexible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not only suboptimal, it breaks the Sessioner's design and would cause bugs. The Sessioner also has many internal states, it expects the only one instance do "Init" (eg: connect to redis) and do session management (eg: Lock, GC).
Although we could make some changes to make a Sessioner work with dynamic Cookie Secure option, but I think it's too complex to do that, and "dynamic Cookie Secure" doesn't seem really bring enough benefit.
The mentioned "AppURL + COOKIE_SECURE option" should be simple enough and I don't see any problem by doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just cookie secure. If the backend wants for example to reconstruct the original URL a client used (for example for audit log), XFP is required for that as well.
Maybe I'll look into sessioner accepting a function (typecheck with Reflect), that seems the simplest option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use AppURL, in many cases, the backend doesn't have the HTTP request context.
So, AppURL should always be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the app runs on multiple URLs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, but the AppURL should also be correct in this case, otherwise the nofication/mail/webhook would stop working.
As long as the AppURL should be always right, then just use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened https://gitea.com/go-chi/session/issues/7.