-
Notifications
You must be signed in to change notification settings - Fork 111
fix: centralize error message sanitization in createApiError #2718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1608197
9bf3265
04cb675
5471208
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@inkeep/agents-core": patch | ||
| --- | ||
|
|
||
| Fix error message sanitization bypass in createApiError for 500-level errors |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,119 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { isUniqueConstraintError, throwIfUniqueConstraintError } from '../error'; | ||
| import { createApiError, isUniqueConstraintError, throwIfUniqueConstraintError } from '../error'; | ||
|
|
||
| describe('sanitizeErrorMessage (via createApiError)', () => { | ||
| async function getResponseBody( | ||
| message: string, | ||
| code: 'internal_server_error' | 'bad_request' = 'internal_server_error' | ||
| ) { | ||
| const exception = createApiError({ code, message }); | ||
| return JSON.parse(await exception.getResponse().text()); | ||
| } | ||
|
|
||
| it('redacts IPv4 addresses with port', async () => { | ||
| const body = await getResponseBody('connect ECONNREFUSED 10.0.0.5:5432'); | ||
| expect(body.detail).toBe('connect ECONNREFUSED [REDACTED_HOST]'); | ||
| expect(body.detail).not.toContain('10.0.0.5'); | ||
| }); | ||
|
|
||
| it('redacts IPv4 addresses without port', async () => { | ||
| const body = await getResponseBody('could not connect to 192.168.1.1'); | ||
| expect(body.detail).not.toContain('192.168.1.1'); | ||
| expect(body.detail).toContain('[REDACTED_HOST]'); | ||
| }); | ||
|
|
||
| it('redacts PostgreSQL connection strings', async () => { | ||
| const body = await getResponseBody('postgresql://appuser:pass@host:5432/db failed'); | ||
| expect(body.detail).toBe('[REDACTED_CONNECTION] failed'); | ||
| expect(body.detail).not.toContain('appuser'); | ||
| }); | ||
|
|
||
| it('redacts connection string containing IP without leaking credentials', async () => { | ||
| const body = await getResponseBody( | ||
| 'postgresql://admin:s3cret@10.0.0.5:5432/mydb connection failed' | ||
| ); | ||
| expect(body.detail).toBe('[REDACTED_CONNECTION] connection failed'); | ||
| expect(body.detail).not.toContain('admin'); | ||
| expect(body.detail).not.toContain('s3cret'); | ||
| expect(body.detail).not.toContain('10.0.0.5'); | ||
| }); | ||
|
|
||
| it('redacts other database connection schemes', async () => { | ||
| const mysql = await getResponseBody('mysql://root:pass@db:3306/app failed'); | ||
| expect(mysql.detail).toBe('[REDACTED_CONNECTION] failed'); | ||
|
|
||
| const redis = await getResponseBody('redis://default:pass@cache:6379 timeout'); | ||
| expect(redis.detail).toBe('[REDACTED_CONNECTION] timeout'); | ||
|
|
||
| const mongo = await getResponseBody('mongodb+srv://user:pass@cluster.example.com/db error'); | ||
| expect(mongo.detail).toBe('[REDACTED_CONNECTION] error'); | ||
| }); | ||
|
|
||
| it('redacts server file paths', async () => { | ||
| const body = await getResponseBody('Error at /var/task/packages/agents-core/dist/index.js:42'); | ||
| expect(body.detail).not.toContain('/var/task'); | ||
| expect(body.detail).toContain('[REDACTED_PATH]'); | ||
| }); | ||
|
|
||
| it('redacts /tmp paths', async () => { | ||
| const body = await getResponseBody('Cannot read /tmp/secrets.json'); | ||
| expect(body.detail).not.toContain('/tmp/secrets.json'); | ||
| expect(body.detail).toContain('[REDACTED_PATH]'); | ||
| }); | ||
|
|
||
| it('redacts sensitive keywords', async () => { | ||
| const body = await getResponseBody('Invalid auth token'); | ||
| expect(body.detail).toBe('Invalid [REDACTED] [REDACTED]'); | ||
| }); | ||
|
|
||
| it('redacts credential keyword', async () => { | ||
| const body = await getResponseBody('Failed to fetch credential'); | ||
| expect(body.detail).toBe('Failed to fetch [REDACTED]'); | ||
| }); | ||
|
|
||
| it('preserves safe messages unchanged', async () => { | ||
| const body = await getResponseBody('Failed to retrieve project'); | ||
| expect(body.detail).toBe('Failed to retrieve project'); | ||
| }); | ||
|
|
||
| it('handles multiple patterns in one message', async () => { | ||
| const body = await getResponseBody( | ||
| 'connect to 10.0.0.5:5432 at /var/log/app.log with password' | ||
| ); | ||
| expect(body.detail).not.toContain('10.0.0.5'); | ||
| expect(body.detail).not.toContain('/var/log'); | ||
| expect(body.detail).not.toContain('password'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('createApiError sanitization integration', () => { | ||
| it('sanitizes 500 response body', async () => { | ||
| const exception = createApiError({ | ||
| code: 'internal_server_error', | ||
| message: 'connect ECONNREFUSED 10.0.0.5:5432', | ||
| }); | ||
| const body = JSON.parse(await exception.getResponse().text()); | ||
| expect(body.detail).not.toContain('10.0.0.5'); | ||
| expect(body.error.message).not.toContain('10.0.0.5'); | ||
| expect(body.status).toBe(500); | ||
| }); | ||
|
|
||
| it('preserves 400 response body unchanged', async () => { | ||
| const message = 'Missing required header: x-api-key with auth token'; | ||
| const exception = createApiError({ code: 'bad_request', message }); | ||
| const body = JSON.parse(await exception.getResponse().text()); | ||
| expect(body.detail).toBe(message); | ||
| expect(body.error.message).toBe(message); | ||
| expect(body.status).toBe(400); | ||
| }); | ||
|
|
||
| it('preserves 404 response body unchanged', async () => { | ||
| const message = 'Agent not found at /var/task/agents'; | ||
| const exception = createApiError({ code: 'not_found', message }); | ||
| const body = JSON.parse(await exception.getResponse().text()); | ||
| expect(body.detail).toBe(message); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR: Missing tests for Issue: The Why: If Fix: Add tests for the 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: |
||
|
|
||
| describe('isUniqueConstraintError', () => { | ||
| describe('PostgreSQL unique violation (23505)', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,17 @@ export const errorResponseSchema = z | |
|
|
||
| export type ErrorResponse = z.infer<typeof errorResponseSchema>; | ||
|
|
||
| function sanitizeErrorMessage(message: string): string { | ||
| return message | ||
amikofalvy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .replace( | ||
| /(?: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]') | ||
amikofalvy marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR: Missing IPv6 address sanitization Issue: The sanitizer only catches IPv4 addresses but completely misses IPv6 addresses. Error messages like 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:
|
||
| .replace(/\/(?:var|tmp|home|usr|etc|opt)\/\S+/g, '[REDACTED_PATH]') | ||
| .replace(/\b(password|token|key|secret|auth|credential)\b/gi, '[REDACTED]'); | ||
pullfrog[bot] marked this conversation as resolved.
Show resolved
Hide resolved
amikofalvy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| export function createApiError({ | ||
| code, | ||
| message, | ||
|
|
@@ -94,16 +105,19 @@ export function createApiError({ | |
| const title = getTitleFromCode(code); | ||
| const _type = `${ERROR_DOCS_BASE_URL}#${code}`; | ||
|
|
||
| const sanitizedMessage = status >= 500 ? sanitizeErrorMessage(message) : message; | ||
|
|
||
| const problemDetails: ProblemDetails = { | ||
| title, | ||
| status, | ||
| detail: message, | ||
| detail: sanitizedMessage, | ||
| code, | ||
| ...(instance && { instance }), | ||
| ...(requestId && { requestId }), | ||
| }; | ||
|
|
||
| const errorMessage = message.length > 100 ? `${message.substring(0, 97)}...` : message; | ||
| const errorMessage = | ||
| sanitizedMessage.length > 100 ? `${sanitizedMessage.substring(0, 97)}...` : sanitizedMessage; | ||
|
|
||
| const responseBody = { | ||
| ...problemDetails, | ||
|
|
@@ -122,7 +136,7 @@ export function createApiError({ | |
|
|
||
| // @ts-expect-error - The HTTPException constructor expects a ContentfulStatusCode, but we're using a number | ||
| // This is safe because we're only using valid HTTP status codes | ||
| return new HTTPException(status, { message, res }); | ||
| return new HTTPException(status, { message: sanitizedMessage, res }); | ||
| } | ||
|
|
||
| export async function handleApiError( | ||
|
|
@@ -195,9 +209,7 @@ export async function handleApiError( | |
| ); | ||
|
|
||
| const sanitizedErrorMessage = | ||
| error instanceof Error | ||
| ? error.message.replace(/\b(password|token|key|secret|auth)\b/gi, '[REDACTED]') | ||
| : 'Unknown error'; | ||
| error instanceof Error ? sanitizeErrorMessage(error.message) : 'Unknown error'; | ||
|
|
||
| const problemDetails: ProblemDetails & { error: { code: ErrorCodes; message: string } } = { | ||
| // type: `${ERROR_DOCS_BASE_URL}#internal_server_error`, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.