Keep the deep link from the mail across the login redirect - #1267
Keep the deep link from the mail across the login redirect#1267joshuakrueger-dfx wants to merge 8 commits into
Conversation
The mail that asks a customer for a verification call links to /settings?a=call, and that parameter is what scrolls the page to the section. A customer who is not logged in when they click it never gets there: the guard sends them to the login and memorizes only the path, so after signing in they land on the settings page with the anchor gone and have to find the section themselves. Two support tickets describe exactly that, and in both accounts the call settings were still untouched at the time of writing. Memorize the query alongside the path. An explicitly passed redirectPath stays as it is. Credentials and personal data are already stripped from the URL before the guard runs, so they are not carried along - measured, not assumed.
The stored return path is handed to outbound links: the mail login builds its redirectUri from it and Alby its redirect. Both filter the live search for that reason - the comment in connect-mail says so - but neither filters what the stored path already carries. Keeping the whole query there would have routed around that: a KYC access hash sits in the URL as code=, is deliberately not stripped on KYC routes, and the 2FA step stores its return path from exactly such a page. It would have gone out in a mail to a freely typed address. Store an allowlisted set instead, currently just the mail anchor. The tests now pin the discarded hash and the navigation target itself, which two mutations had survived.
|
Two commits here: the redirect fix itself, and an allowlist added after review. Carrying the full query into the stored return path would have routed around the filtering that |
`a` is the mail section anchor here, but payment-link routes read the same key as the `amount` shorthand (payment-link.context.tsx). Those routes are unguarded and never reach setRedirect, so the two uses do not collide today. The allowlist comment is what the next person reads before adding a key, and it did not mention the collision. Comment only, no behaviour change.
Review — full runMeasured locally on The diagnosis and the fix are right, and the security framing is not theoretical — Three things before merge. 1. Coverage on the touched files
The PR's own additions are covered — 2. The handbook gate cannot fire, and the new e2e spec has no baselineThe PR adds A deep link surviving login is a user-visible flow with two distinct states worth a baseline (mail link before login, the same anchor after the login redirect). Both belong in the handbook. 3. Two allowlists now guard the same propertyAfter this PR, what may appear in an outbound redirect is governed by two independent lists in two files: Not a defect today. But the next person adding a third case has no single place to look, and the failure mode is a silent leak rather than a broken test. One shared allowlist with per-caller subsets would make the property checkable in one spot. One clarification for the descriptionThe leak is closed for the remembered redirect path, not for navigation in general: Hard requirements
Verified and goodThe double-booking of |
redirectAllowedParams (utils.ts) and personalIbanOnlyParams (personal-iban.ts) encoded the same security property — what may appear in an outbound redirect URI — in two independent lists, kept consistent by convention alone. A mistake there is a silent leak, not a failing test. Both now delegate to allowedParamsOnly in the new src/util/redirect-params.ts, which holds the generic filter and the two named subsets with the reason each key is in them. Their signatures and callers are unchanged.
CONTRIBUTING requires every file a pull request touches to reach 100 % statement, branch, function and line coverage. navigation.hook.ts stood at 45.83 % and utils.ts at 54.75 %; personal-iban.ts was missing one branch. navigation-hook.test.ts covers the parts of useNavigation outside setRedirect: numeric targets, the object branch with clearParams, setParams, clearParams and the /account default in goBack. utils.test.ts covers the remaining exports, including the file, PDF and image helpers, downloadFile, the formatCurrency branches and findCustodyBalanceString. All four files now report 100 % in every column.
The spec had no toHaveScreenshot at all, so the flow was absent from the handbook and handbook-check.yaml could not fire on it either: its path filter covers e2e/screenshots/** and not e2e/**, so a new spec file alone matches nothing. Two baselines, one per visual state a mail recipient goes through — the login the guard sends them to, and the settings screen with the Verification Call section the anchor scrolls to. The second opens settings with ?session= via getCachedAuth, the pattern login-process.spec.ts already uses; it shows the target UI, not the post-login hop, and the spec header says so. Handbook build with both files present: 180 screenshots, no orphan warning, title and description resolved from the new metadata.json entry.
…survives-login # Conflicts: # src/__tests__/personal-iban.test.ts
|
@marassteiner please review This is a PR, not an issue: 13 files against |
2 participants
Symptom (verbatim): "Ja, ich hab eine Nachricht zum Telefonat erhalten, allerdings führt mich der Link nicht zur Termin Zustimmung." (ticket 8700, UserData 400112) and "Ich habe die Info von Ihnen erhalten, dass sie Telefongespräch mit mir wünschen. Ich kann jedoch keine Möglichkeit auf der Internetseite finden einen Termin zu vereinbaren." (ticket 8774, UserData 415448)
Scale: More than ten support tickets in seven days with this pattern; in both accounts above
phoneCallAcceptedandphoneCallTimeswere still empty at the time of writing, so neither customer ever reached the section. Every mail recipient who is not already logged in when clicking the link is affected.Smaller fix considered: Drop the
?a=callanchor from the mail and send customers to the settings page without a jump target — insufficient because then they have to find the section themselves, which is precisely the reported symptom.What this fixes, and what it does not
This fixes the memorized login-return path. It does not change navigation in general.
navigation.hook.ts:36still merges the entire currentsearchinto every stringnavigation, and this pull request leaves that alone. So "the query parameter leak is
closed" would be too broad a claim: what is closed is that the stored return path — the
one
connect-mail.tsxandconnect-alby.tsxturn into an outboundredirectUri— nowcarries only allowlisted keys. A reader should not infer more than that from the title.
Problem
The mail that asks a customer for a verification call links to
https://app.dfx.swiss/settings?a=call. That parameter is what makes the settings page scroll tothe "Verification Call" section (
src/hooks/anchor.hook.ts).A customer who is not logged in when they click it never arrives there. Measured against
develop,phone viewport, no session — exactly how a mail recipient opens the link:
useUserGuardsends them to the login and memorizes the return target aspathnameonly(
navigation.hook.ts:25as it stands ondevelop). After signing in,home.screen.tsx:171navigates to that memorizedpath —
/settings, withouta=call. No scroll, no section: the customer lands at the top of along settings page and has to find it.
Two support tickets describe this, and the account state at the time of writing confirms neither
customer ever got there —
phoneCallAcceptedandphoneCallTimeswere both empty:jedoch keine Möglichkeit auf der Internetseite finden einen Termin zu vereinbaren."
Termin Zustimmung."
Both accounts are KYC level 50, so this is not unfamiliarity with the app.
Change
Store the anchor along with the path when memorizing the return target — through an allowlist, so
only parameters that may reappear in an outbound link are kept. Currently that is
a.An explicitly passed
redirectPathis untouched. This fixes every deep link that goes through thelogin, not just this one.
Why the allowlist, and not the whole query
The stored return path is handed to outbound links:
connect-mail.tsxbuilds theredirectUrifrom it that
signInWithMailsends to the API and that ends up in the magic-link mail, andconnect-alby.tsxbuilds itsredirectthe same way. Both filter the live search for exactlythat reason — the comment there says "Do not copy the entire live search — that would forward
user=/arbitrary= into the magic link." — but neither filters what the stored path itself carries.
Copying the full query would have routed around that. A concrete carrier exists:
code, the KYCaccess hash. It is deliberately not in
urlParamsToRemoveand not stripped on KYC routes, andkyc.screen.tsx:263stores a return path from exactly such a page when 2FA is required. That hashwould then have been sent in a mail to a freely typed address. A test now pins that it is
discarded.
Credentials and personal data (
session,signature,pubkey,mail, name, address, phone) arein
urlParamsToRemoveand already gone before the guard runs — measured:The allowlist is the second layer: it holds even if that order ever changes.
One allowlist, not two
redirectAllowedParams(utils.ts) andpersonalIbanOnlyParams(personal-iban.ts) encoded thesame security property in two independent lists, kept consistent by convention alone — and the
failure mode there is a silent leak, not a failing test. Both now delegate to
allowedParamsOnlyin the new
src/util/redirect-params.ts, which holds the generic filter plus the two namedsubsets with the reason each key is in them. Signatures and call sites are unchanged.
Coverage
Every file this pull request touches, measured per file with
npm run test -- --coverage --collectCoverageFrom=…:src/hooks/navigation.hook.tssrc/util/utils.tssrc/util/personal-iban.tssrc/util/redirect-params.tsFull suite: 76 suites / 865 tests green (was 74 / 799 before this work). Lint: empty output.
Verification
redirectPathis notmodified, a location without a query stores a bare path (no dangling
?), the KYC hash isdiscarded, mixed params keep only what is allowed, the navigation target itself is asserted, and
goBackuses the stored path. Against the previous code: 1 of 3 red (before the allowlist testsexisted).
fix. The coverage work was checked by mutating, not by deleting: the allowlist silently extended
by
code(5 tests fall, including the KYC-hash one), the filter inverted (15 fall), theobject-merge collision rule reversed so the live query wins (exactly 1 falls),
downloadFileignoring
content-disposition,blankedAddresstruncating at full instead of half length —from the head and from the tail separately —,
formatUnitsstripping leading instead of trailingzeros,
handleOpenFileflippingnewTab,findCustodyBalanceStringinverting its match,formatBytesoff by one unit,toBase64reading as text,formatLocationAddresschangingseparator,
extractFilenamekeeping quotes.Two of the thirteen survived the first round — the
downloadFileandblankedAddresstestsasserted around the value instead of on it, so they held 100 % coverage while proving nothing.
Both tests now assert the value itself and both mutations fall.
e2e/mail-deeplink-call-anchor.spec.tsruns green against the dev API: the unauthenticated hopasserts the anchor reaches the login's
redirectUripayload, the authenticated state openssettings with
?session=and requires the Verification Call section to be visible.pr.ymlruns:npm run lint(emptyoutput),
npm run test,npm run build:dev,npm run widget:dev, all green.Handbook
e2e/screenshots/baseline/gains the two states a mail recipient goes through — the login theguard sends them to, and the settings screen with the Verification Call section — plus the
mail-deeplink-call-anchorentry inscripts/handbook/metadata.json. The build was run with bothpresent: 180 screenshots, no orphan warning, title and description resolved from the new entry.
The gate could not have fired on its own.
handbook-check.yamlfilters one2e/screenshots/**,not
e2e/**, so adding a spec file with no baseline matches nothing and the check never runs —the same structural gap raised on #1262, #1274 and #1275. It fires on this pull request only
because the baselines themselves are under the filtered path. Widening the filter to
e2e/**wouldclose it, at the cost of running the 25-minute image build on every e2e change; that is a repo-wide
call and not made here.
Not in this PR
navigation.hook.ts:36— every string navigation stillcarries the whole live search to its target. That is the mechanism behind finding 2 on Label the invoice screen for the payer when opened with ?pay #1274.
Scoping this pull request to
setRedirectwas confirmed as defensible in review; the descriptionabove now says which half is closed.
consent, as intended.
phoneCallStatusisCompletedorFailedstill do not see the section atall (
settings.screen.tsx:301). That is existing, intended behaviour and unrelated to thisredirect. Every other status — including
Suspicious,Repeat,UserRejected,ManualCheckand
Unavailable— does render the section.Final pass (049d199):
Coherent: One title, three parts that all hang off it — the fix that keeps the anchor across the login, the coverage
CONTRIBUTINGrequires for every file the fix touches, and the handbook representation it requires for every screen the fix affects. No half of a change is missing: the new allowlist source has its own test file, each new baseline has itsmetadata.jsonentry, and the spec header states what the second baseline does and does not prove.Nothing extra: The smallest change that removes the symptom is the one line in
navigation.hook.ts:28. Of the rest, 86 % is an explicit requirement — the coverage the review demanded andCONTRIBUTINGcodifies, the handbook baselines, and the tests that prove the fix. The remaining 14 % is the shared allowlist and its tests (148 lines), and that one was a recommendation rather than a hard requirement: the review called it "not a defect today". It is here because a second, silently diverging copy of a security property is the kind of finding that returns, and because deferring it would need the written exception this repository requires for any follow-up — say the word and it moves to its own pull request instead. Deliberately not built: the general query merge innavigation.hook.ts:36(a separate defect, named above and left to its own pull request), widening thehandbook-check.yamlpath filter toe2e/**(repo-wide call, costs a 25-minute image build on every e2e change), and any third screenshot state beyond the two the flow actually has.Sources closed: Review comment 5216499479 — all four hard requirements met (100 % coverage on the touched files, handbook baselines plus metadata entry, description scoped to what is fixed, everything in this pull request), plus its third point, the two allowlists merged into one source. Nothing declined. All three channels re-read at this revision:
pulls/1267/reviews(0),pulls/1267/comments(0),issues/1267/comments(2, both read). No issue is closed by this pull request — the source of the symptom is support tickets 8700 and 8774; #1262, #1274 and #1275 appear above only as the other pull requests where the same handbook filter gap was raised, and nothing is owed to them here. Each commit message checked against its own diff, including1798586("Comment only, no behaviour change" — confirmed, the diff touches nothing but the doc block). That comment moved intoredirect-params.tswith the allowlist merge; all of its substance survived the move verbatim — the double-booking ofa, theamountshorthand inpayment-link.context.tsx, that those routes are unguarded and never reachsetRedirect, the instruction to check both uses before adding a key, and the warning against copying the entire live search.