-
Notifications
You must be signed in to change notification settings - Fork 255
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
fix(clerk-js): Directory traversal relative URL detection #4483
Conversation
🦋 Changeset detectedLatest commit: cb9f3a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
(allowedRedirectOrigins: Array<string | RegExp> | undefined) => (_url: string) => { | ||
if (!allowedRedirectOrigins) { | ||
return true; | ||
export const isAllowedRedirect = |
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.
This function now deals with full URLs, instead of potentially relative URLs as well. This makes our checks more consistent, and removes situations where we treat a relative URL as okay, make it absolute, and then it ends up being problematic.
@@ -84,18 +84,38 @@ describe('isValidUrl(url,base)', () => { | |||
}); | |||
}); | |||
|
|||
describe('isRelativeUrl(url,base)', () => { | |||
describe('isProblematicUrl(url)', () => { |
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.
Could potentially do with more test cases here, but with the shift to checking against full URLs we get more guarantees from the existing origin check.
9876c25
to
cb9f3a1
Compare
Description
Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change