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

Multiple identical Set-Cookie response lines produced #10797

Closed
joakime opened this issue Oct 26, 2023 · 1 comment · Fixed by #10803
Closed

Multiple identical Set-Cookie response lines produced #10797

joakime opened this issue Oct 26, 2023 · 1 comment · Fixed by #10803
Labels
Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc)

Comments

@joakime
Copy link
Contributor

joakime commented Oct 26, 2023

Jetty version(s)
Jetty 12

Jetty Environment
All: ee10, ee9, ee8

Java version/vendor (use: java -version)
OpenJDK 17

OS type/version
Linux / Ubuntu

Description
Multiple Set-Cookie response lines produced.

How to reproduce?

When testing for the bug reported in issue #10794 it was noticed that the response produces multiple Set-Cookie lines.
See below, testing with Jetty 12.0.2 with ee10-demos, using curl.

[tmp]$ curl -vvv http://localhost:8080/ee10-test?a=b
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /ee10-test?a=b HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.81.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 301 Moved Permanently
< Server: Jetty(12.0.2)
< Set-Cookie: visited=yes
< Expires: Thu, 01 Jan 1970 00:00:00 GMT
< Set-Cookie: visited=yes
< Set-Cookie: visited=yes
< Location: /ee10-test/;a=b
< Content-Length: 0
< 
* Connection #0 to host localhost left intact

This produced 3 identical Set-Cookie: visited=yes lines.

@joakime joakime added the Bug For general bugs on Jetty side label Oct 26, 2023
@joakime joakime added the Specification For all industry Specifications (IETF / Servlet / etc) label Oct 26, 2023
@gregw
Copy link
Contributor

gregw commented Oct 27, 2023

@joakime
The org.eclipse.jetty.server.Response class has two methods: static void addCookie(Response response, HttpCookie cookie) and static void replaceCookie(Response response, HttpCookie cookie). The javadoc for replaceCookie says: "Replaces (if already exists, otherwise adds) an HTTP cookie to the response."

We are apparently using addCookie instead of replaceCookie in many/most places.

Simple fix is to change all the calls.... but that will not avoid the mistake being made again in future. Thus I propose that we:

  1. rename replaceCookie to putCookie, as this matches the add/put semantics of response headers.
  2. use putCookie in almost every place.
  3. add a deprecated replaceCookie that calls putCookie.

or maybe we should just make addCookie and replaceCookie do the same thing?

gregw added a commit that referenced this issue Oct 27, 2023
Fix #10797 by make addCookie always replace existing SetCookie fields.
Deprecated replaceCookie method.
gregw added a commit that referenced this issue Oct 31, 2023
Addressed #10797 by make cleanup of addCookie and renaming replaceCookie to putCookie.
Ensured that Jetty code calls putCookie when appropriate.
gregw added a commit that referenced this issue Oct 31, 2023
Addressed #10797 by make cleanup of addCookie and renaming replaceCookie to putCookie.
Ensured that Jetty code calls putCookie when appropriate.
gregw added a commit that referenced this issue Nov 1, 2023
)

Addressed #10797 by make cleanup of addCookie and renaming replaceCookie to putCookie.
Ensured that Jetty code calls putCookie when appropriate.
@gregw gregw closed this as completed Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc)
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants