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

Adding properties #1

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Conversation

breakponchito
Copy link

I created the following PR to make a proposal to include two properties to enable the validations for the Issue eclipse-ee4j#2212 on top of the first PR added by @carryel eclipse-ee4j#2213

The proposal is to add two properties to enable the validation of the invalid characters on Header Name, Header Value or both.

I think this probable could be a good strategy to prevent any backward compatibility issue. By default both properties are set as false:

  • org.glassfish.grizzly.http.STRICT_HEADER_NAME_VALIDATION_RFC_9110
  • org.glassfish.grizzly.http.STRICT_HEADER_VALUE_VALIDATION_RFC_9110

@carryel please check

@carryel
Copy link
Owner

carryel commented Nov 15, 2024

@breakponchito Thanks for your PR! It seems good to activate the validation function with properties.

Here are some review items below.

  1. There is a part where the option is missing in header name validation. The parseHeaderName function consists of two pairs (byte[]-based and Buffer-based). Since the current PR only considers byte[], it seems that it should be applied to Buffer as well.

There are some concerns about header value validation.

  1. The current PR only considers CR, LF, and NUL, but it seems that additional consideration should be given to the format below in order to properly validate.
field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
field-vchar    = VCHAR / obs-text
VCHAR          = %x21-7E ; visible (printing) characters
obs-text       = %x80-FF
SP             = %x20
HTAB           = %x09 ; horizontal tab

See: 
https://datatracker.ietf.org/doc/html/rfc7230#section-3.2
https://datatracker.ietf.org/doc/html/rfc5234#appendix-B.1
  1. A safe and simple way is to copy the completed value and pattern match it at the time the header value is determined, but since it is a basic process performed for each request, there are concerns about performance and inefficiency.
    The best thing would be to have a character validation when parsing header values ​​like header name validation without a separate copy. However, header values ​​support multi-line, so it seems more complicated to apply.
    In https://github.com/eclipse-ee4j/grizzly/blob/master/modules/http/src/main/java/org/glassfish/grizzly/http/util/CookieHeaderParser.java,
    definitions and validation logics such as IS_TOKEN and IS_CONTROL specialized for header values maybe can be added, and there will be a way to utilize them.
    Or it seems that judging by the range of char like https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/headers/HttpHeaderValidationUtil.java#L146 can be a hint.

  2. parseHeaderValue also consists of two pairs (byte[] and Buffer) like parseHeaderName above, so header value validation should be applied together.

  3. As a minor thing, rather than calling Pattern.compile in local function when matching the pattern, it would be better to define it statically as follows:

private static final Pattern REGEX_RFC_9110_INVALID_CHARACTERS_PATTERN = Pattern.compile(REGEX_RFC_9110_INVALID_CHARACTERS);

@carryel carryel merged commit be0e4a1 into carryel:master Nov 18, 2024
@carryel
Copy link
Owner

carryel commented Nov 18, 2024

I think I can make some additional edits based on your PR regarding my comment mentioned above. @breakponchito

carryel added a commit that referenced this pull request Nov 18, 2024
+ Added the validation of HTTP header values
+ Name and value's validation are provided as options. (Improved based on #1 @breakponchito)
@carryel
Copy link
Owner

carryel commented Nov 18, 2024

@breakponchito Thanks. I've applied it with some modifications based on your PR. Feel free to check it out later! 6bb7eab

breakponchito pushed a commit to breakponchito/patched-src-grizzly that referenced this pull request Nov 19, 2024
+ Added the validation of HTTP header values
+ Name and value's validation are provided as options. (Improved based on carryel#1 @breakponchito)
Pandrex247 pushed a commit to Pandrex247/patched-src-grizzly that referenced this pull request Dec 9, 2024
+ Added the validation of HTTP header values
+ Name and value's validation are provided as options. (Improved based on carryel#1 @breakponchito)
breakponchito pushed a commit to breakponchito/patched-src-grizzly that referenced this pull request Dec 11, 2024
+ Added the validation of HTTP header values
+ Name and value's validation are provided as options. (Improved based on carryel#1 @breakponchito)
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.

2 participants