Skip to content

Conversation

@phuongnq
Copy link

Description of the Issue

The HttpOnly and Secure flags for a CSRF cookie should only be set if their respective properties are explicitly true.

This is because HttpOnly and Secure are boolean flags, and even assigning them an empty string can cause these flags to be applied. An example of this behavior can be observed in Tomcat 11. According to the Tomcat 11 changelog:

Refactor the internal representation of the HttpOnly and Secure attributes to use the empty string as the value for consistency with the recent changes to Set-Cookie header generation.

In Tomcat 11, this change results in the HttpOnly and Secure flags being added to cookies even if Spring Security does not intend to set these properties. This creates unexpected behavior where these flags are applied by default, potentially breaking existing configurations or assumptions.

Proposed Solution

Modify the behavior of the CSRF cookie generation to only include the HttpOnly and Secure flags when their properties are explicitly set to true. This ensures compatibility with Tomcat 11 and other servlet containers following a similar approach.

@pivotal-cla
Copy link

@phuongnq Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@phuongnq Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 29, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Dec 9, 2024

Hi, @phuongnq, thanks for the PR, though I'm not sure I'm understanding yet what you are trying to achieve.

In all cases, Spring Security sets Secure and HttpOnly to non-empty values. Do you have a sample of where Spring Security's usage of Cookie#setSecure(false) and Cookie#setHttpOnly(false) are causing Tomcat 11 to misbehave?

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 9, 2024
@phuongnq
Copy link
Author

Hi @jzheaux

Thanks for taking the time to look at the PR.
Actually, Tomcat 11.0.2 has fixed the issue.
Please review the changelog here: https://tomcat.apache.org/tomcat-11.0-doc/changelog.html

Fix: 69478: Correct a regression introduced in 11.0.0-M19 that meant when calling setHttpOnly(boolean) or setSecure(boolean) for a cookie, the respective flags were set regardless of the value passed to the method. (market)

As this is not an issue with the spring-security, let me close the PR.

@phuongnq phuongnq closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: waiting-for-feedback We need additional information before we can continue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants