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

ResponseCookie to allow leading dot in domain name again #23924

Closed
rubasace opened this issue Nov 4, 2019 · 6 comments
Closed

ResponseCookie to allow leading dot in domain name again #23924

rubasace opened this issue Nov 4, 2019 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Milestone

Comments

@rubasace
Copy link

rubasace commented Nov 4, 2019

ResponseCookie was changed and now applies Rfc6265Utils to validate attributes. As stated on issue #23776, we should be strict with output and lenient with input. Reality is that ReactorClientHttpResponse and JettyClientHttpResponse are using ResponseCookie to propagate cookies received. This scenario is cleary input and yet it's still failing when hitting endpoints behind Cloudflare, as they add the HttpOnly cookie with domain=.domain.com as stated here.

Regarding possible solutions, I think the builder itself could have some kind of flag to disable validation, allowing both ReactorClientHttpResponse and JettyClientHttpResponse use it when just propagating cookies from the cookie header.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 4, 2019
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 4, 2019
@rstoyanchev rstoyanchev self-assigned this Nov 4, 2019
@rstoyanchev rstoyanchev added this to the 5.2.2 milestone Nov 4, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 4, 2019

Fair point that for a ClientHttpResponse this represents input.

It makes sense to separate validation from the building of a cookie, so that a client response. Perhaps we can add a validate() method on ResponseCookie that can be invoked by server responses and could also be called by any code that's consuming response cookies from the client side.

@rstoyanchev rstoyanchev added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Nov 4, 2019
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels Nov 4, 2019
@rubasace
Copy link
Author

rubasace commented Nov 4, 2019

I think both make sense, depending on where it's more used, if client or server. What are your thoughts regarding an alternative skipValidation() on the builder so all existing behaviour needs no change apart from the failing clients?

@JorritSalverda
Copy link

After opening a support ticket with our CDN provider we received the following response:

I've reviewed the RFC and there may be some misinterpretation here:

If the first character of the attribute-value string is %x2E ("."):

  Let cookie-domain be the attribute-value without the leading %x2E
  (".") character.

Otherwise:

  Let cookie-domain be the entire attribute-value.

This is saying that if the value of the domain ( domain=) starts with a . then ignore it otherwise if it doesn't begin with a . use the entire value.

This indeed seems to point at your implementation being a bit too strict for pretty common input. Hope this can be made more liberal soon :)

@rstoyanchev
Copy link
Contributor

@JorritSalverda that quote is from section 5.2.3 which is for "User Agent Requirements". The server requirements in 4.1 however point to https://tools.ietf.org/html/rfc1034#section-3.5.

As far as "soon" is concerned, the ticket has a target milestone and a backport scheduled.

@rstoyanchev
Copy link
Contributor

@rubasace regarding skipValidation I think it is preferable to have an affirmative API when possible, but another in between alternative might be to add domain(String domain, boolean validate).

That said I am also considering just dropping the rejection of a leading ".". It seems to have caused quite a bit of disruption and clients are instructed to ignore it by the RFC.

@rubasace
Copy link
Author

rubasace commented Nov 5, 2019

@rstoyanchev definitely agree on the affirmative API.

Regarding the domain(String domain, boolean validate) vs dropping the rejection I'm more inclined on dropping the validation too, as it seems to be different interpretations of the RFC.

@rstoyanchev rstoyanchev changed the title ReactorClientHttpResponse fails with: Invalid first/last char in cookie domain .domain.com ResponseCookie to allow leading dot in domain name Nov 6, 2019
rstoyanchev added a commit that referenced this issue Nov 6, 2019
pull bot pushed a commit to scope-demo/spring-framework that referenced this issue Nov 6, 2019
@rstoyanchev rstoyanchev changed the title ResponseCookie to allow leading dot in domain name ResponseCookie to allow leading dot in domain name again Nov 14, 2019
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) status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

4 participants