Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ HELPSCOUT_BASE_URL=https://api.helpscout.net/v2/
HELPSCOUT_DOCS_BASE_URL=https://docsapi.helpscout.net/v1/
# HELPSCOUT_DISABLE_DOCS=false

# Host allowlist (SSRF defense). The base URLs are constrained to the real
# Help Scout hosts by default so a repointed URL cannot exfiltrate credentials.
# Only set these for self-hosted or proxy setups; comma-separated host list.
# HELPSCOUT_ALLOWED_API_HOSTS=api.helpscout.net,api.helpscout.com
# HELPSCOUT_ALLOWED_DOCS_HOSTS=docsapi.helpscout.net

# Content Redaction
# Default behavior: message content IS shown (REDACT_MESSAGE_CONTENT=false)
# Set to true to hide message bodies in responses for privacy/compliance
Expand Down
72 changes: 72 additions & 0 deletions src/__tests__/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,4 +143,76 @@ describe('Config Validation', () => {
expect(config.logging.level).toBe('info');
});
});

describe('host allowlist (SSRF defense)', () => {
beforeEach(() => {
process.env.HELPSCOUT_CLIENT_ID = 'client-id';
process.env.HELPSCOUT_CLIENT_SECRET = 'client-secret';
delete process.env.HELPSCOUT_ALLOWED_API_HOSTS;
delete process.env.HELPSCOUT_ALLOWED_DOCS_HOSTS;
});

it('accepts the default Help Scout API hosts', async () => {
for (const host of ['api.helpscout.net', 'api.helpscout.com']) {
process.env.HELPSCOUT_BASE_URL = `https://${host}/v2/`;
jest.resetModules();
const { validateConfig } = await import('../utils/config.js');
expect(() => validateConfig()).not.toThrow();
}
});

it('rejects an API base URL pointed at an attacker host', async () => {
process.env.HELPSCOUT_BASE_URL = 'https://evil.example.com/v2/';

jest.resetModules();
const { validateConfig } = await import('../utils/config.js');
expect(() => validateConfig()).toThrow(/HELPSCOUT_BASE_URL host "evil.example.com" is not in the allowlist/);
});

it('rejects a look-alike subdomain not in the allowlist', async () => {
process.env.HELPSCOUT_BASE_URL = 'https://api.helpscout.net.evil.com/v2/';

jest.resetModules();
const { validateConfig } = await import('../utils/config.js');
expect(() => validateConfig()).toThrow(/is not in the allowlist/);
});

it('honors HELPSCOUT_ALLOWED_API_HOSTS override for self-hosted/proxy setups', async () => {
process.env.HELPSCOUT_BASE_URL = 'https://helpscout.internal.corp/v2/';
process.env.HELPSCOUT_ALLOWED_API_HOSTS = 'helpscout.internal.corp';

jest.resetModules();
const { validateConfig } = await import('../utils/config.js');
expect(() => validateConfig()).not.toThrow();
});

it('still requires HTTPS even when the host is allowed', async () => {
process.env.HELPSCOUT_BASE_URL = 'http://api.helpscout.net/v2/';

jest.resetModules();
const { validateConfig } = await import('../utils/config.js');
expect(() => validateConfig()).toThrow(/HELPSCOUT_BASE_URL must use HTTPS/);
});

it('rejects a Docs base URL pointed at an attacker host', async () => {
process.env.HELPSCOUT_DOCS_BASE_URL = 'https://evil.example.com/v1/';

jest.resetModules();
const { validateConfig } = await import('../utils/config.js');
expect(() => validateConfig()).toThrow(/HELPSCOUT_DOCS_BASE_URL host "evil.example.com" is not in the allowlist/);
});

it('accepts the default Docs host and honors its override', async () => {
process.env.HELPSCOUT_DOCS_BASE_URL = 'https://docsapi.helpscout.net/v1/';
jest.resetModules();
let mod = await import('../utils/config.js');
expect(() => mod.validateConfig()).not.toThrow();

process.env.HELPSCOUT_DOCS_BASE_URL = 'https://docs.internal.corp/v1/';
process.env.HELPSCOUT_ALLOWED_DOCS_HOSTS = 'docs.internal.corp';
jest.resetModules();
mod = await import('../utils/config.js');
expect(() => mod.validateConfig()).not.toThrow();
});
});
});
53 changes: 52 additions & 1 deletion src/__tests__/helpscout-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -414,13 +414,64 @@ describe('HelpScoutClient', () => {
};

const transformedError = (client as any).transformError(mockAxiosError);

expect(transformedError).toMatchObject({
code: 'INVALID_INPUT',
message: 'Help Scout API client error: Invalid request'
});
}, 10000);

