Open
Conversation
Add isSafeRedirectUrl() to validate that redirectTo targets only same-origin URLs. Apply the guard in afterAuth() and LoginPage.tsx to block external-domain redirects (CWE-601, issue #8551).
Replace silent block with in-place URL correction: strip the unsafe redirectTo param via setSearchParams while preserving all other query params (email, action, etc.), so the login flow continues uninterrupted.
Reviewer's GuideImplements safe post-authentication redirect handling by validating redirect URLs against the current origin, preventing open redirects, and adds tests plus login-page URL param cleanup to strip unsafe redirect targets. Sequence diagram for safe post-auth redirect handling in afterAuthsequenceDiagram
actor User
participant Browser
participant AuthModule as AuthModule_afterAuth
participant SessionStore as SessionStore_redirectTo
User->>Browser: Complete authentication
Browser->>AuthModule: Call afterAuth()
AuthModule->>SessionStore: getRedirectTo()
SessionStore-->>AuthModule: redirectTo (encoded or null)
AuthModule->>SessionStore: clearRedirectTo()
alt redirectTo present
AuthModule->>AuthModule: decoded = decodeURIComponent(redirectTo)
AuthModule->>AuthModule: isSafeRedirectUrl(decoded)?
alt unsafe decoded
AuthModule->>Browser: window.location.href = /app
else safe decoded
AuthModule->>AuthModule: url = new URL(decoded, window.location.origin)
AuthModule->>AuthModule: pathname = url.pathname
alt pathname matches user-specific pattern
AuthModule->>Browser: window.location.href = /app
else pathname is root or empty
AuthModule->>Browser: window.location.href = /app
else other safe pathname
AuthModule->>Browser: window.location.href = decoded
end
end
else redirectTo missing
AuthModule->>Browser: window.location.href = /app
end
Class-style diagram for updated sign_in session logic and LoginPageclassDiagram
class SignInSession {
+isSafeRedirectUrl(url string) boolean
+afterAuth() void
+getRedirectTo() string
+clearRedirectTo() void
}
class LoginPageComponent {
+redirectTo string
+useEffect_cleanupRedirectTo() void
}
SignInSession <.. LoginPageComponent : uses_isSafeRedirectUrl
LoginPageComponent ..> SignInSession : relies_on_afterAuth
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
LoginPage'suseEffect,decodeURIComponent(redirectTo)is applied directly to a query param value which is typically already decoded; consider wrapping this in a try/catch or only decoding when you know it was encoded to avoid runtime errors on malformed%sequences. - When calling
setSearch, you're mutating the existingURLSearchParamsinstance (prev.delete('redirectTo'); return prev;); it’s safer to construct and return a newURLSearchParams(e.g.const next = new URLSearchParams(prev); next.delete('redirectTo'); return next;) to avoid subtle issues with in-place mutation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `LoginPage`'s `useEffect`, `decodeURIComponent(redirectTo)` is applied directly to a query param value which is typically already decoded; consider wrapping this in a try/catch or only decoding when you know it was encoded to avoid runtime errors on malformed `%` sequences.
- When calling `setSearch`, you're mutating the existing `URLSearchParams` instance (`prev.delete('redirectTo'); return prev;`); it’s safer to construct and return a new `URLSearchParams` (e.g. `const next = new URLSearchParams(prev); next.delete('redirectTo'); return next;`) to avoid subtle issues with in-place mutation.
## Individual Comments
### Comment 1
<location path="src/pages/LoginPage.tsx" line_range="31-36" />
<code_context>
+ // Strip unsafe redirectTo from URL immediately, preserving all other params
+ useEffect(() => {
+ if (!redirectTo) return;
+
+ const decodedRedirect = decodeURIComponent(redirectTo);
+
+ if (!isSafeRedirectUrl(decodedRedirect)) {
+ setSearch((prev) => {
+ prev.delete('redirectTo');
+ return prev;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid mutating the `URLSearchParams` instance returned in `setSearch`'s updater.
The updater receives a `URLSearchParams` used to derive the next state, so mutating it in place and returning the same instance depends on implementation details and shared references. Prefer creating a new instance instead:
```ts
setSearch(prev => {
const next = new URLSearchParams(prev);
next.delete('redirectTo');
return next;
});
```
This keeps updates immutable and avoids subtle state bugs.
```suggestion
if (!isSafeRedirectUrl(decodedRedirect)) {
setSearch((prev) => {
const next = new URLSearchParams(prev);
next.delete('redirectTo');
return next;
});
}
```
</issue_to_address>
### Comment 2
<location path="src/application/session/sign_in.ts" line_range="79-75" />
<code_context>
+ return;
+ }
+
+ const url = new URL(decoded, window.location.origin);
const pathname = url.pathname;
</code_context>
<issue_to_address>
**issue (bug_risk):** Root-path redirects now drop query parameters compared to the previous behavior.
Previously, for `https://ourapp.com/?foo=bar`, we set `url.pathname = '/app'` and redirected with `url.toString()`, so `?foo=bar` (and any hash) were kept. In the new `pathname === '/'` branch, redirecting directly to `/app` loses those parts. If we still need to preserve them, this branch should also modify `url.pathname` and redirect via `url.toString()` instead of using a hardcoded path.
</issue_to_address>
### Comment 3
<location path="src/application/session/sign_in.ts" line_range="72" />
<code_context>
if (redirectTo) {
- const url = new URL(decodeURIComponent(redirectTo));
+ const decoded = decodeURIComponent(redirectTo);
+
+ if (!isSafeRedirectUrl(decoded)) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align `decodeURIComponent` error handling in `afterAuth` with the new usage in the Login page.
`decodeURIComponent(redirectTo)` can throw on malformed input. Please extract a small helper that safely decodes and returns `null` on failure, and reuse it both here and in `LoginPage` so invalid encodings are consistently treated as unsafe redirects.
Suggested implementation:
```typescript
}
}
export function safeDecodeRedirectParam(value: string): string | null {
try {
return decodeURIComponent(value);
} catch {
return null;
}
}
export function afterAuth() {
const redirectTo = getRedirectTo();
clearRedirectTo();
if (redirectTo) {
const decoded = safeDecodeRedirectParam(redirectTo);
if (!decoded || !isSafeRedirectUrl(decoded)) {
window.location.href = '/app';
return;
}
const url = new URL(decoded, window.location.origin);
const pathname = url.pathname;
// Check if URL contains workspace/view UUIDs (user-specific paths)
// Don't redirect to user-specific pages from previous sessions
window.location.href = '/app';
```
` since that file isn't shown).
Here are the changes:
<file_operations>
<file_operation operation="edit" file_path="src/application/session/sign_in.ts">
<<<<<<< SEARCH
}
}
export function afterAuth() {
const redirectTo = getRedirectTo();
clearRedirectTo();
if (redirectTo) {
const decoded = decodeURIComponent(redirectTo);
if (!isSafeRedirectUrl(decoded)) {
window.location.href = '/app';
return;
}
const url = new URL(decoded, window.location.origin);
const pathname = url.pathname;
// Check if URL contains workspace/view UUIDs (user-specific paths)
// Don't redirect to user-specific pages from previous sessions
window.location.href = '/app';
=======
}
}
export function safeDecodeRedirectParam(value: string): string | null {
try {
return decodeURIComponent(value);
} catch {
return null;
}
}
export function afterAuth() {
const redirectTo = getRedirectTo();
clearRedirectTo();
if (redirectTo) {
const decoded = safeDecodeRedirectParam(redirectTo);
if (!decoded || !isSafeRedirectUrl(decoded)) {
window.location.href = '/app';
return;
}
const url = new URL(decoded, window.location.origin);
const pathname = url.pathname;
// Check if URL contains workspace/view UUIDs (user-specific paths)
// Don't redirect to user-specific pages from previous sessions
window.location.href = '/app';
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully align error handling across the app, the `LoginPage` should be updated to import and use `safeDecodeRedirectParam` instead of calling `decodeURIComponent` directly:
1. Import `safeDecodeRedirectParam` from `src/application/session/sign_in` (or wherever this module is exported from).
2. Replace any `decodeURIComponent(redirectTo)` calls in `LoginPage` with `safeDecodeRedirectParam(redirectTo)`.
3. Treat a `null` return from `safeDecodeRedirectParam` the same as an unsafe redirect (e.g. do not redirect or fall back to a safe default such as `/app`), matching the behavior in `afterAuth`.
If `LoginPage` cannot depend on this module for architectural reasons, move `safeDecodeRedirectParam` into a shared utility module and import it from both places instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add safeDecodeRedirectParam helper (returns null on malformed input) and use it in afterAuth and LoginPage instead of bare decodeURIComponent - Restore query-param/hash preservation for root-path redirects in afterAuth - Use immutable URLSearchParams construction in LoginPage setSearch updater
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
closes AppFlowy-IO/AppFlowy#8551
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Harden post-login redirection by validating stored and querystring redirect URLs and falling back to the main app route when targets are unsafe or user-specific.
Bug Fixes:
Enhancements:
Tests: