Security hardening: admin password handling, malware-gate errors, -O-safe validation, basename traversal

[reese8272]

Fixes from a security review of the tools/ scripts (2026-07-02).

Changes

• rebuild_staging_db.sh — the admin password previously reset to admin/admin unconditionally on every run on an internet-facing box. Now: password comes from WOCO_ADMIN_PASSWORD (script refuses to run without it) and is applied only when the admin user is first created; an existing admin's password is never touched. Also: Site domain honors DJANGO_APP_HOSTNAME, setup-SQL path corrected scripts/ → tools/, set -euo pipefail.
• fingerprint.sh — grep exit ≥2 (scanner itself failed: bad path, unreadable tree) was swallowed by 2>/dev/null and reported as "OK: no malware fingerprints" — a false negative in a security gate. Now exits 2 loudly. The documented exit code 2 is now reachable.
• Pipeline assert → explicit raises (pipeline_llm.py, ascc_page_extract.py, ascc_page_processor.py) — API-key presence and LLM-output validation were enforced only by assert, which python -O strips entirely. Converted to RuntimeError/ValueError; the two fallback handlers that caught AssertionError now catch ValueError, so recovery behavior is unchanged. test_pipeline_llm.py updated to expect RuntimeError.
• --basename path traversal — Paths.__init__ in all three stage scripts now rejects basenames containing /, \, or .. (a value like ../../x could write/delete outside tools/wip/ when the stage scripts are invoked directly; ascc_cli.py validates state codes but the scripts are also runnable standalone).

Not touched (flagging, not fixing here)

• ascc_data_munger.py's ~117 inline self-test asserts (same -O concern, but moving them to the test suite is a refactor).
• Pre-existing on staging: 4 test_ascc_cli.py failures from the ASCC2 → ASCC6 default change (tests still expect ASCC2).

Tests

Full tools suite: 159 ran; only those 4 pre-existing test_ascc_cli.py failures remain (present on staging before this branch). fingerprint.sh exercised on both the clean and scanner-error paths.

🤖 Generated with Claude Code

[github-actions]

Code review overview

• Syntax Errors - Score - 10/10
• Code Smells - Score - 9/10
• Bugs - Score - 8/10
• Security Vulnerabilities - Score - 9/10

---

Syntax Errors

No issues found.

---

Code Smells

File: tools/rebuild_staging_db.sh, inline Python blocks

The pattern of passing secrets via environment variables into uv run python ... shell -c with inline Python is functional but fragile and hard to maintain. The inline Python is a single logical line separated by semicolons, which reduces readability and makes future edits error-prone. A standalone management command or a dedicated script would be cleaner and easier to test.

Suggested fix: Extract the admin-creation and site-update logic into proper Django management commands (e.g., ensure_admin, ensure_site) that read from environment variables internally. This separates shell orchestration from Python logic.

---

File: tools/ascc_image_extract.py, tools/ascc_page_extract.py, tools/ascc_page_processor.py — Paths.__init__

The path-traversal guard is duplicated verbatim across three files with identical comments. This is a DRY violation; if the rule ever needs to change (e.g., to also reject null bytes or colons on Windows), it must be updated in three places.

Suggested fix: Extract validate_basename(basename) into a shared utility module (e.g., tools/common.py) and call it from all three Paths.__init__ methods.

---

Bugs

File: tools/ascc_page_processor.py, classify_block (~line 836)

ValueError is now caught in the except clause (correctly replacing AssertionError), but the fallback path silently swallows the error and continues with a word-search heuristic. If the LLM consistently returns a valid JSON response with an unexpected kind value (e.g. "image" instead of "illustration"), the ValueError will always be raised and the fallback will always activate — silently degrading accuracy with no persistent warning logged. The comment acknowledges this but doesn't mitigate the silent-failure risk.

Suggested fix: Add a structured log/warning (e.g., logging.warning(...)) when the fallback path is taken due to a ValueError specifically (as opposed to a JSONDecodeError), so operators can detect systematic LLM drift.

---

File: tools/ascc_page_extract.py, extract_chunk validation (~line 520)

images_above is validated as isinstance(images_above, int) and images_above >= 0, but Python's bool is a subclass of int, so True and False would pass this check (True >= 0 evaluates to True). If the LLM returns true in JSON, it deserializes to Python bool, passes validation, and propagates silently.

Suggested fix:

if not (isinstance(images_above, int) and not isinstance(images_above, bool) and images_above >= 0):
    raise ValueError(f"images_above bad: {images_above!r}")

The same pattern applies to pn in ascc_page_processor.py detect_page_number.

---

File: tools/fingerprint.sh, grep invocation (~line 48)

2>/dev/null was removed from the grep call. This means grep errors (e.g., permission-denied on individual files) now surface on stderr. While the intent is to catch scanner failures, grep exits with code 2 only for hard errors; permission-denied on some files still exits 0 or 1 depending on whether matches were also found. Files that cannot be read are silently skipped by grep rather than causing exit code 2, so the false-negative risk for unreadable files remains.

Suggested fix: Consider adding --no-messages or keeping stderr redirection while capturing stderr separately to distinguish grep infrastructure errors from permission issues on individual files. Alternatively, pre-check directory readability before scanning.

---

Security Vulnerabilities

File: tools/ascc_image_extract.py, tools/ascc_page_extract.py, tools/ascc_page_processor.py — Paths.__init__

The path-traversal guard checks for ".." as a substring, not just as a path component. This means "foo..bar" would be incorrectly rejected. More importantly, on some filesystems or after URL-decoding, encoded traversal sequences (e.g., %2F, null bytes) are not blocked. The check is reasonable for the stated threat model but should be documented as assuming already-decoded, plain string input.

Suggested fix: Use pathlib resolution to assert the constructed path stays within WIP_DIR:

resolved = (WIP_DIR / "cache" / basename).resolve()
if not str(resolved).startswith(str(WIP_DIR.resolve())):
    raise ValueError(...)

This is more robust than substring matching and eliminates false positives on names like "foo..bar".

---

File: tools/rebuild_staging_db.sh

WOCO_ADMIN_PASSWORD is exported into the environment of the uv run subprocess, making it visible in /proc/<pid>/environ on Linux for the lifetime of the process, and potentially in process-listing tools. This is a common and accepted trade-off, but worth noting for an internet-facing box.

Suggested fix: Consider passing the password via a temporary file with restricted permissions and having the Python code read it, or use a secrets manager. At minimum, document the exposure window in the script comments.