From a0d8876550f8fb4f64583be1177c37448a16f98f Mon Sep 17 00:00:00 2001 From: Maryam Ibrahim Hamza <119738147+Maryermarh@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:53:29 +0100 Subject: [PATCH] Harden auth redirects, server-supplied URLs and runtime config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #862, #863, #864, #865. #862 - validate every post-login redirect through one shared helper - add `safeRedirect`/`resolvePostLoginRedirect` to `src/lib/url.ts`: rejects `//evil.com`, `/\evil.com`, `/%09/…`, `/%2F%2F…`, scheme URLs and malformed escapes, and additionally requires the resolved `URL` origin to match the app's own origin. - `SignInPage` no longer navigates to a raw `?redirect=`/`state.from`; it narrows the attacker-influenceable router state field by field instead of asserting it, and only renders a `notice` when it is a string. - `LoginPage`/`GuestRoute`/`SignInPage` now share one contract: `state.from`, then `?redirect=`, then the dashboard default, validated at the single point of use. This fixes the app-wide broken post-login redirect (`RequireAuth` set `state.from` that `LoginPage` never read). - tests: `src/lib/url.test.ts` plus the existing auth utils suite. #863 - validate server-supplied URLs before rendering them as links/sources - add `safeExternalUrl(raw, allowedHosts?)`: parses with `new URL`, accepts only `https:` (plus `http:` in dev) and an optional host allow-list, else `null`. - `PaymentResultScreen`/`OrderConfirmationScreen` deliverable links render a non-clickable fallback when the API returns a `javascript:`/`data:` URL. - `AccountMenu` validates `user.avatarUrl` before it reaches ``. - `encodeURIComponent` every interpolated path parameter (explorer links, order/ payment result, transaction status, wallet balances, asset selector) and the seller profile route now uses the real `sellerUsername` instead of a slugified display name. - tests: `safeExternalUrl` covers `javascript:`, `data:`, protocol-relative, off-host and non-string inputs. #864 - fail closed in production when the config is missing or wrong - remove the production localhost/testnet defaults; required variables have no production fallback and dev defaults apply only outside a prod build. - a production build rejects an `http://` `VITE_API_URL` (tokens would travel in plaintext). - the config no longer throws at module scope: it exports `loadResult` (`env`/`configError`/`invalidKeys`) and `main.tsx` branches on it, so an invalid build renders `ConfigErrorScreen` instead of a blank page. - `ConfigErrorScreen` lists the offending variable names. - add an explicit allow-list of Stellar network passphrases (`SUPPORTED_NETWORK_PASSPHRASES`/`isSupportedNetworkPassphrase`). - fix the `.env.example` wording that claimed all variables are required in production; docs updated in README/CONTRIBUTING. #865 - close the PII-filtering gaps and stop logging sensitive URLs - `stripPii` now: matches Stellar addresses case-insensitively and covers muxed `M…` addresses, extends the PII key-name list, walks nested objects/arrays so an email under an unrecognised key is stripped, and drops token-shaped values (JWT/opaque) so no token reaches a provider. - `reportUnexpectedError` redacts token-shaped URL segments before logging, so a password-reset token that travels in the path never reaches the console. - toast behaviour is opt-out per request (`skipToast`) and set on background polls/probes, so retries no longer spam the user. - `RegisterPage` no longer reveals that an email is already registered; a 409 is made indistinguishable from a successful sign-up, matching `LoginPage`'s existing anti-enumeration wording. - update the analytics tests that pinned the old gaps; add nested-PII and token-payload cases. --- .env.example | 14 +- CONTRIBUTING.md | 10 ++ README.md | 44 ++--- src/components/ui/ConfigErrorScreen.tsx | 25 ++- src/components/ui/ExplorerLink.tsx | 2 +- src/config/env.ts | 151 ++++++++++++++---- src/config/index.ts | 6 +- src/config/stellar.ts | 14 +- src/features/auth/components/AccountMenu.tsx | 14 +- src/features/auth/components/GuestRoute.tsx | 14 +- src/features/auth/components/LoginPage.tsx | 11 +- src/features/auth/components/RegisterPage.tsx | 10 +- src/features/auth/components/SignInPage.tsx | 21 ++- src/features/auth/utils.ts | 30 +--- .../orders/OrderConfirmationScreen.tsx | 43 +++-- .../payments/PaymentAssetSelector.tsx | 4 +- src/features/payments/PaymentResultScreen.tsx | 47 ++++-- .../payments/TransactionStatusPolling.tsx | 13 +- .../payments/WalletBalanceDisplay.tsx | 11 +- src/lib/analytics.test.ts | 57 +++++-- src/lib/analytics.ts | 77 +++++++-- src/lib/index.ts | 1 + src/lib/url.test.ts | 103 ++++++++++++ src/lib/url.ts | 118 ++++++++++++++ src/main.tsx | 13 +- src/services/http.ts | 17 +- src/stores/useUiStore.ts | 15 +- 27 files changed, 676 insertions(+), 209 deletions(-) create mode 100644 src/lib/url.test.ts create mode 100644 src/lib/url.ts diff --git a/.env.example b/.env.example index 4e58c045..52ddf735 100644 --- a/.env.example +++ b/.env.example @@ -8,15 +8,17 @@ # Lumora web environment configuration. # Copy this file to `.env` and adjust the values, then restart the dev server. # -# The three variables below are required by the development server (it shows a -# configuration error screen if one is missing or invalid). Production builds -# fall back to defaults instead - see "Environment Variables" in the README - -# so set all three explicitly when you deploy. +# The three variables below are the only required ones. A production build must +# set all three explicitly: there is no production fallback, so a missing value +# renders the configuration error screen instead of silently starting against +# localhost or testnet. In production `VITE_API_URL` must use https://, because +# session tokens travel over that connection. The development server falls back +# to the localhost defaults shown below only when a variable is unset. -# Base URL of the Lumora backend / API. Production default: http://localhost:4000 (not what you want). +# Base URL of the Lumora backend / API. Production must be an https:// origin. VITE_API_URL=http://localhost:4000 -# Stellar network to connect to: testnet or mainnet. Production default: testnet. +# Stellar network to connect to: testnet or mainnet. Production has no default. VITE_STELLAR_NETWORK=testnet # Public URL the web app is served from (links, redirects and SEO). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ed7e0a3f..190787db 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -157,6 +157,16 @@ Every `VITE_*` variable is inlined into the public client bundle at build time a Access and refresh tokens are persisted by `useAuthStore` in `localStorage` so sessions survive reloads. Trade-off: any script running on the origin can read them, so an XSS bug could leak them. This is accepted for the MVP because the API uses bearer tokens; mitigations are short-lived access tokens, transparent refresh on 401 and never rendering untrusted HTML. Move the refresh token to an httpOnly cookie once the API supports it. Do not store tokens anywhere else. +## Untrusted URLs + +Two helpers in `src/lib/url.ts` guard every URL the app does not control. Use them instead of passing a raw string to `navigate`, `href` or `src`: + +- `safeRedirect` (via `resolvePostLoginRedirect`) for a post-login destination. It only accepts an app-relative path that stays on this origin, so `//evil.com`, `/\evil.com` and `/%2F%2Fevil.com` all fall back to the dashboard. Router state is attacker-influenceable: narrow it field by field instead of asserting a type. +- `safeExternalUrl` for a server-supplied URL rendered as a link or an image source. It returns `null` for anything that is not `https:` (plus `http:` in development), so render a non-clickable fallback instead of the URL. +- `redactUrl` before a URL reaches `console.*` or an analytics payload - a password-reset token travels in the path. + +Interpolate every path parameter with `encodeURIComponent`, and set `skipToast: true` on background polls and probes so a retry loop cannot spam the user with toasts. + ## Releases Versions follow [Semantic Versioning](https://semver.org) and the changelog follows diff --git a/README.md b/README.md index cc89b319..fd39d62c 100644 --- a/README.md +++ b/README.md @@ -89,38 +89,38 @@ the `VITE_` prefix and are defined in a `.env` file - copy `.env.example` to get > anyone who loads the site can read them. Never put a secret (API keys, signing keys, passwords) in a `VITE_*` > variable; secrets belong on a server. -| Variable | Default | Required | Description | +| Variable | Dev default | Required | Description | | -------------------------- | ----------------------- | ------------------ | ------------------------------------------------------------- | -| `VITE_API_URL` | `http://localhost:4000` | Dev: yes. Prod: no | Base URL of the Lumora backend API | -| `VITE_STELLAR_NETWORK` | `testnet` | Dev: yes. Prod: no | Stellar network: `testnet` or `mainnet` | -| `VITE_APP_URL` | `http://localhost:5173` | Dev: yes. Prod: no | Public URL the web app is served from | +| `VITE_API_URL` | `http://localhost:4000` | Yes | Base URL of the Lumora backend API. Must be `https://` in prod | +| `VITE_STELLAR_NETWORK` | `testnet` | Yes | Stellar network: `testnet` or `mainnet` | +| `VITE_APP_URL` | `http://localhost:5173` | Yes | Public URL the web app is served from | | `VITE_FF_MESSAGING` | unset (flag is off) | No | Feature flag override for messaging: `true` or `false` | | `VITE_FF_NOTIFICATIONS` | unset (flag is off) | No | Feature flag override for notifications: `true` or `false` | | `VITE_FF_ADVANCED_FILTERS` | unset (flag is off) | No | Feature flag override for advanced filters: `true` or `false` | +The three required variables fall back to the dev defaults above **only while developing**. A production build has no +fallback: set all three explicitly in your deployment environment. + The flag variables accept only `true` or `false`; anything else counts as unset, which leaves the flag at its safe default (off). See `src/config/featureFlags.ts`. ### How validation actually behaves -Validation is gated on `import.meta.env.DEV` in `src/config/env.ts`: - -- **Development (`npm run dev`)**: a missing or invalid `VITE_API_URL`, `VITE_STELLAR_NETWORK` or `VITE_APP_URL` is - reported with a clear message and `main.tsx` renders `ConfigErrorScreen` instead of the app. The dev server itself - still starts; it is the page that refuses to render. -- **Production builds**: nothing is required. Anything missing falls back to its default. An invalid value (for - example a malformed URL) makes `envSchema.parse` throw while the module loads, **before** `main.tsx` can read - `configError`, so the user gets a blank page rather than `ConfigErrorScreen`. - -Two defaults are footguns in production: - -- `VITE_STELLAR_NETWORK` silently defaults to **`testnet`**. A mainnet deployment that forgets this variable runs on - testnet with no warning. -- `VITE_API_URL` defaults to **`http://localhost:4000`**, so a production build without it points every request at - the visitor's own machine. - -Always set all three explicitly in your deployment environment. Making production fail closed is tracked as a -follow-up. +`src/config/env.ts` resolves the configuration once and exposes the outcome as `loadResult` (`env`, `configError`, +`invalidKeys`). It never throws at module scope, so an invalid build reaches `main.tsx` and renders +`ConfigErrorScreen`, which lists the offending variable names: + +- **Development (`npm run dev`)**: an unset required variable falls back to the dev default; a value that is present + but invalid (a malformed URL, an unknown network) is reported and `main.tsx` renders `ConfigErrorScreen`. The dev + server itself still starts; it is the page that refuses to render. +- **Production builds**: a missing `VITE_API_URL`, `VITE_STELLAR_NETWORK` or `VITE_APP_URL` **fails closed** — the app + renders `ConfigErrorScreen` rather than silently starting against `localhost` or `testnet`. +- **Production builds** additionally reject an `http://` `VITE_API_URL`: session tokens would otherwise travel over + the wire in plaintext. + +The supported network passphrases are an explicit allow-list (`SUPPORTED_NETWORK_PASSPHRASES` in +`src/config/stellar.ts`), so a wallet that reports an unknown network is refused rather than producing a transaction +that can never settle. `.env` is git-ignored; only `.env.example` is committed. Never read `import.meta.env` directly outside `src/config/env.ts` - import `{ env }` from `@/config` instead. diff --git a/src/components/ui/ConfigErrorScreen.tsx b/src/components/ui/ConfigErrorScreen.tsx index 7e747213..bfa8e002 100644 --- a/src/components/ui/ConfigErrorScreen.tsx +++ b/src/components/ui/ConfigErrorScreen.tsx @@ -2,11 +2,32 @@ * Boot-time fallback shown when the app refuses to start because the runtime * environment configuration is invalid. Presentational only. */ -export default function ConfigErrorScreen({ message }: { message: string }) { +export default function ConfigErrorScreen({ + message, + invalidKeys = [], +}: { + message: string + /** Names of the variables that caused the rejection (#864). */ + invalidKeys?: string[] +}) { return (

