zesdex/docs/superpowers/plans/2026-07-16-security-quickfixes.md

# Security Quick-Fixes Implementation Plan

> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.

**Goal:** Fix the two standalone, low-risk findings from the 2026-07-16 audit that don't require touching the OAuth/session/CMS architecture: the misleading doc-comment on the `bash` tool's credential-read behavior, and the rate limiter trusting client-controlled `X-Forwarded-For`/`X-Real-IP` headers.

**Architecture:** No structural changes. Both fixes are localized to a single file each.

**Tech Stack:** Rust, Cargo workspace (`zesdex-backend`, `zesdex-middleware`).

## Global Constraints

- No `#[allow(...)]` lint-bypass attributes may be introduced (workspace `Cargo.toml` denies `dead_code`/`unused`; CLAUDE.md forbids bypass annotations outright).
- Every new/changed `pub fn` needs an accurate doc comment (What/Flow/Why/Return per CLAUDE.md's Code Documentation section).
- Tests are inline `#[cfg(test)] mod tests` blocks in the same file, per CLAUDE.md.
- Run `cargo clippy --workspace --all-targets -- -D warnings` and `cargo test --workspace` before each commit in this plan.

---

### Task 1: Fix misleading doc comment on `Bash::run` re: credential reads

**Context:** The audit flagged `crates/zesdex-backend/src/tool/shell_filter/credentials.rs`'s `check_credential_read` as "dead code that contradicts CLAUDE.md's claim that shell_filter blocks credential leaks." On closer reading, this is **not a behavior bug** — `crates/zesdex-backend/src/tool/shell.rs` has a deliberate, reasoned inline comment (lines 71-73) explaining that credential reads are intentionally allowed locally (the AI needs access; the real threat is committing secrets to a public repo, handled elsewhere). The actual defect is narrower: the doc comment on `run()` (lines 49-51) claims it calls `check_credential_read` when it doesn't, and CLAUDE.md's "Key Patterns" section overstates what `shell_filter` does. This task corrects both to match actual (intentional) behavior — it does **not** change runtime behavior.

**Files:**
- Modify: `crates/zesdex-backend/src/tool/shell.rs:47-59`
- Modify: `/mnt/code/zesdex/CLAUDE.md` (the "Shell safety" line under "Key Patterns")
- Modify: `crates/zesdex-backend/src/tool/shell_filter/credentials.rs` (module doc comment, to mark it as intentionally unused-by-`shell.rs` rather than implying it's wired in)

**Interfaces:**
- Consumes: nothing new.
- Produces: nothing new (doc-only change). No downstream task depends on this.

- [ ] **Step 1: Read the current state to confirm line numbers haven't drifted**

Run: `grep -n "check_credential_read\|Only gate destructive" crates/zesdex-backend/src/tool/shell.rs`
Expected output includes the doc comment around line 49 and the inline comment around line 71.

- [ ] **Step 2: Fix the stale doc comment on `run()`**

In `crates/zesdex-backend/src/tool/shell.rs`, replace:

```rust
    /// Run a bash command (foreground or background) with safety filters and a timeout.
    ///
    /// Flow: extract args → run `check_credential_read` then `check_git_destructive`
    ///       (bail if either rejects) → branch on `run_in_background`: if true, hand off
    ///       to the bg-bash subsystem and return the job ID; else spawn `bash -c`,
    ///       poll with `try_wait`, kill on timeout, format combined stdout+stderr.
    ///
    /// Why: the safety filters run unconditionally so background jobs are also gated;
    /// the timeout is enforced by polling the child rather than relying on a libc alarm
    /// so cleanup stays in Rust.
    ///
    /// Return: exit-code + elapsed-seconds summary line (plus captured output) for
    /// foreground runs, or the job ID for background runs.
```

with:

```rust
    /// Run a bash command (foreground or background) with a safety filter and a timeout.
    ///
    /// Flow: extract args → run `check_git_destructive` (bail if it rejects) → branch on
    ///       `run_in_background`: if true, hand off to the bg-bash subsystem and return the
    ///       job ID; else spawn `bash -c`, poll with `try_wait`, kill on timeout, format
    ///       combined stdout+stderr.
    ///
    /// Why: only destructive git operations are gated here — credential-file reads
    /// (`~/.ssh/id_rsa`, `.netrc`, etc.) are deliberately NOT blocked, since the agent
    /// often needs to read local config for legitimate debugging; the real leak vector
    /// (committing secrets to a remote) is handled by git hooks/user review, not this
    /// tool. `shell_filter::credentials::check_credential_read` exists but is
    /// intentionally not called from here — see its module doc comment. The safety
    /// filter runs unconditionally so background jobs are also gated; the timeout is
    /// enforced by polling the child rather than relying on a libc alarm so cleanup
    /// stays in Rust.
    ///
    /// Return: exit-code + elapsed-seconds summary line (plus captured output) for
    /// foreground runs, or the job ID for background runs.
```

- [ ] **Step 3: Mark `check_credential_read` as intentionally unused, not dead**

Read `crates/zesdex-backend/src/tool/shell_filter/credentials.rs` in full first:

Run: `cat crates/zesdex-backend/src/tool/shell_filter/credentials.rs`

Add a module-level doc comment at the top of the file (before any existing doc comment on `check_credential_read` itself — do not remove the existing function-level doc, just add context above it):

```rust
//! Credential-file-read detection.
//!
//! Not currently called from `tool::shell::Bash::run` — see that function's
//! doc comment for why credential reads are intentionally allowed. This
//! module is kept for callers that DO want to block credential reads (e.g.
//! a future sandboxed/untrusted-tool execution path) and is covered by its
//! own inline tests below.
```

- [ ] **Step 4: Fix CLAUDE.md's overstated claim**

In `/mnt/code/zesdex/CLAUDE.md`, find the line under "Key Patterns":

```
- **Shell safety** — `tool/shell_filter/` blocks credential leaks and destructive git commands.
```

Replace with:

```
- **Shell safety** — `tool/shell_filter/` blocks destructive git commands (`shell_filter::git::check_git_destructive`, called from `tool/shell.rs::Bash::run`). It also contains a `check_credential_read` detector for credential-file reads, but that one is intentionally NOT wired into `Bash::run` today — see the doc comment on `Bash::run` for why.
```

- [ ] **Step 5: Verify the crate still builds and lints clean**

Run: `cargo check -p zesdex-backend`
Expected: no errors (doc-only + comment changes).

Run: `cargo clippy -p zesdex-backend -- -D warnings`
Expected: no new warnings.

- [ ] **Step 6: Commit**

```bash
git add crates/zesdex-backend/src/tool/shell.rs crates/zesdex-backend/src/tool/shell_filter/credentials.rs CLAUDE.md
git commit -m "docs(shell): perbaiki doc comment shell_filter yang menyesatkan soal credential-read"
```

---

### Task 2: Stop trusting client-supplied `X-Forwarded-For`/`X-Real-IP` in the rate limiter

**Context:** `crates/zesdex-middleware/src/rate_limit.rs` derives its per-client bucket key from `X-Forwarded-For`/`X-Real-IP` headers before falling back to the real socket address. Since this middleware isn't behind a trusted reverse proxy today (confirmed: no proxy config anywhere in the workspace), any direct caller can forge these headers to get a fresh rate-limit bucket on every request. This crate is currently unused/orphaned (no axum server exists yet to mount it on — see the separate `2026-07-16-middleware-axum-server.md` plan for that), but the fix belongs here as a standalone code-correctness task since it doesn't depend on that server existing.

**Files:**
- Modify: `crates/zesdex-middleware/src/rate_limit.rs`

**Interfaces:**
- Consumes: nothing new.
- Produces: `RateLimiter`/`RateLimitLayer` public API unchanged in shape; only the client-id derivation logic changes. Any future caller (including the axum-server plan) must pass `ConnectInfo<SocketAddr>` — note this for that plan.

- [ ] **Step 1: Read the current implementation**

Run: `cat crates/zesdex-middleware/src/rate_limit.rs`

Confirm the client-id extraction logic (around lines 190-210 per the audit) checks `X-Forwarded-For` first, then `X-Real-IP`, then falls back to the connection's socket address.

- [ ] **Step 2: Write the failing test**

Add to the `#[cfg(test)] mod tests` block at the bottom of `crates/zesdex-middleware/src/rate_limit.rs` (create the block if none exists yet — confirm via the Step 1 read):

```rust
    #[test]
    fn client_id_ignores_spoofed_forwarded_headers_by_default() {
        // A request carrying a spoofed X-Forwarded-For must NOT be treated
        // as a distinct client from one with a different spoofed value —
        // both should resolve to the same real socket address.
        let socket_addr: std::net::SocketAddr = "127.0.0.1:9999".parse().unwrap();
        let mut headers_a = axum::http::HeaderMap::new();
        headers_a.insert("x-forwarded-for", "1.2.3.4".parse().unwrap());
        let mut headers_b = axum::http::HeaderMap::new();
        headers_b.insert("x-forwarded-for", "5.6.7.8".parse().unwrap());

        let id_a = client_id(&headers_a, socket_addr, false);
        let id_b = client_id(&headers_b, socket_addr, false);

        assert_eq!(
            id_a, id_b,
            "client_id must key on the real socket address when trust_proxy_headers is false, \
             not on attacker-controlled X-Forwarded-For"
        );
    }

    #[test]
    fn client_id_uses_forwarded_header_when_trust_enabled() {
        // When explicitly told to trust a fronting proxy, the header value
        // should be used (this is the opt-in, documented-risk path).
        let socket_addr: std::net::SocketAddr = "127.0.0.1:9999".parse().unwrap();
        let mut headers = axum::http::HeaderMap::new();
        headers.insert("x-forwarded-for", "1.2.3.4".parse().unwrap());

        let id = client_id(&headers, socket_addr, true);
        assert_eq!(id, "1.2.3.4");
    }
```

- [ ] **Step 3: Run the test to verify it fails**

Run: `cargo test -p zesdex-middleware client_id_ignores_spoofed -- --nocapture`
Expected: compile error (`client_id` doesn't yet take a `trust_proxy_headers: bool` parameter) or, if the function already exists without that parameter, a straightforward assertion failure since headers are currently trusted unconditionally.

- [ ] **Step 4: Add a `trust_proxy_headers` flag and make header-trust opt-in**

Locate the existing client-id derivation function (from Step 1) and change its signature to take an explicit trust flag, defaulting callers to `false`. Replace the header-first logic with:

```rust
/// Derive the rate-limit bucket key for one request.
///
/// Flow: if `trust_proxy_headers` is true, use `X-Forwarded-For` (first
///       hop) then `X-Real-IP`; otherwise always use the real connection
///       socket address, ignoring any client-supplied headers.
///
/// Why: without a trusted reverse proxy stripping/overwriting these
/// headers, they are attacker-controlled — trusting them by default lets
/// any direct caller reset their own rate-limit bucket on every request.
/// `trust_proxy_headers` must only be set to `true` when this middleware
/// sits behind a proxy that is known to overwrite (not merge) these headers.
fn client_id(
    headers: &axum::http::HeaderMap,
    socket_addr: std::net::SocketAddr,
    trust_proxy_headers: bool,
) -> String {
    if trust_proxy_headers {
        if let Some(fwd) = headers
            .get("x-forwarded-for")
            .and_then(|v| v.to_str().ok())
            .and_then(|v| v.split(',').next())
            .map(str::trim)
        {
            if !fwd.is_empty() {
                return fwd.to_string();
            }
        }
        if let Some(real_ip) = headers.get("x-real-ip").and_then(|v| v.to_str().ok()) {
            if !real_ip.is_empty() {
                return real_ip.to_string();
            }
        }
    }
    socket_addr.ip().to_string()
}
```

Update every call site of the old client-id function within `rate_limit.rs` (the `Service::call`/`poll_ready` implementation that extracts headers and the socket address from the incoming `Request`) to pass `false` for `trust_proxy_headers` for now, with a `// TODO` is NOT allowed per project convention — instead add it as a named constructor parameter on `RateLimiter`/`RateLimitLayer` so callers decide explicitly:

```rust
impl RateLimiter {
    /// Construct a rate limiter that keys strictly on the real connection
    /// socket address (default, safe when not behind a trusted proxy).
    pub fn new(/* existing params */) -> Self {
        Self::with_proxy_trust(/* existing args */, false)
    }

    /// Construct a rate limiter that additionally trusts
    /// `X-Forwarded-For`/`X-Real-IP` headers — only use this when the
    /// middleware is mounted behind a reverse proxy known to overwrite
    /// (not merge) these headers before they reach this service.
    pub fn with_proxy_trust(/* existing params */, trust_proxy_headers: bool) -> Self {
        // existing construction logic, storing trust_proxy_headers on self
    }
}
```

(Exact existing constructor parameters depend on `RateLimiter`'s current fields, visible from the Step 1 read — thread `trust_proxy_headers: bool` through as an additional stored field alongside them.)

- [ ] **Step 5: Run the tests to verify they pass**

Run: `cargo test -p zesdex-middleware client_id -- --nocapture`
Expected: both new tests pass.

- [ ] **Step 6: Run the full middleware test suite and clippy**

Run: `cargo test -p zesdex-middleware && cargo clippy -p zesdex-middleware -- -D warnings`
Expected: all pass, no new warnings.

- [ ] **Step 7: Commit**

```bash
git add crates/zesdex-middleware/src/rate_limit.rs
git commit -m "fix(middleware): jangan percaya header X-Forwarded-For/X-Real-IP secara default di rate limiter"
```