Repository navigation
Harden auth redirects, server-supplied URLs, production config and PII scrubbing (#862-#865) - #1034
Merged
DevMuhdishaq merged 2 commits intoOct 20, 2026
Conversation
Closes Dfunder#862, Dfunder#863, Dfunder#864, Dfunder#865. Dfunder#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. Dfunder#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 `<img src>`. - `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. Dfunder#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. Dfunder#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.
|
@Maryermarh Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
Author
|
@ayshadogo conflict resolved |
6 tasks
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
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.
Summary
Four security/robustness issues on the auth, URL-handling and config surfaces, addressed together because they share the same helpers.
#862 — unvalidated
?redirect=and a bypassablesafeRedirectSignInPagenavigated to a raw?redirect=/state.from, andsafeRedirectonly rejected values that did not start with/or started with//— so/\evil.comand/%09/evil.comgot through. On top of that,RequireAuthwrotestate={{ from }}thatLoginPagenever read, so post-login redirects were broken app-wide.safeRedirectand a newresolvePostLoginRedirectnow live insrc/lib/url.tswith one tested definition. They reject anything that is not a single-leading-slash app path, reject/followed by\, decode before checking (so encoded whitespace/slash variants are caught), and requirenew URL(value, location.origin).origin === location.origin.unknown, so router state is narrowed, not asserted.SignInPagereadsnoticeonly when it is a string.state.from→?redirect=→/dashboard, validated at the single point of use.#863 — server-supplied URLs rendered without validation
safeExternalUrl(raw, allowedHosts?)parses withnew URL, accepts onlyhttps:(plushttp:in dev), optionally restricts the host, and returnsnullotherwise.javascript:/data:deliverable URL now renders a non-clickable "link unavailable" span instead of an<a href>;AccountMenuvalidatesuser.avatarUrlbefore<img src>.encodeURIComponent-encoded (explorer links, order/payment result, transaction status, wallet balances, asset selector).sellerUsernameinstead of a slugified display name.#864 — production silently defaulted to localhost and testnet
VITE_API_URL,VITE_STELLAR_NETWORKorVITE_APP_URLin a production build fails closed; the dev defaults apply only outside a prod build.http://VITE_API_URLis rejected — session tokens would otherwise travel in plaintext.loadResult(env,configError,invalidKeys) andmain.tsxbranches on it, so an invalid build rendersConfigErrorScreen(now listing the offending variable names) instead of a blank page.SUPPORTED_NETWORK_PASSPHRASES/isSupportedNetworkPassphrase).#865 — PII scrubbing gaps and a token in the console
stripPiimatches Stellar addresses case-insensitively and covers muxedM…addresses, extends the PII key list, recurses through nested objects/arrays so an email under an unrecognised key is stripped, and drops token-shaped values (JWT / opaque blobs) so no token reaches a provider.reportUnexpectedErrorredacts token-shaped URL segments before logging — the password-reset token travels in the path.skipToast) and applied to background polls and probes, so a retry loop no longer spams the user.RegisterPageno longer reveals that an email is already registered: a 409 is made indistinguishable from a successful sign-up, matchingLoginPage's existing anti-enumeration copy.Tests
src/lib/url.test.ts:safeRedirect(including the three bypasses from the issue),resolvePostLoginRedirect,safeExternalUrl(javascript:,data:, protocol-relative, off-host),redactUrl.src/lib/analytics.test.ts: the tests that previously documented the lowercase-address and nested-PII gaps now assert they are stripped; added muxed-address, nested-email and token-payload cases.Note on scope
Some files in this area carried pre-existing syntax corruption on
main(duplicate imports/identifiers inuseUiStore,TransactionStatusPolling,WalletBalanceDisplay, a missinguseQueryimport inOrderConfirmationScreen, a duplicatedAuthProviderimport inmain.tsx). Those files are touched by this PR, so the corruption was repaired as part of applying the fix. Corrupt files outside this scope were left untouched.Closes #862
Closes #863
Closes #864
Closes #865