Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,38 @@ impl Database {
Ok(())
}

/// DB の一貫したスナップショットを `dest` に書き出す。
///
/// 単純なファイルコピーでは WAL に未反映のトランザクションが取り残され、
/// 古い、または壊れた複製ができる。`VACUUM INTO` は writer 接続上で
/// 単一ファイルの完結したコピーを作るため、-wal / -shm を伴わない。
///
/// `strip_tokens` を立てると、複製側の `accounts.token` を空にしてから
/// 返す。キーチェーンが永続しない環境では DB に API トークンが平文で
/// 残るため、持ち出す成果物からは落とせるようにする。
pub fn backup_to(&self, dest: &Path, strip_tokens: bool) -> Result<(), NoteDeckError> {
if dest.exists() {
// VACUUM INTO は既存ファイルへの書き出しを拒否する
std::fs::remove_file(dest).map_err(|e| {
NoteDeckError::Database(rusqlite::Error::SqliteFailure(
rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_CANTOPEN),
Some(format!("failed to replace existing backup: {e}")),
))
})?;
Comment on lines +717 to +724

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

set -euo pipefail

# Locate the backup implementation and relevant tests.
fd -a 'db.rs' .
echo '---'
ast-grep outline src/db.rs --view expanded || true
echo '---'
rg -n "backup_to|VACUUM INTO|remove_file|dest.exists|same.*path|file identity|metadata" src/db.rs
echo '---'
sed -n '660,780p' src/db.rs

Repository: notedeck-dev/notecli

Length of output: 13512


🏁 Script executed:

set -euo pipefail

# Inspect the backup tests and surrounding setup to see whether same-path behavior is covered.
sed -n '1330,1518p' src/db.rs

Repository: notedeck-dev/notecli

Length of output: 6083


🏁 Script executed:

set -euo pipefail

# Check whether Database retains the source path and whether any same-file guard exists.
sed -n '95,190p' src/db.rs
echo '---'
rg -n "same.*file|canonical|metadata\\(|dev\\(|ino\\(|path:" src/db.rs

Repository: notedeck-dev/notecli

Length of output: 4278


Reject the live database path here. If dest points to the open database file, remove_file(dest) unlinks the live DB before VACUUM INTO, so existing connections keep using the unlinked inode while the path is recreated as the backup. Compare dest against the current database filename before deletion and add a same-path regression test.

🤖 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/db.rs` around lines 717 - 724, In the backup flow around dest.exists(),
compare dest with the current database filename before removing it and reject
the operation when they refer to the same path, preserving the live database.
Add a regression test covering a same-path VACUUM INTO attempt and verify the
database file is not deleted or replaced.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant file first
ast-grep outline src/db.rs --view expanded

# Read the relevant section around the reported lines
sed -n '680,780p' src/db.rs

# Look for other backup / VACUUM INTO / strip_tokens handling
rg -n "VACUUM INTO|strip_tokens|backup|remove_file\\(|-wal|-shm|journal" src/db.rs

Repository: notedeck-dev/notecli

Length of output: 12915


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '160,190p' src/db.rs
sed -n '1460,1515p' src/db.rs

# Search for any existing cleanup of SQLite sidecars around backup / temp DB handling
rg -n "suffix in \\[\"\", \"-wal\", \"-shm\"\\]|remove_file\\(.*-wal|remove_file\\(.*-shm|backup_to_overwrites_an_existing_file|backup_to_can_strip_tokens_from_the_copy" src/db.rs

Repository: notedeck-dev/notecli

Length of output: 3286


🌐 Web query:

SQLite VACUUM INTO existing destination WAL SHM sidecar files strip_tokens backup overwrite

💡 Result:

The SQLite VACUUM INTO command requires that the destination file does not exist [1][2]. If the destination file already exists, the command will fail with an error [1][2]. It cannot be used to overwrite an existing database file directly. Regarding WAL (Write-Ahead Logging) and SHM (shared-memory) sidecar files: 1. VACUUM INTO creates a new, compacted database file [1][2]. It does not create WAL or SHM files for the destination at the moment of creation [3][4]. 2. When restoring a database, existing WAL (-wal) and SHM (-shm) sidecar files associated with the destination must be manually deleted before the new database file is put in place [5]. These sidecar files are intrinsically linked to a specific database file state and are not compatible with a different or restored version of the database [5][6]. 3. If the destination file exists (and for some reason you are replacing it), you must remove the old database file along with its associated *-wal and *-shm sidecar files to prevent corruption or invalid state [5]. In summary, VACUUM INTO is a safe, non-destructive operation because it enforces the absence of the destination file [1]. If you intend to use it as part of an update or restoration process, you must manually clear the destination directory of the old database and its sidecar files before executing the command [5].

Citations:


🌐 Web query:

SQLite stale WAL file replaced database file ignored salt checksum same path

💡 Result:

When you replace an SQLite database file while leaving an old Write-Ahead Log (-wal) and shared memory (-shm) file in place, SQLite will attempt to process the stale files during the next connection [1][2][3]. Because the WAL file contains metadata including "salt" values that must match the current database file header, a mismatch will occur when they do not align [4][5][6]. Here is how SQLite handles this situation: 1. Salt Mismatch: The WAL header contains two "salt" values [7][5]. When SQLite opens a database, it checks if the salt values in the -wal file match the database file [4][6]. If you have replaced the database file, the old WAL file will contain salts from the previous database [1][3]. SQLite sees this mismatch and correctly identifies the WAL as "stale" or belonging to a different database instance [3]. 2. Checksum Verification: Beyond salt checks, SQLite verifies the integrity of each "frame" (transaction) in the WAL file by computing a checksum that includes the header and frame data [8][4][6]. If the WAL is stale, or if there is a mismatch during the recovery/checkpointing process, the checksum verification will fail [8][4][6]. When a frame fails checksum validation, SQLite stops processing the WAL at that point [8][9][10]. 3. Outcome: If the WAL and SHM files do not match the new database file, the database can appear "corrupt" or "malformed," potentially leading to error messages such as "database disk image is malformed" [1][3]. SQLite does not automatically "ignore" these files in a way that preserves data; instead, it may attempt to roll back or checkpoint them, which can lead to data loss or corruption of the newly placed database file [3][9][11]. To safely replace a database file in WAL mode, you must delete the accompanying -wal and -shm files alongside the main database file [1][2][3]. The only safe way to remove these files without manual deletion is to ensure all connections are closed properly, allowing SQLite to clean them up, or by opening the database and closing it immediately via the SQLite API [11][10]. Manually replacing only the.db file while leaving stale auxiliary files in the same path is a frequent cause of database corruption [1][3].

Citations:


Remove SQLite sidecars when overwriting backups. Deleting only dest leaves any existing dest-wal/dest-shm files behind, so an overwritten backup can still expose old WAL contents and break the strip-tokens guarantee. Remove the full SQLite file set or write to a fresh temp path and rename it into place.

🤖 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/db.rs` around lines 717 - 724, Update the backup overwrite handling
around the dest.exists() check to remove SQLite’s associated sidecar files,
including dest-wal and dest-shm, before VACUUM INTO writes the replacement.
Preserve the existing error mapping and ensure stale WAL/SHM contents cannot
remain alongside the overwritten backup.

}
{
let conn = self.lock_write()?;
conn.execute("VACUUM INTO ?1", params![dest.to_string_lossy()])?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the file structure and inspect the relevant area in src/db.rs
git ls-files 'src/db.rs' 'src/*.rs' | sed -n '1,20p'
wc -l src/db.rs
sed -n '680,760p' src/db.rs

Repository: notedeck-dev/notecli

Length of output: 3457


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the exact function around the VACUUM INTO call and any path validation nearby
sed -n '700,750p' src/db.rs
printf '\n---\n'
rg -n "to_string_lossy|VACUUM INTO|PathBuf|canonicalize|metadata|permissions" src/db.rs

Repository: notedeck-dev/notecli

Length of output: 2905


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect permission-hardening logic and any path validation around backup callers
sed -n '120,210p' src/db.rs
printf '\n--- callers ---\n'
rg -n "backup_to\(" -S .
printf '\n--- path validation ---\n'
rg -n "to_str\(|to_string_lossy\(|is_utf8|utf-8|UTF-8|non-UTF|Path::new|OsStr|OsString" src/db.rs src/*.rs

Repository: notedeck-dev/notecli

Length of output: 4491


Reject non-UTF-8 backup paths. to_string_lossy() can change the filename passed to VACUUM INTO, so the backup may be written under a different path than the one later hardened and reopened. Reject non-UTF-8 paths or use an OS-string-preserving binding.

🤖 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/db.rs` at line 728, Update the backup logic around the VACUUM INTO
execution to reject destinations whose OsStr cannot be represented as UTF-8
before invoking SQLite, rather than using dest.to_string_lossy(). Preserve the
existing path used for subsequent hardening and reopening, and return an
appropriate error for non-UTF-8 backup paths.

}
Self::restrict_permissions(dest);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Propagate permission-hardening failures for backups.

restrict_permissions ignores set_permissions errors, so this method can return success with a group/world-readable backup containing API tokens. Make permission enforcement fallible for backups and fail or clean up when owner-only permissions cannot be applied.

🤖 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/db.rs` at line 730, Update the backup flow around restrict_permissions so
permission enforcement returns and propagates set_permissions failures instead
of being ignored. Make restrict_permissions fallible, have its caller fail the
backup when owner-only permissions cannot be applied, and clean up the
destination if required by the existing backup error path.


if strip_tokens {
let copy = Connection::open(dest)?;
copy.execute("UPDATE accounts SET token = ''", [])?;
}
Comment on lines +732 to +735

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the file and inspect the relevant area around the reported lines.
ast-grep outline src/db.rs --view expanded > /tmp/db_outline.txt
sed -n '700,780p' src/db.rs | cat -n

# Find the strip_tokens implementation and any related backup/publish logic.
rg -n "strip_tokens|VACUUM|secure_delete|Connection::open\\(dest\\)|UPDATE accounts SET token" src/db.rs

Repository: notedeck-dev/notecli

Length of output: 5002


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect DB initialization pragmas and any backup-related tests/docs.
sed -n '1,260p' src/db.rs | cat -n
printf '\n--- backup tests ---\n'
sed -n '1460,1525p' src/db.rs | cat -n
printf '\n--- search for secure_delete / temp backup handling ---\n'
rg -n "secure_delete|temp file|tempfile|rename\\(|VACUUM INTO|backup_to\\(" src

Repository: notedeck-dev/notecli

Length of output: 14773


Make strip_tokens sanitize the backup before publishing it.
UPDATE accounts SET token = '' only clears the logical value; the original bytes can still remain in SQLite pages unless secure_delete/VACUUM is used. Because this writes straight to dest, a failed strip also leaves an unsanitized backup behind. Stage it in a temp path, scrub there, then rename into place and clean up on error.

🤖 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/db.rs` around lines 732 - 735, Update the strip_tokens branch in the
backup flow to write to a temporary destination, enable SQLite secure deletion
and vacuum after clearing account tokens, then atomically rename the sanitized
temporary file to dest. Ensure temporary files are removed on any failure so an
unsanitized or partial backup is never published.

Ok(())
}

/// Delete a single note from the cache (e.g. when a deletion event is received).
pub fn delete_cached_note(&self, note_id: &str) -> Result<(), NoteDeckError> {
let conn = self.lock_write()?;
Expand Down Expand Up @@ -1433,6 +1465,51 @@ mod tests {
}
}

#[test]
fn backup_to_produces_a_readable_copy_with_data() {
let (dir, db) = temp_db();
db.upsert_account(&sample_account()).unwrap();
let dest = dir.path().join("backup.db");

db.backup_to(&dest, false).unwrap();

// VACUUM INTO は単一ファイルで完結する (-wal を伴わない)
assert!(dest.exists());
assert!(!dir.path().join("backup.db-wal").exists());

let restored = Database::open(&dest).unwrap();
let accounts = restored.load_accounts().unwrap();
assert_eq!(accounts.len(), 1);
assert_eq!(accounts[0].token, "test-token");
}

#[test]
fn backup_to_can_strip_tokens_from_the_copy() {
let (dir, db) = temp_db();
db.upsert_account(&sample_account()).unwrap();
let dest = dir.path().join("backup.db");

db.backup_to(&dest, true).unwrap();

let restored = Database::open(&dest).unwrap();
assert_eq!(restored.load_accounts().unwrap()[0].token, "");
// 元の DB は触らない
assert_eq!(db.load_accounts().unwrap()[0].token, "test-token");
}

#[test]
fn backup_to_overwrites_an_existing_file() {
let (dir, db) = temp_db();
db.upsert_account(&sample_account()).unwrap();
let dest = dir.path().join("backup.db");
std::fs::write(&dest, b"stale").unwrap();

db.backup_to(&dest, false).unwrap();

let restored = Database::open(&dest).unwrap();
assert_eq!(restored.load_accounts().unwrap().len(), 1);
}

#[test]
fn account_upsert_and_load() {
let (_dir, db) = temp_db();
Expand Down
Loading