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

Add a way to set custom cookie parsers #34081

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

m4tt30c91
Copy link

Add a way to set custom cookie parsers to be compliant with rfc6265 section 4.1.1

java.net.HttpCookie::parse still follows rfc2965: https://docs.oracle.com/javase/8/docs/api/java/net/HttpCookie.html#parse-java.lang.String-
That has been obsoleted from rfc6265: https://datatracker.ietf.org/doc/html/rfc6265
That in section 4.1.1 states the following:

Each cookie begins with a name-value-pair, followed by zero or more attribute-value pairs.
Servers SHOULD NOT send Set-Cookie headers that fail to conform to the following grammar:

So the MUST constraint from rfc2965 has been relaxed to SHOULD NOT in rfc6265

I think that taking this into account, it may also be a good idea to collapse some features between the Jetty response connector and the JDK response connector, and provide a way to customize the cookie parser.

Add a way to set custom cookie parsers to be compliant with rfc6265
section 4.1.1
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 12, 2024
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Dec 12, 2024
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

We could consider a Parser contract in ResponseCookie to customize parsing in client responses, but first could you add a little more detail around what you want to customize?

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Feb 3, 2025
@m4tt30c91
Copy link
Author

Hi @rstoyanchev ,

Thanks for the feedback.
The idea here is having something to deal with non-custom cookie headers.
To give you an idea the RFC state this: https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.1
set-cookie-string = cookie-pair *( ";" SP cookie-av )

So things like expiration and similar are after the cookie-pair, but the section states that:

Servers SHOULD NOT send Set-Cookie headers that fail to conform to the following grammar:

So it is not a MUST NOT, it is a SHOULD NOT.

We have a case where an external provider send Set-Cookie headers with a different syntax, such as prefixing the cookie-pair with cookie-av (which is not "totally" against the current version of the RFC).

Unfortunately we have no control on the provider and the HttpCookie from java.net is relying on rfc2965 (which was in fact a MUST NOT), so it fails at parsing time.

It would be nice to have a way to set a custom cookie parser, such that the way the Set-Cookie header is managed is actually customizable.

In the PR I've proposed a way to abstract the feature, let me know if is it fine; I have to align the branch with the current upstream, I can do that if the proposal is fine.

Thanks,
Matteo.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 3, 2025
@rstoyanchev rstoyanchev self-assigned this Feb 3, 2025
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Feb 3, 2025
@rstoyanchev rstoyanchev added this to the 7.0.0-M2 milestone Feb 3, 2025
@rstoyanchev
Copy link
Contributor

I think we can do this, but I would move the contract to ResponseCookie.Parser and make the default implementation package private in the same package as the responses. In other words, it's just a callback to customize the parsing of a response cookie that's otherwise handled internally by default.

@m4tt30c91
Copy link
Author

Yeap, that is fine :)
Is it ok if I provide a commit with the change?

@rstoyanchev
Copy link
Contributor

Yes, feel to free to send updates.

@m4tt30c91
Copy link
Author

Ok thank you, I will update the PR asap :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants