Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Only save Session once to avoid duplicate session cookie #2193

Merged
merged 2 commits into from
Feb 3, 2022
Merged

Only save Session once to avoid duplicate session cookie #2193

merged 2 commits into from
Feb 3, 2022

Conversation

saurori
Copy link
Contributor

@saurori saurori commented Dec 17, 2021

Addresses this Issue: #2112

Session().Save() was being called multiple times. If the session data changes between calls to Save(), the session cookie value would be different and therefore set multiple times. Session().Save() was also called in flash persist() and was therefore coupled tightly to Flash functionality.

Session saving is now done only once in Render() or Redirect(). There is still the possibility of a user calling c.Session().Save() in a request handler or similar and cause a duplicate session cookie to write.

@saurori saurori requested a review from a team as a code owner December 17, 2021 19:57
@@ -103,7 +103,6 @@ func (ri RouteInfo) ServeHTTP(res http.ResponseWriter, req *http.Request) {

events.EmitPayload(EvtRouteStarted, payload)
err := a.Middleware.handler(ri)(c)
c.Flash().persist(c.Session())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not clear why this is needed, so I removed it. persist() no longer saves Session, so is this needed? Only reference is: #2054

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@saurori
Copy link
Contributor Author

saurori commented Feb 2, 2022

Commenting to remove stale. Still relevant, waiting for review 😢

Copy link
Member

@paganotoni paganotoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@fasmat fasmat changed the base branch from main to development February 3, 2022 13:14
@fasmat fasmat merged commit 5867200 into gobuffalo:development Feb 3, 2022
@saurori saurori deleted the dupe-session branch February 3, 2022 15:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants