Skip to content

Commit 657c9f9

Browse files
mrdailey99claude
andcommitted
fix: address PR #116 review comments
- Validate OAuth state parameter in listenForCallback (CSRF protection) - Fix PowerShell URL injection in openBrowser by passing URL via $args[0] - Fix status command to fall through to stored credentials on invalid env var - Apply username from live fetchKeyStatus response in status command - Persist username/tier/expires_at from login and rotate exchange responses - Fix httpsRequest to respect URL port and add 30s request timeout - Fix docs: QH URL default and validation_warning scope Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 10a1fc5 commit 657c9f9

7 files changed

Lines changed: 63 additions & 21 deletions

File tree

docs/mcp.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ sf provar auth clear
200200
| Variable | Purpose | Default |
201201
| ------------------------ | ------------------------------------- | ------------------------------------------------- |
202202
| `PROVAR_API_KEY` | API key for Quality Hub validation | None — falls back to `~/.provar/credentials.json` |
203-
| `PROVAR_QUALITY_HUB_URL` | Override the Quality Hub API base URL | Production URL |
203+
| `PROVAR_QUALITY_HUB_URL` | Override the Quality Hub API base URL | Dev API Gateway URL (`/dev`) |
204204

205205
---
206206

@@ -371,7 +371,7 @@ Validates an XML test case for schema correctness (validity score) and best prac
371371
| `best_practices_violations` | array | Best-practices violations with `rule_id`, `severity`, `weight`, `message` |
372372
| `best_practices_rules_evaluated` | integer | How many best-practices rules were checked |
373373
| `validation_source` | string | `quality_hub`, `local`, or `local_fallback` — see Authentication section |
374-
| `validation_warning` | string | Present when `validation_source` is `local_fallback`explains why |
374+
| `validation_warning` | string | Present when `validation_source` is `local` (onboarding) or `local_fallback` (explains why API failed) |
375375

376376
**Key schema rules:** TC_001 (missing XML declaration), TC_002 (malformed XML), TC_003 (wrong root element), TC_010/011/012 (missing/invalid id/guid), TC_031 (invalid apiCall guid), TC_034/035 (non-integer testItemId).
377377

