# Security Audit — v1.38.0 + post-v1.38 work

Audit performed against `main` @ `fe7126c4` (post-#1505/#1506).
Audit mode ON. Cross-referenced v1.36.2 SEC findings to avoid re-filing.

#### SEC-V1.38-1: `llm_config.api_base` logged with userinfo intact at info / debug level
- **Difficulty:** easy
- **Location:** `src/llm/local.rs:128-135, 164-171, 376-380`; `src/llm/hyde.rs:21-25`; `src/llm/summary.rs:27-31`; `src/llm/doc_comments.rs:144-148`
- **Description:** `LlmConfig::resolve` (`src/llm/mod.rs:280-323`) only synthesizes a redacted `safe_url` for the http/https warning path; it never strips userinfo from the stored `api_base` field. Five downstream sites then log the *raw* `api_base` via `%llm_config.api_base` / `%self.api_base`. If an operator sets `CQS_LLM_API_BASE=https://user:secret@vllm.internal/v1` (a documented form for self-hosted vLLM/OpenAI-compatible endpoints), every `LocalProvider::new` and every per-pass startup emits a `tracing::info!` line carrying the secret. With the daemon running under systemd-journald (the OB-V1.30-1 default), the credential lands in journald and is retained per the rotation policy. SEC-V1.36-2 closed the related "every CQS_* env var dumped" path for env vars but left the resolved-config field unguarded. Note also `tracing::debug!(?config, ...)` at `src/config.rs:758` re-logs the loaded config struct via `Debug`, surfacing the same field whenever debug logging is on.
- **Suggested fix:** Add a `redacted_api_base()` method on `LlmConfig` that uses the same scheme-find / `@`-find logic already in `resolve()`, then route every logging site through it. As a smaller patch, mutate `LlmConfig.api_base` to its redacted form *after* the `Client::builder()` build (the Client already has the original credentials embedded in its keep-alive connection); the field then becomes safe to log and `Debug`-print. Add a regression test that calls `LlmConfig::resolve` with a `user:pass@` URL and asserts neither `format!("{:?}", cfg)` nor `format!("{}", cfg.api_base)` contains `pass`.

#### SEC-V1.38-2: `parsed_anthropic_message` debug log can echo prompt content
- **Difficulty:** easy
- **Location:** `src/llm/batch.rs:86-93`
- **Description:** Partial follow-up to SEC-V1.36-2 / P2-#33. The raw HTTP error body is no longer logged, but `parsed.error.message` is logged at debug level as `parsed_anthropic_message`. Anthropic's 400-class responses regularly echo offending input fragments — schema validation errors include the failing JSON path values, and `invalid_request_error` for content policy includes the rejected content snippet. With `RUST_LOG=cqs=debug` (recommended in `CONTRIBUTING.md`), submitting a chunk that contains an embedded API key (real or simulated by the chunk) into the LLM batch can land that key in journald via Anthropic's echo. The `redacted_api_message` path correctly avoids this for the user-visible `LlmError::Api`; the debug log undoes the same protection.
- **Suggested fix:** Drop `parsed_anthropic_message` from the debug log; keep `body_len` + `parsed.error.type` (the structural code, e.g. `"invalid_request_error"`, which doesn't echo input). For local debugging the operator can read the raw response by re-running with `tracing::trace!` (which is gated even tighter than debug).

#### SEC-V1.38-3: `write_proposed_patch` non-atomic write — interrupted run leaves corrupt patch on disk
- **Difficulty:** easy
- **Location:** `src/doc_writer/rewriter.rs:608-617`
- **Description:** `--improve-docs` proposed-patch path uses a bare `std::fs::write(&patch_path, unified.as_bytes())`. A SIGINT / OOM kill / power loss between the `create_dir_all` and the syscall completion leaves a truncated `.patch` file at the final path. The user's review workflow is `git diff` followed by `git apply .cqs/proposed-docs/**/*.patch` (and per #1506, `<ref-dir>/proposed-docs/**/*.patch` for refs). `git apply` on a truncated unified diff produces partial source-file changes — silent corruption of source. The same module already has an `atomic_write` helper at line 634 (used by `rewrite_file`) that does temp-write + rename. The proposed-patch arm bypasses it.
- **Suggested fix:** Route line 617 through the existing `atomic_write` (`fn atomic_write(path: &Path, data: &[u8]) -> io::Result<()>`) or `crate::fs::atomic_replace`. Same one-line change; same crash-safety guarantee `--apply` already gets.

#### SEC-V1.38-4: `cmd_ref_update` skips the SEC-V1.30.1-6 symlink-redirect warning that `cmd_ref_add` carries
- **Difficulty:** medium
- **Location:** `src/cli/commands/infra/reference.rs:631-641` (vs. the original `cmd_ref_add` at `:222-260`)
- **Description:** `cmd_ref_add` (since #1222 / SEC-V1.30.1-6) emits a loud `tracing::warn!` + stderr `WARN:` when `dunce::canonicalize` redirects the user-supplied source path through a symlink. The matching `cmd_ref_update` arm reads `ref_config.source` (the *stored* path from `.cqs.toml`), checks `source.exists()` without canonicalizing, then hands it to `enumerate_files` which canonicalizes inside. If between `cqs ref add` and `cqs ref update` the source path got swapped to a symlink — e.g. an operator pointed `vendored-deps/` at `~/work/customer-A-private/` after the initial add — the update silently re-indexes the new target. No symlink notice fires, so the operator who pulls a poisoned ref from somewhere they didn't expect has no signal. The same window applies to `cmd_ref_reindex` per #1506 (calls into the same `cmd_ref_update` body via the new flag-parity path).
- **Suggested fix:** Lift `symlink_redirect_warning(&source_input, &canonical_source)` out of `cmd_ref_add` into a `pub(crate)` helper and call it from `cmd_ref_update` as well, after `enumerate_files` has canonicalized. The check is cheap (lexical normalization, no FS calls beyond the canonicalize already performed) and surfaces the same threat — vendored content swap — that SEC-V1.30.1-6 was filed for.

#### SEC-V1.38-5: `serve` `chunk_detail` / `static_asset` log user-controlled path strings at info level without size cap
- **Difficulty:** easy
- **Location:** `src/serve/handlers.rs:202`; `src/serve/assets.rs:58`
- **Description:** Both handlers emit `tracing::info!(chunk_id = %id, ...)` / `tracing::info!(path = %path, ...)` against axum-extracted path params. The auth middleware sits inside the route layer (good — these only fire post-auth), but the path values are unbounded user input. An attacker with a valid token (e.g. someone given temporary local access for pair debugging) can inflate journald by sending hundreds of KB-long path strings — each request appends them verbatim to the journal at info level (the daemon's default for OB-V1.30-1). A 100 KB path × 1000 requests = 100 MB of journal bloat per attacker session. Less load-bearing: the SEC-V1.30.1-2 OB-V1.30.1-10 fix demoted `serve::search` query-text logging from info → debug specifically because of journald retention; these two handlers were missed.
- **Suggested fix:** Cap the logged value at ~256 chars: `tracing::info!(chunk_id = %id.chars().take(256).collect::<String>(), ...)`. Or move the user-controlled value to debug level and log only `id_len` at info, mirroring the SEC-V1.30.1-2 pattern in `serve::search`.

#### SEC-V1.38-6: `validate_and_read_file` uses pre-canonicalize path for existence + size checks (TOCTOU window plus redundant existence check)
- **Difficulty:** medium
- **Location:** `src/cli/commands/io/read.rs:31-63`
- **Description:** Sequence: `dunce::canonicalize(&file_path)` (line 42) → `file_path.exists()` (line 47) → `std::fs::metadata(&file_path)` (line 53) → `std::fs::read_to_string(&canonical)` (line 62). Two issues. (1) `canonical` already proved `file_path` exists (`canonicalize` requires it), so line 47's `exists()` on the *unresolved* path is dead — and worse, on an interleaving where a symlink at `file_path` got swapped between line 42 and line 47, `exists()` could re-evaluate a now-broken link as false. (2) line 53 calls `metadata(&file_path)` (the *unresolved* path) for the size cap, but line 62 reads `&canonical`. A symlink swap between line 53 and line 62 — file_path now points to /etc/passwd, canonical still points to the original location — bypasses the size cap on the read. SECURITY.md acknowledges TOCTOU as a generic standard-FS-race, but the inconsistent use of `file_path` vs `canonical` widens the window unnecessarily.
- **Suggested fix:** After line 42, drop line 47 entirely (canonicalize already proved existence) and run line 53 against `&canonical` instead of `&file_path` (so the size check and the read both reference the same resolved inode). Optional belt: open the file once via `File::open(&canonical)` then derive metadata + content from the open handle, eliminating the residual race.

#### SEC-V1.38-7: `run_git_diff`'s base-ref check misses `--upload-pack=…` family of arg-injection refs
- **Difficulty:** easy
- **Location:** `src/cli/commands/mod.rs:574-582`
- **Description:** Validation rejects `b.starts_with('-')` and `b.contains('\0')`. That blocks `--upload-pack=...` because it starts with `-`. But `git diff` accepts `--` as an end-of-options sentinel; without it, refs that *contain* `..` or that match git's range syntax (`HEAD...attacker-controlled`) are valid. More relevant: refs containing `\n` / `\r` aren't blocked (git strips them at parse time, but the validation fails to assert that). The `b.starts_with('-')` check is also insufficient if a future caller ever prepends positional args before `b` — e.g. a refactor that adds `cmd.arg("--no-pager"); cmd.arg(b);` doesn't change behavior, but a refactor that reorders `cmd.arg(b); cmd.arg("--something");` makes `b = "--malicious"` reachable since `cmd.arg` doesn't shift the leading-dash check. The defensive layer is too thin.
- **Suggested fix:** After the existing checks, prepend the `--` sentinel before the user-supplied ref: `cmd.args(["--no-pager", "diff", "--no-color", "--", b])` — actually that doesn't work for `diff` since `--` separates revs from paths there; use `cmd.args(["--no-pager", "diff", "--no-color"]).arg(b)` and add a stricter regex check on `b` (alphanumeric + `._/~^@-` only, length ≤ 255). Also reject `\n`, `\r`, `\t` explicitly to match git's own ref-name validation rules per `git check-ref-format`.

#### SEC-V1.38-8: `tracing::debug!(path = %path.display(), ?config, "Loaded config")` dumps full Config including secrets-bearing fields
- **Difficulty:** easy
- **Location:** `src/config.rs:758`
- **Description:** The `Config` struct includes `llm_api_base: Option<String>` and is serialized via `?config` (Debug). With `RUST_LOG=cqs=debug` set, every `Config::load` call emits the entire struct including `llm_api_base` — and any `https://user:pass@host` userinfo therein. Same redaction gap as SEC-V1.38-1 but at the config-load site rather than the LLM-resolve site. The check applies even to read-only paths (every `cqs search` re-loads config), so the leak surface is every command, not just `cqs index --llm-summaries`.
- **Suggested fix:** Implement a hand-written `Debug` for `Config` that elides `llm_api_base`'s userinfo (delegate to the same redactor as SEC-V1.38-1's helper). Or remove the `?config` from this specific line — `path = %path.display()` is enough to confirm "we loaded a config from here" without spilling the contents.

#### SEC-V1.38-9: `enforce_concurrency_cap` cap defaults from env without validating upper bound
- **Difficulty:** medium
- **Location:** `src/serve/mod.rs:318-320`; `src/limits.rs::serve_max_concurrent_requests` (search by name)
- **Description:** `Semaphore::new(serve_max_concurrent_requests())` sizes the cap from a single env var read at startup. If an operator sets `CQS_SERVE_MAX_CONCURRENT_REQUESTS=4294967295` (or any unrealistically large number) the semaphore allocates a 32-bit counter that effectively disables the cap, defeating SEC-V1.36-9 / #1461. There's no max-bound validation. Combined with `serve --no-auth --bind 0.0.0.0` (an explicit but documented opt-in), an attacker can saturate the daemon with any number of concurrent in-flight requests, each pulling 64 KiB of body buffer + a SQLite read connection from the pool.
- **Suggested fix:** Cap the env override to a sane ceiling at parse time: `min(env_or_default, 1024)`. The semaphore stays correctly bounded regardless of operator-supplied values. Log a `tracing::warn!` when the env override would have exceeded the ceiling so the operator sees the clamp.

#### SEC-V1.38-10: `find_pdf_script` candidate-ownership check + python exec are racy under symlink swap
- **Difficulty:** medium
- **Location:** `src/convert/pdf.rs:88-141`
- **Description:** `validate_script_safety(&candidate)` (line 127) checks `metadata(path)` for uid + mode; the script then gets `Command::new(&python).arg(&script_path)` (line 30 via subsequent `convert_pdf_to_md`) where `script_path` may be the *string* of `candidate.to_string_lossy().to_string()`. Between the metadata check and the python interpreter `open()` of the script, a co-located attacker who can write to any directory in the candidate chain (e.g., `scripts/` if the user runs `cqs convert` from a project with a contributor-supplied `scripts/`) can replace `pdf_to_md.py` with a symlink to their own script. Python opens the symlink target, executes it. SEC-V1.25-8 added the ownership check but didn't address the swap window. Also: line 117-135 iterates candidates including `current_exe()'s ../scripts/pdf_to_md.py`, which on a writable install dir (`~/.cargo/bin/`) is exploitable if anyone else writes to that dir. Documented as a generic FS race in SECURITY.md, but the gap is wider than the doc suggests because the check + exec aren't on the same fd.
- **Suggested fix:** Open the script with `File::open(path)` after canonicalize, fstat the open fd via `metadata().unwrap()` on the file (not the path) — `File::metadata` runs against the inode, not the path, closing the symlink-swap window. Then pass `/proc/self/fd/<n>` (Linux) or the canonicalized path (other platforms) to python so the same inode is what gets executed. Alternative: ship the script embedded via `include_str!` exactly as `umap.rs` already does (`UMAP_SCRIPT`), eliminating both the on-disk script and the entire ownership-check problem. The PDF script is small enough to embed.

---

## Summary

10 findings against current `main` (post-#1505/#1506). Themes:

- **Userinfo leakage in API base logs** (SEC-V1.38-1, SEC-V1.38-8): `LlmConfig::api_base` is logged raw at five sites and the full `Config` is `?config`-printed at debug; no redactor on the field itself. Fix: redact at the type level (`Debug` impl + helper getter) so all sites are protected.
- **Prompt-content echo via Anthropic error messages** (SEC-V1.38-2): SEC-V1.36-2's `body=%body` removal didn't cover the parsed `error.message` field, which Anthropic populates with input fragments. Drop it.
- **Patch-write atomicity** (SEC-V1.38-3): #1506's `proposed-docs` patches use bare `fs::write`. Crash mid-write → corrupt diff → silent source-file damage on `git apply`. Reuse the existing `atomic_write` helper in the same file.
- **Symlink-redirect notice missing on update path** (SEC-V1.38-4): SEC-V1.30.1-6 only landed in `cmd_ref_add`. `cmd_ref_update` (and #1506's reindex parity arm) silently re-indexes through swapped symlinks.
- **Path TOCTOU + redundant checks in `cqs read`** (SEC-V1.38-6): inconsistent use of `file_path` vs `canonical` widens the symlink-swap window between size-cap and read.
- **Defensive layers thinning out**: `git diff` ref validation (SEC-V1.38-7) and `cqs serve` concurrency cap (SEC-V1.38-9) both have validation that's barely enough today and breaks under foreseeable refactors / hostile env vars.
- **Subprocess script swap window** (SEC-V1.38-10): SEC-V1.25-8 closed the ownership check but the validate-then-exec gap remains; embed the script (mirror `umap.rs`).
- **Path-param log inflation** (SEC-V1.38-5): two serve handlers missed the OB-V1.30.1-10 demote-to-debug pattern; cap the value or move to debug.

No prompt-injection sandbox regressions found — `sanitize_untrusted` and the per-prompt nonce sentinel still hold (the speculatively-mentioned `build_prompt_with_envelope` from the audit prompt does not exist in the current tree). #1506's `<ref-dir>/proposed-docs/<rel>.patch` construction is structurally safe because chunks are canonicalized at index time, but the non-atomic write (SEC-V1.38-3) is the real defect on that path. #1505's `--model` drift check is sound — it only adds a refuse-to-run guard, no new attack surface.
