fce834e8 - Report an expired Taro LNURL login as "LNURL login expired" - #1480
Conversation
|
@TaprootFreak @davidleomay could please add the label in this PR so the CI runs please? |
|
EN: DE: DetailsThis PR comes from a fork (
Once CI is green, the PR will be marked ready for review. |
|
EN: DE: DetailsStatus
Findings by round and how they were resolved First stage
Second stage
Local verification (Node 20): lint ✅, 145 suites / 2228 tests ✅, |
|
EN: DE: DetailsHead: Review passes (quality and logic lanes in parallel per pass; per-round findings and resolutions are listed in the review-status comment above)
CI at this head (runs started by the
Comments
Mergeability
Still for the code-owner reviewer
|
|
dfx pr guard EN: Thanks for your contribution! This repository follows A38. A38 pass: author local-CI report accepted for this head. DE: Danke für deinen Beitrag! In diesem Repository gilt A38. A38 pass: Autor-Local-CI-Report für diesen Head akzeptiert. Details
|
|
EN: DE: Details{
"base": "bfa7d994a010834936fe240dec6c1520942cd4f3",
"head": "68ba53faa0ef448461cdc7bb3a9e04795a51e767",
"phase": "applied",
"pr": 1480,
"reasons": [
"Required CI check not green: .github/workflows/pr.yml / Lint and Markdown format",
"CI status not green: A38 / report (develop) (failure)"
],
"repo": "DFXswiss/app",
"state": "draft"
} |
When the LNURL challenge is no longer known to the API (404 after its 5-minute TTL), the Taro login poller now rejects with a dedicated "LNURL login expired" message instead of "Authentication failed". Every other poll failure (network errors, 5xx, unexpected errors) still reports "Authentication failed".
Rename the poller test to connect-taro.test.tsx and extend it to the whole component so the touched file reaches 100 % statement, branch, function and line coverage: reconnect with a session address, the Taro link and Open app button, polling until the challenge completes, cleanup on unmount, and the connect error with its retry button.
The status poller fires every second without waiting for the previous request. A request still in flight when the login had already failed could reject after the user retried and reset the new challenge, which stopped the new poller and left the retry hanging. Responses that arrive after polling stopped (settled, restarted or unmounted) are now ignored.
The API deletes an LNURL challenge on the first completed status read, so an overlapping status request could fail with 404 and drop a login that had just succeeded. The poller now sends the next request only after the previous one has settled. A 404 also follows a wallet callback the API refused (for example a blocked IP country), which can happen long before the challenge expires. 'LNURL login expired' is therefore reported only when the challenge is at least 5 minutes old, the API's challenge lifetime; an earlier 404 stays 'Authentication failed'. The test types now use Blockchain and LnurlAuthStatus instead of plain string and object.
The API removes a challenge in a cleanup job once it is older than 5 minutes, not exactly 5 minutes after creating it.
68ba53f to
e91d12b
Compare
|
EN: DE: Details
Durations rounded up to whole seconds / Laufzeiten auf ganze Sekunden aufgerundet. Original report / Originalbericht{
"head": "e91d12b6f7f2bf6412e9123ee1e1e6e25b77bec0",
"private": false,
"recorded_at": "2026-09-14T17:14:52Z",
"repo": "DFXswiss/app",
"required": [
"lint"
],
"runs": [
{
"command": "agent a38 job commands --config '{\"env\":{\"CI\":\"true\"},\"npm\":{\"canaries\":[\"react-app-rewired/package.json\",\"react-scripts/package.json\",\"typescript/lib/typescript.js\",\"prettier/package.json\"],\"node_major\":20},\"steps\":[[\"npm\",\"run\",\"lint\"],[\"npm\",\"run\",\"format:md:check\"]]}'",
"duration_s": 65.51911624986678,
"exit_code": 0,
"id": "lint",
"name": "Lint and Markdown format",
"result": "pass",
"timeout_s": 600.0
}
],
"schema": "dfx-local-ci/v1"
} |
|
EN: DE: Details{
"base": "904a5961bbde2e3b7687f77b1953e30c2fd790b8",
"head": "e91d12b6f7f2bf6412e9123ee1e1e6e25b77bec0",
"pr": 1480,
"repo": "DFXswiss/app",
"runs": [
{
"run_id": 34873605902,
"workflow": ".github/workflows/codeql.yml"
},
{
"run_id": 34873605986,
"workflow": ".github/workflows/pr-review-bot.yml"
},
{
"run_id": 34873606123,
"workflow": ".github/workflows/pr.yml"
},
{
"run_id": 34880894330,
"workflow": ".github/workflows/ci-on-ready.yaml"
}
]
} |
|
EN: DE: DetailsThis PR was converted back to Draft by the PR guard after
|
3 participants
EN:
When a Taro login challenge is no longer known to the API (404) after its 5-minute lifetime, the login now fails with "LNURL login expired" instead of "Authentication failed"; every other poll failure, including an earlier 404, still reports "Authentication failed". Client error reports can then tell a normal timeout apart from real login failures, apart from the edge cases listed in the details. The status poller now sends one request at a time and ignores responses that arrive after polling stopped, so a stale or overlapping response can no longer drop a successful or retried login. Declared deviations: no Playwright/handbook baseline and no full-stack E2E test for the Taro error state (see details).
DE:
Kennt die API eine Taro-Login-Challenge nach Ablauf ihrer 5-minütigen Gültigkeit nicht mehr (404), schlägt der Login jetzt mit "LNURL login expired" statt "Authentication failed" fehl; alle anderen Poll-Fehler, auch ein früherer 404, melden weiterhin "Authentication failed". Damit lässt sich ein normaler Timeout in den Client-Fehlerberichten von echten Login-Fehlern unterscheiden, bis auf die in den Details genannten Grenzfälle. Der Status-Poller sendet jetzt nur noch eine Anfrage zur Zeit und ignoriert Antworten nach dem Ende des Pollings, sodass eine veraltete oder überlappende Antwort keinen erfolgreichen oder erneuten Login mehr verwirft. Deklarierte Abweichungen: keine Playwright-/Handbook-Baseline und kein Full-Stack-E2E-Test für den Taro-Fehlerzustand (siehe Details).
Details
Problem
ConnectTaropolledgetLnurlAuth(k1)withsetIntervalevery second, and its.catch()turned every rejection intonew Error('Authentication failed'). The API's cleanup job, which runs every 5 minutes, removes LNURL challenges older than 5 minutes. After that the status call returns 404, so the user sees (and the app reports) "Authentication failed" even though the login simply timed out.Change (
src/components/home/wallet/connect-taro.tsx)'LNURL login expired'only when it is anApiExceptionwithstatusCode === 404and the challenge is at leastLNURL_CHALLENGE_LIFETIME_MS(5 minutes) old. Challenge age is measured from when polling for that challenge started.'Authentication failed'.LNURL_CHALLENGE_LIFETIME_MSmirrors the API's challenge lifetime and has to be kept in sync by hand.'Authentication failed'. An explicit reason from the status endpoint would remove both edge cases.'Authentication failed':ApiExceptionwith status 0 (network), 5xx, other statuses, non-ApiExceptionerrors.ApiExceptionis imported from@dfx.swiss/react. It is the same class the SDK's API hook throws, withstatusCodetaken from the HTTP response.'LNURL login expired'is a new string used only for this flow. It deliberately does not reuse the existing "Login link expired" message, which belongs to the email magic-link sign-in. Like the previous'Authentication failed', it has no entry insrc/translations/languages/*.json.Fix: polling races (pre-existing, found in review of this PR)
setIntervalfired every second without waiting for the previous request. The API deletes the challenge on the first completed status read, so a concurrent request got a 404. If that 404 arrived before the success response, a login that had just succeeded was rejected. Polling is now serial: the next request is scheduled only after the previous one has settled.Reported pre-existing defect outside this repository
The API deletes a completed login's token roughly 30–75 s after the wallet approved it (a 30-second cleanup job plus its start jitter), and deletes the challenge on the first completed status read. If the app only polls again after that window (for example because the browser stayed in the background while the user approved in the Taro app), the status read fails with 404 although the login succeeded. It then shows
'Authentication failed'(or'LNURL login expired'once the challenge is 5 minutes old). This repository cannot fix it, because the token is already gone server-side; it is reported to the API maintainers with the fix direction (keep completed entries longer and make the completed status read idempotent).Related change in the API
This PR changes which message a timed-out Taro login reports. Classifying that message at a lower severity in client error reports is part of the API's error ingestion and ships as a separate change there. It needs nothing further from this repository.
Tests (
src/__tests__/connect-taro.test.tsx, new)ApiExceptiononce the challenge is exactly 5 minutes old →LNURL login expired, and the QR link disappearsApiException1 ms before that →Authentication failedApiException0 / 500, a plainError, and a non-ApiExceptionobject withstatusCode: 404(all after the lifetime) →Authentication failedsetIntervaleach makes at least one test fail.@dfx.swiss/react's ESM build is not loadable under this Jest setup (other tests mock the package too), so the test mock defines a minimalApiExceptionwith the same shape.Coverage (per touched file,
npm run test -- --coverage --collectCoverageFrom='src/components/home/wallet/connect-taro.tsx')src/components/home/wallet/connect-taro.tsxDeclared deviation: Handbook coverage (CONTRIBUTING.md → Handbook)
This PR changes the error text of one Taro-connect error state (the expired challenge) and adds no Playwright baseline or
scripts/handbook/metadata.jsonentry for it. No committed baseline shows that screen state: the existing/connectspec ine2e/home.spec.tsonly checks that the page body is visible and takes no screenshot. Adding one would need a spec (new, or an extension of that one) that fakes the LNURL endpoints and waits past the challenge lifetime, plus a matching reality-declaration entry. That is far more than this change, whose behavior is fully covered by the unit test above. The layout is unchanged; only the message text differs. The reviewer decides whether to accept this deviation.Declared deviation: Full-stack E2E tests (CONTRIBUTING.md → Full-stack E2E tests)
This PR changes what one screen state shows and adds no case to
e2e-stack/. No full-stack spec covers the Taro/LNURL login today: the two/connectcases ine2e-stack/specs/auth.spec.ts(logged out, and with a signature session) check the route, the wallet grid and that no address prompt appears, but never select Taro or run an LNURL login. Reaching the expired state needs one of two things. Either a real wait of more than 5 minutes followed by a signed LNURL login callback, which the API refuses as expired and then removes the challenge. Or time control on both the API and the browser.e2e-stack/has neither an LNURL login signing helper (its signature helpers sign EVM messages) nor time control, and it runs the API withDISABLED_PROCESSES=*, which switches off process-gated cron jobs (docs/test-architecture.md, Known gaps). A case would therefore add a new LNURL signing helper plus more than 5 minutes of idle waiting to every full run, or new time control on both sides, which is well beyond this change. The unit test above covers the client-side behavior fully; what it does not prove is that the real API answers an expired challenge with HTTP 404 end to end. The reviewer decides whether to accept this deviation.Reality declaration
No e2e/full-stack fake is added, removed or changed (only Jest module mocks in a unit test), so
docs/test-architecture.mdis unchanged.Verification (local, Node 20)
npm run lint✅, Prettier check on changed files ✅CI=true npm test -- --watchAll=false: 145 suites / 2228 tests passed ✅npm run build:dev✅ (only the existing source-map warnings fromnode_modules),npm run widget:dev✅