Feature/strict crlf for chunked transfer coding - #2220
Conversation
…hunked Transfer Coding" (eclipse-ee4j#2217) + When the org.glassfish.grizzly.http.STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112 option is enabled, only CRLF is allowed as the chunk-size line terminator in Chunked Transfer Coding. + Added testcase depending on whether option is present or not
|
Maven version should be upgraged in Github Actions env. |
+ Added a step for installing maven v3.9.9 by referring to https://github.com/eclipse-ee4j/glassfish/blob/master/.github/workflows/build-windows.yml.
|
@arjantijms @dmatej If possible, please review this as well. This is also a patch for security enhancement. |
| private static final int[] DEC = HexUtils.getDecBytes(); | ||
|
|
||
| public static final String STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112 = "org.glassfish.grizzly.http.STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112"; | ||
| private static final boolean isStrictChunkedTransferCodingLineTerminatorSet = Boolean.parseBoolean(System.getProperty(STRICT_CHUNKED_TRANSFER_CODING_LINE_TERMINATOR_RFC_9112)); |
There was a problem hiding this comment.
Would you please move this initialization from system property to the constructor and change the static field to instance field?
With that, it's more deterministic when the system property is read and the system property can be reliably set from code before creating the instance, not only using a JVM option.
|
|
||
| private final int maxHeadersSize; | ||
|
|
||
| public ChunkedTransferEncoding(final int maxHeadersSize) { |
There was a problem hiding this comment.
If you can, would you create an overloaded constructor that accepts Properties and attempts to read the config from there before it reads the config from System properties?
And to complement it, would you also create overloaded constructors with a Properties argument in HttpServerFilter, HttpCodecFilter, and HttpClientFilter that pass it to ChunkedTransferEncoding?
If not, I'll try to do that in another PR, but it would really help. With this, it would be then possible to configure this option in constructor, individually for each server config, rather than using a system property for all listeners. Later we would integrate it in GlassFish to allow setting this property via standard GlassFish config for each listenerrather than via a global system property.
There was a problem hiding this comment.
I understand. I'll look into it later.
There was a problem hiding this comment.
@OndroMih I applied your suggestion.
While modifying, I thought a little about the priority of Properties and system properties and how values are applied. The current implementation is to apply only one of the two, and to give priority to the Properties setting.
PS) Another way is to merge or override the settings.
…hunked Transfer Coding" (eclipse-ee4j#2217) + Constructors with a Properties argument are supported. Therefore, the functionality is available in each listener using separate Properties. The relationship between Properties and a global system property is mutually exclusive, and Properties has priority.
3 participants
#2218 changed to be based on the main branch.
This is a PR for Issue #2217 based on main branch.