-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
There was a problem hiding this 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 | ||
*/ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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>? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
### 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 <[email protected]> Signed-off-by: Sean Owen <[email protected]>
This did not update the |
Less lenient RFC6265 parsing, but with a RFC6265_LEGACY mode for those that want it.