[MNY-310] Dashboard: Improve login redirect path validation #8422
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughDecoded redirect path is validated by decoding the input, requiring the decoded path to start with "/", resolving it against the base URL "https://thirdweb.com", and returning true only if the resulting URL's hostname equals "thirdweb.com". Decode or URL construction failures return false. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Validator as isValidEncodedRedirectPath
participant URL as URL constructor
Caller->>Validator: provide encodedPath
Validator->>Validator: try decodeURIComponent(encodedPath)
alt decode failed
Validator-->>Caller: return false
else decode succeeded
Validator->>Validator: if not decoded.startsWith("/")
alt not start with "/"
Validator-->>Caller: return false
else starts with "/"
Validator->>URL: new URL(decoded, "https://thirdweb.com")
alt URL construction failed
Validator-->>Caller: return false
else URL built
Validator->>Validator: check url.hostname === "thirdweb.com"
alt hostname matches
Validator-->>Caller: return true
else mismatch
Validator-->>Caller: return false
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-06-05T13:59:49.886ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (2)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8422 +/- ##
=======================================
Coverage 54.85% 54.85%
=======================================
Files 919 919
Lines 60853 60853
Branches 4142 4142
=======================================
Hits 33378 33378
Misses 27373 27373
Partials 102 102
🚀 New features to boost your workflow:
|
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/src/app/login/isValidEncodedRedirectPath.ts (1)
4-8: Strong security improvement for preventing open redirects.The validation logic effectively prevents open redirect attacks by:
- Ensuring the decoded path is relative (starts with "/")
- Resolving it against the thirdweb.com base URL
- Verifying the resulting hostname remains "thirdweb.com"
This correctly handles several attack vectors:
- Protocol-relative URLs (
//evil.com) pass the first check but fail the hostname verification- Absolute URLs without "/" are rejected immediately
- Path traversal attempts normalize to paths on thirdweb.com
- Query parameters and fragments remain safe
Optional enhancement: Consider making the protocol-relative URL rejection more explicit by checking that the path does not start with "//":
const decodedPath = decodeURIComponent(encodedPath); -if (!decodedPath.startsWith("/")) { +if (!decodedPath.startsWith("/") || decodedPath.startsWith("//")) { return false; }While the current implementation already prevents "//" paths via the hostname check, an explicit rejection would make the security intent clearer and provide defense in depth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/dashboard/src/app/login/isValidEncodedRedirectPath.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-05T13:59:49.886Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 7285
File: apps/dashboard/src/app/(app)/(dashboard)/published-contract/components/uri-based-deploy.tsx:57-57
Timestamp: 2025-06-05T13:59:49.886Z
Learning: In the thirdweb dashboard Next.js app, when using loginRedirect() in server components, ensure to add a return statement after the redirect call to prevent further code execution and potential security issues.
Applied to files:
apps/dashboard/src/app/login/isValidEncodedRedirectPath.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Size
🔇 Additional comments (1)
apps/dashboard/src/app/login/isValidEncodedRedirectPath.ts (1)
1-13: Verification confirms the function is correctly integrated into the redirect flow.The function is imported and called in the login page server component, where it validates the redirect path before passing it to the client component. The validated path is then used safely in
router.replace()within the client component. No alternative redirect implementations or bypass paths were found in the login directory. The implementation follows security best practices with server-side validation preceding client-side usage.
Merge activity
|
MananTank
changed the title
<!--
## title your PR with this format: "[SDK/Dashboard/Portal] Feature/Fix: Concise title for the changes"
If you did not copy the branch name from Linear, paste the issue tag here (format is TEAM-0000):
## Notes for the reviewer
Anything important to call out? Be sure to also clarify these in your comments.
## How to test
Unit tests, playground, etc.
-->
<!-- start pr-codex -->
---
## PR-Codex overview
This PR modifies the `isValidEncodedRedirectPath` function to enhance its validation logic for decoded paths, ensuring they start with a single slash and belong to the `thirdweb.com` domain.
### Detailed summary
- Removed comments about decoding URI components and path validation.
- Added a check to ensure `decodedPath` starts with a single slash.
- Introduced a `URL` object to validate that the hostname is `thirdweb.com`.
- Simplified the return logic for invalid paths.
> ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}`
<!-- end pr-codex -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Bug Fixes**
* Enhanced login redirect validation to ensure redirects are properly verified and authenticated for the correct domain.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
bot
force-pushed
the
mny-310
branch
from
cd6b912 to
c63ef1c
Compare
vercel
bot
temporarily deployed
to
Production – thirdweb_playground
PR-Codex overview
This PR modifies the
isValidEncodedRedirectPathfunction to enhance validation of the decoded path by checking if it starts with a single slash and ensuring the hostname isthirdweb.com.Detailed summary
falseif thedecodedPathdoes not start with a/.URLobject to validate that the hostname isthirdweb.com.Summary by CodeRabbit