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:

    /// 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:

    /// 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):

//! 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

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):

    #[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:

/// 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:

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

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"