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

Add semicolonIsNormalChar and maxParams properties to the server config #11416

Open
wants to merge 4 commits into
base: 4.8.x
Choose a base branch
from

Conversation

glorrian
Copy link
Contributor

@glorrian glorrian commented Dec 6, 2024

fix #11411

@glorrian
Copy link
Contributor Author

glorrian commented Dec 9, 2024

glad to receive your review @yawkat @graemerocher @dstepanov @sdelamo

@@ -598,6 +606,39 @@ public void setNotFoundOnMissingBody(boolean notFoundOnMissingBody) {
this.notFoundOnMissingBody = notFoundOnMissingBody;
}

/**
* @return {@code true} if ';' is normal, {@code false} otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Please describe it a bit more, it's not clear that this is relating to query parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

edited, describe it really clear now i guess

@yawkat yawkat requested a review from dstepanov December 10, 2024 10:17
@yawkat yawkat added the type: improvement A minor improvement to an existing feature label Dec 10, 2024
@yawkat yawkat added this to the 4.8.0 milestone Dec 10, 2024
* @return {@code true} if the semicolon is a normal character, {@code false} otherwise.
* @since 4.8
*/
public boolean isSemicolonIsNormalChar() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this belongs in HttpServerConfiguration since I am unsure the other server implementations (jetty, tomcat etc.) allow this kind of configuration. For the moment it should be moved to NettyHttpServerConfiguration. @yawkat WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

Copy link
Member

@yawkat yawkat Dec 10, 2024

Choose a reason for hiding this comment

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

Hm actually since this also affects our non-netty copy of QueryStringDecoder, I guess it's fine here

Copy link
Contributor Author

@glorrian glorrian Dec 10, 2024

Choose a reason for hiding this comment

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

@graemerocher AbstractNettyHttpRequest uses HttpServerConfig interface for a config field, that's why i wrote it there, but it seems strange to me too, i think it should use NettyHttpServerConfig interface due to strong AbstractNettyHttpRequest bind to Netty

/**
* @return true if the semicolon handle as a normal character, false otherwise.
*/
boolean isSemicolonIsNormalChar();
Copy link
Contributor

Choose a reason for hiding this comment

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

to avoid a breaking change this should be a default method that returns false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

*/
public void setSemicolonIsNormalChar(boolean semicolonIsNormalChar) {
this.semicolonIsNormalChar = semicolonIsNormalChar;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any changes to DefaultFormUrlEncodedDecoder that would respect this configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@glorrian
Copy link
Contributor Author

@graemerocher ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

Add config option for QueryStringDecoder semicolonIsNormalChar
3 participants