-
Notifications
You must be signed in to change notification settings - Fork 395
Improve redirect uri validation #2883
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: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2883 +/- ##
============================================
+ Coverage 57.75% 57.80% +0.05%
+ Complexity 9001 8986 -15
============================================
Files 668 668
Lines 50143 50146 +3
Branches 10863 10863
============================================
+ Hits 28959 28987 +28
+ Misses 17155 17124 -31
- Partials 4029 4035 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 improves the OAuth2 callback URI validation logic by enhancing regular expression handling and adding more robust validation checks. The changes aim to prevent unintended matches while preserving intentional wildcard patterns.
Key changes:
- Added explicit null check for regexp parameter before attempting regex matching
- Implemented selective dot escaping that only escapes dots followed by alphanumeric characters
- Simplified the validation flow by separating string comparison from regex matching
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Escape dots only when followed by a letter/digit (so .com, .org, etc. get escaped), | ||
but don't touch .* or .+ or .{n} . | ||
*/ | ||
regexp = regexp.replaceAll("(?<!\\\\)\\.(?=[A-Za-z0-9])", "\\\\."); |
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.
The regex pattern modification could introduce security vulnerabilities. Modifying user-provided regex patterns at runtime can lead to unexpected behavior or bypass security checks. Consider validating the original regex pattern instead of modifying it, or use a whitelist of allowed patterns.
Escape dots only when followed by a letter/digit (so .com, .org, etc. get escaped), | |
but don't touch .* or .+ or .{n} . | |
*/ | |
regexp = regexp.replaceAll("(?<!\\\\)\\.(?=[A-Za-z0-9])", "\\\\."); | |
Do not modify user-provided regex patterns at runtime. | |
Use the registered regex as provided. | |
*/ | |
// regexp = regexp.replaceAll("(?<!\\\\)\\.(?=[A-Za-z0-9])", "\\\\."); |
Copilot uses AI. Check for mistakes.
Escape dots only when followed by a letter/digit (so .com, .org, etc. get escaped), | ||
but don't touch .* or .+ or .{n} . | ||
*/ | ||
regexp = regexp.replaceAll("(?<!\\\\)\\.(?=[A-Za-z0-9])", "\\\\."); |
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.
The complex regex pattern with multiple escaped backslashes is difficult to read and maintain. Consider extracting this pattern to a named constant with clear documentation explaining its purpose and behavior.
Copilot uses AI. Check for mistakes.
but don't touch .* or .+ or .{n} . | ||
*/ | ||
regexp = regexp.replaceAll("(?<!\\\\)\\.(?=[A-Za-z0-9])", "\\\\."); | ||
return callbackURI.matches(regexp); |
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.
The matches() method is vulnerable to ReDoS (Regular Expression Denial of Service) attacks with malicious regex patterns. Consider adding timeout limits or using a safer regex matching approach to prevent potential DoS attacks.
Copilot uses AI. Check for mistakes.
Proposed changes in this pull request
This pull request updates the callback URI validation logic in the OAuth2 authorization request validator. The main change is an improvement to how regular expressions are handled to ensure more accurate matching of callback URIs.
Callback URI validation improvements:
.com
,.org
), which prevents unintended matches and avoids interfering with wildcard patterns like.*
or.+
or.{n}
.