From 6ef16f12b6776fb5bd1778df62717ed205a9f957 Mon Sep 17 00:00:00 2001 From: Michal Date: Fri, 17 Jul 2026 21:06:29 +0100 Subject: [PATCH] fix(cli): login over HTTPS + suggest last-used email auth.ts talked to mcpd over node:http unconditionally with port:url.port, so an https:// mcpdUrl connected to the ingress on :80, got a 301 redirect with an empty body (status < 400), and JSON.parse('') crashed login with "Unexpected end of JSON input". api-client.ts already handled this; the auth commands did not. - All 4 auth request fns now pick https/443 and the TLS driver from the URL protocol, mirroring api-client.ts. - parseJsonResponse() guard turns any non-JSON <400 body into a clear "Invalid response from mcpd" error instead of a raw SyntaxError. - login now suggests the previously-used email (when a session for the same mcpd exists) so pressing Enter reuses it after a session expires. - Regression tests drive the real default* request fns against a local HTTP server (valid JSON, empty body, HTML redirect body, https driver selection) and cover the email-suggestion behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/cli/src/commands/auth.ts | 78 +++++++++++++++++------ src/cli/tests/commands/auth.test.ts | 96 ++++++++++++++++++++++++++++- 2 files changed, 155 insertions(+), 19 deletions(-) diff --git a/src/cli/src/commands/auth.ts b/src/cli/src/commands/auth.ts index 46813a3..a1a907e 100644 --- a/src/cli/src/commands/auth.ts +++ b/src/cli/src/commands/auth.ts @@ -1,12 +1,13 @@ import { Command } from 'commander'; import http from 'node:http'; +import https from 'node:https'; import { loadConfig } from '../config/index.js'; import type { ConfigLoaderDeps } from '../config/index.js'; import { saveCredentials, loadCredentials, deleteCredentials } from '../auth/index.js'; import type { CredentialsDeps } from '../auth/index.js'; export interface PromptDeps { - input(message: string): Promise; + input(message: string, defaultValue?: string): Promise; password(message: string): Promise; } @@ -30,19 +31,35 @@ interface LoginResponse { user: { email: string }; } -function defaultLoginRequest(mcpdUrl: string, email: string, password: string): Promise { +// mcpd always answers these endpoints with JSON. A non-JSON body on a <400 +// response means we didn't actually reach mcpd (e.g. an ingress redirect landing +// on the wrong protocol/port), so surface a clear error instead of a raw +// SyntaxError stack trace. +function parseJsonResponse(raw: string, context: string): T { + try { + return JSON.parse(raw) as T; + } catch { + const preview = raw.trim() ? `: ${raw.slice(0, 200)}` : ' (empty body)'; + throw new Error( + `Invalid ${context} response from mcpd. Check that mcpdUrl points to an mcpd server${preview}`, + ); + } +} + +export function defaultLoginRequest(mcpdUrl: string, email: string, password: string): Promise { return new Promise((resolve, reject) => { const url = new URL('/api/v1/auth/login', mcpdUrl); + const isHttps = url.protocol === 'https:'; const body = JSON.stringify({ email, password }); const opts: http.RequestOptions = { hostname: url.hostname, - port: url.port, + port: url.port || (isHttps ? 443 : 80), path: url.pathname, method: 'POST', timeout: 10000, headers: { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(body) }, }; - const req = http.request(opts, (res) => { + const req = (isHttps ? https : http).request(opts, (res) => { const chunks: Buffer[] = []; res.on('data', (chunk: Buffer) => chunks.push(chunk)); res.on('end', () => { @@ -55,7 +72,11 @@ function defaultLoginRequest(mcpdUrl: string, email: string, password: string): reject(new Error(`Login failed (${res.statusCode}): ${raw}`)); return; } - resolve(JSON.parse(raw) as LoginResponse); + try { + resolve(parseJsonResponse(raw, 'login')); + } catch (err) { + reject(err as Error); + } }); }); req.on('error', (err) => reject(new Error(`Cannot reach mcpd: ${err.message}`))); @@ -68,15 +89,16 @@ function defaultLoginRequest(mcpdUrl: string, email: string, password: string): function defaultLogoutRequest(mcpdUrl: string, token: string): Promise { return new Promise((resolve) => { const url = new URL('/api/v1/auth/logout', mcpdUrl); + const isHttps = url.protocol === 'https:'; const opts: http.RequestOptions = { hostname: url.hostname, - port: url.port, + port: url.port || (isHttps ? 443 : 80), path: url.pathname, method: 'POST', timeout: 10000, headers: { 'Authorization': `Bearer ${token}` }, }; - const req = http.request(opts, (res) => { + const req = (isHttps ? https : http).request(opts, (res) => { res.resume(); res.on('end', () => resolve()); }); @@ -86,18 +108,19 @@ function defaultLogoutRequest(mcpdUrl: string, token: string): Promise { }); } -function defaultStatusRequest(mcpdUrl: string): Promise { +export function defaultStatusRequest(mcpdUrl: string): Promise { return new Promise((resolve, reject) => { const url = new URL('/api/v1/auth/status', mcpdUrl); + const isHttps = url.protocol === 'https:'; const opts: http.RequestOptions = { hostname: url.hostname, - port: url.port, + port: url.port || (isHttps ? 443 : 80), path: url.pathname, method: 'GET', timeout: 10000, headers: { 'Content-Type': 'application/json' }, }; - const req = http.request(opts, (res) => { + const req = (isHttps ? https : http).request(opts, (res) => { const chunks: Buffer[] = []; res.on('data', (chunk: Buffer) => chunks.push(chunk)); res.on('end', () => { @@ -106,7 +129,11 @@ function defaultStatusRequest(mcpdUrl: string): Promise { reject(new Error(`Status check failed (${res.statusCode}): ${raw}`)); return; } - resolve(JSON.parse(raw) as StatusResponse); + try { + resolve(parseJsonResponse(raw, 'status')); + } catch (err) { + reject(err as Error); + } }); }); req.on('error', (err) => reject(new Error(`Cannot reach mcpd: ${err.message}`))); @@ -115,9 +142,10 @@ function defaultStatusRequest(mcpdUrl: string): Promise { }); } -function defaultBootstrapRequest(mcpdUrl: string, email: string, password: string, name?: string): Promise { +export function defaultBootstrapRequest(mcpdUrl: string, email: string, password: string, name?: string): Promise { return new Promise((resolve, reject) => { const url = new URL('/api/v1/auth/bootstrap', mcpdUrl); + const isHttps = url.protocol === 'https:'; const payload: Record = { email, password }; if (name) { payload['name'] = name; @@ -125,13 +153,13 @@ function defaultBootstrapRequest(mcpdUrl: string, email: string, password: strin const body = JSON.stringify(payload); const opts: http.RequestOptions = { hostname: url.hostname, - port: url.port, + port: url.port || (isHttps ? 443 : 80), path: url.pathname, method: 'POST', timeout: 10000, headers: { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(body) }, }; - const req = http.request(opts, (res) => { + const req = (isHttps ? https : http).request(opts, (res) => { const chunks: Buffer[] = []; res.on('data', (chunk: Buffer) => chunks.push(chunk)); res.on('end', () => { @@ -140,7 +168,11 @@ function defaultBootstrapRequest(mcpdUrl: string, email: string, password: strin reject(new Error(`Bootstrap failed (${res.statusCode}): ${raw}`)); return; } - resolve(JSON.parse(raw) as LoginResponse); + try { + resolve(parseJsonResponse(raw, 'bootstrap')); + } catch (err) { + reject(err as Error); + } }); }); req.on('error', (err) => reject(new Error(`Cannot reach mcpd: ${err.message}`))); @@ -150,9 +182,15 @@ function defaultBootstrapRequest(mcpdUrl: string, email: string, password: strin }); } -async function defaultInput(message: string): Promise { +async function defaultInput(message: string, defaultValue?: string): Promise { const { default: inquirer } = await import('inquirer'); - const { answer } = await inquirer.prompt([{ type: 'input', name: 'answer', message }]); + const question: { type: 'input'; name: 'answer'; message: string; default?: string } = { + type: 'input', name: 'answer', message, + }; + if (defaultValue) { + question.default = defaultValue; + } + const { answer } = await inquirer.prompt([question]); return answer as string; } @@ -202,7 +240,11 @@ export function createLoginCommand(deps?: Partial): Command { }, credentialsDeps); log(`Logged in as ${result.user.email} (admin)`); } else { - const email = await prompt.input('Email:'); + // If we were previously logged into this same mcpd (e.g. the session + // just expired), suggest the last-used email so Enter reuses it. + const existing = loadCredentials(credentialsDeps); + const suggestedEmail = existing && existing.mcpdUrl === mcpdUrl ? existing.user : undefined; + const email = await prompt.input('Email:', suggestedEmail); const password = await prompt.password('Password:'); const result = await loginRequest(mcpdUrl, email, password); diff --git a/src/cli/tests/commands/auth.test.ts b/src/cli/tests/commands/auth.test.ts index 2193254..c568bee 100644 --- a/src/cli/tests/commands/auth.test.ts +++ b/src/cli/tests/commands/auth.test.ts @@ -2,7 +2,9 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { mkdtempSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { createLoginCommand, createLogoutCommand } from '../../src/commands/auth.js'; +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { createLoginCommand, createLogoutCommand, defaultStatusRequest } from '../../src/commands/auth.js'; import { saveCredentials, loadCredentials } from '../../src/auth/index.js'; import { saveConfig, DEFAULT_CONFIG } from '../../src/config/index.js'; @@ -94,6 +96,53 @@ describe('login command', () => { expect(capturedUrl).toBe('http://custom:3100'); }); + it('suggests the previously-used email when a session for the same mcpd exists', async () => { + saveConfig({ ...DEFAULT_CONFIG, mcpdUrl: 'http://custom:3100' }, { configDir: tempDir }); + saveCredentials({ token: 'stale', mcpdUrl: 'http://custom:3100', user: 'returning@test.com' }, { configDir: tempDir }); + let suggestedDefault: string | undefined = 'UNSET'; + const cmd = createLoginCommand({ + configDeps: { configDir: tempDir }, + credentialsDeps: { configDir: tempDir }, + prompt: { + // Simulate the user pressing Enter: return the suggested default unchanged. + input: async (_msg, def) => { suggestedDefault = def; return def ?? ''; }, + password: async () => 'newpass', + }, + log, + loginRequest: async (_url, email) => ({ token: 'fresh', user: { email } }), + logoutRequest: async () => {}, + statusRequest: async () => ({ hasUsers: true }), + bootstrapRequest: async () => ({ token: '', user: { email: '' } }), + }); + await cmd.parseAsync([], { from: 'user' }); + + expect(suggestedDefault).toBe('returning@test.com'); + const creds = loadCredentials({ configDir: tempDir }); + expect(creds!.user).toBe('returning@test.com'); + }); + + it('does not suggest an email from a different mcpd', async () => { + saveConfig({ ...DEFAULT_CONFIG, mcpdUrl: 'http://server-a:3100' }, { configDir: tempDir }); + saveCredentials({ token: 'stale', mcpdUrl: 'http://server-b:3100', user: 'elsewhere@test.com' }, { configDir: tempDir }); + let suggestedDefault: string | undefined = 'UNSET'; + const cmd = createLoginCommand({ + configDeps: { configDir: tempDir }, + credentialsDeps: { configDir: tempDir }, + prompt: { + input: async (_msg, def) => { suggestedDefault = def; return 'typed@test.com'; }, + password: async () => 'pass', + }, + log, + loginRequest: async (_url, email) => ({ token: 'tok', user: { email } }), + logoutRequest: async () => {}, + statusRequest: async () => ({ hasUsers: true }), + bootstrapRequest: async () => ({ token: '', user: { email: '' } }), + }); + await cmd.parseAsync([], { from: 'user' }); + + expect(suggestedDefault).toBeUndefined(); + }); + it('allows --mcpd-url flag override', async () => { let capturedUrl = ''; const cmd = createLoginCommand({ @@ -177,6 +226,51 @@ describe('login bootstrap flow', () => { }); }); +// Regression: `mcpctl login` crashed with "JSON Parse error: Unexpected EOF" +// when mcpdUrl used https. defaultStatusRequest hardcoded the plain-http driver +// and port 80, so an https mcpdUrl hit the ingress on :80, got a redirect with an +// empty body (status < 400), and JSON.parse('') threw a raw SyntaxError. +describe('defaultStatusRequest (real HTTP)', () => { + let server: http.Server; + let baseUrl: string; + let handler: (req: http.IncomingMessage, res: http.ServerResponse) => void; + + beforeEach(async () => { + handler = (_req, res) => { res.statusCode = 200; res.end(JSON.stringify({ hasUsers: true })); }; + server = http.createServer((req, res) => handler(req, res)); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as AddressInfo; + baseUrl = `http://127.0.0.1:${port}`; + }); + + afterEach(async () => { + await new Promise((resolve) => server.close(() => resolve())); + }); + + it('parses a valid JSON status response', async () => { + const status = await defaultStatusRequest(baseUrl); + expect(status.hasUsers).toBe(true); + }); + + it('rejects with a clear error (not a raw SyntaxError) on an empty body', async () => { + handler = (_req, res) => { res.statusCode = 200; res.end(''); }; + await expect(defaultStatusRequest(baseUrl)).rejects.toThrow(/Invalid status response from mcpd.*empty body/); + }); + + it('rejects with a clear error on a non-JSON (HTML redirect) body', async () => { + handler = (_req, res) => { res.statusCode = 200; res.end('301 Moved'); }; + await expect(defaultStatusRequest(baseUrl)).rejects.toThrow(/Invalid status response from mcpd/); + }); + + it('uses the TLS driver for an https URL (would silently succeed over plain http before the fix)', async () => { + const { port } = server.address() as AddressInfo; + // Point an https URL at the plain-http test server. The https driver cannot + // complete a TLS handshake with it, so this must fail to connect. The old + // code always used the http driver and would have resolved instead. + await expect(defaultStatusRequest(`https://127.0.0.1:${port}`)).rejects.toThrow(/Cannot reach mcpd/); + }); +}); + describe('logout command', () => { it('removes credentials on logout', async () => { saveCredentials({ token: 'tok', mcpdUrl: 'http://x:3100', user: 'alice' }, { configDir: tempDir });