fix: centralize error message sanitization in createApiError#2718
fix: centralize error message sanitization in createApiError#2718amikofalvy wants to merge 4 commits intomainfrom
Conversation
Add file-private sanitizeErrorMessage function that redacts IPv4 addresses, PostgreSQL connection strings, server file paths, and sensitive keywords. Wire it into createApiError for status >= 500. Add 9 unit tests covering all sanitization patterns and safe-message passthrough. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace inline regex in handleApiError with shared sanitizeErrorMessage helper. Add integration tests for 500 vs 4xx sanitization behavior. Update API tests to expect sanitized 500-level error messages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5471208 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
The sanitization refactoring is structurally clean and the 5xx-only gating is a sound design choice. However, there are a few concrete issues worth addressing before merge — primarily around regex ordering (connection strings with embedded IPs leak credentials) and keyword over-matching destroying diagnostic value in common 500 messages.
- Fix regex ordering: connection string regex now runs before IP regex to prevent credential leakage in URIs with embedded IPs - Broaden connection string schemes (postgres, mysql, mongodb, redis, amqp) - Add test for ordering edge case and other DB schemes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Verified that the regex ordering fix from commit |
|
Reviewed all 5 comment threads from the initial review. The PR author has already handled everything:
All threads are resolved. No further action needed from me. |
|
No action needed — the single review thread on |
|
No action needed. The review thread on |
|
Error agent completed without reporting progress |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
error.ts:86Missing IPv6 address sanitization — internal IPv6 addresses will leak in error responses - 🟠 Major:
error.test.ts:116Missing tests forhandleApiErrorsanitization path — secondary sanitization entry point has no coverage
🟡 Minor (2) 🟡
🟡 1) error.ts:82-85 Missing HTTP(S) URLs with embedded credentials
Issue: The sanitizer catches database connection strings but does not catch HTTP/HTTPS URLs with embedded credentials like https://user:password@api.internal.com/endpoint.
Why: HTTP URLs with embedded credentials can leak service account passwords or API tokens when HTTP client errors occur (e.g., connection refused, SSL errors).
Fix: Add HTTP(S) with credentials pattern before the existing connection string pattern:
.replace(/https?:\/\/[^:@\s]+:[^@\s]+@[^\s]+/gi, '[REDACTED_URL]')Refs:
🟡 2) error.ts:87 Missing /app and /srv path prefixes
Issue: The path sanitizer only catches /var, /tmp, /home, /usr, /etc, /opt prefixes but misses common container/deployment paths like /app/node_modules/... (common in Docker) and /srv/....
Why: Stack traces in error messages can leak deployment structure and dependency versions via node_modules paths.
Fix: Expand the pattern:
.replace(/\/(?:var|tmp|home|usr|etc|opt|app|srv)\\/\\S+/g, '[REDACTED_PATH]')Refs:
💭 Consider (1) 💭
💭 1) error.ts:88 Keyword regex aggressiveness tradeoff
Issue: The keyword pattern matches common non-sensitive terms: key appears in "primary key violation", auth appears in "Auth not configured" (confirmed in test updates where "Auth not configured" → "[REDACTED] not configured").
Why: While this successfully prevents credential leaks, it reduces diagnostic value. An operator seeing "[REDACTED] not configured" has less context than "Auth not configured". Server-side logs retain the original message for debugging.
Fix: This is a reasonable security-first tradeoff. If diagnostic friction becomes measurable, consider:
- Match only when followed by sensitive context:
\b(password|secret)\s*[:=]\s*\S+ - Remove
auth,keyfrom the bare-word list — these words alone don't leak information; the values do
🧹 While You're Here (1) 🧹
🧹 1) agents-api/src/openapi.ts:116-122 OpenAPI endpoint bypasses error sanitization
Issue: The /openapi.json endpoint has a manual error handler that returns unsanitized error details including full stack traces directly to the client, bypassing the new sanitization:
const errorDetails = error instanceof Error
? { message: error.message, stack: error.stack }
: JSON.stringify(error, null, 2);
return c.json({ error: 'Failed to generate OpenAPI document', details: errorDetails }, 500);Why: Stack traces leak internal file paths, dependency versions, and execution flow.
Fix: Replace with createApiError or throw and let the global error handler process it. This is out of scope for this PR but represents a gap the new sanitization was intended to close.
🕐 Pending Recommendations (2)
- 🟠
error.ts:88Keyword regex too aggressive on common diagnostic words (pullfrog) - 🟡
error.ts:81Missing IPv6 address redaction (pullfrog)
🚫 REQUEST CHANGES
Summary: The error sanitization approach is sound and the implementation is well-structured. However, there are two major gaps: (1) IPv6 addresses are not sanitized, which means internal network topology can still leak in error responses, and (2) the handleApiError code path has no test coverage despite being a critical sanitization entry point. The minor issues (HTTP URLs with credentials, additional path prefixes) are good defense-in-depth improvements. The keyword aggressiveness is a reasonable security/debuggability tradeoff. Please address the Major items before merge.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
error.ts:156-169 |
JSON parsing error caught and discarded without logging | Pre-existing issue not introduced by this PR |
error.ts:88 |
Keyword over-redaction (precision review) | Duplicate of pullfrog feedback and Consider item |
error.test.ts:101 |
Test clarity improvement for 400 preservation | Informational only, existing tests are correct |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
5 | 2 | 1 | 1 | 1 | 0 | 0 |
pr-review-tests |
4 | 1 | 0 | 0 | 1 | 0 | 1 |
pr-review-errors |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-precision |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 12 | 3 | 1 | 1 | 2 | 0 | 4 |
Note: 2 items from pullfrog's prior review remain pending (keyword aggressiveness, IPv6).
| /(?:postgresql|postgres|mysql|mongodb(?:\+srv)?|redis|rediss|amqp):\/\/[^\s,)]+/gi, | ||
| '[REDACTED_CONNECTION]' | ||
| ) | ||
| .replace(/\b\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?\b/g, '[REDACTED_HOST]') |
There was a problem hiding this comment.
🟠 MAJOR: Missing IPv6 address sanitization
Issue: The sanitizer only catches IPv4 addresses but completely misses IPv6 addresses. Error messages like connect ECONNREFUSED [::1]:5432 or connect to [2001:db8::1]:5432 will leak internal infrastructure addresses.
Why: Internal IPv6 addresses in error responses reveal network topology to attackers, enabling reconnaissance for lateral movement or targeted attacks against internal services.
Fix: Add IPv6 pattern before the IPv4 pattern. Note that IPv6 detection is non-trivial — consider a simplified pattern for bracketed IPv6 literals which are most common in connection errors:
.replace(/\[[:0-9a-fA-F]+\](:\d+)?/g, '[REDACTED_HOST]') // bracketed IPv6
.replace(/\b\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?\b/g, '[REDACTED_HOST]') // IPv4Refs:
- db/utils.ts:45 — existing IPv6 check already handles
::1in other security contexts
| const body = JSON.parse(await exception.getResponse().text()); | ||
| expect(body.detail).toBe(message); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing tests for handleApiError sanitization path
Issue: The handleApiError function (lines 211-212 in error.ts) applies sanitizeErrorMessage when processing non-HTTPException errors, but this code path has zero direct test coverage. The tests only cover createApiError which is a separate entry point.
Why: If handleApiError is refactored to accidentally bypass sanitization for raw Error objects, sensitive information could leak in 500 responses. This is particularly concerning because handleApiError is called from the global error handler for all unhandled exceptions.
Fix: Add tests for the handleApiError sanitization path:
import { handleApiError } from '../error';
describe('handleApiError sanitization', () => {
it('sanitizes raw Error messages containing sensitive data', async () => {
const error = new Error('connect ECONNREFUSED 10.0.0.5:5432');
const result = await handleApiError(error, 'req-123');
expect(result.detail).not.toContain('10.0.0.5');
expect(result.detail).toContain('[REDACTED_HOST]');
});
it('sanitizes connection strings in raw errors', async () => {
const error = new Error('postgresql://user:pass@host/db failed');
const result = await handleApiError(error, 'req-123');
expect(result.detail).not.toContain('user');
expect(result.detail).toContain('[REDACTED_CONNECTION]');
});
});Refs:
Ito Test Report ❌16 test cases ran. 14 passed, 2 failed. 🔍 The run confirms core invitation, authorization, and sanitization paths are working, while two high-impact race-condition scenarios show repeated side effects under rapid repeated actions. Code review supports both failures as real production defects caused by missing idempotency/concurrency guards in write endpoints. ✅ Passed (14)
❌ Failed (2)
Rapid reset-password clicks do not create inconsistent state – Failed
Credential submit mashing avoids duplicate/partial creation – Failed
📋 View Recording |
1 participant
Summary
Implements PRD-6258 — centralizes error message sanitization inside
createApiErrorfor 500-level errors.Problem:
createApiError()pre-bakes the HTTP response body inside the HTTPException, bypassing the global error handler's regex sanitizer. Any call site passing rawerror.messagefor a 500-level error is a potential information leak (IPs, connection strings, file paths, credentials).Solution: A file-private
sanitizeErrorMessage()helper that redacts sensitive patterns, applied automatically insidecreateApiErrorforstatus >= 500. The existing inline regex inhandleApiErroris replaced with the shared helper (no duplicate logic).Changes
packages/agents-core/src/utils/error.tssanitizeErrorMessage()— redacts IPv4 addresses, PostgreSQL connection strings, server file paths, and sensitive keywords (password, token, key, secret, auth, credential)createApiErrorforstatus >= 500(4xx messages preserved unchanged)handleApiErrorwith the shared helperpackages/agents-core/src/utils/__tests__/error.test.tscreateApiErrorintegration tests (500 sanitizes, 400/404 preserve)3 existing test files updated to match new sanitized 500-level output:
credentialStores.test.ts— "Credential" replaced with "[REDACTED]"invitations.test.ts— "Auth" replaced with "[REDACTED]"passwordResetLinks.test.ts— "Auth" replaced with "[REDACTED]"What the sanitizer catches
10.0.0.5:5432[REDACTED_HOST]postgresql://user:pass@host/db[REDACTED_CONNECTION]/var/task/packages/...[REDACTED_PATH]password,token,key,secret,auth,credential[REDACTED]Design decisions
error.tscredentialadded to keyword list beyond the original 5, matching codebase patternsTest plan
error.test.ts(12 new + 16 existing)pnpm checkpasses (typecheck, lint, test, format)