diff --git a/docs/TODO.md b/docs/TODO.md index 6e5d53fb..4538b94c 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -14,6 +14,17 @@ doc. See [`ROADMAP.md`](ROADMAP.md) for the curated, public-facing direction. ## Next release after 1.8.21 — reported 2026-09-30 +- [x] Review and repair open paid-download PRs #161 and #162, refresh both + branches from main, run independent and combined isolated suites, and verify + rootless file permissions in disposable scratch storage. Combined result: + 1,585 passed, zero failed, four existing tests ignored. See the + [review evidence and remaining acceptance work](pr-review-20260930.md). +- [ ] Integrate the reviewed PR branches into the next release and run funded + candidate acceptance, including Tor-only transport and payments with change. + PRs remain open; the reviewed code has not been deployed to live wallets. +- [ ] Design durable recovery for an accepted payment whose response is lost. + Preserve the truthful unconfirmed-refund warning and prevent automatic + duplicate payment while that recovery work is outstanding. - [ ] **ThinkPad X250 kiosk: Bitcoin installation version selector is unreadable and appears underneath the pruning information.** Operator reports white styling with invisible text on the actual kiosk; the same flow works in remote diff --git a/docs/pr-review-20260930.md b/docs/pr-review-20260930.md new file mode 100644 index 00000000..98bdbdb6 --- /dev/null +++ b/docs/pr-review-20260930.md @@ -0,0 +1,113 @@ +# Paid-download PR review — 2026-09-30 + +## Scope and result + +Reviewed both open PRs from the repository pull-request list: [#161](https://source.archipelago-foundation.org/lfg2025/archy/pulls/161) +and [#162](https://source.archipelago-foundation.org/lfg2025/archy/pulls/162). +Both branches were updated from main, repaired and tested independently and +together. Their existing remote branches were advanced without rewriting the +contributors' history. They remain open for integration into the release after +1.8.21; no reviewed code was merged into main or deployed to a live wallet. +The signed 1.8.21 artifacts are unchanged. + +| Candidate | Tested commit | Isolated backend result | +| --- | --- | --- | +| PR #161 | `971d4777` | 1,576 passed, 0 failed, 4 existing tests ignored | +| PR #162 | `0677924a` | 1,568 passed, 0 failed, 4 existing tests ignored | +| Both together | `4bf4bf1a` | 1,585 passed, 0 failed, 4 existing tests ignored | + +Both individual branches also passed production `cargo check`, with the +repository's existing 16 warnings. The combined merge required no conflict +resolution. Backend tests ran through `scripts/test-backend-isolated.sh` so they +could not access host wallets, native services or production container storage. + +## Findings and repairs + +### #161 — payment delivery and file readability + +- The branch conflicted with newer mint-fee, keyset-ID and truthful refund + reporting fixes. Preserve those implementations from main; do not reintroduce + its older unconditional “refunded” messages or duplicate keyset resolution. +- Opening a file before charging, then reopening/reading it afterward, still + permits a read failure after payment. Prepare the complete requested bytes + before redemption, including ranged reads. Tests delete or alter the backing + file during payment verification and still receive the prepared original data. +- Empty/out-of-bounds/reversed ranges could fail after redemption, and empty + files could underflow the range calculation. Validate ranges before charging + and return HTTP 416 when unsatisfiable. +- `chmod a+r` unnecessarily changed the permissions of shared paid/private + files. Read restricted FileBrowser files through the rootless namespace while + retaining their mode. Scope the fallback to regular files canonically inside + FileBrowser storage, and reject unauthorized peers before reading. +- A single-delivery flag must also prevent redirects and ambiguous transport + retries. Payment-bearing GET and POST requests now retain the first HTTP + response and do not retry after timeouts or disconnects that might follow + delivery. Refused connections and normal nonpayment browsing retain the + appropriate retry behavior. +- Interrupted response bodies now report the outcome using the actual local + refund result. Seller explanations are bounded, stripped of control + characters and explicitly identified as peer text. +- Original permission tests silently returned when run as root. Replacement + tests inject read/payment boundary failures, exercise them under the isolated + runner, and assert that read failures never invoke redemption. + +### #162 — saving purchases in Files + +- Its host-permission repair overlapped 1.8.21's authenticated Files API path. + Review of [FileBrowser v2.63.23's resource handler](https://github.com/filebrowser/filebrowser/blob/v2.63.23/http/resource.go) + showed that `override=false` checks for existence separately from opening + with truncation. It does not guarantee no overwrites under concurrent saves. +- The proposed direct path exposed the final filename before the write + completed. Both direct and namespace paths now finish a private temporary + file and publish it through a no-clobber hard link, retrying numbered names. +- Plain `ln` could place a temporary file inside an existing directory instead + of treating the destination as a collision. Use `ln -T`; existing directories + and dangling symlinks are conflicts, never replacement targets. +- Add filename and destination checks, unique temporary names, bounded name + retries, synchronization before publication, and exact input-length checks. + Truncated pipe input cannot become a completed purchased file. +- Files storage remains optional. An unavailable copy destination does not + undo the purchase or create a fake FileBrowser installation; the durable + purchased-content cache remains primary. + +## Additional verification on the development node + +Used disposable scratch directories only, then removed them: + +- Reproduced a FileBrowser-style rootless-owned 0640 upload. The host backend + UID could not read it. `podman unshare cat` returned identical bytes without + changing its 0640 mode. +- Ran the exact namespace writer script with four concurrent writers against + a directory owned by the container UID range. Every file had unique naming, + exact bytes, the expected owner and mode, and no remaining temporary file. +- Sent truncated input to the namespace writer and verified refusal, no final + file and temporary-file cleanup. + +The isolated tests additionally exercised 24 simultaneous direct writes, +existing-file preservation, symlink/directory conflicts, collision exhaustion, +root-independent permission failures, read-before-redemption ordering, +authorization, redirects and peer disconnects. + +Logs on the development box: +`/tmp/archy-pr161-tests.log`, `/tmp/archy-pr162-tests.log`, +`/tmp/archy-pr-integration-tests.log`, `/tmp/archy-pr161-check.log`, +`/tmp/archy-pr162-check.log`, `/tmp/archy-pr-userns-scratch-test.log`. + +## Next-release acceptance and limits + +- Integrate the reviewed branches and repeat the release gates against the + final release commit if additional code changes land. +- Perform funded peer-to-peer acceptance on the candidate build, including a + Tor-only purchase and a purchase requiring change, before the next release. + The new review branches were not deployed to funded live wallets here. +- These PRs do not implement durable payment receipts. If a seller redeems a + payment and the connection subsequently loses the response, the buyer may + receive an unconfirmed-refund warning. Do not represent that warning as proof + of a refund or automatically charge the buyer again. Receipt-based recovery + remains separate follow-up work. +- Abrupt process termination can leave a hidden namespace temporary file; + ordinary write failures and truncated input are tested to clean up. The final + filename is published only after complete input, and existing files remain + protected. +- The separately reported X250 kiosk version-selector rendering issue remains + open in `TODO.md` and requires validation on the actual kiosk.