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

fix: don't update session on GET with token already set #2483

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

chrispatmore
Copy link
Contributor

@chrispatmore chrispatmore commented Oct 2, 2023

Motivation:

The updated CSRF handler would attempt to set the token into the session even when a new one was
not generated, this opened a timing issue where a GET request that started during / before a POST
but ended after would revert the changed token, leaving the user stuck

Contributes To: #2447

@chrispatmore chrispatmore force-pushed the 2447-csrf-trapping-pt2 branch from 17ec627 to e56daf8 Compare October 2, 2023 08:53
@chrispatmore
Copy link
Contributor Author

Sorry @pmlopes @vietj tried to pull down my fix to #2447 early to try it out in our actual scenario and discovered a bug. Hopefully clear what it was from above. fixed it on my end, contributing it back now

@chrispatmore
Copy link
Contributor Author

Not sure why those tests are failing, seems completely unrelated to me

@tsegismont
Copy link
Contributor

@chrispatmore I've restarted the jobs

@chrispatmore
Copy link
Contributor Author

Still not sure about the failure @tsegismont ?
FYI i have tagged this to the closed issue, do you want me to raise a new issue for this fix (I didn't initially, because its not released yet)

@chrispatmore chrispatmore force-pushed the 2447-csrf-trapping-pt2 branch from e56daf8 to 4e30296 Compare October 9, 2023 14:42
Signed-off-by: Chris Patmore <chrism.patmore@btinternet.com>
@chrispatmore chrispatmore force-pushed the 2447-csrf-trapping-pt2 branch from 4e30296 to 6f0dd29 Compare October 10, 2023 08:34
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @chrispatmore

@tsegismont
Copy link
Contributor

@chrispatmore #2447 has milestone is 4.5.0 but the PR which fixed has milestone 5.0.0.

Has the fix been backported to the 4.x branch?

@tsegismont tsegismont merged commit 76d60be into vert-x3:master Oct 10, 2023
6 checks passed
@chrispatmore chrispatmore deleted the 2447-csrf-trapping-pt2 branch October 10, 2023 10:19
@vietj
Copy link
Contributor

vietj commented Oct 26, 2023

@chrispatmore can you provide the same PR for the 4.x branch ?

@chrispatmore
Copy link
Contributor Author

chrispatmore commented Oct 27, 2023

Sure I can do that.

Edit: The 4.x branch is a reasonable amount further behind than I was expecting, I assume you don't want me to pull in things from other changes as much as possible?

@chrispatmore chrispatmore restored the 2447-csrf-trapping-pt2 branch October 27, 2023 08:01
@chrispatmore chrispatmore deleted the 2447-csrf-trapping-pt2 branch October 27, 2023 08:05
@chrispatmore
Copy link
Contributor Author

@vietj does this meet your expectation: #2500

@tsegismont
Copy link
Contributor

Edit: The 4.x branch is a reasonable amount further behind than I was expecting, I assume you don't want me to pull in things from other changes as much as possible?

Right

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

Successfully merging this pull request may close these issues.

3 participants