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

Fix/jetty 9.4.x cookie cutter legacy #9352

Merged
merged 6 commits into from Feb 15, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Feb 14, 2023

Less lenient RFC6265 parsing, but with a RFC6265_LEGACY mode for those that want it.

lorban and others added 4 commits February 10, 2023 14:19
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@gregw gregw requested review from lorban and sbordet February 14, 2023 23:45
sbordet
sbordet previously approved these changes Feb 15, 2023
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Please add the suggested fix, otherwise LGTM.

* ; US-ASCII characters excluding CTLs,
* ; whitespace DQUOTE, comma, semicolon,
* ; and backslash
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the comment because it's not what the code does (the code does not check for , " and \.
Alternatively, can this block be merged with the block below which is identical and has a clarifying comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the comment.
It is not identical to the code below. This is the original code and the code below is updated by @lorban.

lorban
lorban previously approved these changes Feb 15, 2023
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

I would clean up the commented-out bits of the test, otherwise LGTM.

new Param("A= 1; B=2; C=3", "A=1", "B=2", "C=3"),
new Param("A=\"1; B=2\"; C=3", "C=3"),
new Param("A=\"1; B=2; C=3"),
new Param("A=\"1 B=2\"; C=3", "A=1 B=2", "C=3"), // Why should A be rejected? Shouldn't it be A=<1 B=2>?
Copy link
Contributor

@lorban lorban Feb 15, 2023

Choose a reason for hiding this comment

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

This comment either is outdated and should go away, or space characters should be rejected and there's a bug left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to keep us much legacy behaviour as possible for these lenient cases, so I don't want to change the code. I'll remove the comment.

new Param("A=1\"; B=2; C=3", "B=2", "C=3"),
new Param("A=1\"1; B=2; C=3", "B=2", "C=3"),
/* TODO Use Legacy mode for these
new Param("A=\" 1\"; B=2; C=3", "A=1", "B=2", "C=3"), // Why should the whitespaces be trimmed? They were not in the prev impl.
Copy link
Contributor

Choose a reason for hiding this comment

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

These assertions should be adapted. In these four cases, A should be ignored but B and C are still valid, aren't they?

@gregw gregw dismissed stale reviews from lorban and sbordet via 13ba312 February 15, 2023 11:58
@gregw gregw requested review from sbordet and lorban February 15, 2023 11:58
@gregw gregw merged commit 1be1401 into jetty-9.4.x Feb 15, 2023
2 checks passed
@gregw gregw deleted the fix/jetty-9.4.x-cookie-cutter-legacy branch February 15, 2023 21:24
srowen pushed a commit to apache/spark that referenced this pull request Mar 1, 2023
### What changes were proposed in this pull request?
This pr aims upgrade jetty from 9.4.50.v20221201 to 9.4.51.v20230217.

### Why are the changes needed?
The main change of this version includes:

- jetty/jetty.project#9352
- jetty/jetty.project#9345

The release notes as follows:

- https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.51.v20230217

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass Github Actions

Closes #40214 from LuciferYang/jetty-9451.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
@atennapel
Copy link

atennapel commented May 1, 2023

This did not update the getSetCookie in HttpCookie. It will cause a IllegalStateException if RFC6265_LEGACY is used. Unless I am mistaken... Created issue here: #9717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants