-
Notifications
You must be signed in to change notification settings - Fork 841
Relax URL match assertion in ResetPasswordIT test #3693
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
Conversation
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 PR relaxes the URL matching assertion in the notAutoLoginAfterResetPassword test to accommodate different environment configurations. However, the change introduces significant security and validation concerns.
Key Change:
- Modified URL assertion from
startsWith("https://example.redirect.com/?code=")tocontains("://example.redirect.com/?code=")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| webDriver.clickAndWait(By.xpath("//input[@value='Sign in']")); | ||
|
|
||
| assertThat(webDriver.getCurrentUrl()).startsWith("https://example.redirect.com/?code="); | ||
| assertThat(webDriver.getCurrentUrl()).contains("://example.redirect.com/?code="); |
Copilot
AI
Dec 5, 2025
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 change from startsWith("https://example.redirect.com/?code=") to contains("://example.redirect.com/?code=") weakens the assertion in a potentially problematic way:
-
Protocol validation is removed: The original assertion enforced HTTPS, which is critical for OAuth authorization codes to prevent man-in-the-middle attacks. The new assertion allows HTTP or any other protocol.
-
URL structure validation is weakened: The original
startsWithensures the redirect URL begins with the expected pattern. The newcontainscould match URLs likehttp://malicious.com?next=http://example.redirect.com/?code=where the legitimate redirect is embedded elsewhere in the URL.
If the goal is to handle different environments (http in dev/test, https in production), consider a more specific assertion like:
assertThat(webDriver.getCurrentUrl()).matches("^https?://example\\.redirect\\.com/\\?code=.*");This maintains validation that the URL starts with the expected host while allowing both protocols.
| assertThat(webDriver.getCurrentUrl()).contains("://example.redirect.com/?code="); | |
| assertThat(webDriver.getCurrentUrl()).matches("^https?://example\\.redirect\\.com/\\?code=.*"); |
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.
I like this
strehle
left a comment
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.
I would use copilt suggestions but you can decide
f4ca91b to
7173557
Compare
relax url matching for differences in different environments