Repository navigation
feat: encryption at rest — encrypted backups + encrypted-volume deploy posture (#67) - #69
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThis PR adds encrypted streaming backups, staged database/blob restores with rollback and maintenance recovery, admin APIs and UI, audit logging, Docker-secret deployment configuration, container hardening, and extensive integration and end-to-end coverage. Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Notion Comment |
Fold multi-persona review findings into the plan: stage-then-promote restore (KTD10) so R9 holds on all paths, BACKUP_FORMAT_VERSION gate (KTD4), zip-slip blob path validation (KTD11), dedicated operator_audit table (KTD6), crash-safe durable maintenance flag + pool-safe advisory lock (KTD5), and differentiated U7 UI states. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Adds src/backup/crypto.ts, a thin libsodium wrapper for the encrypted backup plan (KTD1): deriveKey() via Argon2id (crypto_pwhash, libsodium's OWASP-aligned MODERATE params) plus createEncryptStream/createDecryptStream over crypto_secretstream_xchacha20poly1305, chunked at 64 KiB. A fixed- length header (magic, version, salt, KDF params, secretstream header) precedes the ciphertext; writeHeader/readHeader (de)serialize it. Uses sodium-native (native, prebuilt via prebuildify - verified to load cleanly under Bun with no compile step) and adds it to package.json's trustedDependencies. Ships a small hand-written sodium-native.d.ts ambient declaration since the only published @types/sodium-native predates the installed v5.x line by several majors. 20 new tests cover round-trips (small/empty/multi-chunk/boundary-aligned/ unaligned writes), deriveKey determinism and salt/password sensitivity, header round-tripping and corruption handling, wrong-key decryption failure, and tamper detection (mid-stream and final-chunk byte flips, truncation) - all fail before any plaintext is produced. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Add the DB export/import primitives for the encryption-at-rest backup plan: exportDatabase()/importDatabase() stream the full database as NDJSON in FK-safe order, wipeDatabase() clears it in reverse order for a future force-replace restore, and table-order.ts enumerates every persistent table (ephemeral session/rate_limit/idempotency excluded) so new tables can't silently drop out of a backup. Also adds the operator_audit table (id, actor, action, outcome, at) to record backup export/restore events, wired into the schema barrel with its migration. Verified with a Testcontainers-backed round-trip integration test: full seed -> export -> wipe -> import reproduces every table's rows exactly, FK order lets firearm_document import right after firearm, ephemeral tables are excluded, and a regression guard compares table-order.ts against the live information_schema. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…nts (U8) Move the Postgres password and Better Auth signing secret out of plain compose environment variables and into Docker secrets (files under ./secrets/, mounted read-only at /run/secrets/*), resolved into DATABASE_URL/BETTER_AUTH_SECRET at container start by a new docker-entrypoint.sh (R16). Apply baseline container hardening — read-only rootfs, explicit writable tmpfs, dropped capabilities, no-new-privileges — to the db, migrate, and app services (R18), and document the Postgres data and upload volumes as the attach points for an encrypted host disk (R17). Update setup.sh, README, CONTRIBUTING, and docs/deployment.md so the documented setup/dev flows create and read the new secret files instead of plaintext .env values. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Admin-only /backup screen: export (password + confirm, explicit
no-recovery warning, native <form> POST download so a GB-scale bundle
is never buffered client-side) and restore (file + password upload via
fetch(), refuse-unless-empty messaging, force-replace behind a
type-to-confirm "REPLACE ALL DATA" phrase, distinct copy per
discriminated outcome, and session invalidation + redirect to /login
on success since the users table was just replaced).
- app/(admin)/backup/{page,backup-panel,export-panel,restore-panel,constants}.tsx
- components/ui/confirm-dialog.tsx: add confirmDisabled for the
type-to-confirm guard
- app/api/admin/backup/export/route.ts: accept a form-encoded password
body (the real <form> submit) alongside the existing JSON contract
- app/(auth)/login/login-form.tsx: show "Instance restored — please
sign in" after a force-invalidated session redirect
- app/(app)/app-shell.tsx: add the admin "Backup" nav link
- next.config.ts: externalize sodium-native so Turbopack's build
tracing doesn't break its native addon's runtime path resolution
(ADDON_NOT_FOUND) — this was blocking every build, not just U7
- e2e/backup.spec.ts: export/restore flows, no-recovery warning,
refuse-unless-empty (real bundle), force-replace phrase gating,
mocked outcome messaging, and post-restore redirect
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Simplify: bufferDbExport tallies rows in-loop and returns a Buffer (drop a full-payload string split + round-trip); parallelize blob stats; mkdir staging dir once per dir. Restore UI: validate the route outcome against the copy map and fall back to the unexpected-error display instead of crashing on an unmodeled (bad_request/error) outcome. Review fixes: F2 empty-instance restore now wipes before promote inside its transaction so restoring onto an instance holding the bootstrap admin no longer collides on user.email; bound untrusted KDF params from the bundle header before deriveKey to prevent a pre-auth Argon2id resource blowup. Test: gating.test.ts self-provisions its documented seeded admin in beforeAll, fixing a pre-existing full-suite flake where a sibling test deleting the shared ambient-DB admin made these assertions order-dependent. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
backend
documentation
There was a problem hiding this comment.
Pull request overview
Implements an encryption-at-rest strategy via admin-only encrypted backup/export + restore (DB + blobs) and a hardened “encrypted-volume-ready” Docker deployment posture (Docker secrets, read-only rootfs, reduced caps), plus supporting UI and documentation.
Changes:
- Added streaming encrypted backup bundle format (manifest + NDJSON + blobs) with libsodium-based crypto, plus admin export/restore services and routes.
- Introduced
operator_audittable + logging for backup export/restore attempts, and comprehensive unit/integration/e2e coverage for the backup feature. - Updated deployment posture: Docker secrets, entrypoint secret resolution, container hardening, and operator docs for encrypted host volumes.
Reviewed changes
Copilot reviewed 47 out of 50 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/db/schema.ts | Re-exports operator audit schema. |
| src/db/operator-audit-schema.ts | Adds operator_audit table schema + constraint/index. |
| src/db/migrations/meta/_journal.json | Registers migration 0018. |
| src/db/migrations/0018_spotty_blonde_phantom.sql | Creates operator_audit table + index. |
| src/backup/table-order.ts | Defines export/wipe order + ephemeral exclusions. |
| src/backup/sodium-native.d.ts | Adds minimal ambient typings for sodium-native. |
| src/backup/restore-service.ts | Implements stage-then-promote restore flow + force rollback envelope. |
| src/backup/manifest.ts | Adds bundle manifest format + parsing/validation + versioning. |
| src/backup/maintenance.ts | Adds durable maintenance flag + pool-safe advisory lock helper. |
| src/backup/export-service.ts | Implements admin-gated streaming encrypted backup creation. |
| src/backup/db-import.ts | Adds NDJSON import + wipe helpers. |
| src/backup/db-export.ts | Adds NDJSON export for all persistent tables. |
| src/backup/bundle.ts | Implements tar bundle writer/reader with KTD11 safety checks. |
| src/backup/audit.ts | Adds operator audit logging helper. |
| src/backup/tests/routes.test.ts | Route-level backup export/restore tests + audit assertions. |
| src/backup/tests/export-service.test.ts | Export-service crypto/bundle round-trip + streaming behavior tests. |
| src/backup/tests/db-roundtrip.test.ts | Export/import round-trip + schema coverage regression guard. |
| src/backup/tests/crypto.test.ts | Crypto round-trip + tamper/wrong-key tests. |
| src/backup/tests/bundle.test.ts | Bundle format + zip-slip/symlink/bounds tests. |
| src/auth/tests/gating.test.ts | Fixes flake by self-seeding admin in suite. |
| setup.sh | Updates first-run checks for Docker-secret-based secrets. |
| secrets/README.md | Documents Docker secrets setup/rotation + local tooling usage. |
| README.md | Updates quickstart/dev docs for Docker secrets + links encryption ops doc. |
| package.json | Adds sodium-native + tar-stream dependencies and types. |
| next.config.ts | Excludes sodium-native from server bundling for runtime addon resolution. |
| e2e/backup.spec.ts | Adds admin backup UI e2e coverage (export + restore flows). |
| docs/operations/encryption-at-rest.md | Adds threat matrix + encrypted host volume guidance + backup caveats. |
| docs/deployment.md | Updates deployment docs for Docker secrets + encrypted volume posture. |
| Dockerfile | Adds docker-entrypoint.sh and sets ENTRYPOINT. |
| docker-entrypoint.sh | Resolves *_FILE secrets and assembles DATABASE_URL at startup. |
| docker-compose.yml | Adds Docker secrets + hardening + encrypted-volume-ready mounts. |
| CONTRIBUTING.md | Updates dev setup instructions for Docker secrets. |
| components/ui/confirm-dialog.tsx | Adds confirmDisabled to support type-to-confirm guards. |
| bun.lock | Locks new dependencies/types. |
| app/api/admin/backup/restore/route.ts | Adds streamed admin restore route + outcome mapping + auditing. |
| app/api/admin/backup/export/route.ts | Adds admin export route supporting form submit + auditing. |
| app/(auth)/login/login-form.tsx | Adds “restored” login callout message. |
| app/(app)/app-shell.tsx | Adds admin nav link to Backup screen. |
| app/(admin)/backup/restore-panel.tsx | Adds restore UI + force-replace confirm + session invalidation. |
| app/(admin)/backup/page.tsx | Adds admin backup page with defense-in-depth gate. |
| app/(admin)/backup/export-panel.tsx | Adds export UI with no-recovery warning + form-submit download flow. |
| app/(admin)/backup/constants.ts | Adds restore outcome copy + force phrase constant. |
| app/(admin)/backup/backup-panel.tsx | Composes export + restore panels. |
| .gitignore | Ignores secrets directory except README. |
| .env.example | Updates env template to reflect Docker-secret-based secrets. |
| .dockerignore | Excludes secrets from build context except README. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
128-144: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLoad
BETTER_AUTH_SECRETin the host-development flow.This flow creates and exports only the Postgres secret. Host-side
bun run devandseed-admindo not pass throughdocker-entrypoint.sh, sosecrets/better_auth_secret.txtis never loaded. Add its creation and explicit.env.local/export configuration, matchingCONTRIBUTING.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 128 - 144, The host-development setup instructions must also provision and load BETTER_AUTH_SECRET. Update the README flow around the Postgres secret, DATABASE_URL export, and bun run dev commands to create secrets/better_auth_secret.txt and explicitly configure or export BETTER_AUTH_SECRET for host-side dev and seed-admin, matching the established instructions in CONTRIBUTING.md.
🧹 Nitpick comments (2)
src/backup/export-service.ts (1)
80-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlob stat/read TOCTOU can break a mid-flight export.
sizeis captured bylistUploadBlobs(stat) but the content is opened later inblobEntriesFor(createReadStream).tar-streamrequires the streamed byte count to match the declaredsizeexactly, so a blob deleted (e.g. orphan sweep) or resized between listing and streaming produces an ENOENT / size-mismatch that destroys the encrypt stream and fails the download. Confirm uploads underactiveStorageRoot()are effectively immutable for the export window, or re-statat open time and skip/abort deterministically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backup/export-service.ts` around lines 80 - 91, Update blobEntriesFor to eliminate the stat/read race between listUploadBlobs and createReadStream: at stream-open time, re-stat each file under activeStorageRoot and verify it still exists with the expected size before yielding it. If the file is missing or resized, apply a deterministic skip or abort behavior that preserves tar-stream’s exact declared byte count and prevents the encryption stream from receiving inconsistent data.app/(admin)/backup/restore-panel.tsx (1)
112-132: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffNo abort/timeout on the restore
fetch— an indefinite hang looks identical to progress.
postRestorehas noAbortController/timeout; if the server stalls or the connection drops mid-stream during a force-replace,await fetch(...)can hang indefinitely with the UI stuck on "Applying changes…" and no way for the operator to cancel or know whether it's still working. This is adjacent to the already-tracked "no crash recovery for interrupted force-restore" finding, but is the client-side counterpart — no signal/escape hatch exists here regardless of what the server does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(admin)/backup/restore-panel.tsx around lines 112 - 132, Update the restore flow centered on runRestore and postRestore to use an AbortController with a finite timeout, passing its signal into the restore fetch. Clear the timeout when the request settles, and handle abort/timeout results by stopping the applying timer, returning the UI to a non-progress state, and surfacing a clear failure outcome so the operator is not left waiting indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/`(admin)/backup/export-panel.tsx:
- Around line 46-57: Keep the export action disabled for the entire page session
after a valid submission. Update the status gating in canExport and the onSubmit
flow around ASSUME_STARTED_MS so the transition to "started" does not re-enable
the button; preserve the existing optimistic status readout while preventing any
subsequent concurrent submission.
In `@app/`(admin)/backup/restore-panel.tsx:
- Around line 44-58: Update postRestore to safely transport non-Latin-1
passwords by percent-encoding the password before assigning X-Backup-Password,
then update the restore route handler to decode that header exactly once before
validation. Preserve the existing header-based transport and ensure ASCII
passwords retain their current behavior.
In `@app/api/admin/backup/export/route.ts`:
- Around line 56-72: Guard the success-path recordOperatorEvent call in POST
with the same non-blocking error handling used by the failure paths, such as a
swallowed catch, so audit-write failures cannot discard the successfully created
bundle or alter the export response.
- Around line 95-105: Update readPassword to reject passwords shorter than the
required minimum length before returning them for key derivation, while
preserving the existing type and empty-value validation. Synchronize the
corresponding UI and error-message copy with the same minimum-length
requirement.
In `@app/api/admin/backup/restore/route.ts`:
- Around line 79-89: Guard the success-path recordOperatorEvent call so an
audit-write failure cannot prevent the completed restore outcome from being
returned. Match the existing catch behavior used on the failure path, while
preserving the outcome JSON and status derived from outcome.kind and
OUTCOME_STATUS.
In `@CONTRIBUTING.md`:
- Line 44: Update the setup instructions around DATABASE_URL and
BETTER_AUTH_SECRET to avoid placing shell substitution in .env.local. Provide an
explicit shell export or file-generation command that evaluates
secrets/postgres_password.txt when constructing DATABASE_URL, and show a
concrete BETTER_AUTH_SECRET value rather than an unevaluated expression.
- Around line 38-41: Update the secret-generation commands in the setup
instructions so secrets are created only when their files are absent, preserving
existing PostgreSQL and authentication credentials on reruns. Apply this
behavior to both postgres_password.txt and better_auth_secret.txt, or add an
explicit warning and confirmation before overwriting them.
In `@docker-entrypoint.sh`:
- Around line 27-34: The DATABASE_URL assembly in the entrypoint interpolates
POSTGRES_PASSWORD without enforcing URL-safe characters. Update this block to
either percent-encode the password before constructing DATABASE_URL or validate
the documented hex-only password contract and fail fast on invalid characters;
ensure malformed passwords are never exported in the connection URL.
In `@README.md`:
- Around line 102-105: Update the README backup guidance near the
encryption-at-rest reference to clarify that pg_dump backs up only PostgreSQL
data and excludes uploaded document blobs. Direct operators to use the encrypted
application backup or separately back up the uploads volume so restores include
uploaded documents.
- Around line 55-59: Update the README secret-file creation commands near the
PostgreSQL and Better Auth secret generation steps to explicitly create both
files with owner-only permissions (0600), using a temporary umask or an
equivalent permission-setting approach. Apply the same change to the
corresponding repeated instructions, while preserving the existing secret
generation commands and filenames.
- Around line 128-131: Update the setup commands around
secrets/postgres_password.txt so the openssl generation runs only when the file
does not already exist. Preserve existing passwords across repeated setup passes
while keeping the DATABASE_URL and Docker startup commands unchanged.
In `@setup.sh`:
- Around line 74-86: The setup instructions and secret-file handling must
enforce owner-only permissions. In the secret creation flow around the displayed
Docker secret commands and the preflight checks, set umask 077 before generating
files, then validate existing secrets and reject or chmod them to owner-only
permissions before continuing; retain the existing non-empty-file checks.
In `@src/backup/__tests__/routes.test.ts`:
- Around line 181-185: Update the environment cleanup logic in the shown test
teardown to use delete process.env.DATABASE_URL when ORIGINAL_DATABASE_URL is
undefined, while preserving restoration of ORIGINAL_DATABASE_URL when it exists.
In `@src/backup/db-export.ts`:
- Around line 25-37: Update exportDatabase to execute the entire
EXPORT_TABLE_ORDER iteration and all selectAllRows calls within one REPEATABLE
READ or serializable transaction, using the transaction handle for every read
before producing the Readable stream; preserve the existing table and row export
order and output format.
In `@src/backup/db-import.ts`:
- Around line 66-90: Update the NDJSON import flow around createInterface and
the for-await loop to enforce a hard maximum line length before JSON.parse or
row processing. Fail immediately with a clear error when a line exceeds the
limit, while preserving existing handling for valid lines and blank lines.
In `@src/backup/restore-service.ts`:
- Around line 149-152: Update restore() so withRestoreAdvisoryLock() wraps the
entire restore flow from its beginning, before recreateStagingSchema() or
staging-directory creation. Move enterMaintenance() and its corresponding
exit/cleanup inside that lock, including the force path, so shared restore state
is accessed only while the advisory lock is held.
In `@src/db/migrations/0018_spotty_blonde_phantom.sql`:
- Line 7: Add src/db/migrations/ to the SQLFluff ignore configuration in
.sqlfluff, while preserving the existing PostgreSQL dialect setting, so
generated migration SQL is excluded from linting and does not block just
ci-check.
---
Outside diff comments:
In `@README.md`:
- Around line 128-144: The host-development setup instructions must also
provision and load BETTER_AUTH_SECRET. Update the README flow around the
Postgres secret, DATABASE_URL export, and bun run dev commands to create
secrets/better_auth_secret.txt and explicitly configure or export
BETTER_AUTH_SECRET for host-side dev and seed-admin, matching the established
instructions in CONTRIBUTING.md.
---
Nitpick comments:
In `@app/`(admin)/backup/restore-panel.tsx:
- Around line 112-132: Update the restore flow centered on runRestore and
postRestore to use an AbortController with a finite timeout, passing its signal
into the restore fetch. Clear the timeout when the request settles, and handle
abort/timeout results by stopping the applying timer, returning the UI to a
non-progress state, and surfacing a clear failure outcome so the operator is not
left waiting indefinitely.
In `@src/backup/export-service.ts`:
- Around line 80-91: Update blobEntriesFor to eliminate the stat/read race
between listUploadBlobs and createReadStream: at stream-open time, re-stat each
file under activeStorageRoot and verify it still exists with the expected size
before yielding it. If the file is missing or resized, apply a deterministic
skip or abort behavior that preserves tar-stream’s exact declared byte count and
prevents the encryption stream from receiving inconsistent data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c7ac0707-d58d-423a-8676-a308ce539ccc
⛔ Files ignored due to path filters (2)
.env.exampleis excluded by!.env*bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (48)
.dockerignore.gitignoreCONTRIBUTING.mdDockerfileREADME.mdapp/(admin)/backup/backup-panel.tsxapp/(admin)/backup/constants.tsapp/(admin)/backup/export-panel.tsxapp/(admin)/backup/page.tsxapp/(admin)/backup/restore-panel.tsxapp/(app)/app-shell.tsxapp/(auth)/login/login-form.tsxapp/api/admin/backup/export/route.tsapp/api/admin/backup/restore/route.tscomponents/ui/confirm-dialog.tsxdocker-compose.ymldocker-entrypoint.shdocs/deployment.mddocs/operations/encryption-at-rest.mddocs/plans/2026-07-12-001-feat-encryption-at-rest-backups-plan.mde2e/backup.spec.tsnext.config.tspackage.jsonsecrets/README.mdsetup.shsrc/auth/__tests__/gating.test.tssrc/backup/__tests__/bundle.test.tssrc/backup/__tests__/crypto.test.tssrc/backup/__tests__/db-roundtrip.test.tssrc/backup/__tests__/export-service.test.tssrc/backup/__tests__/restore-service.test.tssrc/backup/__tests__/routes.test.tssrc/backup/audit.tssrc/backup/bundle.tssrc/backup/crypto.tssrc/backup/db-export.tssrc/backup/db-import.tssrc/backup/export-service.tssrc/backup/maintenance.tssrc/backup/manifest.tssrc/backup/restore-service.tssrc/backup/sodium-native.d.tssrc/backup/table-order.tssrc/db/migrations/0018_spotty_blonde_phantom.sqlsrc/db/migrations/meta/0018_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/operator-audit-schema.tssrc/db/schema.ts
unclesp1d3r
force-pushed
the
feat/encryption-at-rest-backups
branch
from
July 13, 2026 01:51
efb882b to
10272fa
Compare
coderabbitai
Bot
added
dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/backup/__tests__/crypto.test.ts (1)
162-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard against non-integer length.
Buffer.alloc(CHUNK_SIZE * 3.5)throwsRangeErrorifCHUNK_SIZEis ever odd. It's fine whileCHUNK_SIZEis a power of two, but the test silently couples to that. Prefer an explicit floor to keep the "3.5 chunks" intent robust to futureCHUNK_SIZEchanges.♻️ Suggested tweak
- const plaintext = Buffer.alloc(CHUNK_SIZE * 3.5); + const plaintext = Buffer.alloc(Math.floor(CHUNK_SIZE * 3.5));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backup/__tests__/crypto.test.ts` at line 162, Update the plaintext allocation in the crypto test to explicitly floor CHUNK_SIZE * 3.5 before passing it to Buffer.alloc, preserving the intended 3.5-chunk test size while ensuring the length is always an integer.docker-compose.yml (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale guard description. These are now Docker secret files, not env vars, so the
${VAR:?...}parameter-expansion guard doesn't apply to them. Compose does abort when a secret's source file is missing, but via secret resolution — not a${VAR:?}check. Reword to avoid sending operators looking for a guard that isn't in this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` around lines 33 - 35, Update the comment above the secret declarations to describe the actual Docker secret-file resolution behavior: Compose aborts when secrets/postgres_password.txt or secrets/better_auth_secret.txt is missing, rather than via a ${VAR:?} environment-variable guard. Remove the stale claim that these files are validated by parameter expansion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backup/restore-service.ts`:
- Around line 171-182: Update the catch handling in stageBundle so only
DecryptionAuthError maps to wrong_password_or_tampered, while preserving the
existing VersionMismatchSignal and NotEmptySignal mappings. Allow other DB/IO
failures from importIntoStaging or pool.connect to bubble up, or route them
through a distinct internal-failure outcome.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 33-35: Update the comment above the secret declarations to
describe the actual Docker secret-file resolution behavior: Compose aborts when
secrets/postgres_password.txt or secrets/better_auth_secret.txt is missing,
rather than via a ${VAR:?} environment-variable guard. Remove the stale claim
that these files are validated by parameter expansion.
In `@src/backup/__tests__/crypto.test.ts`:
- Line 162: Update the plaintext allocation in the crypto test to explicitly
floor CHUNK_SIZE * 3.5 before passing it to Buffer.alloc, preserving the
intended 3.5-chunk test size while ensuring the length is always an integer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e8f8d72f-6131-4d9f-b92a-305e44d042da
⛔ Files ignored due to path filters (2)
.env.exampleis excluded by!.env*bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (48)
.dockerignore.gitignoreCONTRIBUTING.mdDockerfileREADME.mdapp/(admin)/backup/backup-panel.tsxapp/(admin)/backup/constants.tsapp/(admin)/backup/export-panel.tsxapp/(admin)/backup/page.tsxapp/(admin)/backup/restore-panel.tsxapp/(app)/app-shell.tsxapp/(auth)/login/login-form.tsxapp/api/admin/backup/export/route.tsapp/api/admin/backup/restore/route.tscomponents/ui/confirm-dialog.tsxdocker-compose.ymldocker-entrypoint.shdocs/deployment.mddocs/operations/encryption-at-rest.mddocs/plans/2026-07-12-001-feat-encryption-at-rest-backups-plan.mde2e/backup.spec.tsnext.config.tspackage.jsonsecrets/README.mdsetup.shsrc/auth/__tests__/gating.test.tssrc/backup/__tests__/bundle.test.tssrc/backup/__tests__/crypto.test.tssrc/backup/__tests__/db-roundtrip.test.tssrc/backup/__tests__/export-service.test.tssrc/backup/__tests__/restore-service.test.tssrc/backup/__tests__/routes.test.tssrc/backup/audit.tssrc/backup/bundle.tssrc/backup/crypto.tssrc/backup/db-export.tssrc/backup/db-import.tssrc/backup/export-service.tssrc/backup/maintenance.tssrc/backup/manifest.tssrc/backup/restore-service.tssrc/backup/sodium-native.d.tssrc/backup/table-order.tssrc/db/migrations/0018_spotty_blonde_phantom.sqlsrc/db/migrations/meta/0018_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/operator-audit-schema.tssrc/db/schema.ts
🚧 Files skipped from review as they are similar to previous changes (40)
- app/(admin)/backup/backup-panel.tsx
- src/auth/tests/gating.test.ts
- app/(auth)/login/login-form.tsx
- package.json
- src/backup/sodium-native.d.ts
- app/api/admin/backup/export/route.ts
- components/ui/confirm-dialog.tsx
- Dockerfile
- src/db/schema.ts
- secrets/README.md
- CONTRIBUTING.md
- app/(admin)/backup/constants.ts
- app/(app)/app-shell.tsx
- app/(admin)/backup/export-panel.tsx
- app/(admin)/backup/page.tsx
- docker-entrypoint.sh
- next.config.ts
- src/backup/audit.ts
- src/backup/table-order.ts
- .gitignore
- src/backup/maintenance.ts
- .dockerignore
- src/backup/export-service.ts
- src/db/migrations/meta/_journal.json
- src/backup/tests/export-service.test.ts
- docs/operations/encryption-at-rest.md
- e2e/backup.spec.ts
- src/db/operator-audit-schema.ts
- src/backup/bundle.ts
- app/api/admin/backup/restore/route.ts
- src/backup/db-export.ts
- src/backup/db-import.ts
- src/backup/manifest.ts
- src/backup/tests/bundle.test.ts
- src/backup/tests/restore-service.test.ts
- src/backup/tests/db-roundtrip.test.ts
- src/backup/tests/routes.test.ts
- README.md
- app/(admin)/backup/restore-panel.tsx
- src/backup/crypto.ts
Add an explicit same-origin guard (src/backup/same-origin.ts) to both admin backup routes, checked right after the admin gate: they are plain Route Handlers never routed through Better Auth's own handler, so Better Auth's origin checks never applied to them, leaving only the session cookie's sameSite attribute as implicit CSRF protection on the app's highest-blast- radius endpoints. The guard checks Origin, falling back to Referer then Sec-Fetch-Site, against the request's own origin and BETTER_AUTH_URL. Enforce a 12-character minimum on the export password (src/backup/password-policy.ts), server-side in the export route and client-side in the export panel; restore intentionally keeps no such minimum since its password must match whatever encrypted the given bundle. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…aintenance guard, error classification Four fixes to the restore core from code review: 1. Per-run unique staging/snapshot schema names (restore_staging_<id> / restore_snapshot_<id>) plus a single advisory lock held for restore()'s ENTIRE body (staging through promote, both empty-instance and force paths) instead of just forcePromote's promote step. Two concurrent restores now fully serialize instead of racing to promote into public at the same time. 2. Boot-time crash recovery: maintenance.ts now records which snapshot schema belongs to an in-progress force-restore, and exports recoverInterruptedRestore(db, uploadDir) to roll the DB back from that snapshot, restore blobs from the newest pre-restore directory, and sweep any leftover restore_staging_*/restore_snapshot_* schemas and restore-staging-* temp dirs. Wired into a new instrumentation.ts register() hook, guarded on NEXT_RUNTIME=nodejs and DATABASE_URL, and never throws out. 3. Exported assertWritesAllowed(db) + MaintenanceModeError from maintenance.ts for the write-path enforcement worker to consume. 4. restore()'s staging-error classification now only maps genuine crypto auth failures (DecryptionAuthError/InvalidHeaderError) to wrong_password_or_tampered; every other staging error (ENOSPC, malformed tar, path traversal, ...) re-throws so the route's own catch-all surfaces a generic failure instead of misleading the operator. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
A force-restore sets the durable maintenance flag, but nothing outside src/backup/maintenance.ts read it, so ordinary writes proceeded unguarded during the restore's wipe+promote window. Wire assertWritesAllowed into every write-authorization entry point (authorize.ts's resolveCreateOwner/authorizeUpdate/ authorizeOwnerOnlyUpdate/authorizeDelete/authorizeAndDeleteParent, accessory-visibility.ts's authorizeMount, accessories/service.ts's bespoke requireEditPermission gate, grants.ts's createGrant/ revokeGrant, and the settings/admin-user actions that bypass those helpers) so every create/update/delete/grant/settings-change/ admin-user-op is blocked while a restore is active. Reads are untouched. Made the check itself cheap: assertWritesAllowed now does a single SELECT instead of also running the restore path's CREATE SCHEMA/TABLE IF NOT EXISTS ensure-step on every call, and fails open (allows writes) when Postgres reports the maintenance relation doesn't exist yet (42P01) - i.e. no restore has ever run on this instance. The SELECT runs inside db.transaction(...) rather than a bare db.execute(...): most callers pass an already-open transaction, and a caught 42P01 from a bare execute would still leave that transaction aborted at the protocol level (25P02), poisoning every later statement in it even though the JS exception was swallowed. Wrapping it lets drizzle's nested-transaction support use a SAVEPOINT/ROLLBACK TO SAVEPOINT instead, so the caller's transaction stays usable. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
priority:medium
and removed
enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/auth/accessory-visibility.ts (1)
88-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClose the maintenance race before the mount write
assertWritesAllowed(tx)only checks the flag once, andrestore()does not coordinate with normal write transactions. A restore can flip maintenance on after this read and still let the rest of the transaction continue, so this check needs to happen immediately before the actual mutation or be backed by a lock writers also honor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/auth/accessory-visibility.ts` around lines 88 - 95, Update authorizeMount so the maintenance-state validation is performed immediately before its actual database mutation, or reuse a lock shared with restore() and other write transactions. Ensure the mutation cannot proceed if maintenance begins after the initial assertWritesAllowed(tx) check, while preserving the existing transaction flow.src/backup/__tests__/routes.test.ts (1)
190-239: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDelete
DATABASE_URLinstead of assigningundefined
process.env.DATABASE_URL = undefinedleaves the key present as"undefined"under Node/Bun, so a later lazy reconnect can pick up a bogus URL. Usedelete process.env.DATABASE_URLin the no-original branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backup/__tests__/routes.test.ts` around lines 190 - 239, Update the no-original branch in the afterAll cleanup to remove DATABASE_URL with the delete operator instead of assigning undefined. Preserve restoring ORIGINAL_DATABASE_URL unchanged when it exists, so later lazy connections use the correct configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backup/maintenance.ts`:
- Around line 502-547: Update recoverInterruptedRestore so exitMaintenance and
cleanup of the restore snapshot occur only after rollback, blob restoration, and
snapshot removal complete successfully; preserve the maintenance flag and
snapshot schema when any recovery step fails, allowing a later retry. Ensure
sweepLeftoverSchemas does not remove the retained restore_snapshot schema after
a failed recovery, while keeping cleanup behavior for successful recovery.
---
Outside diff comments:
In `@src/auth/accessory-visibility.ts`:
- Around line 88-95: Update authorizeMount so the maintenance-state validation
is performed immediately before its actual database mutation, or reuse a lock
shared with restore() and other write transactions. Ensure the mutation cannot
proceed if maintenance begins after the initial assertWritesAllowed(tx) check,
while preserving the existing transaction flow.
In `@src/backup/__tests__/routes.test.ts`:
- Around line 190-239: Update the no-original branch in the afterAll cleanup to
remove DATABASE_URL with the delete operator instead of assigning undefined.
Preserve restoring ORIGINAL_DATABASE_URL unchanged when it exists, so later lazy
connections use the correct configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e290a85d-bce7-4f63-9712-ff5c8214f8a8
📒 Files selected for processing (25)
app/(admin)/backup/export-panel.tsxapp/(admin)/users/__tests__/actions.test.tsapp/(admin)/users/actions.tsapp/(app)/settings/__tests__/actions.test.tsapp/(app)/settings/actions.tsapp/api/admin/backup/export/route.tsapp/api/admin/backup/restore/route.tsinstrumentation.tssrc/auth/accessory-visibility.tssrc/auth/authorize.tssrc/auth/grants.tssrc/backup/__tests__/db-roundtrip.test.tssrc/backup/__tests__/export-service.test.tssrc/backup/__tests__/maintenance.test.tssrc/backup/__tests__/restore-service.test.tssrc/backup/__tests__/routes.test.tssrc/backup/__tests__/write-path-maintenance-guard.test.tssrc/backup/db-import.tssrc/backup/export-service.tssrc/backup/maintenance.tssrc/backup/password-policy.tssrc/backup/restore-service.tssrc/backup/same-origin.tssrc/domain/accessories/service.tssrc/domain/action-result.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- app/(admin)/backup/export-panel.tsx
- app/api/admin/backup/export/route.ts
- src/backup/export-service.ts
- app/api/admin/backup/restore/route.ts
- src/backup/tests/db-roundtrip.test.ts
- src/backup/tests/export-service.test.ts
…lation & unknown-table tests Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
… JSDoc Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…es + recovery/exhaustiveness hardening - Unify the empty-instance (F2) and force-replace (F3) restore promote paths through the same maintenance+snapshot+rollback envelope (forcePromote/emptyInstancePromote -> single `promote`). Closes a TOCTOU window where a non-force restore never raised the maintenance flag, so a concurrent ordinary write during staging/promote was neither blocked nor recoverable if the promote then failed. - `undoBlobSwap` now logs loudly and re-throws instead of swallowing rollback failures. When rollback itself fails inside `promote`, the error propagates as a generic thrown error (not a reassuring `rolled_back` outcome) and the maintenance flag is deliberately left active so writes stay blocked and recovery can retry. - `recoverInterruptedRestore` only clears the maintenance flag once recovery genuinely succeeded or wasn't needed; a partial `rollbackLiveFromSnapshot` failure now leaves the flag active and preserves the snapshot schema, logging a MANUAL INTERVENTION REQUIRED message. `sweepLeftoverSchemas` excludes any snapshot schema still referenced by an active flag. - `restore()`'s staging cleanup now logs drop/rm failures instead of swallowing them; `stageBundle`'s `BundleEvent` handling is now an exhaustive switch with a `never`-checked default; `_testFaultInjection` is now runtime-guarded to refuse outside `NODE_ENV=test`. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…er outcome message + instrumentation test Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…sage out of redundant UI - justfile test recipe also runs __tests__/instrumentation.test.ts in its own process (mock.module is process-global and would bleed into bun test src). - restore-panel no longer renders outcome.message: it paraphrases the curated per-kind copy, so showing both was redundant and tripped a Playwright strict-mode duplicate-text match. message stays in the JSON response + the client_error fallback. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
enhancement
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backup/restore-service.ts (1)
661-664: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the success-path cleanup best-effort.
IfDROP SCHEMAorcommitBlobSwap()fails after the restore has already committed, this path turns a successful restore into a generic error and leaves cleanup artifacts behind. Mirror the rollback path here: log cleanup failures and let the successful restore stand.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backup/restore-service.ts` around lines 661 - 664, Update the success path around the DROP SCHEMA execution and commitBlobSwap call to make cleanup best-effort: wrap each cleanup operation using the rollback path’s existing error-logging pattern, log failures, and continue returning the already-successful restore result instead of propagating cleanup errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/backup/restore-service.ts`:
- Around line 661-664: Update the success path around the DROP SCHEMA execution
and commitBlobSwap call to make cleanup best-effort: wrap each cleanup operation
using the rollback path’s existing error-logging pattern, log failures, and
continue returning the already-successful restore result instead of propagating
cleanup errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 447c8aab-0241-498b-8281-32fe4b526436
📒 Files selected for processing (15)
__tests__/instrumentation.test.tsapp/(admin)/backup/restore-panel.tsxapp/api/admin/backup/export/route.tsapp/api/admin/backup/restore/route.tsjustfilesrc/backup/__tests__/crypto.test.tssrc/backup/__tests__/db-roundtrip.test.tssrc/backup/__tests__/export-service.test.tssrc/backup/__tests__/maintenance.test.tssrc/backup/__tests__/restore-service.test.tssrc/backup/__tests__/routes.test.tssrc/backup/crypto.tssrc/backup/db-import.tssrc/backup/maintenance.tssrc/backup/restore-service.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- app/api/admin/backup/export/route.ts
- app/api/admin/backup/restore/route.ts
- src/backup/tests/db-roundtrip.test.ts
- src/backup/tests/restore-service.test.ts
- app/(admin)/backup/restore-panel.tsx
- src/backup/tests/routes.test.ts
- src/backup/db-import.ts
- src/backup/maintenance.ts
- src/backup/crypto.ts
…sword, doc clarity (PR review) Addresses CodeRabbit PR review findings on the deploy/secret-handling docs and scripts: - CONTRIBUTING.md: generate secrets/*.txt only when absent instead of overwriting them on every setup pass (an existing Postgres volume keeps its original password, so a silent overwrite locks devs out). Also stop instructing readers to embed unevaluated `$(cat ...)` shell substitution directly in .env.local — mise parses it as a literal dotenv file and never expands it; show the shell-evaluated file-generation commands instead. - docker-entrypoint.sh: enforce the documented hex-only password contract before interpolating POSTGRES_PASSWORD into DATABASE_URL, failing fast with a clear message instead of silently exporting a connection string a non-hex password (containing @ : / ? # %, etc.) would corrupt. - README.md: create the Docker secret files with owner-only permissions (umask 077) in both the quick-start and from-source flows; make the developer-loop postgres_password.txt generation idempotent for the same reason as CONTRIBUTING.md; clarify that `pg_dump` is database-only and does not capture uploaded document blobs (they live on the separate uploads volume), pointing readers at the in-app encrypted backup or a separate uploads-volume backup. - setup.sh: print owner-only (umask 077) secret-creation instructions, and have the preflight actively tighten existing secret files to 0600 (best-effort) rather than only checking they're non-empty. Verified with shellcheck (setup.sh, docker-entrypoint.sh) and pre-commit run --files, both clean. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…st/migration nits (PR review) Resolves remaining PR-review findings on the backup backend/tests: - db-export.ts: correct the exportDatabase doc comment. selectAllRows buffers a whole table into memory via a single SELECT before yielding any of its rows, so it is table-by-table buffering, not row-level streaming from Postgres. Also note that export-service.ts's bufferDbExport still buffers the full concatenated NDJSON output for its own reasons (tar header byte length), which is a property of that caller, not of exportDatabase. - routes.test.ts: use `delete process.env.DATABASE_URL` instead of assigning `undefined`, which coerces to the string "undefined" and would leave a bogus DATABASE_URL for whichever test file runs next. isMaintenanceActive() (maintenance.ts) was NOT removed: it is exercised directly by maintenance.test.ts and restore-service.test.ts as a flag-state assertion helper, so it is not dead code. The underlying write-blocking concern the review raised is already handled independently by assertWritesAllowed, which is wired into authorize.ts, grants.ts, accessory-visibility.ts, restore-service.ts, and domain/accessories/service.ts, with dedicated integration coverage in write-path-maintenance-guard.test.ts. The migration/.sqlfluff finding (0018_spotty_blonde_phantom.sql) was declined: sqlfluff is not wired into this repo's pre-commit hooks or `just ci-check`, and every existing drizzle-generated migration has the same quoting/indent/line-length shape, so hand-editing this one generated file would not fix anything actually blocking CI. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…eview fix (PR review) - export-panel: keep the export button disabled for the rest of the view's lifetime once submitted, not just while status === "pending". Previously the button re-enabled the moment the optimistic "started" state kicked in (ASSUME_STARTED_MS after submit), letting an admin fire a second full-instance encrypted export while the first was still streaming server-side. - restore-panel: reject restore passwords containing characters above U+00FF before ever calling fetch(), since Headers/fetch throw synchronously for header values outside the Latin-1 byte range (curly quotes, CJK, emoji, currency symbols like the euro sign). Surfaces an actionable field-level validation error instead of the previous opaque client_error fallback. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…mand Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…oded header (PR review) Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
dependencies
7 of 16 tasks
Merged
8 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Reviewer's Guide
Scope: 66 files · +11,660 / −63 · 26 commits — one cohesive feature (encryption-at-rest), so it reviews as a unit rather than splitting. | Risk: 🟠 High inherent (crypto · admin auth · data migration · destructive restore · PII), well-mitigated (110 backup tests incl. fault-injection + Testcontainers, e2e, 2 adversarial review rounds with all findings fixed). | Change mix: 34 source · 13 test · 6 docs · 5 deploy · 3 migration.
Suggested review order (highest-leverage first)
src/backup/crypto.ts(+628) — the trust root: Argon2id +secretstream, header parse, bounded KDF params. Verify no plaintext escapes on auth failure.src/backup/restore-service.ts(+670) — the crown jewel: stage-then-promote (nothing live until the whole bundle authenticates), unified maintenance+snapshot+rollback envelope, per-run schemas. Read alongsiderestore-service.test.ts(fault-injection rollback, last-chunk tamper, concurrent-restore).src/backup/maintenance.ts(+592) — durable flag, pool-safe advisory lock, crash recovery (recoverInterruptedRestore),assertWritesAllowed(the write-path guard).src/backup/bundle.ts(+423) — streaming tar + zip-slip / path-traversal choke point (KTD11).src/backup/db-{export,import}.ts+table-order.ts— FK-safe NDJSON round-trip, snapshot-isolated export, bounded import.app/api/admin/backup/*— admin gate → same-origin CSRF → streaming; discriminated outcomes;operator_audit.app/(admin)/backup/*— no-recovery warning, type-to-confirm force-replace, session invalidation, double-submit guard.docs/operations/encryption-at-rest.mdthreat matrix.Review checklist (tailored)
Crypto / security
Restore correctness (destructive path)
Data migration
operator_auditmigration0018is additive/safe;table-order.tscovers every persistent table (guard test present)Tests
Summary
Adds a coherent data-at-rest story to MagStacker in two complementary layers, resolving the direction of #67:
Plan:
docs/plans/2026-07-12-001-feat-encryption-at-rest-backups-plan.md(implementation-ready; strengthened from a multi-persona doc review before implementation).What's included (by implementation unit)
src/backup/crypto.ts): Argon2id KDF (sodium-native, OWASP MODERATE) + chunked authenticatedsecretstream(XChaCha20-Poly1305) streams with a self-describing header. No hand-rolled crypto.src/backup/bundle.ts,manifest.ts): streaming tar (manifest.json+db.ndjson+blobs/) composed with the crypto stream; nothing buffered whole. Zip-slip / path-traversal defense at the read choke point.src/backup/db-export.ts,db-import.ts,table-order.ts): FK-safe NDJSON round-trip over every persistent table (ephemeral tables excluded), with a schema-diff guard test. Adds theoperator_audittable (migration0018).restore-service.ts,maintenance.ts): stage-then-promote — nothing touches live data until the whole bundle authenticates (holds on both empty and force paths, incl. a tamper in the final chunk);BACKUP_FORMAT_VERSIONcompatibility gate; refuse-unless-empty by default; force-replace with both-store snapshot/rollback.app/api/admin/backup/*): streaming request/response, discriminated restore outcome,operator_auditlogging on success and failure.app/(admin)/backup/*): password + no-recovery warning, direct streamed download, upload restore with differentiated outcome messaging, type-to-confirm force-replace, session-invalidation on success.docs/operations/encryption-at-rest.md): LUKS/encrypted-cloud-volume how-to + threat-coverage matrix.Test plan
just ci-checkgreen: Biome lint + format,tsc --noEmit, pre-commit hooks, fullbun test(600 pass), Playwright e2e (34 passed incl.e2e/backup.spec.ts).docker compose config+ boot smoke (secrets resolve, read-only rootfs, uploads writable) verified during U8.Review Findings — all resolved
The multi-persona code review surfaced the findings below. All are now fixed in this PR (commits
6aa5333,8591cd8,9caec0a,19fc7a9), each with tests.assertWritesAllowed(db)(a cheap single flagSELECT, fail-open on missing infra) is called from every write authorization helper (resolveCreateOwner,authorizeUpdate,authorizeOwnerOnlyUpdate,authorizeDelete,authorizeAndDeleteParent,authorizeMount) plus the non-helper mutation sites (grants create/revoke, accessories' bespoke gate, magpul-mode settings, admin create/ban/unban). Ordinary user writes are refused withMaintenanceModeErrorduring a force-restore; reads and the restore's own promote are unaffected.restore_staging_<id>/restore_snapshot_<id>schemas, and the advisory lock now wraps the entire restore body (staging + promote, both paths) so restores fully serialize.recoverInterruptedRestore()runs from Next.jsinstrumentation.tson boot (guarded onDATABASE_URL, never fails boot): it rolls back an interrupted force-restore from the snapshot schema, restores stranded blobs, clears the stuck flag, and sweeps leftover staging schemas/dirs.exportDatabaseis now snapshot-isolated. The export consumption runs inside arepeatable read/read onlytransaction, so the bundle is internally consistent even under concurrent writes.INSERTcollided with the live bootstrap admin onuser.email).Origin→Referer→Sec-Fetch-Site, 403 on mismatch), no longer relying solely on Better Auth's default cookieSameSite.wrong_password_or_tampered; other errors (ENOSPC, malformed tar, unknown table) surface distinctly.deriveKey(pre-auth Argon2id resource-blowup prevention).Also from the review/simplify passes: the restore UI validates the route outcome and falls back gracefully instead of crashing on an unmodeled outcome;
bufferDbExporttallies rows in-loop and returns a Buffer; parallel blob stats; single stagingmkdir; and a pre-existing full-suite flake was fixed (gating.test.tsself-seeds its documented admin).Closes #67.
Comprehensive review round 2 — all findings resolved
A second multi-agent review (correctness, tests, silent-failures, type-design, comments) surfaced further findings; all are fixed in commits
32c4922,5b399da,6104cbc,59ff896,29b9a7a:undoBlobSwapandrecoverInterruptedRestorenow log and, on failure, leave maintenance active (blocking writes) instead of reporting a clean rollback / clearing the flag over a half-wiped DB; the active snapshot is excluded from the boot sweep.recordOperatorEventis.catch+logged (no logging hiccup masks a real restore result); export streams get an error listener.KdfParams.algis validated;BundleEventhandling is exhaustiveness-checked;_testFaultInjectionis guarded against production use.Sec-Fetch-Site/default-deny, unknown-table rejection, andinstrumentation.register()(now run in CI in its own process).