Skip to content

Fix GraphQL field pruning under foreign keys#128

Merged
brucehart merged 9 commits into
feature/security-hardeningfrom
pr-127-feedback
Jul 13, 2026
Merged

Fix GraphQL field pruning under foreign keys#128
brucehart merged 9 commits into
feature/security-hardeningfrom
pr-127-feedback

Conversation

@brucehart

Copy link
Copy Markdown
Owner

Summary

  • delete GraphQL field rows before their parent entries during prune
  • preserve compatibility with databases that predate the graphql_fields table
  • add a regression test that prunes an import containing GraphQL field metadata

Testing

  • cargo test --all-features --locked
  • git diff --check

Feedback addressed

Addresses the P2 review finding on #127: #127 (comment)

Stacked on #127.

@brucehart

Copy link
Copy Markdown
Owner Author

Recursive review findings

[MEDIUM] src/commands/prune.rs:149 — External deletion can outlive rollback or break surviving blobs

Expected behavior: External body files should only be deleted after the database prune commits, and never while a surviving blob still references the same resolved file.

Observed behavior: Files are removed inside the SQLite transaction before FTS/blob deletion and transaction commit. A failing blob-delete trigger produced exit status 1, rolled all database rows back, but removed all three external files. A second reproduction used two blob rows sharing one external path; pruning one import removed the file while the other import retained a live reference.

Why this matters: An ordinary database failure or an untrusted/malformed database can cause irreversible body-file loss and leave surviving entries broken.

Suggested fix: Collect candidates during the transaction, commit database changes first, exclude candidates whose canonical paths are still referenced by surviving blobs, then perform best-effort filesystem deletion. Add rollback and shared-path regression tests.

Confidence: high.

[MEDIUM] src/commands/pii.rs:343 — PII redaction is not atomic

Expected behavior: A failed PII rewrite should leave the database unchanged, matching the transactional behavior of redact.

Observed behavior: Entry updates and replacement blobs autocommit one row at a time. With a trigger rejecting the second entry update, the command exited 1 after the first entry had already switched to a redacted hash, the second remained unchanged, and both original secret blobs remained. Sensitive-write finalization was skipped.

Why this matters: Users can receive a failure while the database is partially rewritten and still contains superseded sensitive data, undermining the physical-removal guarantee in #127.

Suggested fix: Run the complete PII scan/rewrite and FTS maintenance in one SQLite transaction, commit only after every entry succeeds, and finalize sensitive deletion after commit. Add a forced-failure rollback test.

Confidence: high.

[MEDIUM] src/commands/redact.rs:790 and src/commands/pii.rs:158 — Failed output rewrites leave unredacted artifacts

Expected behavior: The requested output path should become visible only after a successful redaction, or be removed when the operation fails.

Observed behavior: The source database is copied to the final output path before matcher/regex validation and before rewriting. Running redact with an output path and an invalid body regex exited 1 but left a readable redacted.db containing all source entries and secrets. PII uses the same ordering.

Why this matters: A file named as a redacted output can actually be an untouched sensitive copy after an error, creating a realistic accidental-disclosure hazard.

Suggested fix: Validate options before copying, stage output rewrites in a sibling temporary database, and atomically publish the final path only after rewrite/finalization succeeds; clean up staged database sidecars on every error. Add redaction and PII failure-artifact tests.

Confidence: high.

Blockers

None classified high severity.

Must-fix

  • Make prune filesystem deletion commit-safe and reference-safe.
  • Make PII rewriting transactional.
  • Prevent failed output rewrites from publishing unredacted databases.

Nice-to-have

None from this pass.

Stage output database copies until redaction completes so failures preserve any existing destination. Wrap PII entry rewrites in a single transaction, defer prune filesystem deletion until after commit, and retain external files still referenced by surviving blobs. Add regressions for rollback, publication failure, overwrite, and shared external paths.
@brucehart

Copy link
Copy Markdown
Owner Author

Implemented all three must-fix findings in stacked PR #129: #129

The fix stages output databases until successful publication, wraps PII entry rewrites in one transaction, delays prune filesystem deletion until after SQLite commit, and protects external paths still referenced by surviving blobs. Added focused regressions; the full all-features suite passes (201 tests), along with clippy and reduced-feature checks.

Propagate SQLite decoding errors while protecting surviving external blob paths so malformed databases roll back before filesystem deletion. Clean rollback-journal sidecars with staged databases and add regressions for both cases.
Back up an existing destination database and its SQLite sidecars as a recoverable set before publishing staged output. Move orphaned-blob cleanup into the PII and redact transactions so cleanup errors roll back every logical rewrite. Add restore, replacement, and cleanup-failure regressions.
Simulate a failed final rename and verify the prior destination database and every SQLite sidecar are restored without leaving a backup directory.
Propagate malformed entry hashes, orphan blob hashes, and pruned external paths instead of silently skipping them. Add regressions proving malformed pruned paths and entry hashes roll back the import transaction.
Fail closed on every prune row decode
Fail closed on malformed prune paths
Make sensitive rewrites and prune cleanup atomic
@brucehart
brucehart merged commit a3b61ab into feature/security-hardening Jul 13, 2026
4 checks passed
@brucehart
brucehart deleted the pr-127-feedback branch July 13, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant