Skip to content

[UNDERTOW-2580] Fix SameSite and cookie attributes #1761

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[UNDERTOW-2580] Fix SameSite and cookie attributes #1761

wants to merge 1 commit into from

Conversation

@baranowb
Copy link
Contributor

@baranowb baranowb commented Jul 18, 2025

@baranowb baranowb requested review from fl4via and ropalka July 18, 2025 11:20
@baranowb baranowb added bug fix Contains bug fix(es) enhancement Enhances existing behaviour or code under verification Currently being verified (running tests, reviewing) before posting a review to contributor waiting CI check Ready to be merged but waiting for CI check new feature / API change New feature to be introduced or a change to the API (non suitable to minor releases) waiting peer review PRs that edit core classes might require an extra review on hold PR awaits non CI/Review holdover labels Jul 18, 2025
@baranowb
Copy link
Contributor Author

baranowb commented Jul 18, 2025 •
edited
Loading

Ive put this on HOLD as it needs review and discussion. Also 2.3.x is target as its the one that I can test with reproducer.

baranowb
baranowb commented Aug 27, 2025
View reviewed changes
servlet/src/main/java/io/undertow/servlet/spec/ServletCookieAdaptor.java
this.sameSite = sameSite;
return this;
}
// @Override
Copy link
Contributor Author

@baranowb baranowb Aug 27, 2025

Choose a reason for hiding this comment

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

This and other instances should maybe be just turned into dummy methods with @Depracated?
Im fairly sure samesite might have worked if used on certain layer, its just integration was broken.

@baranowb baranowb force-pushed the UNDERTOW-2580 branch 4 times, most recently from 0c0c0e7 to ae74ad3 Compare September 2, 2025 13:24
@baranowb
Copy link
Contributor Author

baranowb commented Sep 3, 2025 •
edited
Loading

I just briefly went over Servlet 6.1 and this stood out as something that might be needed here? d7b2f72#diff-28250838bdef188a6b0e5f836b1ac9fc386ca42f97873d9ecdd6b4b0d6aa3323L110

I will take a look tomorrow.
Indeed its somewhat duplicated in different place here: ae74ad3#diff-aed38908a5b2c705ed9212fcfb3e4b11caf2ecda88d05f9a5a97a5a68dbef3e6R46

@baranowb
Copy link
Contributor Author

baranowb commented Sep 3, 2025 •
edited
Loading

d7b2f72#diff-88a5392afc0673ed5ca3b2fe41f74dd7e1dc8d38ba5a0762088f69f2d68bea3cR403 this will for sure, as this isnt mandated in draft.

Im going to add Hold to this until Servlet stuff is merged. Still needs review.

baranowb
baranowb commented Sep 4, 2025
View reviewed changes
core/src/main/java/io/undertow/server/session/SessionCookieConfig.java

public SessionCookieConfig setComment(final String comment) {
this.comment = comment;
setAttribute(COOKIE_COMMENT_ATTR, comment);
Copy link
Contributor Author

@baranowb baranowb Sep 4, 2025

Choose a reason for hiding this comment

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

setAttribute(COOKIE_SAME_SITE_ATTR, String.valueOf(sameSite), false); - needs flag.

@baranowb baranowb mentioned this pull request Sep 5, 2025
Open
@baranowb
Copy link
Contributor Author

baranowb commented Sep 5, 2025

Closing, this PR is sort of obsolete and has flaw in setAttribute handling other than sameSite. Once Upstream is accepted it should be used as template for backport.

@baranowb baranowb closed this Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ropalka ropalka Awaiting requested review from ropalka

@fl4via fl4via Awaiting requested review from fl4via

Assignees

No one assigned

Labels

bug fix Contains bug fix(es) enhancement Enhances existing behaviour or code new feature / API change New feature to be introduced or a change to the API (non suitable to minor releases) on hold PR awaits non CI/Review holdover under verification Currently being verified (running tests, reviewing) before posting a review to contributor waiting CI check Ready to be merged but waiting for CI check waiting peer review PRs that edit core classes might require an extra review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@baranowb