Skip to content

Commit 38168dc

Browse files
steipeteYigtwxx
andcommitted
fix(vault): validate credential input safely
Co-authored-by: Yigtwxx <yigiterdogan023@gmail.com>
1 parent 607ec3e commit 38168dc

4 files changed

Lines changed: 233 additions & 11 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
- Honor `MCPORTER_OAUTH_NO_BROWSER` across serve/daemon OAuth flows, fail once with actionable reauthorization guidance without logging authorization URLs, and restart daemons when the normalized setting changes. (PR #284 / issue #283, thanks @vitalijssilins)
1818
- Accept RFC 7591 dynamic-client-registration arrays and timestamps in `mcporter vault set` while preserving null-compatible partial client information and provider metadata. (PR #288 / issue #286, thanks @feniix)
19+
- Sanitize malformed `mcporter vault set` JSON diagnostics and reject non-finite `expires_at` / `expiresAt` token values before persistence. (Follow-up to PR #287, thanks @Yigtwxx)
1920

2021
### Tooling
2122

src/cli/vault-command.ts

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ interface VaultPayload {
99
readonly clientInfo?: OAuthClientInformationMixed;
1010
}
1111

12+
type VaultPayloadSource = { kind: 'file'; path: string } | { kind: 'stdin' };
13+
1214
export interface VaultCommandOptions {
1315
readonly readStdin?: () => Promise<string>;
1416
}
@@ -44,7 +46,7 @@ async function handleVaultSet(
4446
throw new CliUsageError(`Unknown vault set argument '${args[0]}'.`);
4547
}
4648
const definition = runtime.getDefinition(server);
47-
const payload = validateVaultPayload(JSON.parse(await readPayload(source, options)));
49+
const payload = validateVaultPayload(parseVaultPayload(await readPayload(source, options), source));
4850
await saveVaultEntry(definition, {
4951
tokens: payload.tokens,
5052
...(payload.clientInfo ? { clientInfo: payload.clientInfo } : {}),
@@ -65,7 +67,7 @@ async function handleVaultClear(runtime: Pick<Runtime, 'getDefinition'>, args: s
6567
console.log(`Cleared OAuth vault entry for '${definition.name}'`);
6668
}
6769

68-
function consumeVaultPayloadSource(args: string[]): { kind: 'file'; path: string } | { kind: 'stdin' } {
70+
function consumeVaultPayloadSource(args: string[]): VaultPayloadSource {
6971
const fileIndex = args.indexOf('--tokens-file');
7072
const stdinIndex = args.indexOf('--stdin');
7173
if (fileIndex !== -1 && stdinIndex !== -1) {
@@ -86,10 +88,7 @@ function consumeVaultPayloadSource(args: string[]): { kind: 'file'; path: string
8688
throw new CliUsageError('Usage: mcporter vault set <server> (--tokens-file <path> | --stdin)');
8789
}
8890

89-
async function readPayload(
90-
source: { kind: 'file'; path: string } | { kind: 'stdin' },
91-
options: VaultCommandOptions
92-
): Promise<string> {
91+
async function readPayload(source: VaultPayloadSource, options: VaultCommandOptions): Promise<string> {
9392
if (source.kind === 'file') {
9493
return fs.readFile(source.path, 'utf8');
9594
}
@@ -107,6 +106,20 @@ async function readPayload(
107106
});
108107
}
109108

109+
function parseVaultPayload(raw: string, source: VaultPayloadSource): unknown {
110+
try {
111+
return JSON.parse(raw) as unknown;
112+
} catch {
113+
// V8 parser diagnostics can quote a prefix of the credential input. Name
114+
// only the source so secrets and token fragments cannot reach logs.
115+
throw new CliUsageError(
116+
source.kind === 'file'
117+
? `Vault payload file '${source.path}' is not valid JSON.`
118+
: 'Vault payload from stdin is not valid JSON.'
119+
);
120+
}
121+
}
122+
110123
function validateVaultPayload(value: unknown): VaultPayload {
111124
if (!value || typeof value !== 'object') {
112125
throw new CliUsageError('Vault payload must be a JSON object.');
@@ -143,11 +156,13 @@ function validateOAuthTokens(tokens: Record<string, unknown>): void {
143156
throw new CliUsageError(`Vault payload tokens.${key} must be a string.`);
144157
}
145158
}
146-
if (
147-
tokens.expires_in !== undefined &&
148-
(!Number.isFinite(tokens.expires_in) || typeof tokens.expires_in !== 'number')
149-
) {
150-
throw new CliUsageError('Vault payload tokens.expires_in must be a finite number.');
159+
// Keep the write boundary aligned with isStoredOAuthTokens: zero, negative,
160+
// and fractional values remain valid, but every accepted alias is finite.
161+
for (const key of ['expires_in', 'expires_at', 'expiresAt'] as const) {
162+
const value = tokens[key];
163+
if (value !== undefined && (typeof value !== 'number' || !Number.isFinite(value))) {
164+
throw new CliUsageError(`Vault payload tokens.${key} must be a finite number.`);
165+
}
151166
}
152167
}
153168

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
import { spawn } from 'node:child_process';
2+
import fs from 'node:fs/promises';
3+
import os from 'node:os';
4+
import path from 'node:path';
5+
import process from 'node:process';
6+
import { fileURLToPath } from 'node:url';
7+
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
8+
import type { ServerDefinition } from '../src/config.js';
9+
import { loadVaultEntry } from '../src/oauth-vault.js';
10+
11+
const CLI_ENTRY = fileURLToPath(new URL('../dist/cli.js', import.meta.url));
12+
const SERVER_URL = 'https://example.test/mcp';
13+
const definition: ServerDefinition = {
14+
name: 'demo',
15+
command: { kind: 'http', url: new URL(SERVER_URL) },
16+
auth: 'oauth',
17+
};
18+
19+
interface CliResult {
20+
exitCode: number | null;
21+
stdout: string;
22+
stderr: string;
23+
}
24+
25+
describe('built vault CLI input boundary', () => {
26+
const originalDataHome = process.env.XDG_DATA_HOME;
27+
let configPath: string;
28+
let dataHome: string;
29+
let tempDir: string;
30+
31+
beforeEach(async () => {
32+
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-vault-cli-'));
33+
dataHome = path.join(tempDir, 'data');
34+
configPath = path.join(tempDir, 'mcporter.json');
35+
process.env.XDG_DATA_HOME = dataHome;
36+
await fs.writeFile(
37+
configPath,
38+
JSON.stringify({ imports: [], mcpServers: { demo: { baseUrl: SERVER_URL, auth: 'oauth' } } }),
39+
'utf8'
40+
);
41+
});
42+
43+
afterEach(async () => {
44+
if (originalDataHome === undefined) delete process.env.XDG_DATA_HOME;
45+
else process.env.XDG_DATA_HOME = originalDataHome;
46+
await fs.rm(tempDir, { recursive: true, force: true });
47+
});
48+
49+
it('does not echo malformed credential input through parser diagnostics', async () => {
50+
const marker = 'VAULT_SECRET_MARKER_7H3K9';
51+
const result = await runVault(['--stdin'], `${marker} malformed`);
52+
const output = `${result.stdout}\n${result.stderr}`;
53+
54+
expect(result.exitCode).not.toBe(0);
55+
expect(output).toContain('Vault payload from stdin is not valid JSON.');
56+
expect(output).not.toContain(marker);
57+
expect(output).not.toContain('VAULT_SECR');
58+
expect(output).not.toContain('SyntaxError');
59+
expect(output).not.toContain('Unexpected token');
60+
});
61+
62+
it('names a malformed credential file without echoing its contents', async () => {
63+
const marker = 'VAULT_FILE_SECRET_MARKER_2Q8M4';
64+
const payloadPath = path.join(tempDir, 'tokens.json');
65+
await fs.writeFile(payloadPath, `${marker} malformed`, 'utf8');
66+
const result = await runVault(['--tokens-file', payloadPath]);
67+
const output = `${result.stdout}\n${result.stderr}`;
68+
69+
expect(result.exitCode).not.toBe(0);
70+
expect(output).toContain(`Vault payload file '${payloadPath}' is not valid JSON.`);
71+
expect(output).not.toContain(marker);
72+
expect(output).not.toContain('VAULT_FILE_SECR');
73+
expect(output).not.toContain('SyntaxError');
74+
});
75+
76+
it.each([
77+
['expires_at string', JSON.stringify(payload({ expires_at: 'soon' })), 'tokens.expires_at'],
78+
['expiresAt string', JSON.stringify(payload({ expiresAt: 'soon' })), 'tokens.expiresAt'],
79+
['expires_at positive infinity', rawPayload('expires_at', '1e999'), 'tokens.expires_at'],
80+
['expiresAt negative infinity', rawPayload('expiresAt', '-1e999'), 'tokens.expiresAt'],
81+
])('rejects invalid %s', async (_name, input, field) => {
82+
const result = await runVault(['--stdin'], input);
83+
expect(result.exitCode).not.toBe(0);
84+
expect(`${result.stdout}\n${result.stderr}`).toContain(`${field} must be a finite number`);
85+
});
86+
87+
it('rejects a JSON NaN literal at the sanitized parse boundary', async () => {
88+
const result = await runVault(['--stdin'], rawPayload('expires_at', 'NaN'));
89+
expect(result.exitCode).not.toBe(0);
90+
expect(`${result.stdout}\n${result.stderr}`).toContain('Vault payload from stdin is not valid JSON.');
91+
});
92+
93+
it.each([
94+
['expires_at', 0],
95+
['expiresAt', 1_754_600_000.5],
96+
] as const)('persists finite %s values and unrelated valid credential fields', async (field, value) => {
97+
const input = {
98+
tokens: {
99+
access_token: 'fake-access-token',
100+
token_type: 'Bearer',
101+
refresh_token: 'fake-refresh-token',
102+
scope: 'read write',
103+
issuer: 'https://issuer.example',
104+
expires_in: 3600.25,
105+
[field]: value,
106+
id_token: 'fake-id-token',
107+
},
108+
clientInfo: {
109+
client_id: 'fake-client',
110+
redirect_uris: ['https://example.test/callback'],
111+
grant_types: ['authorization_code', 'refresh_token'],
112+
response_types: ['code'],
113+
client_name: null,
114+
provider_metadata: { tenant: 'demo' },
115+
},
116+
};
117+
118+
const result = await runVault(['--stdin'], JSON.stringify(input));
119+
expect(result.exitCode, result.stderr).toBe(0);
120+
await expect(loadVaultEntry(definition)).resolves.toMatchObject(input);
121+
});
122+
123+
async function runVault(sourceArgs: string[], input?: string): Promise<CliResult> {
124+
const home = path.join(tempDir, 'home');
125+
await fs.mkdir(home, { recursive: true });
126+
return new Promise<CliResult>((resolve, reject) => {
127+
const child = spawn(
128+
process.execPath,
129+
[CLI_ENTRY, '--config', configPath, 'vault', 'set', 'demo', ...sourceArgs],
130+
{
131+
cwd: tempDir,
132+
env: {
133+
...process.env,
134+
HOME: home,
135+
XDG_CONFIG_HOME: path.join(home, 'config'),
136+
XDG_DATA_HOME: dataHome,
137+
XDG_CACHE_HOME: path.join(home, 'cache'),
138+
XDG_STATE_HOME: path.join(home, 'state'),
139+
MCPORTER_NO_FORCE_EXIT: '1',
140+
},
141+
stdio: ['pipe', 'pipe', 'pipe'],
142+
}
143+
);
144+
let stdout = '';
145+
let stderr = '';
146+
child.stdout.setEncoding('utf8');
147+
child.stderr.setEncoding('utf8');
148+
child.stdout.on('data', (chunk: string) => (stdout += chunk));
149+
child.stderr.on('data', (chunk: string) => (stderr += chunk));
150+
child.once('error', reject);
151+
child.once('exit', (exitCode) => resolve({ exitCode, stdout, stderr }));
152+
child.stdin.end(input);
153+
});
154+
}
155+
});
156+
157+
function payload(expiry: Record<string, unknown>): Record<string, unknown> {
158+
return { tokens: { access_token: 'fake-access-token', token_type: 'Bearer', ...expiry } };
159+
}
160+
161+
function rawPayload(field: string, value: string): string {
162+
return `{"tokens":{"access_token":"fake-access-token","token_type":"Bearer","${field}":${value}}}`;
163+
}

tests/vault-validation.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,18 @@ describe('vault command input validation', () => {
5555
{ tokens: { access_token: 'token', token_type: 'Bearer', expires_in: Number.POSITIVE_INFINITY } },
5656
'tokens.expires_in must be a finite number',
5757
],
58+
[
59+
{ tokens: { access_token: 'token', token_type: 'Bearer', expires_at: 'soon' } },
60+
'tokens.expires_at must be a finite number',
61+
],
62+
[
63+
{ tokens: { access_token: 'token', token_type: 'Bearer', expires_at: Number.NaN } },
64+
'tokens.expires_at must be a finite number',
65+
],
66+
[
67+
{ tokens: { access_token: 'token', token_type: 'Bearer', expiresAt: Number.NEGATIVE_INFINITY } },
68+
'tokens.expiresAt must be a finite number',
69+
],
5870
[
5971
{ tokens: { access_token: 'token', token_type: 'Bearer' }, clientInfo: [] },
6072
"Vault payload 'clientInfo' must be an object",
@@ -84,4 +96,35 @@ describe('vault command input validation', () => {
8496
message as string
8597
);
8698
});
99+
100+
it('sanitizes malformed stdin JSON without retaining its input prefix', async () => {
101+
const marker = 'VAULT_SECRET_MARKER_7H3K9';
102+
const error = await handleVault(runtime, ['set', 'calendar', '--stdin'], {
103+
readStdin: async () => `${marker} malformed`,
104+
}).then(
105+
() => undefined,
106+
(reason: unknown) => reason
107+
);
108+
109+
expect(error).toBeInstanceOf(Error);
110+
expect((error as Error).message).toBe('Vault payload from stdin is not valid JSON.');
111+
expect(String(error)).not.toContain(marker);
112+
expect((error as Error).stack).not.toContain(marker);
113+
});
114+
115+
it('names a malformed credential file without echoing its contents', async () => {
116+
const payloadPath = path.join(tempDir, 'tokens.json');
117+
const marker = 'VAULT_FILE_SECRET_MARKER_2Q8M4';
118+
await fs.writeFile(payloadPath, `${marker} malformed`, 'utf8');
119+
120+
const error = await handleVault(runtime, ['set', 'calendar', '--tokens-file', payloadPath]).then(
121+
() => undefined,
122+
(reason: unknown) => reason
123+
);
124+
125+
expect(error).toBeInstanceOf(Error);
126+
expect((error as Error).message).toBe(`Vault payload file '${payloadPath}' is not valid JSON.`);
127+
expect(String(error)).not.toContain(marker);
128+
expect((error as Error).stack).not.toContain(marker);
129+
});
87130
});

0 commit comments

Comments
 (0)