fix(api): sanitize error details in HTTP responses (ENG-1668)

[limaronaldo]

Problema

Endpoints da API retornavam details: String(error) (52 ocorrências no router), vazando paths absolutos, connection strings, stack traces e potenciais segredos para clientes. task.lastError também era postado bruto em comentários do Linear.

Solução

• Novo helper sanitizeErrorForResponse em packages/api/src/core/errors.ts:
  • usa apenas a primeira linha da mensagem, truncada em 200 chars
  • retorna Internal error se detectar padrões sensíveis (paths, connection strings, stacks, api keys/tokens, process.env, hosts internos)
• Substituídas todas as 52 ocorrências de details: String(error) no router.ts (logs server-side via console.error mantidos)
• lastError sanitizado antes de postar comentário de falha no Linear (2 sites)

Testes

• 12 testes unitários novos (errors.test.ts) — 12 pass / 0 fail
• tsc --noEmit limpo

Closes ENG-1668

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba67dab032

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Default unmatched internal errors to a generic response

When an SDK, database, or filesystem exception contains sensitive data in a format absent from the finite denylist, this branch returns it verbatim to every converted HTTP response. For example, the production Dockerfiles run under /app, but an ENOENT message containing /app/packages/api/.env is not matched, and Incorrect API key provided: sk-proj-abcdefghijklmnopqrstuvwxyz also bypasses both credential patterns; both cases were reproduced against this helper. Unknown internal errors should therefore be generic by default, with only explicitly safe messages allowed through.

AGENTS.md reference: AGENTS.md:L63-L71

Useful? React with 👍 / 👎.

[limaronaldo]

Light Review — PR #428 (ENG-1668): HTTP error response sanitization

Verdict: changes-needed (one HIGH gap in the denylist that undermines the sanitizer's stated purpose)

Summary

New packages/api/src/core/errors.ts exports sanitizeErrorForResponse(error), applying a denylist of SENSITIVE_PATTERNS regexes to redact paths, connection strings, stack trace frames, secret-like key=value pairs, common token prefixes, process.env references, and loopback hosts from the first line of an error message, truncating to 200 chars, defaulting to "Internal error" on any match or non-Error/non-string input. router.ts was mechanically updated (~52 call sites) to route details: fields through this helper instead of raw String(error). Verified against the current router.ts (fetched from main) that console.error(...) full-detail server-side logging is preserved unchanged at every touched call site — the sanitization only affects what's returned to the client, not what's logged internally, which is the correct pattern.

Findings

• [HIGH] packages/api/src/core/errors.ts (SENSITIVE_PATTERNS) — filesystem path detection only covers a hardcoded allowlist of root directories, missing common production layouts.
  The path pattern is /(?:\/(?:Users|home|var|etc|tmp|opt|private|srv)\/|[A-Za-z]:\\)/. This misses any absolute path outside that specific root set — most notably /app/... (the default WORKDIR in most Docker/container images, including typical Node/Bun deployments), /data/..., /mnt/..., /code/..., etc. A realistic production error like ENOENT: /app/config/secrets.json not found or a stack frame rooted at /app/dist/router.js:120:5 would pass through to the client unredacted, defeating the sanitizer's core purpose for exactly the deployment topology this app is likely to run in. Cross-model review (codex exec) independently flagged this exact gap. Recommend either widening the pattern to match any absolute POSIX path (\/[A-Za-z0-9_.-]+(?:\/[A-Za-z0-9_.-]+)+) rather than an enumerated root allowlist, or explicitly adding /app and other common container roots, plus a regression test asserting /app/... is redacted.

• [MEDIUM] packages/api/src/core/errors.ts — no pattern for AWS-style or other structured credential formats.
  SENSITIVE_PATTERNS covers sk|pk|ghp|gho|ghs|xox[abps]-prefixed tokens but has no pattern for AWS access keys (AKIA[0-9A-Z]{16}), JWTs (eyJ... base64 segments), or generic 32+ char hex/opaque secrets. This is an inherent limitation of a denylist/blocklist approach (as opposed to an allowlist of known-safe error shapes), and is lower severity than the path-detection gap since these are less likely to appear in typical application error messages versus stack traces/file paths, but worth tracking as follow-up hardening.

• [LOW] packages/api/src/core/errors.ts — only the literal loopback triple (localhost, 127.0.0.1, 0.0.0.0, ::1) is redacted; no pattern for private-range IPs (10.x, 172.16-31.x, 192.168.x) or internal hostnames (e.g., *.internal, *.local).
  Low severity since these leak less directly actionable information than a filesystem path or credential, but still contributes to internal topology disclosure.

• [LOW / scope note, not a bug] packages/api/src/router.ts — two call sites (approx. lines 123 and 511) apply sanitizeErrorForResponse to processedTask.lastError values embedded in GitHub PR comment bodies, not JSON HTTP responses.
  This is a reasonable and likely intentional generalization (comment bodies are also externally visible surfaces), but worth a one-line confirmation from the author that this was deliberate, since it changes existing comment content, not just error-response payloads.

Cross-model review

codex exec -m gpt-5.6-terra reviewed the diff independently and returned one HIGH-confidence Medium-severity finding — the same /app/... path-detection gap identified above — corroborating it as the primary blocker-adjacent issue in this diff. Given that this sanitizer's entire purpose is to be a leak boundary, and the most common containerized deployment path pattern falls through it unredacted, this is elevated to HIGH in this review (server-side logging is preserved so no new information is lost by fixing it, and the fix is a small, low-risk regex change).

[limaronaldo]

fixed in 0e5b56b

• Replaced the enumerated path-root allowlist with a general absolute-path matcher (POSIX with >=2 segments, or Windows drive paths), so /app/... (and any other container WORKDIR) is caught without needing to keep a root list in sync. Reproduced and fixed: ENOENT: /app/packages/api/.env not found.
• Widened the credential-phrase pattern to allow words between the sensitive noun and the colon (e.g. "API key provided:"), and the vendor-token pattern to accept hyphenated prefixes. Reproduced and fixed: Incorrect API key provided: sk-proj-abcdefghijklmnopqrstuvwxyz.
• Added regression tests: /app/node_modules/..., an arbitrary Windows drive path, and both reviewer-reported bypass strings verbatim.
• Full bun test + tsc --noEmit green; pre-existing unrelated Playwright test failures unchanged (same 37 failures before/after, confirmed by diff).