Add methods to augment allowed headers and parameters in StrictHttpFi… #15048
Conversation
baezzys
force-pushed
the
enhance/strict-http-firewall-flexibility
branch
from
eddb7bb
to
85974bb
Compare
spring-projects-issues
added
the
status: waiting-for-triage
baezzys
force-pushed
the
enhance/strict-http-firewall-flexibility
branch
2 times, most recently
from
e9a14df
to
ca559ab
Compare
There was a problem hiding this comment.
Thanks, @baezzys! There is an issue with having both set and add that I didn't see earlier, which is that it assumes that the application wants to do Predicate#and and not Predicate#or. It doesn't give the developer much more power.
Also, I think that addAllowHeaderValues makes it seem like you are listing additional ways that headers would be allowed, which is the opposite of the implementation.
Instead, please let's introduce public static defaults for each, like so:
public static final Predicate<String> ALLOWED_HEADER_NAMES = ...;
public static final Predicate<String> ALLOWED_HEADER_VALUES = ...;
public static final Predicate<String> ALLOWED_PARAMETER_NAMES = ...;
public static final Predicate<String> ALLOWED_PARAMETER_VALUES = ...;So that an application can do:
firewall.setAllowedHeaderValues(ALLOWED_HEADER_VALUES.and((value) -> !value.contains("\t")))
jzheaux
added
in: web
|
Thank you for the review, @jzheaux. However, since the setters are not static, would it be acceptable to change them to public instead of public static? |
|
I think they should be |
baezzys
force-pushed
the
enhance/strict-http-firewall-flexibility
branch
from
ca559ab
to
2d9cf7f
Compare
|
Thank you for the feedback, @jzheaux I have updated the Please review the changes and let me know if any further adjustments are needed. |
baezzys
force-pushed
the
enhance/strict-http-firewall-flexibility
branch
2 times, most recently
from
fb826ed
to
e52b14d
Compare
- Introduced public static final Predicates for allowed header names, header values, parameter names, and parameter values, and changed their corresponding setters to static. - Removed addAllowedHeaderNames, addAllowedHeaderValues, addAllowedParameterNames, and addAllowedParameterValues methods.
baezzys
force-pushed
the
enhance/strict-http-firewall-flexibility
branch
from
e52b14d
to
6a022bc
Compare
| @@ -134,13 +135,13 @@ public class StrictHttpFirewall implements HttpFirewall { | |||
|
|
|||
| private static final Predicate<String> HEADER_VALUE_PREDICATE = (s) -> HEADER_VALUE_PATTERN.matcher(s).matches(); | |||
|
|
|||
| private Predicate<String> allowedHeaderNames = ASSIGNED_AND_NOT_ISO_CONTROL_PREDICATE; | |||
There was a problem hiding this comment.
Let's keep both. There is the public constant that folks can use to build expressions and then there is whatever in the internal setting. You'd want something like this:
private Predicate<String> allowedHeaderNames = ALLOWED_HEADER_NAMES;
public static final Predicate<String> ALLOWED_HEADER_NAMES = ASSIGNED_AND_NOT_ISO_CONTROL_PREDICATE;| @@ -435,9 +436,9 @@ public void setAllowUrlEncodedLineSeparator(boolean allowUrlEncodedLineSeparator | |||
| * @see Character#isISOControl(int) | |||
| * @see Character#isDefined(int) | |||
| */ | |||
| public void setAllowedHeaderNames(Predicate<String> allowedHeaderNames) { | |||
| public static void setAllowedHeaderNames(Predicate<String> allowedHeaderNames) { | |||
There was a problem hiding this comment.
Please keep the methods non-static. We only need the public constants to be static.
| Assert.notNull(allowedHeaderNames, "allowedHeaderNames cannot be null"); | ||
| this.allowedHeaderNames = allowedHeaderNames; |
There was a problem hiding this comment.
Please keep this line as it was.
| Assert.notNull(allowedHeaderValues, "allowedHeaderValues cannot be null"); | ||
| this.allowedHeaderValues = allowedHeaderValues; | ||
| ALLOWED_HEADER_VALUES = allowedHeaderValues; |
There was a problem hiding this comment.
Please keep this line as it was.
| Assert.notNull(allowedParameterNames, "allowedParameterNames cannot be null"); | ||
| this.allowedParameterNames = allowedParameterNames; |
There was a problem hiding this comment.
Please keep this line as it was
| throw new RequestRejectedException("The request was rejected because the header: \"" + name | ||
| + " \" has a value \"" + value + "\" that is not allowed."); | ||
| } | ||
| } | ||
|
|
||
| private void validateAllowedParameterName(String name) { | ||
| if (!StrictHttpFirewall.this.allowedParameterNames.test(name)) { | ||
| if (!StrictHttpFirewall.this.ALLOWED_PARAMETER_NAMES.test(name)) { |
There was a problem hiding this comment.
Please leave this line as it was.
| throw new RequestRejectedException( | ||
| "The request was rejected because the parameter name \"" + name + "\" is not allowed."); | ||
| } | ||
| } | ||
|
|
||
| private void validateAllowedParameterValue(String name, String value) { | ||
| if (!StrictHttpFirewall.this.allowedParameterValues.test(value)) { | ||
| if (!StrictHttpFirewall.this.ALLOWED_PARAMETER_VALUES.test(value)) { |
There was a problem hiding this comment.
Please leave this line as it was.
|
|
||
| @BeforeEach | ||
| public void initializeFirewall() { | ||
| StrictHttpFirewall.ALLOWED_HEADER_NAMES = (s) -> ASSIGNED_AND_NOT_ISO_CONTROL_PATTERN.matcher(s).matches(); |
There was a problem hiding this comment.
I think these will already be set by the object itself. They won't need to be reset if you follow the earlier suggestions.
| public void getFirewalledRequestGetHeaderWhenSetNotAllowedHeaderNamesThenException() { | ||
| this.firewall.setAllowedHeaderNames((name) -> !name.equals("bad name")); | ||
| this.firewall | ||
| .setAllowedHeaderNames(this.firewall.ALLOWED_HEADER_NAMES.and((name) -> !name.equals("worst name"))); |
There was a problem hiding this comment.
After the above changes, this will change to something closer to:
this.firewall.setAllowedHeaderNames(
StrictHttpFirewall.ALLOWED_HEADER_NAMES.and((name) -> !name.equals("worst name"));| this.request.addHeader("good name", "bad value"); | ||
| this.request.addHeader("best name", "worst value"); | ||
|
|
||
| this.firewall.setAllowedHeaderValues((value) -> !value.equals("bad value")); |
There was a problem hiding this comment.
Since the second call overrides the first, you can leave this line out and it won't change the result of the test.
Description
This pull request introduces new methods in the StrictHttpFirewall class that allow for the augmentation of the sets of allowable header names, header values, parameter names, and parameter values. The newly introduced methods (addAllowedHeaderNames, addAllowedHeaderValues, addAllowedParameterNames, and addAllowedParameterValues) ensure that users can add to the existing security settings without losing the benefits of the default protections.
This closes #13639