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

chore: improve sgid state validation #386

Merged
merged 7 commits into from
Jan 6, 2025

Conversation

zhongliang02
Copy link
Contributor

@zhongliang02 zhongliang02 commented Jan 3, 2025

Context

The SGID OIDC login flow, while currently safe, is prone to open redirect vulnerabilities related to the landingUrl parameter. This URL is passed into the state parameter and is derived from query params. There is insufficient validation in between the steps of the login flow, thus is quite prone to open redirect vulnerabilities.

This has occurred in kampung-spirit recently and could be partially exploitable in armoury.

Approach

To mitigate this, we propose the following changes:

  1. Client-side validation: Implement validation of the landingUrl on the client side and fallback to a safe URL.

  2. Server-side validation: Add server-side validation in both legs of OIDC and always fallback to a safe URL.

  3. Error fallback component: Add validation for the landingUrl in the error fallback component.

Risks

  • Disruption to login flow if validation logic is too strict.
  • callbackUrlSchema adjusted to fail gracefully and default to HOME.
    • This seems like the intended behavior, but may affect codepaths that relied on callbackUrlSchema throwing errors.

Testing

Happy SGID flow

https://starter-5jcverdf5-ogp-tooling.vercel.app/sign-in/sgid/callback?code=YOUR_CODE&state=%7B%22landingUrl%22%3A%22https%3A%2F%2Fstarter-5jcverdf5-ogp-tooling.vercel.app%2Fhome%22%7D

Happy SGID flow with custom path `/404'

Happy email flow

  • Tested email login flow, no issues

Testing client side validation

With bad callbackUrl https://starter-5jcverdf5-ogp-tooling.vercel.app/sign-in?callbackUrl=https://evil.com, the request to sgid login state.landingUrl fallbacks to https://starter-5jcverdf5-ogp-tooling.vercel.app/home

Testing server side validation on leg 1

With bad state.landingUrl submitted to /api/trpc/auth.sgid.login, response redirectUrl has query state {"landingUrl":"https://starter-5jcverdf5-ogp-tooling.vercel.app/home"}

Testing server side validation on leg 2

With bad state.landingUrl submitted to /api/trpc/auth.sgid.callback, response json

[{"result":{"data":{"json":{
  "selectProfileStep": true,
  "redirectUrl": "/sign-in/select-profile?callbackUrl=https%3A%2F%2Fstarter-5jcverdf5-ogp-tooling.vercel.app%2Fhome"
}}}}]

Testing error fallback component

redirectUrl in SgidErrorFallback is sanitized.

Copy link

vercel bot commented Jan 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
starter-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 3, 2025 10:09am

@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Jan 3, 2025

Datadog Report

Branch report: 01-03-chore_improve_sgid_state_validation
Commit report: a774b01
Test service: starter-kit

✅ 0 Failed, 11 Passed, 0 Skipped, 10.4s Total Time
🔻 Test Sessions change in coverage: 1 decreased (-0.09%)

🔻 Code Coverage Decreases vs Default Branch (1)

  • vitest run --coverage 16.34% (-0.09%) - Details

@zhongliang02 zhongliang02 requested a review from karrui January 3, 2025 07:03
Copy link
Collaborator

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm

@karrui karrui merged commit 77e7e86 into main Jan 6, 2025
14 checks passed
@karrui karrui deleted the 01-03-chore_improve_sgid_state_validation branch January 6, 2025 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants