-
Notifications
You must be signed in to change notification settings - Fork 0
Add support checking same security matchers human reviewers #22
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces a mechanism to detect and prevent the configuration of multiple security filter chains with identical request matchers. It achieves this by adding a validator to the Sequence diagram for FilterChainProxy initialization with validatorsequenceDiagram
participant WebSecurity
participant FilterChainProxy
participant DefaultFilterChainValidator
WebSecurity->>FilterChainProxy: setFilterChainValidator(new DefaultFilterChainValidator())
FilterChainProxy->>DefaultFilterChainValidator: new DefaultFilterChainValidator()
WebSecurity->>FilterChainProxy: afterPropertiesSet()
FilterChainProxy->>DefaultFilterChainValidator: validate(filterChains)
loop For each filterChain in filterChains
DefaultFilterChainValidator->>filterChain: getRequestMatcher()
end
alt Duplicate matchers found
DefaultFilterChainValidator-->>WebSecurity: Throw IllegalArgumentException
else No duplicate matchers
DefaultFilterChainValidator-->>FilterChainProxy: (Validation successful)
end
Updated class diagram for OrRequestMatcherclassDiagram
class OrRequestMatcher {
-List~RequestMatcher~ requestMatchers
+OrRequestMatcher(requestMatchers: List~RequestMatcher~)
+matches(request: HttpServletRequest): boolean
+matcher(request: HttpServletRequest): MatchResult
+equals(o: Object): boolean
+hashCode(): int
+toString(): String
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @GuusArts - I've reviewed your changes - here's some feedback:
Overall Comments:
- The exception message in
DefaultFilterChainValidator
could be improved to suggest usingrequestMatchers
instead ofsecurityMatcher
when defining multiple security filter chains. - Consider adding an equals and hashCode implementation to
AnyRequestMatcher
.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
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.
Pull Request Overview
This pull request introduces validation to prevent the configuration of multiple security filter chains with identical request matchers. Key changes include adding equals and hashCode methods to the OrRequestMatcher, enhancing the filter chain validator to check for duplicate matchers, and adding a corresponding negative test case.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
web/src/main/java/org/springframework/security/web/util/matcher/OrRequestMatcher.java | Added equals and hashCode methods for proper matcher comparison |
config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java | Added test to verify that duplicate security matchers throw a BeanCreationException |
config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java | Modified duplicate matcher check and adjusted error messaging for clarity |
config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java | Integrated the new filter chain validator into the filter chain proxy |
+ " matcher " + chain.getRequestMatcher() + ". If you are using multiple <http> namespace " | ||
+ "elements, you must use a 'pattern' attribute to define the request patterns to which they apply."); | ||
if (test instanceof DefaultSecurityFilterChain securityFilterChain) { | ||
if (chain.getRequestMatcher().equals(securityFilterChain.getRequestMatcher())) { |
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.
[nitpick] Consider refactoring the duplicate matcher check into a separate helper method to improve readability and reduce nested logic.
if (chain.getRequestMatcher().equals(securityFilterChain.getRequestMatcher())) { | |
if (hasDuplicateMatcher(chain, securityFilterChain)) { |
Copilot uses AI. Check for mistakes.
|
Closes spring-projectsgh-15982
Summary by Sourcery
Add validation to prevent duplicate security matchers in Spring Security configuration
New Features:
Enhancements:
Tests: