fix: address Sourcery review comments

- 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 commit is contained in:
Lucas.Xu
2026-03-03 14:06:51 +08:00
parent 5a6840b5dd
commit e7320b4ec9
3 changed files with 53 additions and 13 deletions

View File

@@ -1,4 +1,4 @@
import { afterAuth, isSafeRedirectUrl } from '../sign_in';
import { afterAuth, isSafeRedirectUrl, safeDecodeRedirectParam } from '../sign_in';
// Mock localStorage
const localStorageMock = (() => {
@@ -122,8 +122,14 @@ describe('afterAuth', () => {
expect(window.location.href).toBe('/app');
});
it('redirects to /app for root path', () => {
localStorage.setItem('redirectTo', encodeURIComponent('http://localhost/'));
it('redirects to /app path while preserving query params for root path', () => {
localStorage.setItem('redirectTo', encodeURIComponent('http://localhost/?foo=bar'));
afterAuth();
expect(window.location.href).toBe('http://localhost/app?foo=bar');
});
it('redirects to /app when stored redirectTo has malformed encoding', () => {
localStorage.setItem('redirectTo', '%zz');
afterAuth();
expect(window.location.href).toBe('/app');
});
@@ -152,3 +158,21 @@ describe('afterAuth', () => {
expect(localStorage.getItem('redirectTo')).toBeNull();
});
});
describe('safeDecodeRedirectParam', () => {
it('decodes a valid percent-encoded string', () => {
expect(safeDecodeRedirectParam('https%3A%2F%2Fevil.com')).toBe('https://evil.com');
});
it('returns the string unchanged when nothing is encoded', () => {
expect(safeDecodeRedirectParam('/settings')).toBe('/settings');
});
it('returns null for a malformed percent-encoded sequence', () => {
expect(safeDecodeRedirectParam('%zz')).toBeNull();
});
it('returns null for a lone percent sign', () => {
expect(safeDecodeRedirectParam('%')).toBeNull();
});
});

View File

@@ -40,6 +40,18 @@ export function withSignIn() {
};
}
/**
* Decodes a percent-encoded redirect parameter, returning null on malformed input
* so that bad values are always treated as unsafe rather than crashing.
*/
export function safeDecodeRedirectParam(value: string): string | null {
try {
return decodeURIComponent(value);
} catch {
return null;
}
}
/**
* Returns true only if the URL is safe to redirect to after authentication.
* Safe means: a relative path (starts with "/" but NOT "//") OR
@@ -69,9 +81,9 @@ export function afterAuth() {
clearRedirectTo();
if (redirectTo) {
const decoded = decodeURIComponent(redirectTo);
const decoded = safeDecodeRedirectParam(redirectTo);
if (!isSafeRedirectUrl(decoded)) {
if (!decoded || !isSafeRedirectUrl(decoded)) {
window.location.href = '/app';
return;
}
@@ -87,7 +99,9 @@ export function afterAuth() {
// Don't redirect to user-specific pages from previous sessions
window.location.href = '/app';
} else if (pathname === '/' || !pathname) {
window.location.href = '/app';
// Preserve query params and hash but redirect to /app path
url.pathname = '/app';
window.location.href = url.toString();
} else {
window.location.href = decoded;
}

View File

@@ -9,7 +9,7 @@ import { LOGIN_ACTION } from '@/components/login/const';
import { EnterPassword } from '@/components/login/EnterPassword';
import { ForgotPassword } from '@/components/login/ForgotPassword';
import { SignUpPassword } from '@/components/login/SignUpPassword';
import { isSafeRedirectUrl } from '@/application/session/sign_in';
import { isSafeRedirectUrl, safeDecodeRedirectParam } from '@/application/session/sign_in';
import { AFConfigContext } from '@/components/main/app.hooks';
function LoginPage() {
@@ -26,12 +26,14 @@ function LoginPage() {
useEffect(() => {
if (!redirectTo) return;
const decodedRedirect = decodeURIComponent(redirectTo);
const decodedRedirect = safeDecodeRedirectParam(redirectTo);
if (!isSafeRedirectUrl(decodedRedirect)) {
if (!decodedRedirect || !isSafeRedirectUrl(decodedRedirect)) {
setSearch((prev) => {
prev.delete('redirectTo');
return prev;
const next = new URLSearchParams(prev);
next.delete('redirectTo');
return next;
});
}
}, [redirectTo, setSearch]);
@@ -42,9 +44,9 @@ function LoginPage() {
}
if (isAuthenticated && redirectTo) {
const decodedRedirect = decodeURIComponent(redirectTo);
const decodedRedirect = safeDecodeRedirectParam(redirectTo);
if (decodedRedirect !== window.location.href) {
if (decodedRedirect && decodedRedirect !== window.location.href) {
window.location.href = decodedRedirect;
}
}