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.

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

Fix #9334 Cookie Compliance #9402

Merged
merged 3 commits into from Feb 21, 2023
Merged

Fix #9334 Cookie Compliance #9402

merged 3 commits into from Feb 21, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Feb 21, 2023

Fix incorrect change to RFC6265 to not support dollars in cookie names.

@gregw
Fix #9334 Cookie Compliance
298a881 
Fix incorrect change to RFC6265 to not support dollars in cookie names.

Signed-off-by: gregw <gregw@webtide.com>
@gregw gregw requested a review from sbordet February 21, 2023 05:46
@gregw gregw mentioned this pull request Feb 21, 2023
Merged
@gregw
Fix #9334 Cookie Compliance
78c9074 
Fix incorrect change to RFC6265 to not support dollars in cookie names.
Included updates and tests from #9399

Signed-off-by: gregw <gregw@webtide.com>
@gregw
Fix #9334 Cookie Compliance
79bba38 
Fix incorrect change to RFC6265 to not support dollars in cookie names.
Included updates and tests from #9399

Signed-off-by: gregw <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Feb 21, 2023

@sbordet @lorban We were too tough on RFC2965 with our new RFC6265 parser. An attribute from an old cookie is a valid cookie name in RFC6265. So for example: name=value; $Domain=1 is two cookies, one called "name" and the other called "$Domain". We don't see these cookies in Servlet 5.0, only because the Cookie class cannot carry them. In Servlet 6.0 it can.
So in this PR, the low level tests all look for multiple cookies in that situation, but in the high level test (ServerHttpCookieTest), the dollar cookies are ignored (but test will need to be updated for jetty-11 & 12).

Best way to see how this now works is to look at the test case: RFC6265CookieParserTest#testRFC2965Single and look at how each compliance mode is tested.

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 21, 2023
View reviewed changes
jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java
cookie.setPath(path);
if (comment != null)
cookie.setComment(comment);
response.addCookie(cookie);

Check warning

Code scanning / CodeQL

Failure to use secure cookies

Cookie is added to response without the 'secure' flag being set.
@gregw gregw merged commit 4d14641 into jetty-10.0.x Feb 21, 2023
@gregw gregw deleted the jetty-10-9334-RFC6265-Cookie-attributes branch February 21, 2023 10:32
gregw added a commit that referenced this pull request Feb 21, 2023
@gregw
updated from 12 with #9402
2eabeb2 
Signed-off-by: gregw <gregw@webtide.com>
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Feb 23, 2023
Merge remote-tracking branch 'upstream/jetty-12.0.x' into jetty-12.0.…
d260492 
…x-old-docs-remove-logging-sections

* upstream/jetty-12.0.x:
  Issue jetty#9403 sendError(404) not setError(404) for DefaultServlet (jetty#9404)
  Fix bad merges from jetty-11
  revert upgrade from 3aaa65c
  Fix osgi dependencies for update to org.eclipse.osgi.services.
  Add ignore .checkstyle
  fix style
  Fixed Merge remote-tracking branch 'origin/jetty-11.0.x' into jetty-12.0.x
  fixing merge
  Fix jetty#9334 Cookie Compliance (jetty#9402)
  Bump maven-deploy-plugin from 3.0.0 to 3.1.0
  Bump asciidoctorj-diagram from 2.2.3 to 2.2.4
  Bump jakarta.servlet.jsp-api from 3.0.0 to 3.1.1
  Bump maven-invoker-plugin from 3.4.0 to 3.5.0
  Bump maven.surefire.plugin.version from 3.0.0-M8 to 3.0.0-M9
  Bump maven-javadoc-plugin from 3.4.1 to 3.5.0
  Bump tycho-p2-repository-plugin from 3.0.1 to 3.0.2
  Bump maven.version from 3.8.7 to 3.9.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

Assignees
No one assigned
Labels
High Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gregw @sbordet