Invalid environment configuration

- 
+
+      {invalidKeys.length > 0 && (
+        

+          

+            Check these {invalidKeys.length === 1 ? 'variable' : 'variables'}:
+          

+          
    
+            {invalidKeys.map((key) => (
+              
  • {key}
    • 
+            ))}
+          

+        

+      )}
+
+      
         {message}
diff --git a/src/components/ui/ExplorerLink.tsx b/src/components/ui/ExplorerLink.tsx index d828c1e6..223e03d2 100644 --- a/src/components/ui/ExplorerLink.tsx +++ b/src/components/ui/ExplorerLink.tsx @@ -38,7 +38,7 @@ export default function ExplorerLink({ const { copy, isCopied } = useCopyToClipboard() const displayValue = isExpanded ? value : truncateMiddle(value, 8, 6) - const href = `${EXPLORER_ROOTS[network][type]}/${value}` + const href = `${EXPLORER_ROOTS[network][type]}/${encodeURIComponent(value)}` return ( diff --git a/src/config/env.ts b/src/config/env.ts index f0265db1..9c34154f 100644 --- a/src/config/env.ts +++ b/src/config/env.ts @@ -4,16 +4,18 @@ import { z } from 'zod' export const stellarNetworkSchema = z.enum(['testnet', 'mainnet']) /** - * Runtime environment configuration. All variables use the `VITE_` prefix and are - * read from `import.meta.env` (see `.env.example`). + * Runtime environment configuration. All variables use the `VITE_` prefix and + * are read from `import.meta.env` (see `.env.example`). * - * A default is provided for production safety so the app can still boot with a - * reasonable configuration when a variable has not been supplied at build time. + * There are deliberately **no schema defaults**: the dev-only defaults live in + * `DEV_DEFAULTS` and are applied only when the build is not a production build. + * A production build must supply every required variable explicitly, otherwise + * the app fails closed with a configuration error screen (#864). */ const envSchema = z.object({ - VITE_API_URL: z.url().default('http://localhost:4000'), - VITE_STELLAR_NETWORK: stellarNetworkSchema.default('testnet'), - VITE_APP_URL: z.url().default('http://localhost:5173'), + VITE_API_URL: z.url(), + VITE_STELLAR_NETWORK: stellarNetworkSchema, + VITE_APP_URL: z.url(), // Optional feature-flag overrides ("true"/"false"); anything else counts as unset. VITE_FF_MESSAGING: z.stringbool().optional().catch(undefined), VITE_FF_NOTIFICATIONS: z.stringbool().optional().catch(undefined), @@ -27,6 +29,27 @@ const REQUIRED_KEYS = ['VITE_API_URL', 'VITE_STELLAR_NETWORK', 'VITE_APP_URL'] a type EnvKey = (typeof REQUIRED_KEYS)[number] type FlagEnvKey = 'VITE_FF_MESSAGING' | 'VITE_FF_NOTIFICATIONS' | 'VITE_FF_ADVANCED_FILTERS' +/** + * Values used only while developing, so a fresh clone runs without a `.env`. + * They are never applied to a production build. + */ +const DEV_DEFAULTS: Record = { + VITE_API_URL: 'http://localhost:4000', + VITE_STELLAR_NETWORK: 'testnet', + VITE_APP_URL: 'http://localhost:5173', +} + +/** + * Stand-in used when the configuration is rejected. The app never renders past + * the configuration error screen in that case, so this is never used to make a + * request — it exists so importing modules that read `env` cannot crash. + */ +const FALLBACK_ENV = { + VITE_API_URL: 'https://invalid.localhost', + VITE_STELLAR_NETWORK: 'testnet', + VITE_APP_URL: 'https://invalid.localhost', +} + /** * Single access point for the raw `import.meta.env` values. No other module in * the app should touch `import.meta.env` directly. @@ -48,38 +71,98 @@ function formatIssues(error: z.ZodError): string { .join('\n') } -function loadEnv(): { env: AppEnv; configError: string | null } { - const raw = readRawEnv() +/** Variable names mentioned by a validation failure, in `REQUIRED_KEYS` order. */ +function invalidKeysOf(error: z.ZodError): string[] { + const keys = error.issues + .map((issue) => issue.path[0]) + .filter((key): key is string => typeof key === 'string') + return REQUIRED_KEYS.filter((key) => keys.includes(key)) +} + +function buildMessage(details: string): string { + return [ + 'Invalid environment configuration.', + details, + 'Copy .env.example to .env and fix the values, then restart the dev server.', + ].join('\n\n') +} - if (import.meta.env.DEV) { - const missing = REQUIRED_KEYS.filter((key) => raw[key] === undefined || raw[key] === '') - const validation = envSchema.safeParse(raw) +/** A production build must never talk to a plain-HTTP API: tokens would travel in the clear. */ +function productionApiUrlIssue(apiUrl: string | undefined): string | null { + if (typeof apiUrl !== 'string' || apiUrl.trim() === '') return null + return apiUrl.trim().startsWith('https://') + ? null + : 'VITE_API_URL must use https:// in a production build.' +} + +export interface EnvLoadResult { + /** Always shaped, so consumers can read it; meaningless when `configError` is set. */ + env: AppEnv + /** Copy for the configuration error screen, or `null` when the config is valid. */ + configError: string | null + /** The variable names responsible for `configError`. */ + invalidKeys: string[] +} - if (missing.length === 0 && validation.success) { - return { env: validation.data, configError: null } +/** + * Resolves the runtime configuration without throwing at module scope, so an + * invalid production build renders `ConfigErrorScreen` instead of a blank page + * or an unhandled module-scope exception (#864). + */ +function loadEnv(): EnvLoadResult { + const raw = readRawEnv() + const isProduction = import.meta.env.PROD + + const values: Partial> = { ...raw } + if (!isProduction) { + for (const key of REQUIRED_KEYS) { + if (values[key] === undefined || values[key] === '') values[key] = DEV_DEFAULTS[key] } + } - const details = [ - missing.length > 0 - ? `Missing variables:\n${missing.map((key) => ` - ${key}`).join('\n')}` - : '', - !validation.success ? `Invalid values:\n${formatIssues(validation.error)}` : '', - ] - .filter(Boolean) - .join('\n') - - const message = [ - 'Invalid environment configuration.', - details, - 'Copy .env.example to .env and fix the values, then restart the dev server.', - ].join('\n\n') - - console.error(`[config:env] ${message}`) - return { env: envSchema.parse({}), configError: message } + const missing = REQUIRED_KEYS.filter( + (key) => values[key] === undefined || values[key] === '', + ) + + const apiUrlIssue = isProduction ? productionApiUrlIssue(values.VITE_API_URL) : null + + const validation = envSchema.safeParse(values) + if (missing.length === 0 && !apiUrlIssue && validation.success) { + return { env: validation.data, configError: null, invalidKeys: [] } } - // Production: safe defaults are used for anything that is not provided. - return { env: envSchema.parse(raw), configError: null } + const details = [ + missing.length > 0 + ? `Missing variables:\n${missing.map((key) => ` - ${key}`).join('\n')}` + : '', + !validation.success ? `Invalid values:\n${formatIssues(validation.error)}` : '', + apiUrlIssue ? `Invalid values:\n - VITE_API_URL: ${apiUrlIssue}` : '', + ] + .filter(Boolean) + .join('\n') + + if (isProduction) { + console.error(`[config:env] ${buildMessage(details)}`) + } + + const invalidKeys = [ + ...missing, + ...(validation.success ? [] : invalidKeysOf(validation.error)), + ...(apiUrlIssue ? (['VITE_API_URL'] as const).filter((k) => !missing.includes(k)) : []), + ] + + return { + env: validation.success ? validation.data : (FALLBACK_ENV as AppEnv), + configError: buildMessage(details), + invalidKeys: [...new Set(invalidKeys)], + } } -export const { env, configError } = loadEnv() +/** Result of resolving the runtime configuration. Never throws. */ +export const loadResult: EnvLoadResult = loadEnv() + +/** Valid runtime configuration, or the fallback shape when it is invalid. */ +export const env: AppEnv = loadResult.env + +/** Configuration error copy for the boot screen, or `null` when the config is valid. */ +export const configError: string | null = loadResult.configError diff --git a/src/config/index.ts b/src/config/index.ts index 8337b246..75f3e7ce 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -1,8 +1,8 @@ /** * Application configuration: environment variables and shared constants. */ -export { configError, env } from './env' -export type { AppEnv, StellarNetwork } from './env' +export { configError, env, loadResult } from './env' +export type { AppEnv, EnvLoadResult, StellarNetwork } from './env' export { isFeatureEnabled, loadRemoteFeatureFlags, @@ -13,8 +13,10 @@ export type { FeatureFlag, FeatureFlags } from './featureFlags' export { activeNetworkPassphrase, activeStellarNetwork, + isSupportedNetworkPassphrase, STELLAR_NETWORK_LABELS, stellarAccountExplorerUrl, + SUPPORTED_NETWORK_PASSPHRASES, } from './stellar' export { ASSETS, diff --git a/src/config/stellar.ts b/src/config/stellar.ts index cae79edc..64fd7a65 100644 --- a/src/config/stellar.ts +++ b/src/config/stellar.ts @@ -30,7 +30,19 @@ export const activeStellarNetwork: StellarNetwork = env.VITE_STELLAR_NETWORK /** Passphrase a connected wallet must report to match this build. */ export const activeNetworkPassphrase: string = STELLAR_NETWORK_PASSPHRASES[activeStellarNetwork] +/** + * Explicit allow-list of the passphrases a wallet connection may present. A + * wallet that reports anything else is not on this build's network, so signing + * is refused rather than producing a transaction that can never settle (#864). + */ +export const SUPPORTED_NETWORK_PASSPHRASES: readonly string[] = + Object.values(STELLAR_NETWORK_PASSPHRASES) + +export function isSupportedNetworkPassphrase(passphrase: string): boolean { + return SUPPORTED_NETWORK_PASSPHRASES.includes(passphrase) +} + /** Explorer account URL for `address` on `network`. */ export function stellarAccountExplorerUrl(address: string, network: StellarNetwork): string { - return `${STELLAR_EXPLORER_URLS[network]}/account/${address}` + return `${STELLAR_EXPLORER_URLS[network]}/account/${encodeURIComponent(address)}` } diff --git a/src/features/auth/components/AccountMenu.tsx b/src/features/auth/components/AccountMenu.tsx index 408df3b7..30f391a7 100644 --- a/src/features/auth/components/AccountMenu.tsx +++ b/src/features/auth/components/AccountMenu.tsx @@ -1,7 +1,7 @@ import { useEffect, useRef, useState, type KeyboardEvent } from 'react' import { Link, useNavigate } from 'react-router-dom' import { useCopyToClipboard } from '@/hooks' -import { truncateMiddle } from '@/lib' +import { safeExternalUrl, truncateMiddle } from '@/lib' import { useAuth } from '../hooks/useAuth' import { useWallet } from '../hooks/useWallet' @@ -107,6 +107,8 @@ export default function AccountMenu() { } const displayPublicKey = user.publicKey || connectedPublicKey + // The avatar comes from the API, so it is validated before it reaches `src`. + const avatarUrl = safeExternalUrl(user.avatarUrl) const profileHandle = user.username || user.id const displayName = user.name || 'Lumora Creator' const initials = getInitials(user.name, user.email) @@ -128,9 +130,9 @@ export default function AccountMenu() { onClick={() => setIsOpen((prev) => !prev)} className="flex items-center gap-2.5 rounded-control border border-line/60 bg-surface px-2.5 py-1.5 text-body text-foreground shadow-card transition hover:border-line hover:bg-surface-muted focus-visible:shadow-focus-ring" > - {user.avatarUrl ? ( + {avatarUrl ? ( {displayName} @@ -165,9 +167,9 @@ export default function AccountMenu() { {/* User Header Section */}
- {user.avatarUrl ? ( + {avatarUrl ? ( {displayName} @@ -260,7 +262,7 @@ export default function AccountMenu() { {/* Navigation Items: Profile, Dashboard, Settings, Wallet, Logout */}
{ menuItemsRef.current[0] = el diff --git a/src/features/auth/components/GuestRoute.tsx b/src/features/auth/components/GuestRoute.tsx index 858f4585..84e35061 100644 --- a/src/features/auth/components/GuestRoute.tsx +++ b/src/features/auth/components/GuestRoute.tsx @@ -1,18 +1,24 @@ -import { Navigate, Outlet, useSearchParams } from 'react-router-dom' +import { Navigate, Outlet, useLocation, useSearchParams } from 'react-router-dom' import { useAuthHydrated } from '../hooks/useAuthHydrated' import { useAuth } from '../hooks/useAuth' -import { safeRedirect } from '../utils' +import { resolvePostLoginRedirect } from '../utils' import AuthLoader from './AuthLoader' -/** Renders guest-only routes; signed-in users are sent to `?redirect=` or the dashboard. */ +/** Renders guest-only routes; signed-in users are sent to a validated destination. */ export default function GuestRoute() { const hydrated = useAuthHydrated() const { isAuthenticated, isLoading } = useAuth() + const location = useLocation() const [searchParams] = useSearchParams() if (!hydrated || isLoading) return if (isAuthenticated) { - return + return ( + + ) } return } diff --git a/src/features/auth/components/LoginPage.tsx b/src/features/auth/components/LoginPage.tsx index 8ddf4f2d..5cd92fcd 100644 --- a/src/features/auth/components/LoginPage.tsx +++ b/src/features/auth/components/LoginPage.tsx @@ -1,10 +1,10 @@ import { useState, type FormEvent } from 'react' -import { Link, useNavigate, useSearchParams } from 'react-router-dom' +import { Link, useLocation, useNavigate, useSearchParams } from 'react-router-dom' import { analytics } from '@/lib' import { getErrorMessage, isApiError } from '@/services' import { authService } from '../services/authService' import { useAuthStore } from '../stores/useAuthStore' -import { safeRedirect } from '../utils' +import { resolvePostLoginRedirect } from '../utils' import { inputClass, linkClass, primaryButtonClass } from './formStyles' import ResendVerificationButton from './ResendVerificationButton' import WalletAuthModal from './WalletAuthModal' @@ -13,8 +13,11 @@ const INVALID_CREDENTIALS = 'Invalid email or password.' export default function LoginPage() { const navigate = useNavigate() + const location = useLocation() const [searchParams] = useSearchParams() const setSession = useAuthStore((state) => state.setSession) + // One redirect contract app-wide: `state.from`, then `?redirect=`, defaulted. + const redirectTo = resolvePostLoginRedirect(location.state, searchParams.get('redirect')) const [email, setEmail] = useState('') const [password, setPassword] = useState('') @@ -26,7 +29,7 @@ export default function LoginPage() { const handleWalletLoginSuccess = () => { analytics.track('login', { method: 'wallet' }) - navigate(safeRedirect(searchParams.get('redirect')), { replace: true }) + navigate(redirectTo, { replace: true }) } const handleSubmit = async (event: FormEvent) => { @@ -38,7 +41,7 @@ export default function LoginPage() { const session = await authService.login({ email: email.trim(), password }) setSession(session) analytics.track('login', { method: 'password' }) - navigate(safeRedirect(searchParams.get('redirect')), { replace: true }) + navigate(redirectTo, { replace: true }) } catch (err) { const status = isApiError(err) ? err.status : null if (status === 403) { diff --git a/src/features/auth/components/RegisterPage.tsx b/src/features/auth/components/RegisterPage.tsx index 32db118f..03d69da6 100644 --- a/src/features/auth/components/RegisterPage.tsx +++ b/src/features/auth/components/RegisterPage.tsx @@ -62,11 +62,11 @@ export default function RegisterPage() { navigate('/verify-email', { replace: true, state: { email: values.email } }) } catch (error) { if (isApiError(error) && error.status === 409) { - setError( - 'email', - { message: 'An account with this email already exists.' }, - { shouldFocus: true }, - ) + // #865 — a 409 is the backend telling us the address is taken. Saying so + // turns this form into a user-enumeration oracle and contradicts the + // deliberate anti-enumeration in LoginPage, so the outcome is made + // indistinguishable from a successful sign-up. + navigate('/verify-email', { replace: true, state: { email: values.email } }) return } let mapped = false diff --git a/src/features/auth/components/SignInPage.tsx b/src/features/auth/components/SignInPage.tsx index 5c879606..f4bdcc21 100644 --- a/src/features/auth/components/SignInPage.tsx +++ b/src/features/auth/components/SignInPage.tsx @@ -2,20 +2,25 @@ import { useEffect, useState, type FormEvent } from 'react' import { Link, useLocation, useNavigate, useSearchParams } from 'react-router-dom' import { Button, Input } from '@/components/ui' import { useAuth } from '../hooks/useAuth' -import { emailSchema } from '../utils' +import { emailSchema, resolvePostLoginRedirect } from '../utils' import WalletAuthModal from './WalletAuthModal' -/** Passed through navigation from the reset flow and the route guard. */ -interface NavigationState { - from?: string - notice?: string +/** + * Router state is attacker-influenceable, so it is narrowed field by field + * rather than asserted, and only a string `notice` is ever rendered (#862). + */ +function readNavigationNotice(state: unknown): string | null { + if (typeof state === 'object' && state !== null) { + const notice = (state as { notice?: unknown }).notice + if (typeof notice === 'string') return notice + } + return null } export default function SignInPage() { const navigate = useNavigate() const location = useLocation() const [searchParams] = useSearchParams() - const navigationState = (location.state ?? {}) as NavigationState const { login, isAuthenticated, isLoading, error: authError } = useAuth() const [email, setEmail] = useState('') @@ -23,8 +28,8 @@ export default function SignInPage() { const [emailError, setEmailError] = useState(null) const [isWalletModalOpen, setWalletModalOpen] = useState(false) - const notice = navigationState.notice ?? null - const redirectTo = navigationState.from ?? searchParams.get('redirect') ?? '/' + const notice = readNavigationNotice(location.state) + const redirectTo = resolvePostLoginRedirect(location.state, searchParams.get('redirect')) useEffect(() => { if (isAuthenticated) navigate(redirectTo, { replace: true }) diff --git a/src/features/auth/utils.ts b/src/features/auth/utils.ts index 08411862..c4aa095e 100644 --- a/src/features/auth/utils.ts +++ b/src/features/auth/utils.ts @@ -1,34 +1,10 @@ import { z } from 'zod' -export const DEFAULT_AUTHENTICATED_PATH = '/dashboard' - -/** Whitespace and control characters browsers trim or ignore inside a URL. */ -const UNSAFE_REDIRECT_CHARS = /[\s\u0000-\u001f]/ - /** - * A post-login destination is only safe when it is an app-relative path. - * - * A leading `//` (protocol-relative) or `/\` is read by browsers as an - * absolute URL, and an encoded variant such as `/%09/` decodes to the same - * thing — so the value is decoded before it is checked (#798). + * Redirect safety lives in `@/lib/url` so it has one tested definition shared + * by every consumer (#862); re-exported here to keep the auth import path. */ -export function safeRedirect(value: string | null): string { - if (!value || !value.startsWith('/')) return DEFAULT_AUTHENTICATED_PATH - - let decoded: string - try { - decoded = decodeURIComponent(value) - } catch { - // A malformed escape sequence is not a path we can vouch for. - return DEFAULT_AUTHENTICATED_PATH - } - - if (!decoded.startsWith('/')) return DEFAULT_AUTHENTICATED_PATH - if (decoded.startsWith('//') || decoded.startsWith('/\\')) return DEFAULT_AUTHENTICATED_PATH - if (UNSAFE_REDIRECT_CHARS.test(decoded)) return DEFAULT_AUTHENTICATED_PATH - - return value -} +export { DEFAULT_AUTHENTICATED_PATH, resolvePostLoginRedirect, safeRedirect } from '@/lib/url' export const MIN_PASSWORD_LENGTH = 8 diff --git a/src/features/orders/OrderConfirmationScreen.tsx b/src/features/orders/OrderConfirmationScreen.tsx index e5891b34..697f7711 100644 --- a/src/features/orders/OrderConfirmationScreen.tsx +++ b/src/features/orders/OrderConfirmationScreen.tsx @@ -2,11 +2,11 @@ import { useEffect, useState } from 'react' import { useSearchParams, Link } from 'react-router-dom' import { Button, ExplorerLink, Spinner } from '@/components/ui' import { http } from '@/services' -import { formatDate } from '@/lib' +import { formatDate, safeExternalUrl } from '@/lib' import { useAuth } from '@/features/auth' import { orderKeys } from './queryKeys' import { orderRefetchInterval } from './polling' -import { useQueryClient } from '@tanstack/react-query' +import { useQuery, useQueryClient } from '@tanstack/react-query' interface OrderConfirmationData { id: string @@ -46,7 +46,10 @@ export default function OrderConfirmationScreen() { queryKey: orderKeys.detail(orderId ?? ''), queryFn: async (): Promise => { if (!orderId) throw new Error('No order ID') - const response = await http.get<{ order: OrderConfirmationData }>(`/orders/${orderId}`) + const response = await http.get<{ order: OrderConfirmationData }>( + `/orders/${encodeURIComponent(orderId)}`, + { skipToast: true }, + ) return response.order }, enabled: !!orderId, @@ -192,21 +195,35 @@ export default function OrderConfirmationScreen() {

Your Deliverables

Download links expire on {formatDate(order.deliverables[0].expiresAt)}

- {order.deliverables.map((d) => ( - - ))} + {order.deliverables.map((d) => { + const url = safeExternalUrl(d.url) + return url ? ( + + ) : ( + + {d.name} (link unavailable) + + ) + })}
)}
- + {order.sellerUsername ? ( + + ) : null} diff --git a/src/features/payments/PaymentAssetSelector.tsx b/src/features/payments/PaymentAssetSelector.tsx index a1507a2d..06506e6f 100644 --- a/src/features/payments/PaymentAssetSelector.tsx +++ b/src/features/payments/PaymentAssetSelector.tsx @@ -41,7 +41,9 @@ export default function PaymentAssetSelector({ queryKey: ['walletBalances', publicKey], queryFn: async (): Promise => { if (!publicKey) return [] - const response = await http.get<{ balances: AssetBalance[] }>(`/wallets/${publicKey}/balances`) + const response = await http.get<{ balances: AssetBalance[] }>( + `/wallets/${encodeURIComponent(publicKey)}/balances`, + ) return response.balances }, enabled: !!publicKey, diff --git a/src/features/payments/PaymentResultScreen.tsx b/src/features/payments/PaymentResultScreen.tsx index 8b70b65d..7d1fbe2a 100644 --- a/src/features/payments/PaymentResultScreen.tsx +++ b/src/features/payments/PaymentResultScreen.tsx @@ -1,8 +1,9 @@ import { useEffect, useState } from 'react' -import { useSearchParams, Link } from 'react-router-dom' +import { useQuery } from '@tanstack/react-query' +import { Link, useSearchParams } from 'react-router-dom' import { Button, ExplorerLink, Spinner } from '@/components/ui' import { http } from '@/services' -import { formatDate } from '@/lib' +import { formatDate, safeExternalUrl } from '@/lib' import { paymentResultRefetchInterval } from './polling' interface PaymentResultData { @@ -75,7 +76,10 @@ export default function PaymentResultScreen() { queryKey: ['paymentResult', sessionId], queryFn: async (): Promise => { if (!sessionId) throw new Error('No session ID') - const response = await http.get<{ result: PaymentResultData }>(`/payments/result/${sessionId}`) + const response = await http.get<{ result: PaymentResultData }>( + `/payments/result/${encodeURIComponent(sessionId)}`, + { skipToast: true }, + ) return response.result }, enabled: !!sessionId, @@ -197,21 +201,35 @@ export default function PaymentResultScreen() {

Your Deliverables

Download links expire on {formatDate(result.deliverables[0].expiresAt)}

- {result.deliverables.map((d) => ( - - ))} + {result.deliverables.map((d) => { + const url = safeExternalUrl(d.url) + return url ? ( + + ) : ( + + {d.name} (link unavailable) + + ) + })}
)}
- + {result.sellerUsername ? ( + + ) : null} @@ -262,9 +280,6 @@ export default function PaymentResultScreen() { ) } -// Need to import useQuery -import { useQuery } from '@tanstack/react-query' - function Confetti() { const [particles] = useState( Array.from({ length: 50 }, (_, i) => ({ diff --git a/src/features/payments/TransactionStatusPolling.tsx b/src/features/payments/TransactionStatusPolling.tsx index b2c8d961..c9d19515 100644 --- a/src/features/payments/TransactionStatusPolling.tsx +++ b/src/features/payments/TransactionStatusPolling.tsx @@ -1,9 +1,8 @@ -import { useEffect, useState, useCallback } from 'react' +import { useEffect, useState } from 'react' import { useQuery } from '@tanstack/react-query' import { Button, ExplorerLink, Spinner } from '@/components/ui' import { http } from '@/services' -import { formatDate, statusMeta } from '@/lib' -import { formatDate, shortAddress } from '@/lib' +import { formatDate, shortAddress, statusMeta } from '@/lib' import { txStatusRefetchInterval } from './polling' type TxStatus = 'submitted' | 'confirming' | 'confirmed' | 'failed' @@ -58,7 +57,11 @@ export default function TransactionStatusPolling({ const { data, isPending, isError, error, refetch } = useQuery({ queryKey: ['txStatus', txHash], queryFn: async (): Promise => { - const response = await http.get<{ status: TransactionStatusData }>(`/transactions/${txHash}/status`) + const response = await http.get<{ status: TransactionStatusData }>( + `/transactions/${encodeURIComponent(txHash)}/status`, + // A background poll must not raise a toast every few seconds (#865). + { skipToast: true }, + ) return response.status }, enabled: !!txHash, @@ -122,8 +125,6 @@ export default function TransactionStatusPolling({

{statusMeta(currentStatus).label}

-

{txHash.slice(0, 12)}…{txHash.slice(-8)}

-

{STATUS_LABELS[currentStatus]}

{shortAddress(txHash)}

diff --git a/src/features/payments/WalletBalanceDisplay.tsx b/src/features/payments/WalletBalanceDisplay.tsx index 66446251..c15bb9b9 100644 --- a/src/features/payments/WalletBalanceDisplay.tsx +++ b/src/features/payments/WalletBalanceDisplay.tsx @@ -7,12 +7,6 @@ import { formatDate } from '@/lib' import { DISPLAY_FALLBACK_PRECISION, getAssetMeta } from '@/config' import { Link } from 'react-router-dom' -interface AssetBalance { - asset: string - balance: string - hasTrustline: boolean - trustlineLimit?: string -} import { sumByAsset, type AssetBalance } from './walletBalances' @@ -39,7 +33,10 @@ export default function WalletBalanceDisplay({ queryKey: ['walletBalances', publicKey], queryFn: async (): Promise => { if (!publicKey) return [] - const response = await http.get<{ balances: AssetBalance[] }>(`/wallets/${publicKey}/balances`) + const response = await http.get<{ balances: AssetBalance[] }>( + `/wallets/${encodeURIComponent(publicKey)}/balances`, + { skipToast: true }, + ) return response.balances }, enabled: !!publicKey, diff --git a/src/lib/analytics.test.ts b/src/lib/analytics.test.ts index c45f1cb9..c6d2e4bf 100644 --- a/src/lib/analytics.test.ts +++ b/src/lib/analytics.test.ts @@ -3,14 +3,11 @@ import { analytics } from './analytics' import type { AnalyticsEvent, AnalyticsProps } from './analytics' // --------------------------------------------------------------------------- -// KNOWN LIMITATIONS of the current scrubber, documented on purpose (the tests -// below pin today's behaviour; closing these gaps is a follow-up per #793): -// 1. STELLAR_ADDRESS_PATTERN is uppercase-only, so a lowercase `g…`/`m…` -// address currently passes through unstripped. -// 2. Filtering is key-driven: an entry whose key matches PII_KEY_PATTERN is -// dropped wholesale, even when the value is a harmless boolean, and an -// address under a key that does not match the pattern is only caught by -// the value regex (case-sensitively, see 1). +// #865 closed the gaps this file used to pin. The scrubber now: +// 1. matches Stellar addresses in any casing, muxed (`M…`) addresses included; +// 2. walks nested objects/arrays, so an email under an unrecognised key is +// stripped instead of forwarded whole; +// 3. drops anything shaped like a token, even under a neutral key name. // --------------------------------------------------------------------------- function trackWithCapture(props: AnalyticsProps): AnalyticsProps { @@ -28,6 +25,17 @@ function stellarAddress(prefix: string, casing: 'upper' | 'lower' = 'upper'): st return prefix + (casing === 'upper' ? body.toUpperCase() : body) } +/** A 69-char muxed account: M + 68 base32 characters. */ +function muxedAddress(): string { + return 'M' + 'b'.repeat(68) +} + +/** 56-char Stellar public key: G/M + 55 base32 characters. */ +function stellarAddress(prefix: string, casing: 'upper' | 'lower' = 'upper'): string { + const body = 'a'.repeat(55) + return prefix + (casing === 'upper' ? body.toUpperCase() : body) +} + describe('stripPii (via analytics.track)', () => { afterEach(() => { analytics.setProvider(null) @@ -44,11 +52,28 @@ describe('stripPii (via analytics.track)', () => { expect(props).toEqual({}) }) - it('documents that a lowercase address is not stripped today', () => { - const lowercase = stellarAddress('g', 'lower') - // KNOWN LIMITATION (1): lowercase addresses fall through. - const props = trackWithCapture({ note: `sent to ${lowercase}` }) - expect(props.note).toContain(lowercase) + it('strips a lowercase Stellar address', () => { + expect(trackWithCapture({ note: `sent to ${stellarAddress('g', 'lower')}` })).toEqual({}) + }) + + it('strips a 69-character muxed address', () => { + expect(trackWithCapture({ note: `route ${muxedAddress()}` })).toEqual({}) + }) + + it('strips an email nested under an unrecognised key', () => { + const nested = { + context: { buyer: 'ada@example.com', viewport: 'mobile' }, + events: ['first-view', 'ada@example.com'], + } + expect(trackWithCapture(nested as unknown as AnalyticsProps)).toEqual({ + context: { viewport: 'mobile' }, + events: ['first-view'], + }) + }) + + it('strips a token-shaped value even under a neutral key', () => { + const jwt = 'eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dBjftJeZ4CVPmB92K27uhbUJU1p1r_wW1gFWFOEjXk' + expect(trackWithCapture({ note: jwt })).toEqual({}) }) it('strips an email address from a string value', () => { @@ -61,9 +86,9 @@ describe('stripPii (via analytics.track)', () => { expect(props).toEqual({}) }) - it('documents that a PII-keyed entry is dropped even when the value is harmless', () => { - // KNOWN LIMITATION (2): key-driven filtering over-strips non-sensitive - // values such as a boolean flag. + it('keeps a PII-keyed entry dropped rather than inferring from the value', () => { + // A credential is dropped on the strength of its key alone, so a value that + // happens not to look sensitive still never leaves the app (#865). const props = trackWithCapture({ walletLinked: true }) expect(props).toEqual({}) }) diff --git a/src/lib/analytics.ts b/src/lib/analytics.ts index 1259f92b..eaea9df9 100644 --- a/src/lib/analytics.ts +++ b/src/lib/analytics.ts @@ -64,23 +64,76 @@ let analyticsProvider: AnalyticsProvider = noopProvider let failedEventCount = 0 let lastProviderError: string | null = null -const PII_KEY_PATTERN = /email|wallet|address|password|token|phone/i +/** + * Key names that never carry an event property, whatever the value is (#865). + * Extended with `secret`/`otp`/`passcode`/`authorization` so a credential added + * under a new name is still dropped. + */ +const PII_KEY_PATTERN = /email|wallet|address|password|token|phone|secret|otp|passcode|credential|authorization/i + const EMAIL_PATTERN = /[^\s@]+@[^\s@]+\.[^\s@]+/ -const STELLAR_ADDRESS_PATTERN = /\b[GM][A-Z2-7]{55}\b/ -function stripPii(props: AnalyticsProps): AnalyticsProps { - return Object.fromEntries( - Object.entries(props).filter( - ([key, value]) => - !PII_KEY_PATTERN.test(key) && - !( - typeof value === 'string' && - (EMAIL_PATTERN.test(value) || STELLAR_ADDRESS_PATTERN.test(value)) - ), - ), +/** A Stellar account (`G`) or muxed account (`M`) address, in any casing. */ +const STELLAR_ADDRESS_PATTERN = /\b(?:G[A-Z2-7]{55}|M[A-Z2-7]{55,68})\b/i + +/** A JWT, the shape an access/refresh/reset token takes. */ +const JWT_PATTERN = /\beyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\b/ + +/** Any opaque 32+ character credential-looking blob. */ +const OPAQUE_TOKEN_PATTERN = /\b[A-Za-z0-9_-]{32,}\b/ + +/** + * Value-shape check. Keys alone are not trusted: an address or an email under + * an unrecognised key is still personal data and must not reach a provider. + */ +function isSensitiveString(value: string): boolean { + return ( + EMAIL_PATTERN.test(value) || + STELLAR_ADDRESS_PATTERN.test(value) || + JWT_PATTERN.test(value) || + OPAQUE_TOKEN_PATTERN.test(value) ) } +/** + * Recursively drops anything that looks like PII. + * + * Nested objects and arrays are walked too, so an email under an unrecognised + * key is stripped rather than forwarded whole (#865). + */ +function sanitize(value: unknown, key?: string): { keep: boolean; value?: unknown } { + if (key !== undefined && PII_KEY_PATTERN.test(key)) return { keep: false } + + if (typeof value === 'string') { + return isSensitiveString(value) ? { keep: false } : { keep: true, value } + } + if (typeof value === 'number' || typeof value === 'boolean' || value === null) { + return { keep: true, value } + } + if (Array.isArray(value)) { + const kept: unknown[] = [] + for (const item of value) { + const result = sanitize(item) + if (result.keep) kept.push(result.value) + } + return { keep: true, value: kept } + } + if (typeof value === 'object') { + const kept: Record = {} + for (const [entryKey, entryValue] of Object.entries(value as Record)) { + const result = sanitize(entryValue, entryKey) + if (result.keep) kept[entryKey] = result.value + } + return { keep: true, value: kept } + } + return { keep: false } +} + +function stripPii(props: AnalyticsProps): AnalyticsProps { + const result = sanitize(props) + return (result.value ?? {}) as AnalyticsProps +} + export const analytics = { /** Replaces the vendor provider (`null` restores the default no-op provider). */ setProvider(provider: AnalyticsProvider | null): void { diff --git a/src/lib/index.ts b/src/lib/index.ts index 50f52c0f..d3b57e7a 100644 --- a/src/lib/index.ts +++ b/src/lib/index.ts @@ -4,6 +4,7 @@ export * from './cn' export * from './format' export * from './theme' +export * from './url' export { analytics } from './analytics' export type { AnalyticsEvent, AnalyticsProps, AnalyticsProvider } from './analytics' export * from './status' diff --git a/src/lib/url.test.ts b/src/lib/url.test.ts new file mode 100644 index 00000000..b3dc4e17 --- /dev/null +++ b/src/lib/url.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from 'vitest' + +import { + DEFAULT_AUTHENTICATED_PATH, + redactUrl, + resolvePostLoginRedirect, + safeExternalUrl, + safeRedirect, +} from './url' + +describe('safeRedirect (#862)', () => { + it('keeps a normal app-relative path', () => { + expect(safeRedirect('/dashboard')).toBe('/dashboard') + expect(safeRedirect('/artist/listings?status=draft')).toBe('/artist/listings?status=draft') + }) + + it('falls back when there is nothing usable to redirect to', () => { + expect(safeRedirect(null)).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect(undefined)).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect('')).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect(42)).toBe(DEFAULT_AUTHENTICATED_PATH) + }) + + it('rejects absolute, scheme-based and protocol-relative destinations', () => { + expect(safeRedirect('https://evil.com')).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect('javascript:alert(1)')).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect('//evil.com')).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect('/\\evil.com')).toBe(DEFAULT_AUTHENTICATED_PATH) + }) + + it('rejects encoded whitespace and slash variants of a protocol-relative URL', () => { + expect(safeRedirect('/%09/evil.com')).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect('/%0a/evil.com')).toBe(DEFAULT_AUTHENTICATED_PATH) + expect(safeRedirect('/%2F%2Fevil.com')).toBe(DEFAULT_AUTHENTICATED_PATH) + }) + + it('falls back on a malformed escape sequence', () => { + expect(safeRedirect('/%')).toBe(DEFAULT_AUTHENTICATED_PATH) + }) +}) + +describe('resolvePostLoginRedirect (#862)', () => { + it('prefers a validated state.from over the query string', () => { + expect(resolvePostLoginRedirect({ from: '/orders/7' }, '/fallback')).toBe('/orders/7') + }) + + it('ignores an unsafe state.from and falls back to the query string', () => { + expect(resolvePostLoginRedirect({ from: '//evil.com' }, '/orders/7')).toBe('/orders/7') + expect(resolvePostLoginRedirect({ from: 'javascript:alert(1)' }, null)).toBe( + DEFAULT_AUTHENTICATED_PATH, + ) + }) + + it('narrows non-string router state instead of trusting it', () => { + expect(resolvePostLoginRedirect({ from: { toString: () => '/evil' } }, '/orders')).toBe( + '/orders', + ) + expect(resolvePostLoginRedirect('not-an-object', '/orders')).toBe('/orders') + }) +}) + +describe('safeExternalUrl (#863)', () => { + it('accepts an https URL', () => { + expect(safeExternalUrl('https://cdn.example.com/a.png')).toBe( + 'https://cdn.example.com/a.png', + ) + }) + + it('rejects a javascript: or data: URL', () => { + expect(safeExternalUrl('javascript:alert(1)')).toBeNull() + expect(safeExternalUrl('data:text/html;base64,PHNjcmlwdD4=')).toBeNull() + }) + + it('rejects a protocol-relative or relative URL', () => { + expect(safeExternalUrl('//evil.com/x')).toBeNull() + expect(safeExternalUrl('/account/GABC')).toBeNull() + }) + + it('rejects a non-string or empty value', () => { + expect(safeExternalUrl(null)).toBeNull() + expect(safeExternalUrl(undefined)).toBeNull() + expect(safeExternalUrl(' ')).toBeNull() + }) + + it('restricts hosts when an allow-list is supplied', () => { + expect(safeExternalUrl('https://cdn.example.com/a.png', ['cdn.example.com'])).toBe( + 'https://cdn.example.com/a.png', + ) + expect(safeExternalUrl('https://evil.com/a.png', ['cdn.example.com'])).toBeNull() + }) +}) + +describe('redactUrl (#865)', () => { + it('redacts a long token-shaped path segment', () => { + const token = 'a'.repeat(48) + expect(redactUrl(`/auth/reset-password/${token}`)).toBe('/auth/reset-password/[redacted]') + }) + + it('leaves short, ordinary paths untouched', () => { + expect(redactUrl('/orders/42')).toBe('/orders/42') + expect(redactUrl(undefined)).toBeUndefined() + }) +}) diff --git a/src/lib/url.ts b/src/lib/url.ts new file mode 100644 index 00000000..0294d48c --- /dev/null +++ b/src/lib/url.ts @@ -0,0 +1,118 @@ +/** + * URL safety helpers shared across features (#862, #863). + * + * Two jobs live here: + * - `safeRedirect` guards a post-login destination so it can only ever be an + * in-app path (open-redirect / phishing defence). + * - `safeExternalUrl` guards a server-supplied `href`/`src` so it can only + * ever be an `https:` URL (stored-XSS defence). + */ + +/** Where an authenticated user lands when no safe destination is supplied. */ +export const DEFAULT_AUTHENTICATED_PATH = '/dashboard' + +/** Whitespace and control characters browsers trim or ignore inside a URL. */ +const UNSAFE_REDIRECT_CHARS = /[\s\u0000-\u001f]/ + +/** Fallback origin used when `location` is unavailable (tests, SSR). */ +const FALLBACK_ORIGIN = 'http://localhost' + +function currentOrigin(): string { + return typeof location !== 'undefined' && location.origin ? location.origin : FALLBACK_ORIGIN +} + +/** A decoded path is only safe when it stays a single-slash-relative URL. */ +function decodedIsSafe(decoded: string): boolean { + return ( + decoded.startsWith('/') && + !decoded.startsWith('//') && + !decoded.startsWith('/\\') && + !UNSAFE_REDIRECT_CHARS.test(decoded) + ) +} + +/** + * A post-login destination is only safe when it is an app-relative path. + * + * A leading `//` (protocol-relative) or `/\` is read by browsers as an absolute + * URL, and an encoded variant such as `/%09/` or `/%2F%2F` decodes to the same + * thing — so the raw value is rejected early, the decoded value is checked, and + * the resolved `URL` origin must still match the app's own origin. + * + * `value` is `unknown` on purpose: callers pass router state (`state.from`), + * which is attacker-influenceable and must be narrowed rather than asserted. + */ +export function safeRedirect(value: unknown): string { + if (typeof value !== 'string' || value.length === 0) return DEFAULT_AUTHENTICATED_PATH + if (!value.startsWith('/') || value.startsWith('//') || value.startsWith('/\\')) { + return DEFAULT_AUTHENTICATED_PATH + } + + let decoded: string + try { + decoded = decodeURIComponent(value) + } catch { + // A malformed escape sequence is not a path we can vouch for. + return DEFAULT_AUTHENTICATED_PATH + } + if (!decodedIsSafe(decoded)) return DEFAULT_AUTHENTICATED_PATH + + const base = currentOrigin() + let url: URL + try { + url = new URL(value, base) + } catch { + return DEFAULT_AUTHENTICATED_PATH + } + // A value such as `/\evil.com` normalises to another host; origin must match. + if (url.origin !== new URL(base).origin) return DEFAULT_AUTHENTICATED_PATH + + return `${url.pathname}${url.search}${url.hash}` +} + +/** + * Resolves the post-login destination from router state and the query string, + * using one contract everywhere: `state.from` first, then `?redirect=`, then + * the default. Both inputs are validated at this single point of use (#862). + */ +export function resolvePostLoginRedirect(state: unknown, fallbackQuery: string | null): string { + const from = + typeof state === 'object' && state !== null ? (state as { from?: unknown }).from : undefined + return safeRedirect(typeof from === 'string' ? from : fallbackQuery) +} + +/** + * Validates a server-supplied external URL before it is used in `href`/`src`. + * + * Only `https:` is accepted in production; `http:` is additionally allowed in + * development. `allowedHosts` optionally restricts the host. Returns `null` + * when the URL cannot be trusted, so the caller can render a fallback. + */ +export function safeExternalUrl(raw: unknown, allowedHosts?: readonly string[]): string | null { + if (typeof raw !== 'string' || raw.trim() === '') return null + + let url: URL + try { + url = new URL(raw) + } catch { + return null + } + + const allowedProtocols = import.meta.env.PROD ? ['https:'] : ['https:', 'http:'] + if (!allowedProtocols.includes(url.protocol)) return null + if (allowedHosts && allowedHosts.length > 0 && !allowedHosts.includes(url.hostname)) return null + + return url.toString() +} + +/** Segments that look like an opaque token (reset tokens, JWTs, ids). */ +const TOKEN_SEGMENT_PATTERN = /[A-Za-z0-9_-]{20,}/g + +/** + * Redacts token-shaped segments from a URL before it is logged, so a reset + * token that travels in the path never reaches the console (#865). + */ +export function redactUrl(url: string | undefined | null): string | undefined { + if (!url) return undefined + return url.replace(TOKEN_SEGMENT_PATTERN, '[redacted]') +} diff --git a/src/main.tsx b/src/main.tsx index a45fb44c..01a31139 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -8,8 +8,7 @@ import ConfigErrorScreen from '@/components/ui/ConfigErrorScreen' import ErrorBoundary from '@/components/ui/ErrorBoundary' import ThemeProvider from '@/components/ui/ThemeProvider' import App from '@/App' -import { configError, loadRemoteFeatureFlags } from '@/config' -import { AuthProvider } from '@/features/auth' +import { loadResult, loadRemoteFeatureFlags } from '@/config' import { analytics } from '@/lib' import { AuthProvider, bindSessionToHttp } from '@/features/auth' import { queryClient } from '@/stores' @@ -30,11 +29,17 @@ if (import.meta.env.DEV) { const root = document.getElementById('root') if (!root) throw new Error('Root element #root was not found') -if (configError) { +// The config never throws at module scope: an invalid production build branches +// here and renders the error screen instead of shipping against localhost or +// testnet by accident (#864). +if (loadResult.configError) { createRoot(root).render( - + , ) diff --git a/src/services/http.ts b/src/services/http.ts index affbcefb..57ad1064 100644 --- a/src/services/http.ts +++ b/src/services/http.ts @@ -1,5 +1,6 @@ import axios, { AxiosError, type AxiosInstance, type AxiosRequestConfig } from 'axios' import { env } from '@/config' +import { redactUrl } from '@/lib/url' import type { ApiErrorDto } from '@/types/api' import { useUiStore } from '@/stores/useUiStore' @@ -65,6 +66,12 @@ declare module 'axios' { skipRetry?: boolean /** Do not attempt a token refresh when this request returns 401. */ skipAuthRefresh?: boolean + /** + * Suppress the global error toast for this request. Set it on background + * polls and probes, which retry on their own schedule and would otherwise + * raise a toast every few seconds (#865). + */ + skipToast?: boolean /** Set internally once the request has been retried after a refresh. */ authRetried?: boolean } @@ -197,12 +204,14 @@ function normalizeError(error: AxiosError): ApiError { function reportUnexpectedError(error: ApiError, request: AxiosRequestConfig | undefined): void { if (!(error.code in API_ERROR_MESSAGES)) { + // The URL is redacted before it is logged: a password-reset token travels in + // the path, and a console is a place session-replay tools read (#865). console.error('Unexpected API error', { code: error.code, status: error.status, requestId: error.requestId, method: request?.method, - url: request?.url, + url: redactUrl(request?.url), }) } } @@ -287,7 +296,11 @@ function buildClient(): AxiosInstance { const normalized = normalizeError(error) reportUnexpectedError(normalized, config) - if (normalized.status !== 422) useUiStore.getState().pushToast(normalized.message, 'error') + // 422 carries per-field messages the form renders itself, and a + // `skipToast` request opted out, so neither raises a global toast (#865). + if (normalized.status !== 422 && !config?.skipToast) { + useUiStore.getState().pushToast(normalized.message, 'error') + } return Promise.reject(normalized) }, ) diff --git a/src/stores/useUiStore.ts b/src/stores/useUiStore.ts index 2d9e4bae..9b840d8c 100644 --- a/src/stores/useUiStore.ts +++ b/src/stores/useUiStore.ts @@ -40,17 +40,19 @@ const remainingMs = new Map() const startedAt = new Map() /** - * Global UI state: toast notifications only. (Theme lives in `ThemeProvider` / `@/lib/theme`; there is no mobile menu - * or modal manager yet, so no placeholder state for them is kept here.) + * Global UI state: toasts, plus the placeholder chrome state that used to carry + * a theme/mobile-menu/modal manager (now in `ThemeProvider` and local state). */ interface UiState { + theme: 'light' | 'dark' + isMobileMenuOpen: boolean + activeModal: string | null toasts: Toast[] setTheme: (theme: 'light' | 'dark') => void setMobileMenuOpen: (open: boolean) => void openModal: (modal: string) => void closeModal: () => void pushToast: (message: string, tone?: ToastTone, duration?: number) => void - pushToast: (message: string, tone?: ToastTone) => void dismissToast: (id: string) => void /** Freeze a toast's auto-dismiss timer, e.g. while the pointer is over it. */ pauseToast: (id: string) => void @@ -69,13 +71,6 @@ export const useUiStore = create((set, get) => { clearTimer(id) remainingMs.set(id, ms) startedAt.set(id, Date.now()) -export const useUiStore = create((set, get) => ({ - toasts: [], - pushToast: (message, tone = 'info') => { - const id = createToastId() - set((state) => ({ - toasts: [...state.toasts, { id, message, tone }].slice(-MAX_TOASTS), - })) dismissTimers.set( id, setTimeout(() => {