src/commands/provar/auth/login.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ export default class SfProvarAuthLogin extends SfCommand<void> {
6565
loginFlowClient.openBrowser(authorizeUrl.toString());
6666

6767
this.log('\nWaiting for authentication... (Ctrl-C to cancel)');
68-
const authCode = await loginFlowClient.listenForCallback(port);
68+
const authCode = await loginFlowClient.listenForCallback(port, state);
6969

7070
// ── Step 5: Exchange code for Cognito tokens ────────────────────────────
7171
const tokens = await loginFlowClient.exchangeCodeForTokens({
@@ -81,7 +81,11 @@ export default class SfProvarAuthLogin extends SfCommand<void> {
8181
const keyData = await qualityHubClient.exchangeTokenForKey(tokens.access_token, baseUrl);
8282

8383
// ── Step 7: Persist the pv_k_ key ──────────────────────────────────────
84-
writeCredentials(keyData.api_key, keyData.prefix, 'cognito');
84+
writeCredentials(keyData.api_key, keyData.prefix, 'cognito', {
85+
username: keyData.username,
86+
tier: keyData.tier,
87+
expires_at: keyData.expires_at,
88+
});
8589

8690
this.log(`\nAuthenticated as ${keyData.username} (${keyData.tier} tier)`);
8791
this.log(`API key stored (prefix: ${keyData.prefix}). Valid until ${keyData.expires_at}.`);

src/commands/provar/auth/rotate.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,11 @@ export default class SfProvarAuthRotate extends SfCommand<void> {
3030

3131
try {
3232
const keyData = await qualityHubClient.rotateKey(stored.api_key, baseUrl);
33-
writeCredentials(keyData.api_key, keyData.prefix, 'cognito');
33+
writeCredentials(keyData.api_key, keyData.prefix, 'cognito', {
34+
username: keyData.username,
35+
tier: keyData.tier,
36+
expires_at: keyData.expires_at,
37+
});
3438
this.log(`API key rotated (new prefix: ${keyData.prefix}). Valid until ${keyData.expires_at}.`);
3539
this.log(" Run 'sf provar auth status' to verify.");
3640
} catch (err) {

src/commands/provar/auth/status.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,20 +24,19 @@ export default class SfProvarAuthStatus extends SfCommand<void> {
2424

2525
if (envKey) {
2626
if (!envKey.startsWith('pv_k_')) {
27-
this.log('API key misconfigured.');
27+
this.log('Warning: PROVAR_API_KEY is set but invalid (does not start with "pv_k_").');
28+
this.log(` Value: "${envKey.substring(0, 10)}..." — ignored for API calls.`);
29+
this.log(' Fix: update PROVAR_API_KEY to a valid pv_k_ key from https://success.provartesting.com');
30+
this.log('');
31+
// Fall through to check stored credentials (matches resolveApiKey behaviour)
32+
} else {
33+
this.log('API key configured');
2834
this.log(' Source: environment variable (PROVAR_API_KEY)');
29-
this.log(` Value: "${envKey.substring(0, 10)}..." does not start with "pv_k_"`);
35+
this.log(` Prefix: ${envKey.substring(0, 12)}`);
3036
this.log('');
31-
this.log(' Validation mode: local only (invalid key — not used for API calls)');
32-
this.log(' Fix: update PROVAR_API_KEY to a valid pv_k_ key from https://success.provartesting.com');
37+
this.log(' Validation mode: Quality Hub API');
3338
return;
3439
}
35-
this.log('API key configured');
36-
this.log(' Source: environment variable (PROVAR_API_KEY)');
37-
this.log(` Prefix: ${envKey.substring(0, 12)}`);
38-
this.log('');
39-
this.log(' Validation mode: Quality Hub API');
40-
return;
4140
}
4241

4342
const stored = readStoredCredentials();
@@ -48,6 +47,7 @@ export default class SfProvarAuthStatus extends SfCommand<void> {
4847
try {
4948
const live = await qualityHubClient.fetchKeyStatus(stored.api_key, getQualityHubBaseUrl());
5049
liveValid = live.valid;
50+
if (live.username) stored.username = live.username;
5151
if (live.tier) stored.tier = live.tier;
5252
if (live.expires_at) stored.expires_at = live.expires_at;
5353
} catch {

src/services/auth/credentials.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,12 @@ export function readStoredCredentials(): StoredCredentials | null {
3838
}
3939
}
4040

41-
export function writeCredentials(key: string, prefix: string, source: StoredCredentials['source']): void {
41+
export function writeCredentials(
42+
key: string,
43+
prefix: string,
44+
source: StoredCredentials['source'],
45+
extra?: { username?: string; tier?: string; expires_at?: string }
46+
): void {
4247
if (!key.startsWith(KEY_PREFIX)) {
4348
throw new Error(`Invalid API key format. Keys must start with "${KEY_PREFIX}".`);
4449
}
@@ -49,6 +54,9 @@ export function writeCredentials(key: string, prefix: string, source: StoredCred
4954
prefix,
5055
set_at: new Date().toISOString(),
5156
source,
57+
...(extra?.username ? { username: extra.username } : {}),
58+
...(extra?.tier ? { tier: extra.tier } : {}),
59+
...(extra?.expires_at ? { expires_at: extra.expires_at } : {}),
5260
};
5361
// mode: 0o600 sets permissions atomically on file creation (POSIX).
5462
// chmodSync handles re-runs on existing files. Both are no-ops on Windows.

src/services/auth/loginFlow.ts

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,9 @@ export function openBrowser(url: string): void {
8282
execFile('open', [url]);
8383
break;
8484
case 'win32':
85-
// cmd.exe interprets '&' in URLs as a command separator, truncating the URL.
86-
// PowerShell's Start-Process passes the URL as a single argument without shell interpretation.
87-
execFile('powershell.exe', ['-NoProfile', '-Command', `Start-Process '${url}'`]);
85+
// Pass the URL via $args[0] so it is never interpolated into the -Command
86+
// string — avoids quote-breaking and injection risk from special characters.
87+
execFile('powershell.exe', ['-NoProfile', '-Command', 'Start-Process $args[0]', '-args', url]);
8888
break;
8989
default:
9090
execFile('xdg-open', [url]);
@@ -97,13 +97,27 @@ export function openBrowser(url: string): void {
9797
* Spin up a temporary localhost HTTP server that accepts exactly one callback
9898
* from Cognito's Hosted UI, extracts the auth code, and shuts down.
9999
*/
100-
export function listenForCallback(port: number): Promise<string> {
100+
export function listenForCallback(port: number, expectedState?: string): Promise<string> {
101101
return new Promise((resolve, reject) => {
102102
const server = http.createServer((req, res) => {
103103
const parsed = new URL(req.url ?? '/', `http://localhost:${port}`);
104104
const code = parsed.searchParams.get('code');
105105
const error = parsed.searchParams.get('error');
106106
const description = parsed.searchParams.get('error_description');
107+
const callbackState = parsed.searchParams.get('state');
108+
109+
if (expectedState && callbackState !== expectedState) {
110+
res.writeHead(400, { 'Content-Type': 'text/html; charset=utf-8' });
111+
res.end(
112+
'<html><body style="font-family:sans-serif;padding:2rem;max-width:480px">' +
113+
'<h2 style="color:#c23934">Authentication failed</h2>' +
114+
'<p>Invalid state parameter — possible CSRF attack. Please try again.</p>' +
115+
'</body></html>'
116+
);
117+
server.close();
118+
reject(new Error('OAuth callback state mismatch — possible CSRF. Try again.'));
119+
return;
120+
}
107121

108122
res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' });
109123
res.end(
@@ -166,6 +180,8 @@ export async function exchangeCodeForTokens(opts: {
166180

167181
// ── Internal HTTPS helper ─────────────────────────────────────────────────────
168182

183+
const REQUEST_TIMEOUT_MS = 30_000;
184+
169185
function httpsPost(
170186
url: string,
171187
body: string,
@@ -176,6 +192,7 @@ function httpsPost(
176192
const req = https.request(
177193
{
178194
hostname: parsed.hostname,
195+
port: parsed.port || undefined,
179196
path: parsed.pathname + parsed.search,
180197
method: 'POST',
181198
headers: {
@@ -191,6 +208,9 @@ function httpsPost(
191208
res.on('end', () => resolve({ status: res.statusCode ?? 0, responseBody: data }));
192209
}
193210
);
211+
req.setTimeout(REQUEST_TIMEOUT_MS, () => {
212+
req.destroy(new Error(`Cognito token exchange timed out after ${REQUEST_TIMEOUT_MS / 1000}s`));
213+
});
194214
req.on('error', reject);
195215
req.write(body);
196216
req.end();
@@ -208,6 +228,6 @@ export const loginFlowClient = {
208228
generateState,
209229
findAvailablePort,
210230
openBrowser,
211-
listenForCallback,
231+
listenForCallback: listenForCallback as (port: number, expectedState?: string) => Promise<string>,
212232
exchangeCodeForTokens,
213233
};

src/services/qualityHub/client.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,8 @@ function isOk(status: number): boolean {
242242
return status >= 200 && status < 300;
243243
}
244244

245+
const REQUEST_TIMEOUT_MS = 30_000;
246+
245247
function httpsRequest(
246248
url: string,
247249
method: string,
@@ -252,6 +254,7 @@ function httpsRequest(
252254
const parsed = new NodeURL(url);
253255
const opts = {
254256
hostname: parsed.hostname,
257+
port: parsed.port || undefined,
255258
path: parsed.pathname + parsed.search,
256259
method,
257260
headers: {
@@ -266,6 +269,9 @@ function httpsRequest(
266269
});
267270
res.on('end', () => resolve({ status: res.statusCode ?? 0, responseBody: data }));
268271
});
272+
req.setTimeout(REQUEST_TIMEOUT_MS, () => {
273+
req.destroy(new Error(`Quality Hub API request timed out after ${REQUEST_TIMEOUT_MS / 1000}s`));
274+
});
269275
req.on('error', reject);
270276
if (body) req.write(body);
271277
req.end();

0 commit comments

Comments
 (0)