From a6a73fed61a6eb06aadc8b7408fee3fc09e04a74 Mon Sep 17 00:00:00 2001
From: pax <paxxe@protonmail.com>
Date: Sat, 11 Apr 2026 16:15:41 -0500
Subject: [PATCH] =?UTF-8?q?security:=20fix=20#4=20=E2=80=94=20chmod=20SQLi?=
 =?UTF-8?q?te=20DB=20+=20WAL/SHM=20sidecars=20to=200o600?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The sites table stores api_key + api_user in plaintext. Previous
behavior left the DB file at the inherited umask (0o644 on most
Linux systems) so any other local user could sqlite3 it open and
exfiltrate every booru API key.

Adds Database._restrict_perms(), called from the lazy conn init
right after _migrate(). Tightens the main file plus the -wal and
-shm sidecars to 0o600. The sidecars only exist after the first
write, so the FileNotFoundError path is expected and silenced.
Filesystem chmod failures are also swallowed for FUSE-mount
compatibility.

behavior change from v0.2.5: ~/.local/share/booru-viewer/booru.db
is now 0o600 even if a previous version created it 0o644.

Audit-Ref: SECURITY_AUDIT.md finding #4
Severity: Medium
---
 booru_viewer/core/db.py | 24 +++++++++++++++++++++++-
 tests/core/test_db.py   | 31 +++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/booru_viewer/core/db.py b/booru_viewer/core/db.py
index 39963b9..45440f2 100644
--- a/booru_viewer/core/db.py
+++ b/booru_viewer/core/db.py
@@ -11,7 +11,7 @@ from dataclasses import dataclass, field
 from datetime import datetime, timezone
 from pathlib import Path
 
-from .config import db_path
+from .config import IS_WINDOWS, db_path
 
 
 def _validate_folder_name(name: str) -> str:
@@ -210,8 +210,30 @@ class Database:
             self._conn.execute("PRAGMA foreign_keys=ON")
             self._conn.executescript(_SCHEMA)
             self._migrate()
+            self._restrict_perms()
         return self._conn
 
+    def _restrict_perms(self) -> None:
+        """Tighten the DB file (and WAL/SHM sidecars) to 0o600 on POSIX.
+
+        The sites table stores api_key + api_user in plaintext, so the
+        file must not be readable by other local users. Sidecars only
+        exist after the first WAL checkpoint, so we tolerate
+        FileNotFoundError. Windows: NTFS ACLs handle this; chmod is a
+        no-op there. Filesystem-level chmod failures are swallowed —
+        better to keep working than refuse to start.
+        """
+        if IS_WINDOWS:
+            return
+        for suffix in ("", "-wal", "-shm"):
+            target = Path(str(self._path) + suffix) if suffix else self._path
+            try:
+                os.chmod(target, 0o600)
+            except FileNotFoundError:
+                pass
+            except OSError:
+                pass
+
     @contextmanager
     def _write(self):
         """Context manager for write methods.
diff --git a/tests/core/test_db.py b/tests/core/test_db.py
index 6413d25..cf36c06 100644
--- a/tests/core/test_db.py
+++ b/tests/core/test_db.py
@@ -13,6 +13,9 @@ These tests lock in the `54ccc40` security/correctness fixes:
 
 from __future__ import annotations
 
+import os
+import sys
+
 import pytest
 
 from booru_viewer.core.db import _validate_folder_name
@@ -42,6 +45,34 @@ def test_validate_folder_name_rejects_traversal():
         _validate_folder_name("~user")     # leading tilde
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only chmod check")
+def test_db_file_chmod_600(tmp_db):
+    """Audit finding #4: the SQLite file must be 0o600 on POSIX so the
+    plaintext api_key/api_user columns aren't readable by other local
+    users on shared workstations."""
+    # The conn property triggers _restrict_perms() the first time it's
+    # accessed; tmp_db calls it via add_site/etc., but a defensive
+    # access here makes the assertion order-independent.
+    _ = tmp_db.conn
+    mode = os.stat(tmp_db._path).st_mode & 0o777
+    assert mode == 0o600, f"expected 0o600, got {oct(mode)}"
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only chmod check")
+def test_db_wal_sidecar_chmod_600(tmp_db):
+    """The -wal sidecar created by PRAGMA journal_mode=WAL must also
+    be 0o600. It carries in-flight transactions including the most
+    recent api_key writes — same exposure as the main DB file."""
+    # Force a write so the WAL file actually exists.
+    tmp_db.add_site("test", "http://example.test", "danbooru")
+    # Re-trigger the chmod pass now that the sidecar exists.
+    tmp_db._restrict_perms()
+    wal = type(tmp_db._path)(str(tmp_db._path) + "-wal")
+    if wal.exists():
+        mode = os.stat(wal).st_mode & 0o777
+        assert mode == 0o600, f"expected 0o600 on WAL sidecar, got {oct(mode)}"
+
+
 def test_validate_folder_name_accepts_unicode_and_punctuation():
     """Common real-world folder names must pass through unchanged. The
     guard is meant to block escape shapes, not normal naming."""