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 methods to augment allowed headers and parameters in StrictHttpFi… #15048

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

baezzys
Copy link
Contributor

@baezzys baezzys commented May 12, 2024

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

@baezzys baezzys force-pushed the enhance/strict-http-firewall-flexibility branch from eddb7bb to 85974bb Compare May 12, 2024 15:17
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 12, 2024
@baezzys baezzys force-pushed the enhance/strict-http-firewall-flexibility branch 2 times, most recently from e9a14df to ca559ab Compare May 15, 2024 07:55
Copy link
Contributor

@jzheaux jzheaux left a 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")))

@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 21, 2024
@baezzys
Copy link
Contributor Author

baezzys commented May 26, 2024

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?

@jzheaux
Copy link
Contributor

jzheaux commented May 31, 2024

I think they should be static so that you don't need an instance of the firewall to refer to them.

@baezzys baezzys force-pushed the enhance/strict-http-firewall-flexibility branch from ca559ab to 2d9cf7f Compare June 1, 2024 03:20
@baezzys
Copy link
Contributor Author

baezzys commented Jun 1, 2024

Thank you for the feedback, @jzheaux

I have updated the StrictHttpFirewall class to change the private Predicates for allowed header names, header values, parameter names, and parameter values to public static. Additionally, I have changed the corresponding setters.

Please review the changes and let me know if any further adjustments are needed.

@baezzys baezzys force-pushed the enhance/strict-http-firewall-flexibility branch 2 times, most recently from fb826ed to e52b14d Compare June 1, 2024 04:49
- 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 baezzys force-pushed the enhance/strict-http-firewall-flexibility branch from e52b14d to 6a022bc Compare June 1, 2024 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StrictHttpFirewall#setAllowedHeaderNames should augment with existing Predicate
3 participants