Skip to content
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

APP-6932 better local redirect validation #390

Merged
merged 19 commits into from
Nov 21, 2024

Conversation

DTCurrie
Copy link
Member

@DTCurrie DTCurrie commented Nov 12, 2024

Updates our backto redirect logic to only allow fully-qualified whitelisted https URLs. This will require a change in app to no longer pass just the current pathname for backto and instead to pass the full current URL.

@DTCurrie DTCurrie added the safe to test Mark as safe to test label Nov 12, 2024
@DTCurrie DTCurrie self-assigned this Nov 12, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024
@ohEmily
Copy link
Member

ohEmily commented Nov 12, 2024

trying to get a grip on this.

[q1] could you remind me why we have both a backto as well as a redirect_uri?

[q2] why can't we go with his proposed fix (which Google also confirmed is the best fix for an "open redirect attack"), which is Only allow fully qualified URLs from a whitelist?

Seems like FusionAuth would allow us to lock down our list of redirect URIs, hence q1 above:
Screenshot 2024-11-12 at 5 18 50 PM

@DTCurrie
Copy link
Member Author

trying to get a grip on this.

[q1] could you remind me why we have both a backto as well as a redirect_uri?

[q2] why can't we go with his proposed fix (which Google also confirmed is the best fix for an "open redirect attack"), which is Only allow fully qualified URLs from a whitelist?

Seems like FusionAuth would allow us to lock down our list of redirect URIs, hence q1 above: Screenshot 2024-11-12 at 5 18 50 PM

The redirect_uri is the path we are redirecting the users to from fusion auth into the app, the backto parameter is something we set in order to redirect the user back to the correct route in app after they have logged in. The flow would go something like:

  1. Logged out user goes to /registry
  2. They click the login button, which redirects them to /login with the current route set as the backto parameter
  3. In /login we check backto to see if it is valid, if so we save it in the session data
  4. They log in, and they are sent back to the redirect_uri, in this case /callback.
  5. In /callback we check the session data for a backto, in this case that would be /registry so we redirect them there instead of /

backto is unrelated to the redirect_uri and something we handle internally. I don't think fusion auth has a handler for this, if it does it would probably be a larger code change right now to use that over the current sessions implementation.

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@DTCurrie DTCurrie removed the request for review from ohEmily November 14, 2024 14:47
@viambot viambot removed the safe to test Mark as safe to test label Nov 14, 2024
@DTCurrie
Copy link
Member Author

Updates our backto redirect logic to only allow fully-qualified whitelisted https URLs. This will require a change in app to no longer pass just the current pathname for backto and instead to pass the full current URL.

is there an app pr up yet with these changes and the goutils bump so we can test?

Not yet, I will set that up.

@DTCurrie
Copy link
Member Author

Potential missing cases:

  • CI/Test envs
  • PR temp envs

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 19, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 19, 2024
@DTCurrie
Copy link
Member Author

Updates our backto redirect logic to only allow fully-qualified whitelisted https URLs. This will require a change in app to no longer pass just the current pathname for backto and instead to pass the full current URL.

is there an app pr up yet with these changes and the goutils bump so we can test?

Not yet, I will set that up.

@jr22 Put up an app-side PR here: https://github.com/viamrobotics/app/pull/6918

Going to make another commit to try and handle CI and temp envs but that should just be additions to the whitelist ideally.

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 19, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 19, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 19, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 19, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 19, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 20, 2024
@DTCurrie DTCurrie merged commit 37789a9 into main Nov 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Mark as safe to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants