128 lines
7.2 KiB
Markdown
128 lines
7.2 KiB
Markdown
# 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.
|
|
|
|
## Authorized merge — 2026-09-30
|
|
|
|
The operator explicitly requested normal merged/closed PR status after review.
|
|
Re-read both PRs and verified their heads still exactly matched the reviewed
|
|
commits. Changes from the integration-test base to main were documentation only.
|
|
Gitea normal merges completed and read-back confirmed `merged=true`, `state=closed`:
|
|
|
|
- #161: `3daea6623be3e2c7222101b8e6ac411423c7e16c`.
|
|
- #162: `b02ba4100d922dd1b75c6a78121ef446c2159a54`.
|
|
|
|
Local next-release lifecycle work will be integrated with this main before the
|
|
next release. Funded release acceptance and the documented delivery-receipt
|
|
limitation remain as recorded above; merging does not claim a new release.
|