Security & Audit

What happened: When an environment variable is declared as type: "number" in clq.config.ts but the actual runtime value is non-numeric — for example, an API key accidentally routed to a port variable — loadConfig() would embed the raw value directly in the error message. clq doctor then printed that error to stdout.

Concrete reproduction:

Root cause: One line in config.ts:

throw errors.missingEnvVar(
  key,
  `${decl.description} (expected a number, got "${raw}")`, // raw = process.env[key]
)

The secret: true flag on the declaration did nothing — no code read it before this path.

Impact: Any CI/CD pipeline that logs clq doctor output — common when it's used as a health check before deployment — would capture the literal secret value in cleartext log storage.

Fix: Replaced the raw value interpolation with a structural description. For non-secret vars: "a non-numeric string of length N". For secret: true vars: "a non-numeric value" — no content, no length, nothing reconstructable. A regression test now asserts that the full fake secret is not a substring of JSON.stringify(output) anywhere in the process's stdout.

What happened: The inspector's response sanitizer (redactSecrets()) only matched four key-name patterns: secret, token, password, and api_key. A tool handler returning data under any other secret-looking key name would leak the value through /api/call and /api/logs responses. The following keys were confirmed to pass through unredacted:

Fix: Expanded SECRET_TERMS to 16 patterns covering the most common real-world naming conventions. Every response from the inspector — tool lists, call results, log entries — is passed through redactSecrets() at the boundary regardless of what the handler returned.

What this fix later broke: See the FIXD regression section below.

What happened: When a tool's after middleware hook threw an error, the original successful result of the tool call was lost. The MCP client received a failure response even though the tool had completed successfully.

// Before fix — if after() throws, result is lost:
for (const mw of [...middleware].reverse()) {
  await mw.after?.(args.ctx, result)  // rejection propagates up
}
return result

Fix: after-hook errors are now caught individually. The error is logged via console.error, the remaining after hooks still run, and the tool's original result is always returned — a broken after-hook can never make a succeeded tool appear to have failed to the MCP client.

// After fix:
for (const mw of [...middleware].reverse()) {
  try {
    await mw.after?.(args.ctx, result)
  } catch (err) {
    console.error(err)  // logged, not swallowed
  }
}
return result  // always returned

Side effect (cosmetic): Two tests that exercise this behavior emit Error: after-exploded stack traces to stderr on every test run, because they don't mock console.error. The tests pass; the stderr output is noise. This is a documented, permanent cosmetic issue.

If a middleware before or after hook never resolves — for example, it hangs on a network call with no timeout, or deadlocks on a resource — the tool call never returns a result or error. The MCP client will eventually time out on its side, but on the server side the promise chain is stuck. Because the stdio MCP transport is sequential in practice, a single hung hook can block all subsequent tool calls, making the server permanently unresponsive for the session.

Why this wasn't fixed: The right solution is to add an AbortSignal to CLQContext that the framework sets with a per-call deadline. Middleware hooks that respect the signal can abort early; hooks that don't will be abandoned when the signal fires. This is a public API change — adding a field to CLQContext — and requires a proper design pass rather than a one-liner patch. Shipping a half-correct timeout (e.g., Promise.race that abandons the hook but doesn't cancel it) would create a worse situation: the hook keeps running in the background, holding resources, while the client receives a timeout error.

Mitigation in the meantime: Middleware hooks that make any I/O call should implement their own timeouts using AbortController or Promise.race with a fallback. This is standard practice for I/O code; CLQ just doesn't enforce it yet.

The inspector reads its session token from the URL on load, then immediately strips it from the address bar using history.replaceState — a correct security behavior that prevents the token from appearing in browser history, referrers, or server logs. But a page reload sends no token, gets a 401, and shows "failed to load: HTTP 401" with no explanation. There is no recovery from this state; the user must close the tab and re-open the original URL from the terminal.

Why the security behavior is correct: The token-stripping is intentional and right. A token in the URL bar is visible in shared screens, browser extensions, browser sync, and referrer headers. The UX problem is the lack of a helpful error state — the page doesn't explain what happened or how to recover. This is a missing error message, not a broken security model.

Why it wasn't fixed in this pass: The audit pass was read-only (no source modifications). A correct fix would add a user-facing error state that says something like: "Token expired from URL — please re-open the link printed by clq inspect." Tracked for a future release.

The SECRET_TERMS list includes cookie and token, which were added to catch HTTP Cookie headers and session tokens — legitimate targets. But because the matching is substring-based, fields like cookieCount, acceptedCookies, and tokenCount are also redacted. The impact is a type mismatch: a numeric value like { tokenCount: 1000 } becomes { tokenCount: "[REDACTED]" } — the field is a string where a number was expected, which can break consumers.

Confirmed false-positive cases: cookieCount, acceptedCookies, cookieBannerVisible, tokenCount, jwtAlgorithm, credentialType.

Why this is a tradeoff, not a bug: The comment in redact.ts explicitly says "Matched as substrings" — this was chosen deliberately for maximum coverage. The alternative is a more precise matching rule that would require more context to get right. The current behavior prioritizes never leaking a real secret over never over-redacting a non-secret field. This tradeoff is explicit and documented, not an accident. Tools that return non-secret fields with these names should rename them to avoid ambiguity.