Skip to content

fix: limit OAuth redirects to local paths #14585

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

Merged
merged 9 commits into from
Sep 10, 2024
Merged

fix: limit OAuth redirects to local paths #14585

merged 9 commits into from
Sep 10, 2024

Conversation

sreya
Copy link
Collaborator

@sreya sreya commented Sep 6, 2024

  • This prevents a malicious user from crafting a redirect URL to a nefarious site under their control.

fixes https://github.com/coder/security/issues/57

- This prevents a malicious user from crafting a redirect
  URL to a nefarious site under their control.
Introduce a utility function `uriFromURL` to strip host information from
redirect URLs, ensuring redirects remain local. This mitigates potential
security risks by preventing redirects to external malicious sites.

Changes:
- Modify OAuth2 and OIDC callback logic to utilize `uriFromURL`.
- Enhance related test cases for preventing external redirects.

This work ensures secure handling of redirect URLs across the application.
- Removed an unnecessary blank line for code consistency.
- Improve handling of redirect logic in FakeIDP.
- Ensure existing CheckRedirect functions are respected.
- Enhance OIDC login test to handle redirects properly.
- Improve handling of redirect logic in FakeIDP by ensuring existing CheckRedirect functions are respected.
- Enhance related test cases for better testing of redirect behaviors.

Changes:
- Store claims if state is available before falling back to existing CheckRedirect.
- Ensure HTTP client transport is set to default in tests before assigning CheckRedirect.
Removing a leftover debug print statement to clean up the code.
@sreya sreya requested a review from Emyrk September 9, 2024 23:21
- Prevent redirecting the last step in the OIDC flow involving the state
  parameter, ensuring it remains part of the core OIDC process.
- This ensures secure and consistent handling of redirects in the OIDC
  login flow.
Update the OAuth redirect URL test to exclude fragment

Previously, the test included a fragment in the constructed URL.
Removing the fragment ensures consistency with the actual URL format
used in the application.
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really clean solution to prevent redirects outside our domain 👍

@sreya sreya merged commit 328e696 into main Sep 10, 2024
26 checks passed
@sreya sreya deleted the jon/fixredirect branch September 10, 2024 14:58
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants