- Notifications
You must be signed in to change notification settings - Fork927
fix(site): sanitize login redirect#15208
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| // `<Navigate>` react would handle the redirect itself and never request the | ||
| // page from the backend. | ||
| if (isSignedIn && isApiRouteRedirect) { | ||
| const sanitizedUrl = new URL(redirectTo, window.location.origin); |
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 does not work for subdomain apps though right? If they are on another domain?
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.
They're not on another domain. The redirect from a workspace proxy subdomain app is to/api/v2/applications/auth-redirect. The redirect sanitization happens there.
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'll trust you 👍
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 could be wrong but that's what I always noticed in my testing.
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.
approving but remove your dep
package.json Outdated
| @@ -9,6 +9,7 @@ | |||
| "storybook": "pnpm run -C site/ storybook" | |||
| }, | |||
| "devDependencies": { | |||
| "@biomejs/biome": "1.9.4", | |||
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.
no
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.
biome extension doesn't work in vscode unless you have dep in root dir 😢
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.
Mainly had a question about whether another file needs to be updated
| } | ||
| if (isSignedIn && !isApiRouteRedirect) { | ||
| return ( | ||
| <Navigate to={redirectUrl ? redirectUrl.pathname : redirectTo} replace /> |
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.
So I poked around a bit more of the code, and I'm just making sure: there's a second file that calls theretrieveRedirect function (LoginPageView.tsx). Do we need to update anything around that?
Because right now, the control flow is set up so that the component:
- Gets the
redirectTovalue from the search param'sredirectparam - Passes it to the
SignInForm - Passes that to the
OAuthSignInForm - As long as GitHub is enabled as an auth method, a button shows up with an href that uses the raw
redirectToas a param
Not sure how the callback logic works, but would the updates to this file catch any issues from there?
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.
Yeah, good find. This is being sent directly to the backend which handles approving the redirect URLs, so it doesn't need any validation from the frontend.
Uh oh!
There was an error while loading.Please reload this page.
69c1d98 intomainUh oh!
There was an error while loading.Please reload this page.
No description provided.