Backups: a shipped backup can be read back, retention stops deleting fresh dumps, and remotes are redacted properly (#753) #790

Merged

claude-bot merged 1 commit from build/753-backup-readback into main 2026-09-23 23:19:53 +00:00

Conversation 0 Commits 1 Files changed 14 +759 -40

claude-bot commented 2026-09-23 22:39:47 +00:00

Collaborator

Copy link

Closes #753. Owner, 2026-09-23 (B10): fix it before release.

Read the second section first. #753 was the bug asked about, and fixing it turned up two worse ones.

1. #753: a shipped backup couldn't be read back

_run_backup_async set destination = str(dest_path), then overwrote it with ship_to_remote(...), which returns the redacted remote spec. The dump was still on disk, but nothing recorded its name. The one reader that opens that value as a file, POST /api/admin/backup/verify, which the setup wizard's "read it back" step (D87) uses, always answered "No local copy" on an instance with off-host backups.

There was a piece #753 didn't mention: when shipping or the audio snapshot failed, destination was never written at all. So a failed row didn't say where its perfectly good dump was either.

Fix:

• destination now only ever holds the local path, written as soon as pg_dump succeeds. A new backup_logs.remote_destination holds the redacted remote, written only when shipping succeeds.
• Admin's backups table shows both. Verify reads the local path, keeps its containment check under the backup root, and never puts the remote spec in detail.

Migration d0e1f2a3b4c5, revising b8c9d0e1f2a3; it's the new head:

• Upgrade moves any destination that doesn't start with / into remote_destination. That test is exact, not a guess: every local path the task ever wrote is under the resolved backup root, and every remote passed validate_remote, which arrived in the same commit as shipping and can't start with /.
• Existing shipped rows end up with no local path. It was never stored, and nothing is guessed from timestamps. Verify says so plainly.
• Downgrade restores the old shape.
• Run with real Alembic on a fresh postgres:16: upgrade, seed old-shape rows, upgrade to head, downgrade one step, upgrade again, with the rows checked after each step. A new test_backup_log_migration.py runs the real upgrade()/downgrade() against seeded rows and pins the head.

2. Two worse bugs, found on the way, both fixed here

Retention deleted the dump it had just written, and recorded success.

• The prune globbed questboard_*.dump, which also matches make backup-now's questboard_manual_<ts>.dump. Both land in the backup root by default. Because m sorts after every digit, each manual dump counted as the newest backup.
• So with as many manual dumps as the retention count (default 5, i.e. one per upgrade), every scheduled run deleted its own fresh dump and still reported success. With fewer, retention silently shrank. With a remote configured, it showed up as an rclone failure instead, because pruning happens before shipping.
• Fix: retention prunes only the exact names the task writes, ^questboard_\d{8}T\d{6}Z\.dump$ and the audio equivalent, and always keeps at least one. I checked the pattern against the scheduled filename format (%Y%m%dT%H%M%SZ), so pruning still works. docs/OPERATIONS.md now says manual dumps are never pruned.

Remote credentials leaked through redaction.

• redact_remote only handled the inline form :backend,params:path. A named remote with parameters, e.g. s3-backups,secret_access_key=…:bucket, is accepted by both validate_remote and rclone, and was passed through verbatim to the log row, the audit log and error text.
• The inline form was also split at the first colon, so a quoted endpoint="https://…" leaked every parameter after it.
• Fix: redact the whole parameter block in both forms, and take the path after the last colon. _redact_text covers both forms too.
• Against main's code, the API test's response body contained the test secret, so the leak was real.

Tests

• Backend:
  • A local-only backup records a path that opens.
  • A shipped backup records both, and verify returns verified: true end to end.
  • A failed shipment still records the local path.
  • A pre-#753 remote-only row is handled without crashing, and a remote left in destination is never opened.
  • The logs API never returns an unredacted remote.
  • Three redaction tests, and the retention regression.
• With main's task and offsite code swapped back in, 8 of the new tests fail.
• Full backend suite: 4060 passed, 13 skipped (Docker, postgres:16, repo root mounted). Ruff 0.4.4 clean.
• Frontend: vitest 1929/1929, eslint 0 errors. Admin.test.jsx covers the table with both fields, remote only, and local only.

Left alone, for a decision later

• GET/PUT /api/admin/backup/config still returns the raw remote to the admin form, by design, since the form round-trips it. If the remote carries inline credentials, they reach the admin's browser. The alternative is write-only credentials, like the AI keys.
• _redact_text only scrubs the remote when rclone echoes it verbatim. If rclone prints the parameters split up or re-quoted, they'd get through.
• The row doesn't record the audio archive's path. It can be derived from the dump's timestamp, but a restore helper would have to know that.
• Retention deletes old local dumps while their success rows remain. Verify reports those honestly as possibly pruned, moved or deleted.

🤖 Generated with Claude Code

Closes #753. Owner, 2026-09-23 (B10): fix it before release. **Read the second section first.** #753 was the bug asked about, and fixing it turned up two worse ones. ## 1. #753: a shipped backup couldn't be read back `_run_backup_async` set `destination = str(dest_path)`, then overwrote it with `ship_to_remote(...)`, which returns the **redacted remote spec**. The dump was still on disk, but nothing recorded its name. The one reader that opens that value as a file, `POST /api/admin/backup/verify`, which the setup wizard's "read it back" step (D87) uses, always answered "No local copy" on an instance with off-host backups. There was a piece #753 didn't mention: when shipping or the audio snapshot **failed**, `destination` was never written at all. So a failed row didn't say where its perfectly good dump was either. **Fix:** - `destination` now only ever holds the local path, written as soon as `pg_dump` succeeds. A new `backup_logs.remote_destination` holds the redacted remote, written only when shipping succeeds. - Admin's backups table shows both. Verify reads the local path, keeps its containment check under the backup root, and never puts the remote spec in `detail`. **Migration** `d0e1f2a3b4c5`, revising `b8c9d0e1f2a3`; it's the new head: - **Upgrade** moves any `destination` that doesn't start with `/` into `remote_destination`. That test is exact, not a guess: every local path the task ever wrote is under the resolved backup root, and every remote passed `validate_remote`, which arrived in the same commit as shipping and can't start with `/`. - **Existing shipped rows** end up with no local path. It was never stored, and nothing is guessed from timestamps. Verify says so plainly. - **Downgrade** restores the old shape. - Run with real Alembic on a fresh postgres:16: upgrade, seed old-shape rows, upgrade to head, downgrade one step, upgrade again, with the rows checked after each step. A new `test_backup_log_migration.py` runs the real `upgrade()`/`downgrade()` against seeded rows and pins the head. ## 2. Two worse bugs, found on the way, both fixed here **Retention deleted the dump it had just written, and recorded success.** - The prune globbed `questboard_*.dump`, which also matches `make backup-now`'s `questboard_manual_<ts>.dump`. Both land in the backup root by default. Because `m` sorts after every digit, each manual dump counted as the **newest** backup. - So with as many manual dumps as the retention count (default 5, i.e. one per upgrade), **every scheduled run deleted its own fresh dump and still reported success**. With fewer, retention silently shrank. With a remote configured, it showed up as an rclone failure instead, because pruning happens before shipping. - **Fix:** retention prunes only the exact names the task writes, `^questboard_\d{8}T\d{6}Z\.dump$` and the audio equivalent, and always keeps at least one. I checked the pattern against the scheduled filename format (`%Y%m%dT%H%M%SZ`), so pruning still works. `docs/OPERATIONS.md` now says manual dumps are never pruned. **Remote credentials leaked through redaction.** - `redact_remote` only handled the inline form `:backend,params:path`. A **named** remote with parameters, e.g. `s3-backups,secret_access_key=…:bucket`, is accepted by both `validate_remote` and rclone, and was passed through **verbatim** to the log row, the audit log and error text. - The inline form was also split at the *first* colon, so a quoted `endpoint="https://…"` leaked every parameter after it. - **Fix:** redact the whole parameter block in both forms, and take the path after the **last** colon. `_redact_text` covers both forms too. - Against main's code, the API test's response body contained the test secret, so the leak was real. ## Tests - **Backend:** - A local-only backup records a path that opens. - A shipped backup records both, and verify returns `verified: true` end to end. - A failed shipment still records the local path. - A pre-#753 remote-only row is handled without crashing, and a remote left in `destination` is never opened. - The logs API never returns an unredacted remote. - Three redaction tests, and the retention regression. - **With main's task and offsite code swapped back in, 8 of the new tests fail.** - **Full backend suite:** 4060 passed, 13 skipped (Docker, postgres:16, repo root mounted). Ruff 0.4.4 clean. - **Frontend:** vitest 1929/1929, eslint 0 errors. `Admin.test.jsx` covers the table with both fields, remote only, and local only. ## Left alone, for a decision later - **`GET`/`PUT /api/admin/backup/config`** still returns the raw `remote` to the admin form, by design, since the form round-trips it. If the remote carries inline credentials, they reach the admin's browser. The alternative is write-only credentials, like the AI keys. - **`_redact_text`** only scrubs the remote when rclone echoes it verbatim. If rclone prints the parameters split up or re-quoted, they'd get through. - **The row doesn't record the audio archive's path.** It can be derived from the dump's timestamp, but a restore helper would have to know that. - **Retention deletes old local dumps while their `success` rows remain.** Verify reports those honestly as possibly pruned, moved or deleted. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-09-23 22:39:47 +00:00

fix(backend): a shipped backup keeps its local path, and can be read back (#753) ... f77e50ccac

backup_logs.destination carried two things. A local-only backup recorded
the dump's path; a backup shipped to an rclone remote (#414) wrote the
redacted remote spec over it, so the dump's filename was lost and the
read-back (POST /api/admin/backup/verify, used by the setup wizard and
Admin) reported "No local copy" for exactly the servers that had set up
an off-host copy.

Two facts, two columns:

- destination is only ever the dump's path on this server, recorded the
  moment pg_dump succeeds, so a run that fails afterwards (audio snapshot,
  shipping) still says where its dump is.
- remote_destination (new) is where it was shipped, as ship_to_remote
  returns it: redacted, shown to a person, never opened.

Migration d0e1f2a3b4c5 adds the column and moves every existing
destination that is not an absolute path into it, leaving destination
NULL. That test is exact rather than heuristic: every local path the task
wrote is under the resolved backup root, and every remote went through
validate_remote, which came in with shipping and cannot begin with "/".
Those rows' local paths were never stored and are not guessed at; the
read-back says no local copy was recorded and that the off-host copy was
not checked. Downgrade drops the column, giving a migrated row its remote
back and keeping a newer row's local path (the older code opens it).

GET /api/admin/backup/logs returns remote_destination beside destination
(additive), and Admin -> Backup shows both.

Two more ways a backup could not be restored, found on the way:

- Retention counted make backup-now's questboard_manual_*.dump files,
  which sort after every digit, as the newest backups. With as many of
  them as the retention count (5 by default), every scheduled run deleted
  the dump it had just written and recorded success. Retention now
  prunes only names the schedule writes, and never a count below 1.
- redact_remote passed a named remote with connection-string parameters
  (s3-backups,secret_access_key=...:bucket) through verbatim, and split
  an inline one at its first colon, so a quoted endpoint="https://..."
  leaked every parameter after it. Both now lose the whole parameter
  block, in the stored row, the audit log and rclone's error text.

The "one head" pin moves from test_notifications to the new migration's
test, per the convention there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude-bot merged commit 3d29db52da into main 2026-09-23 23:19:53 +00:00

claude-bot referenced this pull request from a commit 2026-09-23 23:19:55 +00:00

Merge PR #790: backups — a shipped backup can be read back, retention stops deleting fresh dumps, remotes redacted properly (#753)

claude-bot referenced this pull request 2026-09-23 23:20:01 +00:00

[Backend] A backup shipped off-host cannot be read back: its local path is overwritten by the rclone spec #753

rbrooks referenced this pull request from a commit 2026-09-23 23:56:11 +00:00

Merge main into build/retention-preview (#778)

claude-bot referenced this pull request 2026-09-24 01:57:16 +00:00

Backups: write-only off-site credentials, redaction that doesn't depend on rclone's wording, and the audio archive's path #795

claude-bot referenced this pull request 2026-09-24 01:57:27 +00:00

Build lane 9: Admin (ADMIN-1 to ADMIN-15) #735

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

backend

Issue related to the backend

  

bot

Issue related to the bot

  

bug

Something is not working

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

frontend

Issue related to the frontend

  

help wanted

Need some help

  

invalid

Something is wrong

  

question

More information is needed

  

recording

Issue related to the recording system

  

security

Security hardening / vulnerability work

  

wontfix

This won't be fixed

No labels

backend

bot

bug

duplicate

enhancement

frontend

help wanted

invalid

question

recording

security

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rbrooks/Quest-Board!790

No description provided.