fix(security): harden snapshot ids, auto-merge args and diagnostics reads

Defense-in-depth hardening from a code-scanning review; none of these is
a reachable vulnerability today.

- SnapshotId: the field is private and SnapshotId::parse is the only
  constructor. It accepts exactly 40 or 64 hex digits, so every git
  revision argument is a full object id. Git calls that take the id also
  pass --end-of-options. The runtime API's id check now uses the same
  predicate instead of a copy.
- Auto-merge check: validate repo (owner/name), pr (positive number) and
  agent (short role token) in Rust before python3 is spawned. The runtime
  route answers 400 for a malformed request, and evaluate_auto_merge
  denies it as well so no caller can reach the spawn with bad input.
- Diagnostics log/crash reads: open with O_NOFOLLOW (and O_NONBLOCK so a
  FIFO cannot hang the open), then take metadata from the opened handle.
  This removes the window between the symlink check and the open.
- Add .github/codeql/codeql-config.yml that excludes test paths from
  code scanning. It only takes effect once an advanced-setup workflow
  passes it as config-file.

Checks: cargo test -p codewhale-tui --lib -- snapshot operate::
runtime_api::diagnostics diagnostics_list_and_read revert_file_helper
restore_snapshot: 316 passed, 0 failed, 1 ignored. rustfmt --check clean
on touched files. The npm test / check:web gate was not run (Rust-only
change).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

.github/codeql/codeql-config.yml

@@ -0,0 +1,33 @@
+# CodeQL configuration for code scanning.
+#
+# Test code is out of scope for security alerts: it runs only in CI, talks to
+# in-process loopback servers, and routinely prints fixture secrets in
+# assertion messages to prove they are redacted elsewhere. Those paths are
+# excluded here so alerts point at product code.
+#
+# Inline `#[cfg(test)] mod tests` blocks inside product files cannot be
+# excluded by path; alerts there are dismissed as "used in tests".
+#
+# This file takes effect only with CodeQL advanced setup: the analyze step's
+# `github/codeql-action/init` must pass
+#   config-file: ./.github/codeql/codeql-config.yml
+# Default setup ignores it.
+name: codewhale-codeql
+
+paths-ignore:
+  # Rust integration tests and split-out unit test modules.
+  - "**/tests/**"
+  - "**/tests.rs"
+  - "**/*_tests.rs"
+  - "**/test_support.rs"
+  - "**/*_test_support.rs"
+  # JavaScript / TypeScript test suites.
+  - "**/test/**"
+  - "**/__tests__/**"
+  - "**/*.test.js"
+  - "**/*.test.mjs"
+  - "**/*.test.ts"
+  - "**/*.test.tsx"
+  - "**/*.spec.js"
+  - "**/*.spec.ts"
+  - "**/*.spec.tsx"

crates/tui/src/commands/contract.rs

@@ -3076,7 +3076,7 @@ impl CommandSkillGroupContext for SkillGroupAdapter<'_> {
         Ok(snapshots
             .into_iter()
             .map(|snapshot| SnapshotEntry {
-                id: snapshot.id.0,
+                id: snapshot.id.into_string(),
                 label: snapshot.label,
                 timestamp: snapshot.timestamp,
             })
@@ -3094,7 +3094,9 @@ impl CommandSkillGroupContext for SkillGroupAdapter<'_> {
                 ));
             }
         };
-        repo.restore(&crate::snapshot::SnapshotId(id.to_string()))
+        let id = crate::snapshot::SnapshotId::parse(id)
+            .map_err(|err| format!("Restore failed: {err}"))?;
+        repo.restore(&id)
             .map_err(|err| format!("Restore failed: {err}"))
     }
 

crates/tui/src/core/turn.rs