it('whitelists 422 error body fields and does not propagate echoed input PII', async () => {
const client = new HelpScoutClient();

const mockAxiosError = {
response: {
status: 422,
data: {
code: 'validation_error',
message: 'The email is invalid',
errors: [{ path: 'email', message: 'jane@bigcorp.com is not valid' }],
_embedded: { customer: { email: 'jane@bigcorp.com' } },
},
},
config: { metadata: { requestId: 'test-422' }, url: '/conversations', method: 'post' },
};

const transformed = (client as any).transformError(mockAxiosError);
const serialized = JSON.stringify(transformed);

expect(transformed.code).toBe('INVALID_INPUT');
expect(transformed.details.apiResponse).toEqual({
code: 'validation_error',
message: 'The email is invalid',
});
expect(transformed.details).not.toHaveProperty('validationErrors');
expect(serialized).not.toContain('jane@bigcorp.com');
}, 10000);

it('truncates a long upstream message and drops other 4xx body fields', async () => {
const client = new HelpScoutClient();
const longMessage = 'x'.repeat(500);

const mockAxiosError = {
response: {
status: 400,
data: {
code: 'bad_request',
message: longMessage,
customerEmail: 'leak@bigcorp.com',
},
},
config: { metadata: { requestId: 'test-400-leak' }, url: '/conversations', method: 'get' },
};

const transformed = (client as any).transformError(mockAxiosError);

expect(transformed.details.apiResponse.code).toBe('bad_request');
expect(transformed.details.apiResponse.message).toHaveLength(200);
expect(JSON.stringify(transformed)).not.toContain('leak@bigcorp.com');
}, 10000);

