Skip to content

Commit db855f7

Browse files
fix: address code review findings from quality gate
- Guard response.json() with try-catch for non-JSON responses (502s, CDN errors) - Add AbortSignal.timeout(30s) to prevent hanging on unresponsive API - Validate VALIDKIT_API_URL requires https:// to prevent SSRF - Only send Content-Type header on POST requests (not GET) - Fix isDirectRun detection using import.meta.url (works with npx/symlinks) - Change tsconfig module/moduleResolution to Node16 for correct ESM resolution - Add 4 new tests covering all fixes (34 total, 100% coverage)
1 parent b1be32b commit db855f7

3 files changed

Lines changed: 87 additions & 11 deletions

File tree

src/index.test.ts

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@ describe('ValidKit MCP Server', () => {
467467
);
468468
});
469469

470-
it('GET requests do not include body', async () => {
470+
it('GET requests do not include body or Content-Type', async () => {
471471
const { client } = await createTestClient();
472472
mockFetchResponse(200, { used: 0, limit: 1000 });
473473

@@ -478,6 +478,66 @@ describe('ValidKit MCP Server', () => {
478478

479479
const callArgs = mockFetch.mock.calls[0][1];
480480
expect(callArgs.body).toBeUndefined();
481+
expect(callArgs.headers['Content-Type']).toBeUndefined();
482+
});
483+
484+
it('POST requests include Content-Type', async () => {
485+
const { client } = await createTestClient();
486+
mockFetchResponse(200, { email: 'a@b.com', valid: true });
487+
488+
await client.callTool({
489+
name: 'validate_email',
490+
arguments: { email: 'a@b.com' },
491+
});
492+
493+
const callArgs = mockFetch.mock.calls[0][1];
494+
expect(callArgs.headers['Content-Type']).toBe('application/json');
495+
});
496+
497+
it('rejects non-https VALIDKIT_API_URL', async () => {
498+
process.env.VALIDKIT_API_URL = 'http://evil.com';
499+
const { client } = await createTestClient();
500+
501+
const result = await client.callTool({
502+
name: 'validate_email',
503+
arguments: { email: 'test@gmail.com' },
504+
});
505+
506+
expect(result.isError).toBe(true);
507+
expect(getText(result)).toContain('must use https://');
508+
});
509+
510+
it('handles non-JSON API response gracefully', async () => {
511+
const { client } = await createTestClient();
512+
mockFetch.mockResolvedValueOnce({
513+
ok: false,
514+
status: 502,
515+
json: async () => {
516+
throw new SyntaxError('Unexpected token <');
517+
},
518+
});
519+
520+
const result = await client.callTool({
521+
name: 'validate_email',
522+
arguments: { email: 'test@gmail.com' },
523+
});
524+
525+
expect(result.isError).toBe(true);
526+
expect(getText(result)).toContain('Non-JSON response');
527+
expect(getText(result)).toContain('502');
528+
});
529+
530+
it('includes abort signal for timeout', async () => {
531+
const { client } = await createTestClient();
532+
mockFetchResponse(200, { email: 'a@b.com', valid: true });
533+
534+
await client.callTool({
535+
name: 'validate_email',
536+
arguments: { email: 'a@b.com' },
537+
});
538+
539+
const callArgs = mockFetch.mock.calls[0][1];
540+
expect(callArgs.signal).toBeDefined();
481541
});
482542

483543
it('missing API key error on bulk endpoint', async () => {

src/index.ts

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,20 @@
11
#!/usr/bin/env node
2+
import { fileURLToPath } from 'node:url';
23
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
34
import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js';
45
import { z } from 'zod';
56

67
export const VERSION = '1.1.0';
8+
const REQUEST_TIMEOUT_MS = 30_000;
79

810
export function getApiBaseUrl(): string {
9-
return process.env.VALIDKIT_API_URL || 'https://api.validkit.com';
11+
const url = process.env.VALIDKIT_API_URL || 'https://api.validkit.com';
12+
if (!url.startsWith('https://')) {
13+
throw new Error(
14+
`VALIDKIT_API_URL must use https:// (got "${url}")`
15+
);
16+
}
17+
return url;
1018
}
1119

1220
export function getApiKey(): string {
@@ -28,15 +36,26 @@ export async function callApi(
2836

2937
const response = await fetch(`${getApiBaseUrl()}${path}`, {
3038
method,
39+
signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS),
3140
headers: {
3241
'X-API-Key': apiKey,
33-
'Content-Type': 'application/json',
3442
'User-Agent': `validkit-mcp/${VERSION}`,
43+
...(body !== undefined ? { 'Content-Type': 'application/json' } : {}),
3544
},
3645
...(body !== undefined ? { body: JSON.stringify(body) } : {}),
3746
});
3847

39-
const data = await response.json();
48+
let data: unknown;
49+
try {
50+
data = await response.json();
51+
} catch {
52+
data = {
53+
error: {
54+
message: `Non-JSON response (HTTP ${response.status})`,
55+
},
56+
};
57+
}
58+
4059
return { ok: response.ok, status: response.status, data };
4160
}
4261

@@ -224,11 +243,8 @@ async function main() {
224243
await server.connect(transport);
225244
}
226245

227-
// Only auto-start when run directly (not when imported by tests)
228-
const isDirectRun =
229-
process.argv[1] &&
230-
(process.argv[1].endsWith('/index.js') ||
231-
process.argv[1].endsWith('/index.ts'));
246+
const currentFile = fileURLToPath(import.meta.url);
247+
const isDirectRun = process.argv[1] === currentFile;
232248

233249
if (isDirectRun) {
234250
main().catch((error) => {

tsconfig.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
{
22
"compilerOptions": {
33
"target": "ES2022",
4-
"module": "ES2022",
4+
"module": "Node16",
55
"lib": ["ES2022"],
66
"outDir": "./dist",
77
"rootDir": "./src",
88
"strict": true,
99
"esModuleInterop": true,
1010
"skipLibCheck": true,
1111
"forceConsistentCasingInFileNames": true,
12-
"moduleResolution": "bundler",
12+
"moduleResolution": "Node16",
1313
"resolveJsonModule": true,
1414
"declaration": true,
1515
"declarationMap": true,

0 commit comments

Comments
 (0)