Initial commit 16186 security matchers #11
Conversation
Reviewer's Guide by SourceryThis pull request adds support for checking same security matchers. It prevents the configuration of multiple security matchers with the same pattern, throwing an exception if duplicates are found. It also overrides the equals and hashCode methods in OrRequestMatcher to properly compare and hash OrRequestMatcher instances based on their constituent request matchers. 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
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨No code suggestions found for the PR. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @GuusArts - I've reviewed your changes - here's some feedback:
Overall Comments:
- The exception message in
DefaultFilterChainValidatorcould be improved to suggest usingrequestMatchersinstead ofsecurityMatcherwhen defining multipleSecurityFilterChainbeans. - 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.
|
GuusArts
changed the title
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
🎉 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) |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
There was a problem hiding this comment.
Pull Request Overview
Enhances Spring Security to prevent duplicate HTTP security matchers by introducing a validator and proper equals/hashCode on composite matchers.
- Added
equalsandhashCodeimplementations toOrRequestMatcherfor correct matcher comparison. - Introduced
DefaultFilterChainValidatorand wired it intoWebSecurityto detect duplicate filter chains. - Added a test case in
WebSecurityTeststo verify that configuring two chains with the same matcher throws an exception.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/.../matcher/OrRequestMatcher.java | Bumped copyright year; added equals/hashCode for correct matcher deduplication. |
| config/.../WebSecurityTests.java | Bumped copyright year; imported BeanCreationException; added test for duplicate matcher failure. |
| config/.../DefaultFilterChainValidator.java | Refactored path-order check; implemented checkForDuplicateMatchers to throw on identical matchers. |
| config/.../WebSecurity.java | Wired in the new DefaultFilterChainValidator on the FilterChainProxy. |
Comments suppressed due to low confidence (3)
web/src/main/java/org/springframework/security/web/util/matcher/OrRequestMatcher.java:85
- There are no direct unit tests for the new
equalsandhashCodemethods. Consider adding tests to verify equality and hash code consistency for various matcher combinations.
@Override public boolean equals(Object o) {
config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java:184
- [nitpick] The inner config class name
MultipleSecurityMatchersConfigis generic; renaming toDuplicateSecurityMatchersConfigwould more clearly reflect its purpose in the test.
static class MultipleSecurityMatchersConfig {
config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java:87
- This method mutates the passed-in
chainslist viaremove(0). It may be clearer and safer to iterate by index or operate on a copy to avoid side-effects on the original collection.
private void checkForDuplicateMatchers(List<SecurityFilterChain> chains) {
| @@ -81,6 +82,23 @@ public MatchResult matcher(HttpServletRequest request) { | |||
| return MatchResult.notMatch(); | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals(Object o) { | |||
There was a problem hiding this comment.
The current equals compares the List<RequestMatcher> in order, but OR semantics are commutative. Consider normalizing or using a set so [A, B] and [B, A] are treated as equal.
Copilot uses AI. Check for mistakes.
|
Closes spring-projectsgh-15982
Summary by Sourcery
Add validation for duplicate security matchers in Spring Security configuration
New Features:
Enhancements:
Tests: