docs(security): document payjoin invariants and SECURITY.md maintenance rule - #2569

Claude finished @ethicnology's task in 6m 16s —— View job

Review — PR #2569: payjoin invariants in SECURITY.md

• Read the PR diff (SECURITY.md, AGENTS.md)
• Verify BIP-77 citations and quotes (blocked — see note below)
• Verify referenced code paths and expiry defaults (bull_payjoin)
• Verify pinned dependency versions against pubspec.lock
• Post review summary

Note: I can't submit formal inline review comments (GitHub App limitation), so findings are listed here with file/line references. I also couldn't fetch BIP-77 from this CI environment (WebFetch/curl/gh api to external repos are not in the allowed tools), so the verbatim quotes could not be machine-checked — see finding 3.

Findings

1. SECURITY.md:21 — "the receiver's mailbox has always expired too" is not always true (correctness)

The retries paragraph claims a retry is only possible after the sender session expires (default 24 h), "by which time the receiver's mailbox has always expired too — the retried original never reaches the receiver." The code does not support "always":

• The sender session lifetime is min(requestedExpiry, policy.sessionLifetime) (packages/bull_payjoin/lib/src/engine/payjoin_runtime.dart:511-525, applied to senders at line 251), and policy.sessionLifetime is user-configurable from 1 minute to 24 h via the advanced settings screen (lib/features/settings/ui/screens/bitcoin/payjoin_advanced_settings_screen.dart:128-144). A user who lowers it to, say, 1 h gets an expired-and-retryable sender session while an external receiver's mailbox can still be live.
• The pay/sell flows pass the remaining order-deadline window (lib/features/pay/presentation/pay_bloc.dart:607-625), which can likewise undershoot the receiver's mailbox lifetime.
• Nothing sender-side ever reads the URI's EX: expireAfterSec is purely local (packages/bull_payjoin/lib/src/engine/pdk_payjoin_datasource.dart:212 falls back to PayjoinConstants.defaultExpireAfterSec). So there is no code-level ordering between sender expiry and mailbox expiry.

The conclusion ("at most one original is ever processed") still holds, but via the single-request receiver state machine documented in bullet 2 — not via expiry ordering. Since this document exists precisely to triage future reports, the justification should be the one that actually holds. Suggested rewording: scope the expiry argument to the default configuration ("at the default 24 h lifetime, the mailbox — created before the sender session started — has expired by retry time") and lean on bullet 2 for the general guarantee, noting the lifetime is user-configurable down to 1 minute. Fix this →

2. SECURITY.md:16 — "the directory mailbox expires at the URI's EX timestamp" (accuracy)

EX is a URI fragment parameter. Fragments are never transmitted to any server, so the directory cannot see EX; mailbox lifetime is directory-side policy, and EX is the receiver's advertised expiry to the sender. The argument the bullet makes still works (the sender must treat the session as dead at EX), but a spec-citing security doc shouldn't state a directory behavior the directory can't implement. Suggest: "the receiver advertises the session expiry via the URI's EX fragment parameter, after which the sender treats the session as expired."

3. Verbatim BIP-77 quotes could not be machine-verified (process)

The section quotes seven phrases and six section anchors from BIP-77 as verbatim. This environment has no network access to fetch bitcoin/bips (WebFetch and curl are outside the allowed tools — adding WebFetch to --allowedTools would let me do this next time), so please have a human spot-check the quotes and anchors before merge. Given the new AGENTS.md rule this PR introduces ("every claim must cite an official source… careful review before merging"), the founding example should hold itself to that bar — a paraphrase presented as a verbatim quote would undercut the whole mechanism.

Verified correct

• Pinned versions: pubspec.lock:1503 has exactly payjoin 0.2.1+payjoin-1.0.0-rc.8, matching both links in SECURITY.md:13.
• Both cited file paths exist and say what the doc claims: retry is gated on isExpired && !isCompleted && !isAborted (payjoin_engine.dart:294-302), and the not-conflict-bound behavior matches the engine's own design comments (payjoin_engine.dart:316-318 and 405-411 — "old and new payments need not conflict, so both can confirm").
• The 24 h default is real (payjoin_policy.dart:7,41, payjoin_constants.dart:17).
• The residual-risk paragraph (SECURITY.md:23, re-issued URI) matches the engine's threat model, and the AGENTS.md rule is clear and consistent with the existing Security section's style.

Summary

Docs-only PR, no code regressions possible. The structure and intent are good; the two substantive issues are in the justifications: line 21's expiry-ordering claim ("always expired too") doesn't hold under the user-configurable session lifetime or order-deadline windows, and line 16 attributes EX enforcement to the directory, which never sees the fragment. Both are worth fixing before merge precisely because this document's purpose is to be the authoritative triage reference. The verbatim BIP-77 quotes need a human spot-check since I had no network access.