-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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 methods to augment allowed headers and parameters in StrictHttpFi… #15048
base: main
Are you sure you want to change the base?
Add methods to augment allowed headers and parameters in StrictHttpFi… #15048
Conversation
eddb7bb
to
85974bb
Compare
e9a14df
to
ca559ab
Compare
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.
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")))
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 |
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. |
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.
e52b14d
to
6a022bc
Compare
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