it('should handle 500 server errors with retries', async () => {
const client = new HelpScoutClient();

Expand Down
81 changes: 81 additions & 0 deletions src/__tests__/helpscout-docs-client-redaction.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { describe, it, expect, jest } from '@jest/globals';

// Mock logger to reduce test output noise
jest.mock('../utils/logger.js', () => ({
logger: {
info: jest.fn(),
error: jest.fn(),
debug: jest.fn(),
warn: jest.fn(),
},
}));

import { HelpScoutDocsClient, safeDocsApiResponse } from '../utils/helpscout-docs-client.js';

describe('HelpScoutDocsClient error-body whitelisting', () => {
it('safeDocsApiResponse keeps only code/error/message and truncates message', () => {
const result = safeDocsApiResponse({
code: 'invalid',
error: 'bad input',
message: 'y'.repeat(500),
email: 'jane@bigcorp.com',
stack: 'secret stack',
});

expect(result).toEqual({
code: 'invalid',
error: 'bad input',
message: 'y'.repeat(200),
});
expect(JSON.stringify(result)).not.toContain('jane@bigcorp.com');
expect(JSON.stringify(result)).not.toContain('secret stack');
});

it('safeDocsApiResponse returns undefined for non-object bodies', () => {
expect(safeDocsApiResponse('plain string')).toBeUndefined();
expect(safeDocsApiResponse(undefined)).toBeUndefined();
});

it('does not propagate the verbatim 4xx body into the transformed error', () => {
const client = new HelpScoutDocsClient();

const mockAxiosError = {
response: {
status: 400,
data: {
code: 'invalid_request',
message: 'Collection slug already exists',
submittedBy: 'jane@bigcorp.com',
},
},
config: { docsMetadata: { requestId: 'docs-400', startTime: Date.now() } },
};

const transformed = (client as any).transformError(mockAxiosError);

expect(transformed.code).toBe('INVALID_INPUT');
expect(transformed.details.apiResponse).toEqual({
code: 'invalid_request',
error: undefined,
message: 'Collection slug already exists',
});
expect(JSON.stringify(transformed)).not.toContain('jane@bigcorp.com');
});

it('whitelists the 404 error body as well', () => {
const client = new HelpScoutDocsClient();

const mockAxiosError = {
response: {
status: 404,
data: { code: 'not_found', message: 'No such article', secret: 'jane@bigcorp.com' },
},
config: { docsMetadata: { requestId: 'docs-404', startTime: Date.now() } },
};

const transformed = (client as any).transformError(mockAxiosError);

expect(transformed.code).toBe('NOT_FOUND');
expect(JSON.stringify(transformed)).not.toContain('jane@bigcorp.com');
});
});
86 changes: 86 additions & 0 deletions src/utils/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,66 @@ function parseLogLevel(defaultValue = 'info'): string {
return rawValue && levels.includes(rawValue) ? rawValue : defaultValue;
}

/**
* Parse a comma-separated host allowlist env var into a Set of trimmed,
* non-empty, lower-cased hostnames. Returns null when the env var is unset
* or empty so callers can fall back to the built-in default set.
*/
function parseHostAllowlist(envVar: string | undefined): Set<string> | null {
if (!envVar) return null;
const hosts = envVar
.split(',')
.map(h => h.trim().toLowerCase())
.filter(h => h.length > 0);
return hosts.length > 0 ? new Set(hosts) : null;
}

/**
* Default Help Scout API hosts. The OAuth2 bearer token is sent on every
* authenticated request, so a repointed base URL would exfiltrate it to an
* attacker host. Default-closed to these; override only for self-hosted or
* proxy setups via HELPSCOUT_ALLOWED_API_HOSTS.
*/
const DEFAULT_ALLOWED_API_HOSTS = new Set(['api.helpscout.net', 'api.helpscout.com']);

/**
* Default Help Scout Docs API host. The Docs API key travels via HTTP basic
* auth, which is just as sensitive. Override via HELPSCOUT_ALLOWED_DOCS_HOSTS.
*/
const DEFAULT_ALLOWED_DOCS_HOSTS = new Set(['docsapi.helpscout.net']);

/**
* Validate that a configured base URL resolves to a host in the allowlist.
* `null` allowlist means use the provided default set. Empty/whitespace
* URLs are skipped (the default applies elsewhere). Throws on an invalid URL
* or a host outside the allowlist.
*/
function validateHostAllowlist(
rawUrl: string | undefined,
envVarName: string,
override: Set<string> | null,
defaults: Set<string>,
): void {
if (!rawUrl || !rawUrl.trim()) return;
const allowed = override ?? defaults;
let parsed: URL;
try {
parsed = new URL(rawUrl);
} catch {
throw new Error(
`Security Error: ${envVarName} is not a valid URL.\nCurrent value: ${rawUrl}`,
);
}
if (!allowed.has(parsed.hostname.toLowerCase())) {
throw new Error(
`Security Error: ${envVarName} host "${parsed.hostname}" is not in the allowlist. ` +
`Allowed hosts: ${[...allowed].join(', ')}. ` +
`Set ${envVarName === 'HELPSCOUT_BASE_URL' ? 'HELPSCOUT_ALLOWED_API_HOSTS' : 'HELPSCOUT_ALLOWED_DOCS_HOSTS'} ` +
`to override for self-hosted or proxy setups.`,
);
}
}

export const config: Config = {
helpscout: {
// OAuth2 authentication (Client Credentials flow)
Expand Down Expand Up @@ -123,4 +183,30 @@ export function validateConfig(): void {
'Please use: https://api.helpscout.net/v2/'
);
}

// Constrain HELPSCOUT_BASE_URL to known Help Scout hosts (SSRF defense).
// The OAuth2 bearer token is sent on every authenticated request, so a
// misconfigured or attacker-controlled base URL would leak it.
validateHostAllowlist(
config.helpscout.baseUrl,
'HELPSCOUT_BASE_URL',
parseHostAllowlist(process.env.HELPSCOUT_ALLOWED_API_HOSTS),
DEFAULT_ALLOWED_API_HOSTS,
);

// Same treatment for the Docs API host. The Docs API key travels via HTTP
// basic auth, which is just as sensitive.
if (config.helpscout.docsBaseUrl && !config.helpscout.docsBaseUrl.startsWith('https://')) {
throw new Error(
'Security Error: HELPSCOUT_DOCS_BASE_URL must use HTTPS to protect Docs API credentials in transit.\n' +
`Current value: ${config.helpscout.docsBaseUrl}\n` +
'Please use: https://docsapi.helpscout.net/v1/'
);
}
validateHostAllowlist(
config.helpscout.docsBaseUrl,
'HELPSCOUT_DOCS_BASE_URL',
parseHostAllowlist(process.env.HELPSCOUT_ALLOWED_DOCS_HOSTS),
DEFAULT_ALLOWED_DOCS_HOSTS,
);
}
22 changes: 20 additions & 2 deletions src/utils/helpscout-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,22 @@ import { logger } from './logger.js';
import { cache } from './cache.js';
import { ApiError } from '../schema/types.js';

/**
* Whitelist the fields propagated from an upstream Help Scout error body into
* the MCP tool result. Help Scout's 4xx/422 responses sometimes echo the
* submitted input verbatim, which would otherwise flow customer PII into the
* LLM context (and anywhere that context is logged or shared). Only `code`
* and a length-capped `message` are surfaced.
*/
export function safeApiResponse(data: unknown): { code?: unknown; message?: string } | undefined {
if (!data || typeof data !== 'object') return undefined;
const d = data as Record<string, unknown>;
return {
code: d.code,
message: typeof d.message === 'string' ? d.message.slice(0, 200) : undefined,
};
}

/**
* Connection pool configuration for HTTP agents
*/
Expand Down Expand Up @@ -448,7 +464,9 @@ export class HelpScoutClient {
message: `Help Scout API validation error: ${responseData.message || 'Invalid request data'}`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Top-level error message interpolates upstream responseData.message without the 200-char truncation applied by safeApiResponse

The PR adds safeApiResponse() to whitelist and truncate fields from upstream Help Scout error bodies (capping message to 200 chars in details.apiResponse). However, both the 422 handler (line 464) and the general 4xx handler (line 479) also interpolate responseData.message directly into the top-level message field without any truncation. This means a 500-char upstream message is correctly truncated in details.apiResponse.message but appears in full in the top-level message. The test at line 471 only asserts transformed.details.apiResponse.message has length 200 — it never checks transformed.message, which would be 529 chars. This defeats the stated PII-defense goal if the upstream message field ever echoes user input.

Prompt for agents
In src/utils/helpscout-client.ts, the transformError method has two places where responseData.message is interpolated into the top-level error message without the 200-char truncation that safeApiResponse applies:

1. Line 464 (422 handler): message: Help Scout API validation error: ${responseData.message || 'Invalid request data'}
2. Line 479 (4xx handler): message: Help Scout API client error: ${responseData.message || 'Invalid request'}

Similarly, in src/utils/helpscout-docs-client.ts, the transformError method uses apiMessage (derived from responseData.error or responseData.message without truncation) in the top-level message field at lines 195 and 216.

To fix, truncate the upstream message before interpolating into the top-level message field. For example, extract a helper like:
  const safeMsg = typeof responseData.message === 'string' ? responseData.message.slice(0, 200) : undefined;
and use safeMsg in the template literal. Apply the same treatment to apiMessage in the docs client.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

details: {
requestId,
validationErrors: responseData.errors || responseData,
// Surface only whitelisted fields; do not propagate the verbatim
// body or responseData.errors, which may echo customer input PII.
apiResponse: safeApiResponse(responseData),
suggestion: 'Check the request parameters match Help Scout API requirements',
},
};
Expand All @@ -462,7 +480,7 @@ export class HelpScoutClient {
details: {
requestId,
statusCode: error.response.status,
apiResponse: responseData,
apiResponse: safeApiResponse(responseData),
},
};
}
Expand Down
Loading