@@ -666,7 +666,7 @@ fn snapshot_with_label(
         Ok(repo) => {
             clear_snapshots_disabled_status(workspace, session_id);
             let id = match repo.snapshot_with_session(label, session_id) {
-                Ok(id) => Some(id.0),
+                Ok(id) => Some(id.into_string()),
                 Err(e) => {
                     tracing::warn!(target: "snapshot", "snapshot '{label}' failed: {e}");
                     return None;

crates/tui/src/operate.rs

@@ -772,10 +772,61 @@ pub fn auto_merge_pr_args(repo: &str, pr: &str, agent: &str) -> Vec<String> {
     ]
 }
 
+/// Strictly validate an auto-merge request before any of it reaches argv.
+///
+/// The checker is spawned without a shell, but these values still become
+/// arguments to `python3` and then to `gh`, so they are held to the shapes
+/// GitHub itself allows: `repo` is `owner/name`, `pr` is a positive decimal
+/// number, and `agent` is a short role token. No value may start with `-`.
+pub fn validate_auto_merge_request(request: &AutoMergeRequest<'_>) -> Result<(), String> {
+    fn is_owner(owner: &str) -> bool {
+        (1..=39).contains(&owner.len())
+            && !owner.starts_with('-')
+            && owner
+                .bytes()
+                .all(|b| b.is_ascii_alphanumeric() || b == b'-')
+    }
+    fn is_repo_name(name: &str) -> bool {
+        (1..=100).contains(&name.len())
+            && name != "."
+            && name != ".."
+            && !name.starts_with('-')
+            && name
+                .bytes()
+                .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.'))
+    }
+    let repo_ok = request
+        .repo
+        .split_once('/')
+        .is_some_and(|(owner, name)| is_owner(owner) && is_repo_name(name));
+    if !repo_ok {
+        return Err("repo must be `owner/name` using GitHub name characters".to_string());
+    }
+    let pr_ok = (1..=10).contains(&request.pr.len())
+        && request.pr.bytes().all(|b| b.is_ascii_digit())
+        && request.pr.parse::<u64>().is_ok_and(|n| n > 0);
+    if !pr_ok {
+        return Err("pr must be a positive pull request number".to_string());
+    }
+    let agent_ok = (1..=64).contains(&request.role.len())
+        && !request.role.starts_with('-')
+        && request
+            .role
+            .bytes()
+            .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_'));
+    if !agent_ok {
+        return Err("agent must be 1-64 letters, digits, `-` or `_`".to_string());
+    }
+    Ok(())
+}
+
 pub fn evaluate_auto_merge(
     request: AutoMergeRequest<'_>,
     checker: Option<&Path>,
 ) -> AutoMergeDecision {
+    if let Err(reason) = validate_auto_merge_request(&request) {
+        return AutoMergeDecision::Deny { reason };
+    }
     let Some(checker) = checker else {
         return AutoMergeDecision::Deny {
             reason: "auto-merge checker missing; fail-closed".to_string(),
@@ -2054,6 +2105,52 @@ api_key_env = "CW_OPERATE_MISSING_TEST_KEY"
         let _ = discover_auto_merge_checker(Path::new("/no-ops-here"));
     }
 
+    #[test]
+    fn auto_merge_request_fields_are_validated_before_spawn() {
+        let ok = |repo, pr, role| {
+            validate_auto_merge_request(&AutoMergeRequest { pr, role, repo }).is_ok()
+        };
+        assert!(ok("Hmbown/CodeWhale", "1234", "keel"));
+        assert!(ok("a-b/c.d_e-f", "1", "scout_2"));
+        for (repo, pr, role) in [
+            ("Hmbown", "1", "keel"),
+            ("a/b/../../x", "1", "keel"),
+            ("-x/y", "1", "keel"),
+            ("x/-y", "1", "keel"),
+            ("x/..", "1", "keel"),
+            ("x y/z", "1", "keel"),
+            ("x/y", "0", "keel"),
+            ("x/y", "-1", "keel"),
+            ("x/y", "1 2", "keel"),
+            ("x/y", "12345678901", "keel"),
+            ("x/y", "", "keel"),
+            ("x/y", "1", ""),
+            ("x/y", "1", "--fixture=/x"),
+            ("x/y", "1", "keel ops"),
+        ] {
+            assert!(
+                !ok(repo, pr, role),
+                "{repo:?} {pr:?} {role:?} must be rejected"
+            );
+        }
+        // A malformed request is denied even when a checker exists, so the
+        // checker is never spawned with it.
+        let dir = TempDir::new().expect("temp");
+        let checker = dir.path().join("check-auto-merge.py");
+        fs::write(&checker, "import sys\nsys.exit(0)\n").expect("write");
+        assert!(matches!(
+            evaluate_auto_merge(
+                AutoMergeRequest {
+                    pr: "1",
+                    role: "--policy=x",
+                    repo: "x/y",
+                },
+                Some(&checker),
+            ),
+            AutoMergeDecision::Deny { .. }
+        ));
+    }
+
     #[test]
     fn checker_exit_zero_allows() {
         let dir = TempDir::new().expect("temp");

crates/tui/src/runtime_api.rs

@@ -5139,6 +5139,12 @@ async fn check_operate_auto_merge(
     State(state): State<RuntimeApiState>,
     Json(req): Json<OperateAutoMergeCheckRequest>,
 ) -> Result<Json<OperateAutoMergeCheckView>, ApiError> {
+    crate::operate::validate_auto_merge_request(&crate::operate::AutoMergeRequest {
+        repo: &req.repo,
+        pr: &req.pr,
+        role: &req.agent,
+    })
+    .map_err(ApiError::bad_request)?;
     let checker = crate::operate::discover_auto_merge_checker(&state.workspace);
     let repo = req.repo.clone();
     let pr = req.pr.clone();
@@ -5596,7 +5602,7 @@ async fn revert_thread_file(
 }
 
 fn snapshot_id_is_well_formed(id: &str) -> bool {
-    matches!(id.len(), 40 | 64) && id.bytes().all(|b| b.is_ascii_hexdigit())
+    crate::snapshot::SnapshotId::is_well_formed(id)
 }
 
 fn expected_hash_is_well_formed(hash: &str) -> bool {
@@ -7200,7 +7206,8 @@ async fn restore_snapshot(
 fn restore_snapshot_for_workspace(workspace: &FsPath, id: &str) -> Result<(), ApiError> {
     let repo = crate::snapshot::SnapshotRepo::open_or_init(workspace)
         .map_err(|e| ApiError::internal(format!("Snapshot repo init failed: {e}")))?;
-    let snapshot_id = crate::snapshot::SnapshotId(id.to_string());
+    let snapshot_id = crate::snapshot::SnapshotId::parse(id)
+        .map_err(|e| ApiError::bad_request(format!("Invalid snapshot id: {e}")))?;
     repo.restore(&snapshot_id)
         .map_err(|e| ApiError::internal(format!("Snapshot restore failed: {e}")))
 }

crates/tui/src/runtime_api/diagnostics.rs

@@ -121,10 +121,16 @@ fn list_files(dir: &FsPath, cap: usize) -> Vec<FileEntry> {
 /// loading the whole file. Symlinks are never followed.
 fn read_named_window(dir: &FsPath, name: &str, query: FileReadQuery) -> Result<Value, ApiError> {
     let path = dir.join(name);
-    let metadata = std::fs::symlink_metadata(&path).map_err(|error| match error.kind() {
+    // Open first without following a final symlink, then take metadata from
+    // the handle, so the checked file is the file that is read.
+    let mut file = open_no_follow(&path).map_err(|error| match error.kind() {
         std::io::ErrorKind::NotFound => ApiError::not_found("file not found"),
-        _ => ApiError::internal(format!("file access failed: {error}")),
+        _ if is_symlink_refusal(&error) => ApiError::forbidden("not a regular file"),
+        _ => ApiError::internal(format!("file open failed: {error}")),
     })?;
+    let metadata = file
+        .metadata()
+        .map_err(|error| ApiError::internal(format!("file access failed: {error}")))?;
     if metadata.file_type().is_symlink() || !metadata.is_file() {
         return Err(ApiError::forbidden("not a regular file"));
     }
@@ -145,8 +151,6 @@ fn read_named_window(dir: &FsPath, name: &str, query: FileReadQuery) -> Result<V
         (None, Some(tail)) => size.saturating_sub(tail.min(size)),
         (None, None) => 0,
     };
-    let mut file = File::open(&path)
-        .map_err(|error| ApiError::internal(format!("file open failed: {error}")))?;
     file.seek(SeekFrom::Start(offset))
         .map_err(|error| ApiError::internal(format!("file seek failed: {error}")))?;
     let mut window = Vec::with_capacity(limit.min(64 * 1024));
@@ -167,6 +171,39 @@ fn read_named_window(dir: &FsPath, name: &str, query: FileReadQuery) -> Result<V
     }))
 }
 
+/// Open `path` read-only without following a symlink in its final component.
+/// `O_NONBLOCK` keeps a FIFO planted under the name from hanging the open;
+/// the regular-file check on the handle rejects it afterwards.
+fn open_no_follow(path: &FsPath) -> std::io::Result<File> {
+    let mut options = std::fs::OpenOptions::new();
+    options.read(true);
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::OpenOptionsExt as _;
+        options.custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC | libc::O_NONBLOCK);
+    }
+    #[cfg(windows)]
+    {
+        use std::os::windows::fs::OpenOptionsExt as _;
+        use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_OPEN_REPARSE_POINT;
+        options.custom_flags(FILE_FLAG_OPEN_REPARSE_POINT);
+    }
+    options.open(path)
+}
+
+/// `O_NOFOLLOW` on a symlink fails with `ELOOP` (and `EMLINK` on some BSDs).
+fn is_symlink_refusal(error: &std::io::Error) -> bool {
+    #[cfg(unix)]
+    {
+        matches!(error.raw_os_error(), Some(code) if code == libc::ELOOP || code == libc::EMLINK)
+    }
+    #[cfg(not(unix))]
+    {
+        let _ = error;
+        false
+    }
+}
+
 // ---------------------------------------------------------------------------
 // Directories
 // ---------------------------------------------------------------------------
@@ -355,3 +392,52 @@ pub(super) async fn process_info(State(_state): State<RuntimeApiState>) -> Json<
         "rss_bytes": rss,
     }))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use axum::http::StatusCode;
+
+    fn whole_file() -> FileReadQuery {
+        FileReadQuery {
+            offset: None,
+            limit: None,
+            tail: None,
+        }
+    }
+
+    #[test]
+    fn named_window_reads_a_regular_file() {
+        let dir = tempfile::TempDir::new().expect("temp");
+        std::fs::write(dir.path().join("a.log"), b"hello").expect("write");
+        let body = read_named_window(dir.path(), "a.log", whole_file()).expect("read");
+        assert_eq!(body["bytes"], 5);
+        assert_eq!(body["size"], 5);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn named_window_refuses_a_symlink_at_open() {
+        let dir = tempfile::TempDir::new().expect("temp");
+        let outside = tempfile::TempDir::new().expect("temp");
+        let secret = outside.path().join("secret");
+        std::fs::write(&secret, b"do not serve").expect("write");
+        std::os::unix::fs::symlink(&secret, dir.path().join("a.log")).expect("symlink");
+        let error = read_named_window(dir.path(), "a.log", whole_file())
+            .expect_err("symlink must be refused");
+        assert_eq!(error.status, StatusCode::FORBIDDEN);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn named_window_refuses_a_fifo_without_blocking() {
+        let dir = tempfile::TempDir::new().expect("temp");
+        let fifo = dir.path().join("a.log");
+        let c_path = std::ffi::CString::new(fifo.as_os_str().as_encoded_bytes()).expect("cstr");
+        // SAFETY: `c_path` is a valid NUL-terminated path for the call.
+        assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, 0);
+        let error =
+            read_named_window(dir.path(), "a.log", whole_file()).expect_err("fifo must be refused");
+        assert_eq!(error.status, StatusCode::FORBIDDEN);
+    }
+}

crates/tui/src/snapshot/repo.rs

@@ -22,11 +22,38 @@ use crate::dependencies::ExternalTool;
 
 use super::paths::{ensure_snapshot_dir, snapshot_git_dir};
 
-/// Identifier for a snapshot — currently the underlying git commit SHA.
+/// Identifier for a snapshot — the underlying git commit id.
+///
+/// The field is private: [`SnapshotId::parse`] is the only way to build one,
+/// so every value handed to `git` as a revision is a full SHA-1 or SHA-256
+/// hex object id and can never be read as an option or a revision expression.
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub struct SnapshotId(pub String);
+pub struct SnapshotId(String);
 
 impl SnapshotId {
+    /// Accept exactly a full hex object id: 40 (SHA-1) or 64 (SHA-256)
+    /// ASCII hex digits. Anything else is `InvalidInput`.
+    pub fn parse(id: &str) -> io::Result<Self> {
+        if Self::is_well_formed(id) {
+            Ok(Self(id.to_string()))
+        } else {
+            Err(io::Error::new(
+                io::ErrorKind::InvalidInput,
+                "snapshot id must be a full hexadecimal commit id",
+            ))
+        }
+    }
+
+    /// Whether `id` would be accepted by [`SnapshotId::parse`].
+    pub fn is_well_formed(id: &str) -> bool {
+        matches!(id.len(), 40 | 64) && id.bytes().all(|b| b.is_ascii_hexdigit())
+    }
+
+    /// Take the id string out.
+    pub fn into_string(self) -> String {
+        self.0
+    }
+
     /// Borrow the SHA as a string slice.
     pub fn as_str(&self) -> &str {
         &self.0
@@ -504,7 +531,11 @@ impl SnapshotRepo {
             )));
         }
 
-        Ok(SnapshotId(sha))
+        SnapshotId::parse(&sha).map_err(|_| {
+            io_other(format!(
+                "git commit-tree returned a malformed commit id: {sha:?}"
+            ))
+        })
     }
 
     /// Prefix a snapshot label with its owning session id, if any.
@@ -614,7 +645,7 @@ impl SnapshotRepo {
         let checkout = run_git(
             &self.git_dir,
             &self.work_tree,
-            &["checkout", id.as_str(), "--", ":/"],
+            &["checkout", "--end-of-options", id.as_str(), "--", ":/"],
         )?;
         if !checkout.status.success() {
             return Err(io_other(format!(
@@ -674,6 +705,7 @@ impl SnapshotRepo {
                 "--literal-pathspecs",
                 "ls-tree",
                 "-z",
+                "--end-of-options",
                 id.as_str(),
                 "--",
                 rel.to_str()
@@ -726,6 +758,7 @@ impl SnapshotRepo {
                         "--literal-pathspecs",
                         "diff",
                         "--quiet",
+                        "--end-of-options",
                         id.as_str(),
                         "--",
                         rel.as_str(),
@@ -826,6 +859,7 @@ impl SnapshotRepo {
             let mut args: Vec<String> = vec![
                 "--literal-pathspecs".to_string(),
                 "checkout".to_string(),
+                "--end-of-options".to_string(),
                 id.as_str().to_string(),
                 "--".to_string(),
             ];
@@ -893,7 +927,14 @@ impl SnapshotRepo {
         let diff = run_git(
             &self.git_dir,
             &self.work_tree,
-            &["diff", "--stat", id.as_str(), "--", ":/"],
+            &[
+                "diff",
+                "--stat",
+                "--end-of-options",
+                id.as_str(),
+                "--",
+                ":/",
+            ],
         )?;
         if !diff.status.success() {
             return Err(io_other(format!(
@@ -918,7 +959,14 @@ impl SnapshotRepo {
         let diff = run_git(
             &self.git_dir,
             &self.work_tree,
-            &["diff", "--quiet", id.as_str(), "--", ":/"],
+            &[
+                "diff",
+                "--quiet",
+                "--end-of-options",
+                id.as_str(),
+                "--",
+                ":/",
+            ],
         )?;
         git_diff_matches(diff)
     }
@@ -927,7 +975,14 @@ impl SnapshotRepo {
         let ls = run_git(
             &self.git_dir,
             &self.work_tree,
-            &["ls-tree", "-r", "-z", "--name-only", treeish],
+            &[
+                "ls-tree",
+                "-r",
+                "-z",
+                "--name-only",
+                "--end-of-options",
+                treeish,
+            ],
         )?;
         if !ls.status.success() {
             return Err(io_other(format!(
@@ -1018,12 +1073,14 @@ impl SnapshotRepo {
                 .and_then(|s| s.parse::<i64>().ok())
                 .unwrap_or(0);
             let subject = parts.next().unwrap_or("").to_string();
-            if sha.is_empty() {
+            // `git log --pretty=format:%H` only emits full hex ids; skip anything
+            // else rather than let it become a revision argument later.
+            let Ok(id) = SnapshotId::parse(&sha) else {
                 continue;
-            }
+            };
             let (session_id, label) = Self::decode_session_label(&subject);
             out.push(Snapshot {
-                id: SnapshotId(sha),
+                id,
                 label,
                 timestamp: ts,
                 session_id,
@@ -1543,6 +1600,27 @@ mod tests {
     use std::fs::{File, FileTimes};
     use tempfile::tempdir;
 
+    #[test]
+    fn snapshot_id_parse_accepts_only_full_hex_object_ids() {
+        let sha1 = "0123456789abcdefABCDEF0123456789abcdef01";
+        let sha256 = "a".repeat(64);
+        assert_eq!(SnapshotId::parse(sha1).expect("sha1").as_str(), sha1);
+        assert!(SnapshotId::parse(&sha256).is_ok());
+        for bad in [
+            "",
+            "HEAD",
+            "abc123",
+            "--output=/tmp/x",
+            "-0123456789abcdef0123456789abcdef0123456",
+            "0123456789abcdef0123456789abcdef0123456g",
+            "0123456789abcdef0123456789abcdef01234567~1",
+            "0123456789abcdef0123456789abcdef012345678",
+        ] {
+            let err = SnapshotId::parse(bad).expect_err(bad);
+            assert_eq!(err.kind(), io::ErrorKind::InvalidInput, "{bad:?}");
+        }
+    }
+
     /// Holds the home directory pinned to a tempdir for the lifetime of a test. Also
     /// owns the process-wide env-var mutex so tests across modules
     /// don't trample each other's home env vars.