diff --git a/cli/src/commands/api/lib/body.ts b/cli/src/commands/api/lib/body.ts index a88d7fa..b486c90 100644 --- a/cli/src/commands/api/lib/body.ts +++ b/cli/src/commands/api/lib/body.ts @@ -62,7 +62,19 @@ function parseFieldEntry(entry: string): [string, string] { return [key, value]; } -export type ApiFieldValue = string | number | boolean | null; +type ExactJsonInteger = { + kind: "exact_json_integer"; + json: string; +}; + +function canonicalizeJsonInteger(value: string): string { + const negative = value.startsWith("-"); + const digits = negative ? value.slice(1) : value; + const canonicalDigits = digits.replace(/^0+/, "") || "0"; + return negative && canonicalDigits !== "0" ? `-${canonicalDigits}` : canonicalDigits; +} + +export type ApiFieldValue = string | number | boolean | null | ExactJsonInteger; export type ParsedApiField = { key: string; @@ -80,7 +92,9 @@ function parseTypedFieldValue(value: string): ApiFieldValue { return null; } if (/^-?\d+$/.test(value)) { - return Number(value); + const json = canonicalizeJsonInteger(value); + const parsed = Number(json); + return Number.isSafeInteger(parsed) ? parsed : { kind: "exact_json_integer", json }; } if (value === "@-") { return readFileSync(0, "utf8"); @@ -114,12 +128,30 @@ function parseTypedFields( }); } -function parsedFieldsToRecord(fields: ParsedApiField[]): Record { - const body: Record = {}; +function isExactJsonInteger(value: ApiFieldValue): value is ExactJsonInteger { + return typeof value === "object" && value !== null && value.kind === "exact_json_integer"; +} + +export function formatApiFieldValue(value: ApiFieldValue): string { + if (isExactJsonInteger(value)) { + return value.json; + } + return value === null ? "null" : String(value); +} + +function serializeApiFieldValue(value: ApiFieldValue): string { + return isExactJsonInteger(value) ? value.json : JSON.stringify(value); +} + +function serializeApiFields(fields: ParsedApiField[]): string { + const valuesByKey = new Map(); for (const field of fields) { - body[field.key] = field.value; + valuesByKey.set(field.key, field.value); } - return body; + const properties = [...valuesByKey].map( + ([key, value]) => `${JSON.stringify(key)}:${serializeApiFieldValue(value)}`, + ); + return `{${properties.join(",")}}`; } export type ResolveApiBodyOptions = { @@ -189,7 +221,7 @@ export function resolveApiRequestPayload( if (hasFields) { return { - body: JSON.stringify(parsedFieldsToRecord(parsedFields)), + body: serializeApiFields(parsedFields), queryFields: [], }; } diff --git a/cli/src/commands/api/lib/http.test.ts b/cli/src/commands/api/lib/http.test.ts index c1909cc..fcc94ec 100644 --- a/cli/src/commands/api/lib/http.test.ts +++ b/cli/src/commands/api/lib/http.test.ts @@ -71,6 +71,24 @@ describe("api-body", () => { expect(JSON.parse(payload.body ?? "")).toEqual({ label: "CI Bot", name: "Analytics" }); }); + test("preserves typed integers outside JavaScript's safe range", () => { + const payload = resolveApiRequestPayload({ + method: "POST", + typedFields: ["id=0009007199254740993", "negative=-0009007199254740993", "zero=-000"], + }); + + expect(payload.body).toBe('{"id":9007199254740993,"negative":-9007199254740993,"zero":0}'); + expect(() => JSON.parse(payload.body ?? "")).not.toThrow(); + + expect( + resolveApiHttp({ + endpoint: "/records", + method: "GET", + typedFields: ["id=0009007199254740993"], + }).endpoint, + ).toBe("/records?id=9007199254740993"); + }); + test("keeps an explicit JSON request body from --input", () => { const filePath = join(testHome, "payload.json"); writeFileSync(filePath, '{"label":"raw"}', "utf8"); diff --git a/cli/src/commands/api/lib/http.ts b/cli/src/commands/api/lib/http.ts index fc3a68f..c3dbbe8 100644 --- a/cli/src/commands/api/lib/http.ts +++ b/cli/src/commands/api/lib/http.ts @@ -1,4 +1,8 @@ -import { resolveApiRequestPayload, type ParsedApiField } from "@/commands/api/lib/body.ts"; +import { + formatApiFieldValue, + resolveApiRequestPayload, + type ParsedApiField, +} from "@/commands/api/lib/body.ts"; import type { CommandOutputMode } from "@/lib/command-output.ts"; import { CliError } from "@/lib/errors.ts"; import { encodeManagementEndpoint } from "@/lib/management-endpoint.ts"; @@ -106,7 +110,7 @@ function appendQueryFields(endpoint: string, fields: ParsedApiField[]): string { const separator = endpoint.includes("?") ? "&" : "?"; const searchParams = new URLSearchParams(); for (const field of fields) { - searchParams.append(field.key, field.value === null ? "null" : String(field.value)); + searchParams.append(field.key, formatApiFieldValue(field.value)); } return `${endpoint}${separator}${searchParams.toString()}`; } diff --git a/cli/src/commands/append/lib/data.test.ts b/cli/src/commands/append/lib/data.test.ts index eba5dc6..af272da 100644 --- a/cli/src/commands/append/lib/data.test.ts +++ b/cli/src/commands/append/lib/data.test.ts @@ -3,8 +3,9 @@ import { parseAppendJsonContent } from "@/commands/append/lib/data.ts"; import { CliError } from "@/lib/errors.ts"; describe("parseAppendJsonContent", () => { - test("normalizes inline JSON objects", () => { - expect(parseAppendJsonContent('{ "id": 1 }')).toBe('{"id":1}'); + test("preserves inline JSON without rounding large integers", () => { + const input = '{ "id": 9007199254740993 }'; + expect(parseAppendJsonContent(input)).toBe(input); }); test("rejects non-JSON data", () => { diff --git a/cli/src/commands/append/lib/data.ts b/cli/src/commands/append/lib/data.ts index 87ebfa6..c281e23 100644 --- a/cli/src/commands/append/lib/data.ts +++ b/cli/src/commands/append/lib/data.ts @@ -18,7 +18,8 @@ export function parseAppendJsonContent(dataArg: string): string { } try { - return JSON.stringify(JSON.parse(jsonContent)); + JSON.parse(jsonContent); + return jsonContent; } catch { throw new CliError("Data must be valid JSON."); } diff --git a/cli/src/lib/config-files.ts b/cli/src/lib/config-files.ts index 9285bfa..caabd56 100644 --- a/cli/src/lib/config-files.ts +++ b/cli/src/lib/config-files.ts @@ -2,15 +2,60 @@ import { randomBytes } from "node:crypto"; import { mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { readEnv } from "@/lib/env.ts"; - -function trim(value: string): string { - return value.trim(); -} +import { ConfigurationError } from "@/lib/errors.ts"; function tempSiblingPath(filePath: string): string { return join(dirname(filePath), `.altertable-kv-${randomBytes(8).toString("hex")}`); } +function isFileNotFound(error: unknown): boolean { + return ( + typeof error === "object" && + error !== null && + "code" in error && + (error as { code?: unknown }).code === "ENOENT" + ); +} + +function configFileError(action: string, filePath: string, cause: unknown): ConfigurationError { + return new ConfigurationError(`Unable to ${action} configuration file: ${filePath}`, { cause }); +} + +function configKey(line: string): string { + const separatorIndex = line.indexOf("="); + return line.slice(0, separatorIndex === -1 ? undefined : separatorIndex).trim(); +} + +function readConfigFileIfPresent(filePath: string): string | undefined { + try { + return readFileSync(filePath, "utf8"); + } catch (error) { + if (isFileNotFound(error)) { + return undefined; + } + throw configFileError("read", filePath, error); + } +} + +function writeConfigFileAtomically(filePath: string, content: string, action: string): void { + const temporaryPath = tempSiblingPath(filePath); + let failure: unknown; + try { + writeFileSync(temporaryPath, content, { mode: 0o600 }); + renameSync(temporaryPath, filePath); + } catch (error) { + failure = error; + } + try { + rmSync(temporaryPath, { force: true }); + } catch (error) { + failure ??= error; + } + if (failure !== undefined) { + throw configFileError(action, filePath, failure); + } +} + export function configDir(): string { const override = readEnv("ALTERTABLE_CONFIG_HOME"); if (override) { @@ -29,97 +74,63 @@ export function credentialsFile(): string { } export function kvGet(filePath: string, key: string): string { - try { - const content = readFileSync(filePath, "utf8"); - for (const line of content.split("\n")) { - if (line === "" || line.startsWith("#")) { - continue; - } - const eqIndex = line.indexOf("="); - if (eqIndex === -1) { - continue; - } - const lineKey = trim(line.slice(0, eqIndex)); - if (lineKey === key) { - return trim(line.slice(eqIndex + 1)); - } - } - } catch { + const content = readConfigFileIfPresent(filePath); + if (content === undefined) { return ""; } + for (const line of content.split("\n")) { + if (line === "" || line.startsWith("#")) { + continue; + } + const eqIndex = line.indexOf("="); + if (eqIndex === -1) { + continue; + } + const lineKey = line.slice(0, eqIndex).trim(); + if (lineKey === key) { + return line.slice(eqIndex + 1).trim(); + } + } return ""; } export function kvSet(filePath: string, key: string, value: string): void { - mkdirSync(dirname(filePath), { recursive: true }); - const tmpPath = tempSiblingPath(filePath); - let found = false; - let lines: string[] = []; - try { - lines = readFileSync(filePath, "utf8").split("\n"); - } catch { - lines = []; + mkdirSync(dirname(filePath), { recursive: true }); + } catch (error) { + throw configFileError("write", filePath, error); } - const output: string[] = []; - for (const line of lines) { - if (line === "" && output.length === 0 && lines.length === 1) { - continue; - } - const eqIndex = line.indexOf("="); - const lineKey = eqIndex === -1 ? trim(line) : trim(line.slice(0, eqIndex)); - if (lineKey === key) { - output.push(`${key}=${value}`); + const lines = readConfigFileIfPresent(filePath)?.split("\n") ?? []; + let found = false; + const output = lines + .filter((line) => line !== "" || lines.length > 1) + .map((line) => { + if (configKey(line) !== key) { + return line; + } found = true; - } else { - output.push(line); - } - } - + return `${key}=${value}`; + }); if (!found) { output.push(`${key}=${value}`); } - try { - writeFileSync( - tmpPath, - output - .filter((line, index, array) => { - if (index === array.length - 1 && line === "") { - return false; - } - return true; - }) - .join("\n") + (output.length > 0 ? "\n" : ""), - { mode: 0o600 }, - ); - renameSync(tmpPath, filePath); - } finally { - rmSync(tmpPath, { force: true }); + while (output.at(-1) === "") { + output.pop(); } + writeConfigFileAtomically(filePath, `${output.join("\n")}\n`, "write"); } export function kvUnset(filePath: string, key: string): void { - try { - const lines = readFileSync(filePath, "utf8").split("\n"); - const tmpPath = tempSiblingPath(filePath); - const output: string[] = []; - for (const line of lines) { - const eqIndex = line.indexOf("="); - const lineKey = eqIndex === -1 ? trim(line) : trim(line.slice(0, eqIndex)); - if (lineKey === key) { - continue; - } - output.push(line); - } - try { - writeFileSync(tmpPath, output.join("\n"), { mode: 0o600 }); - renameSync(tmpPath, filePath); - } finally { - rmSync(tmpPath, { force: true }); - } - } catch { - // file does not exist + const content = readConfigFileIfPresent(filePath); + if (content === undefined) { + return; } + + const output = content + .split("\n") + .filter((line) => configKey(line) !== key) + .join("\n"); + writeConfigFileAtomically(filePath, output, "update"); } diff --git a/cli/src/lib/config.test.ts b/cli/src/lib/config.test.ts index e38ede9..59e5192 100644 --- a/cli/src/lib/config.test.ts +++ b/cli/src/lib/config.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdtempSync, readFileSync, rmSync, statSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -12,9 +12,11 @@ import { getQueryDefaultPager, kvGet, kvSet, + kvUnset, resolveApiBase, resolveManagementApiBase, } from "@/lib/config.ts"; +import { ConfigurationError } from "@/lib/errors.ts"; import { profileConfigFile } from "@/lib/profile-store.ts"; let testHome = ""; @@ -45,6 +47,15 @@ describe("config", () => { expect(configGet("missing", profileName)).toBe(""); }); + test("reports configuration I/O failures instead of treating them as missing files", () => { + const directoryPath = join(testHome, "not-a-file"); + mkdirSync(directoryPath); + + expect(() => kvGet(directoryPath, "user")).toThrow(ConfigurationError); + expect(() => kvSet(directoryPath, "user", "alice")).toThrow(ConfigurationError); + expect(() => kvUnset(directoryPath, "user")).toThrow(ConfigurationError); + }); + test("resolves API bases from defaults, stored config, and environment overrides", () => { expect(resolveApiBase(profileName)).toBe("https://api.altertable.ai"); expect(resolveManagementApiBase(profileName)).toBe("https://app.altertable.ai/rest/v1"); diff --git a/cli/src/lib/http.test.ts b/cli/src/lib/http.test.ts index 5416a9f..b7ddb07 100644 --- a/cli/src/lib/http.test.ts +++ b/cli/src/lib/http.test.ts @@ -458,14 +458,93 @@ describe("httpSendStream live timeouts", () => { await reader.read(); expect.unreachable("stream read should have timed out"); } catch (error) { - expect(error).toBeInstanceOf(DOMException); - expect((error as DOMException).message).toBe("stream timed out"); + expect(error).toBeInstanceOf(TimeoutError); + expect((error as TimeoutError).exitCode).toBe(9); + expect((error as TimeoutError).message).toBe( + "Request timed out: POST https://example.com/query", + ); } expect(abortObserved).toBe(true); } finally { globalThis.fetch = originalFetch; } }); + + test("positive readTimeoutMs also aborts stalled error response bodies", async () => { + delete process.env.ALTERTABLE_MOCK_HTTP_FILE; + const originalFetch = globalThis.fetch; + + globalThis.fetch = Object.assign( + async (_url: Parameters[0], init?: Parameters[1]) => { + const signal = init?.signal; + return new Response( + new ReadableStream({ + start(controller) { + signal?.addEventListener("abort", () => { + controller.error(new DOMException("error body timed out", "AbortError")); + }); + }, + }), + { status: 500 }, + ); + }, + { preconnect: originalFetch.preconnect }, + ); + + try { + expect( + httpSendStream({ + method: "POST", + url: "https://example.com/query", + authHeader: "Authorization: Bearer test", + body: "{}", + connectTimeoutMs: 50, + readTimeoutMs: 5, + }), + ).rejects.toBeInstanceOf(TimeoutError); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("reports peer-aborted streams as network errors instead of timeouts", async () => { + delete process.env.ALTERTABLE_MOCK_HTTP_FILE; + const originalFetch = globalThis.fetch; + + globalThis.fetch = Object.assign( + async () => + new Response( + new ReadableStream({ + start(controller) { + controller.error(new DOMException("peer aborted", "AbortError")); + }, + }), + { status: 200 }, + ), + { preconnect: originalFetch.preconnect }, + ); + + try { + const stream = await httpSendStream({ + method: "POST", + url: "https://example.com/query", + authHeader: "Authorization: Bearer test", + body: "{}", + connectTimeoutMs: 50, + readTimeoutMs: 50, + }); + + try { + await stream.getReader().read(); + expect.unreachable("stream read should have failed"); + } catch (error) { + expect(error).toBeInstanceOf(NetworkError); + expect(error).not.toBeInstanceOf(TimeoutError); + } + } finally { + globalThis.fetch = originalFetch; + } + }); }); describe("debug response redaction", () => { diff --git a/cli/src/lib/http.ts b/cli/src/lib/http.ts index 3926cad..9ac7577 100644 --- a/cli/src/lib/http.ts +++ b/cli/src/lib/http.ts @@ -98,10 +98,6 @@ export function resolveFetchTimeoutMs(options: HttpSendOptions): number { return connectTimeoutMs; } -function isAbortError(error: unknown): boolean { - return error instanceof Error && error.name === "AbortError"; -} - /** Flatten an Error's `cause` chain into a readable detail (e.g. fetch failed -> TLS reason). */ function unwrapErrorDetail(error: unknown): string { const messages: string[] = []; @@ -128,9 +124,19 @@ function connectionError(options: HttpSendOptions, cause: unknown): NetworkError ); } +function transportError( + options: HttpSendOptions, + signal: AbortSignal, + cause: unknown, +): NetworkError { + return signal.aborted ? timeoutError(options, cause) : connectionError(options, cause); +} + function streamWithTimeoutCleanup( stream: ReadableStream, clearTimeoutAfterRead: () => void, + signal: AbortSignal, + options: HttpStreamOptions, ): ReadableStream { let reader: ReturnType | undefined; return new ReadableStream({ @@ -149,7 +155,7 @@ function streamWithTimeoutCleanup( } } catch (error) { clearTimeoutAfterRead(); - controller.error(error); + controller.error(transportError(options, signal, error)); } }, cancel(reason) { @@ -408,13 +414,15 @@ async function executeLiveRequest(options: HttpSendOptions): Promise { dispatcher: getSharedDispatcher(), } as RequestInit); } catch (error) { - if (isAbortError(error)) { - throw timeoutError(options, error); - } - throw connectionError(options, error); + throw transportError(options, signal, error); } - const responseBody = await response.text(); + let responseBody: string; + try { + responseBody = await response.text(); + } catch (error) { + throw transportError(options, signal, error); + } logHttpDebug([ `< HTTP/${response.status}`, @@ -440,9 +448,7 @@ async function executeLiveStream(options: HttpStreamOptions): Promise | undefined = setTimeout(() => { - abortController.abort(); - }, connectTimeoutMs); + let timeout: ReturnType | undefined; const clearActiveTimeout = () => { if (timeout === undefined) { return; @@ -450,6 +456,11 @@ async function executeLiveStream(options: HttpStreamOptions): Promise { + clearActiveTimeout(); + timeout = setTimeout(() => abortController.abort(), durationMs); + }; + startTimeout(connectTimeoutMs); logHttpRequest(options); logDebug(`Request: ${options.method} ${options.url}`); @@ -466,10 +477,7 @@ async function executeLiveStream(options: HttpStreamOptions): Promise= 200 && response.status < 300) { @@ -479,13 +487,27 @@ async function executeLiveStream(options: HttpStreamOptions): Promise { - abortController.abort(); - }, readTimeoutMs); - return streamWithTimeoutCleanup(response.body, clearActiveTimeout); + startTimeout(readTimeoutMs); + return streamWithTimeoutCleanup( + response.body, + clearActiveTimeout, + abortController.signal, + options, + ); + } + + if (readTimeoutMs > 0) { + startTimeout(readTimeoutMs); } - const responseBody = await response.text(); + let responseBody: string; + try { + responseBody = await response.text(); + } catch (error) { + throw transportError(options, abortController.signal, error); + } finally { + clearActiveTimeout(); + } throwHttpError( response.status, responseBody, diff --git a/cli/src/lib/profile-configure-core.test.ts b/cli/src/lib/profile-configure-core.test.ts index 14f3a93..c58a6cb 100644 --- a/cli/src/lib/profile-configure-core.test.ts +++ b/cli/src/lib/profile-configure-core.test.ts @@ -1,8 +1,9 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdtempSync, rmSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { configGet } from "@/lib/config.ts"; +import { ConfigurationError } from "@/lib/errors.ts"; import { configureRunClear, configureRunSet } from "@/lib/profile-configure-core.ts"; import { profileExists } from "@/lib/profile-store.ts"; import { createCliRuntime } from "@/lib/runtime.ts"; @@ -91,4 +92,10 @@ describe("configureRunClear", () => { expect(secretGet("api-key", "prod")).toBe(""); expect(secretGet("lakehouse/password", "staging")).toBe(""); }); + + test("reports failures while removing configuration", () => { + mkdirSync(join(testHome, "config")); + + expect(() => configureRunClear()).toThrow(ConfigurationError); + }); }); diff --git a/cli/src/lib/profile-configure-core.ts b/cli/src/lib/profile-configure-core.ts index 59eaa5e..a66c21f 100644 --- a/cli/src/lib/profile-configure-core.ts +++ b/cli/src/lib/profile-configure-core.ts @@ -1,6 +1,6 @@ import { unlinkSync, rmSync, existsSync } from "node:fs"; import { configFile, configGet, configSet, credentialsFile, kvUnset } from "@/lib/config.ts"; -import { CliError } from "@/lib/errors.ts"; +import { CliError, ConfigurationError } from "@/lib/errors.ts"; import { deriveProfileName, listProfiles } from "@/lib/profile/model.ts"; import { ensureProfileExists, @@ -228,14 +228,25 @@ export function configureRunClear(sink: OutputSink = getOutputSink()): void { for (const filePath of [configFile(), credentialsFile()]) { try { unlinkSync(filePath); - } catch { - // already removed + } catch (error) { + if ( + typeof error !== "object" || + error === null || + !("code" in error) || + (error as { code?: unknown }).code !== "ENOENT" + ) { + throw new ConfigurationError(`Unable to remove configuration file: ${filePath}`, { + cause: error, + }); + } } } try { rmSync(profilesDir(), { recursive: true, force: true }); - } catch { - // already removed + } catch (error) { + throw new ConfigurationError(`Unable to remove profiles directory: ${profilesDir()}`, { + cause: error, + }); } getCliRuntime().session = undefined; sink.writeMetadata([ diff --git a/cli/src/lib/profile-store.test.ts b/cli/src/lib/profile-store.test.ts index 71fb878..5a17c43 100644 --- a/cli/src/lib/profile-store.test.ts +++ b/cli/src/lib/profile-store.test.ts @@ -8,6 +8,7 @@ import { ensureProfileExists, ensureProfilesLayout, getActiveProfileName, + getProfileId, profileConfigFile, profileExists, resolveWorkingProfile, @@ -33,7 +34,8 @@ describe("profile store", () => { test("creates the default profile layout and active selection", () => { ensureProfilesLayout(); - expect(existsSync(profileConfigFile(DEFAULT_PROFILE_NAME))).toBe(false); + expect(existsSync(profileConfigFile(DEFAULT_PROFILE_NAME))).toBe(true); + expect(getProfileId(DEFAULT_PROFILE_NAME)).toMatch(/^[0-9a-f-]{36}$/); expect(existsSync(join(testHome, "profiles", DEFAULT_PROFILE_NAME))).toBe(true); expect(kvGet(join(testHome, "config"), "active_profile")).toBe(DEFAULT_PROFILE_NAME); }); diff --git a/cli/src/lib/profile-store.ts b/cli/src/lib/profile-store.ts index 5df3573..0c3a785 100644 --- a/cli/src/lib/profile-store.ts +++ b/cli/src/lib/profile-store.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "node:crypto"; import { existsSync, mkdirSync, readdirSync } from "node:fs"; import { join, resolve, sep } from "node:path"; import { configDir, configFile, kvGet, kvSet } from "@/lib/config-files.ts"; @@ -5,6 +6,8 @@ import { ConfigurationError } from "@/lib/errors.ts"; import { readEnv } from "@/lib/env.ts"; export const DEFAULT_PROFILE_NAME = "default"; +const PROFILE_ID_KEY = "profile_id"; +const PROFILE_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; // Reserved pseudo-profile: the identity in effect when credentials come from // environment variables rather than a stored profile. Never a real directory. @@ -107,9 +110,30 @@ export function profileExists(name: string): boolean { return existsSync(profileDir(name)); } +export function getProfileId(name: string): string | undefined { + if (isFromEnvProfile(name) || !profileExists(name)) { + return undefined; + } + const id = kvGet(profileConfigFile(name), PROFILE_ID_KEY); + return PROFILE_ID_PATTERN.test(id) ? id : undefined; +} + +export function ensureProfileId(name: string): string { + assertSafeProfileName(name); + mkdirSync(profileDir(name), { recursive: true }); + const existingId = getProfileId(name); + if (existingId) { + return existingId; + } + const id = randomUUID(); + kvSet(profileConfigFile(name), PROFILE_ID_KEY, id); + return id; +} + export function ensureProfilesLayout(): void { if (!existsSync(profilesDir())) { mkdirSync(profileDir(DEFAULT_PROFILE_NAME), { recursive: true }); + ensureProfileId(DEFAULT_PROFILE_NAME); } if (kvGet(configFile(), "active_profile").length === 0) { kvSet(configFile(), "active_profile", DEFAULT_PROFILE_NAME); @@ -202,6 +226,7 @@ export function ensureProfileExists(name: string): void { if (!profileExists(name)) { mkdirSync(profileDir(name), { recursive: true }); } + ensureProfileId(name); } export function configGet(key: string, profileName: string): string { diff --git a/cli/src/lib/profile/model.test.ts b/cli/src/lib/profile/model.test.ts index 91a15fa..dc549ab 100644 --- a/cli/src/lib/profile/model.test.ts +++ b/cli/src/lib/profile/model.test.ts @@ -1,9 +1,11 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, renameSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { setCliContext } from "@/context.ts"; +import { credentialsFile, kvGet, kvSet } from "@/lib/config.ts"; import { configureRunSet } from "@/lib/profile-configure-core.ts"; +import { ConfigurationError } from "@/lib/errors.ts"; import { createEmptyProfile, deleteProfile, @@ -13,7 +15,15 @@ import { renameProfile, updateProfile, } from "@/lib/profile/model.ts"; -import { getActiveProfileName, profileExists, setActiveProfile } from "@/lib/profile-store.ts"; +import { + getActiveProfileName, + getProfileId, + ensureProfilesLayout, + profileConfigFile, + profileDir, + profileExists, + setActiveProfile, +} from "@/lib/profile-store.ts"; import { secretGet } from "@/lib/secrets.ts"; import { createFakeKeychain } from "@/test-utils/keychain.ts"; @@ -98,6 +108,7 @@ describe("profile model", () => { const keychain = createFakeKeychain(); keychain.store.secretSet("api-key", "atm_staging", "staging"); keychain.store.secretSet("lakehouse/password", "lakehouse-secret", "staging"); + const profileId = getProfileId("staging"); deleteProfile("staging", keychain.store); @@ -107,7 +118,7 @@ describe("profile model", () => { expect( keychain.calls.some( (args) => - args.includes("delete-generic-password") && args.includes("profile/staging/api-key"), + args.includes("delete-generic-password") && args.includes(`profile/${profileId}/api-key`), ), ).toBe(true); }); @@ -117,7 +128,7 @@ describe("profile model", () => { process.env.ALTERTABLE_SECRET_BACKEND = "keychain"; const keychain = createFakeKeychain(); keychain.store.secretSet("api-key", "atm_staging", "staging"); - keychain.failingDeletes.add("profile/staging/api-key"); + keychain.failingDeletes.add(`profile/${getProfileId("staging")}/api-key`); expect(() => deleteProfile("staging", keychain.store)).toThrow( "Failed to delete secret from macOS keychain", @@ -127,35 +138,204 @@ describe("profile model", () => { expect(keychain.store.secretGet("api-key", "staging")).toBe("atm_staging"); }); + test("restores profile config and secrets when directory deletion fails", () => { + createEmptyProfile("staging"); + updateProfile("staging", { organizationSlug: "acme", environment: "staging" }); + const originalConfig = readFileSync(profileConfigFile("staging")); + const keychain = createFakeKeychain(); + keychain.store.secretSet("api-key", "atm_staging", "staging"); + + expect(() => + deleteProfile("staging", keychain.store, { + renameDirectory(source, target) { + renameSync(profileDir(source), profileDir(target)); + }, + removeDirectory(name) { + rmSync(profileConfigFile(name)); + throw new Error("simulated partial remove failure"); + }, + setActive: setActiveProfile, + }), + ).toThrow("simulated partial remove failure"); + + expect(profileExists("staging")).toBe(true); + expect(readFileSync(profileConfigFile("staging"))).toEqual(originalConfig); + expect(keychain.store.secretGet("api-key", "staging")).toBe("atm_staging"); + }); + + test("restores a legacy configless profile with a stable identity after deletion fails", () => { + mkdirSync(profileDir("configless"), { recursive: true }); + const keychain = createFakeKeychain(); + + expect(() => + deleteProfile("configless", keychain.store, { + renameDirectory(source, target) { + renameSync(profileDir(source), profileDir(target)); + }, + removeDirectory(name) { + rmSync(profileDir(name), { recursive: true }); + throw new Error("simulated remove-then-fail"); + }, + setActive: setActiveProfile, + }), + ).toThrow("simulated remove-then-fail"); + + expect(profileExists("configless")).toBe(true); + expect(getProfileId("configless")).toMatch(/^[0-9a-f-]{36}$/); + }); + + test("reports incomplete recovery after profile deletion fails", () => { + createEmptyProfile("staging"); + const keychain = createFakeKeychain(); + keychain.store.secretSet("api-key", "atm_staging", "staging"); + const secrets = { + ...keychain.store, + secretSet() { + throw new Error("simulated secret restore failure"); + }, + }; + + let thrown: unknown; + try { + deleteProfile("staging", secrets, { + renameDirectory(source, target) { + renameSync(profileDir(source), profileDir(target)); + }, + removeDirectory() { + throw new Error("simulated deletion failure"); + }, + setActive: setActiveProfile, + }); + } catch (error) { + thrown = error; + } + + expect(thrown).toBeInstanceOf(ConfigurationError); + expect((thrown as ConfigurationError).message).toContain("rollback was incomplete"); + expect((thrown as ConfigurationError).cause).toBeInstanceOf(Error); + expect(((thrown as ConfigurationError).cause as Error).message).toBe( + "simulated deletion failure", + ); + expect((thrown as ConfigurationError).details).toContain( + "Rollback failure (restore secret api-key): simulated secret restore failure", + ); + }); + test("renames profile config, active selection, and secrets", async () => { await configureRunSet({ profile: "staging", apiKey: "atm_staging", env: "staging" }); setActiveProfile("staging"); + const profileId = getProfileId("staging"); renameProfile("staging", "acme_staging"); expect(profileExists("staging")).toBe(false); expect(profileExists("acme_staging")).toBe(true); expect(getActiveProfileName()).toBe("acme_staging"); + expect(getProfileId("acme_staging")).toBe(profileId); expect(secretGet("api-key", "acme_staging")).toBe("atm_staging"); expect(secretGet("api-key", "staging")).toBe(""); }); - test("rolls back profile config and Keychain secrets after a rename failure", () => { + test("renames a profile without rewriting Keychain secrets", () => { createEmptyProfile("staging"); process.env.ALTERTABLE_SECRET_BACKEND = "keychain"; const keychain = createFakeKeychain(); keychain.store.secretSet("api-key", "atm_staging", "staging"); keychain.store.secretSet("lakehouse/password", "lakehouse-secret", "staging"); - keychain.failingWrites.add("profile/acme_staging/lakehouse/password"); + const profileId = getProfileId("staging"); + const callsBeforeRename = keychain.calls.length; - expect(() => renameProfile("staging", "acme_staging", keychain.store)).toThrow( - "Failed to store secret in macOS keychain", - ); + renameProfile("staging", "acme_staging", keychain.store); + + expect(getProfileId("acme_staging")).toBe(profileId); + expect(keychain.store.secretGet("api-key", "acme_staging")).toBe("atm_staging"); + expect(keychain.store.secretGet("lakehouse/password", "acme_staging")).toBe("lakehouse-secret"); + const renameCalls = keychain.calls.slice(callsBeforeRename); + expect(renameCalls.some((args) => args.includes("add-generic-password"))).toBe(false); + expect(renameCalls.some((args) => args.includes("delete-generic-password"))).toBe(false); + }); + + test("migrates legacy name-keyed secrets before renaming a profile", () => { + ensureProfilesLayout(); + mkdirSync(profileDir("legacy"), { recursive: true }); + kvSet(credentialsFile(), "profile/legacy/api-key", "atm_legacy"); + + renameProfile("legacy", "renamed"); + + const profileId = getProfileId("renamed"); + expect(profileId).toBeTruthy(); + expect(secretGet("api-key", "renamed")).toBe("atm_legacy"); + expect(kvGet(credentialsFile(), "profile/legacy/api-key")).toBe(""); + expect(kvGet(credentialsFile(), `profile/${profileId}/api-key`)).toBe("atm_legacy"); + }); + + test("rolls back config and secrets when switching the renamed active profile fails", () => { + createEmptyProfile("staging"); + setActiveProfile("staging"); + const keychain = createFakeKeychain(); + keychain.store.secretSet("api-key", "atm_staging", "staging"); + + expect(() => + renameProfile("staging", "acme_staging", keychain.store, { + renameDirectory(source, target) { + renameSync(profileDir(source), profileDir(target)); + }, + removeDirectory(name) { + rmSync(profileDir(name), { recursive: true, force: true }); + }, + setActive(name) { + if (name === "acme_staging") { + throw new Error("simulated active-profile failure"); + } + setActiveProfile(name); + }, + }), + ).toThrow("simulated active-profile failure"); expect(profileExists("staging")).toBe(true); expect(profileExists("acme_staging")).toBe(false); + expect(getActiveProfileName()).toBe("staging"); expect(keychain.store.secretGet("api-key", "staging")).toBe("atm_staging"); - expect(keychain.store.secretGet("lakehouse/password", "staging")).toBe("lakehouse-secret"); expect(keychain.store.secretGet("api-key", "acme_staging")).toBe(""); }); + + test("reports incomplete recovery after profile rename fails", () => { + createEmptyProfile("staging"); + setActiveProfile("staging"); + const keychain = createFakeKeychain(); + + let thrown: unknown; + try { + renameProfile("staging", "acme_staging", keychain.store, { + renameDirectory(source, target) { + renameSync(profileDir(source), profileDir(target)); + }, + removeDirectory(name) { + rmSync(profileDir(name), { recursive: true, force: true }); + }, + setActive(name) { + throw new Error( + name === "acme_staging" + ? "simulated rename completion failure" + : "simulated active restore failure", + ); + }, + }); + } catch (error) { + thrown = error; + } + + expect(thrown).toBeInstanceOf(ConfigurationError); + expect((thrown as ConfigurationError).message).toContain("rollback was incomplete"); + expect(((thrown as ConfigurationError).cause as Error).message).toBe( + "simulated rename completion failure", + ); + expect((thrown as ConfigurationError).details).toContain( + "Rollback failure (restore active profile): simulated active restore failure", + ); + }); + + test("rejects a no-op rename when the source profile does not exist", () => { + expect(() => renameProfile("missing", "missing")).toThrow("Profile not found: missing"); + }); }); diff --git a/cli/src/lib/profile/model.ts b/cli/src/lib/profile/model.ts index b1c109b..84336bd 100644 --- a/cli/src/lib/profile/model.ts +++ b/cli/src/lib/profile/model.ts @@ -1,4 +1,13 @@ -import { renameSync, rmSync } from "node:fs"; +import { randomBytes } from "node:crypto"; +import { + existsSync, + mkdirSync, + readFileSync, + renameSync, + rmSync, + statSync, + writeFileSync, +} from "node:fs"; import { configDir, configFile, @@ -11,9 +20,11 @@ import { import { ConfigurationError } from "@/lib/errors.ts"; import { CONFIG_ENV_NAMES, isSecretEnv, readEnv } from "@/lib/env.ts"; import { - moveProfileSecrets, + migrateProfileSecrets, secretDelete, secretExists, + secretGet, + secretSet, secretStoreDisplay, type SecretStore, } from "@/lib/secrets.ts"; @@ -43,9 +54,77 @@ const PROFILE_SECRET_ACCOUNTS = [ "oauth/refresh-token", ] as const; -type ProfileSecretStore = Pick; +type ProfileSecretStore = Pick< + SecretStore, + "migrateProfileSecrets" | "secretDelete" | "secretGet" | "secretSet" +>; + +const PROFILE_SECRET_STORE: ProfileSecretStore = { + migrateProfileSecrets, + secretDelete, + secretGet, + secretSet, +}; + +type ProfileMutationOperations = { + renameDirectory(source: string, target: string): void; + removeDirectory(name: string): void; + setActive(name: string): void; +}; + +const PROFILE_MUTATION_OPERATIONS: ProfileMutationOperations = { + renameDirectory(source, target) { + renameSync(profileDir(source), profileDir(target)); + }, + removeDirectory(name) { + rmSync(profileDir(name), { recursive: true, force: true }); + }, + setActive: setActiveProfile, +}; + +type ProfileRollbackFailure = { + step: string; + error: unknown; +}; + +function profileMutationErrorDetail(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +function runProfileRollbackStep( + failures: ProfileRollbackFailure[], + step: string, + operation: () => void, +): void { + try { + operation(); + } catch (error) { + failures.push({ step, error }); + } +} -const PROFILE_SECRET_STORE: ProfileSecretStore = { moveProfileSecrets, secretDelete }; +function rethrowProfileMutationFailure( + originalError: unknown, + rollbackFailures: ProfileRollbackFailure[], + operation: string, + recoveryAdvice: string, +): never { + if (rollbackFailures.length === 0) { + throw originalError; + } + throw new ConfigurationError( + `${operation} failed and automatic rollback was incomplete. ${recoveryAdvice}`, + { + cause: originalError, + details: [ + `Original failure: ${profileMutationErrorDetail(originalError)}`, + ...rollbackFailures.map( + ({ step, error }) => `Rollback failure (${step}): ${profileMutationErrorDetail(error)}`, + ), + ].join("\n"), + }, + ); +} type ProfileManagementAuth = "oauth" | "api_key" | "none"; type ProfileLakehouseAuth = "basic_token" | "username_password" | "none"; @@ -408,44 +487,167 @@ export function updateProfile(name: string, update: ProfileUpdate): ProfileInspe return inspectProfile(name); } +type ProfileConfigSnapshot = { + contents: Buffer; + mode: number; +}; + +type ProfileSecretSnapshot = { + account: (typeof PROFILE_SECRET_ACCOUNTS)[number]; + value: string; +}; + +function snapshotProfileConfig(name: string): ProfileConfigSnapshot | undefined { + const filePath = profileConfigFile(name); + if (!existsSync(filePath)) { + return undefined; + } + return { + contents: readFileSync(filePath), + mode: statSync(filePath).mode & 0o777, + }; +} + +function snapshotProfileSecrets( + name: string, + secrets: ProfileSecretStore, +): ProfileSecretSnapshot[] { + return PROFILE_SECRET_ACCOUNTS.map((account) => ({ + account, + value: secrets.secretGet(account, name), + })); +} + +function rollbackProfileRename(options: { + source: string; + target: string; + wasActive: boolean; + operations: ProfileMutationOperations; +}): ProfileRollbackFailure[] { + const { source, target, wasActive, operations } = options; + const failures: ProfileRollbackFailure[] = []; + + if (profileExists(target) && !profileExists(source)) { + runProfileRollbackStep(failures, "restore profile directory", () => { + operations.renameDirectory(target, source); + }); + } + if (wasActive) { + runProfileRollbackStep(failures, "restore active profile", () => { + operations.setActive(source); + }); + } + + if (!profileExists(source)) { + failures.push({ + step: "verify source profile", + error: new Error(`Profile directory is missing: ${source}`), + }); + } + if (profileExists(target)) { + failures.push({ + step: "verify target cleanup", + error: new Error(`Renamed profile directory still exists: ${target}`), + }); + } + return failures; +} + +function rollbackProfileDelete(options: { + name: string; + quarantinedName: string; + configSnapshot: ProfileConfigSnapshot | undefined; + secretSnapshots: ProfileSecretSnapshot[]; + secrets: ProfileSecretStore; + operations: ProfileMutationOperations; +}): ProfileRollbackFailure[] { + const { name, quarantinedName, configSnapshot, secretSnapshots, secrets, operations } = options; + const failures: ProfileRollbackFailure[] = []; + + if (profileExists(quarantinedName) && !profileExists(name)) { + runProfileRollbackStep(failures, "restore profile directory", () => { + operations.renameDirectory(quarantinedName, name); + }); + } + + runProfileRollbackStep(failures, "restore profile configuration", () => { + mkdirSync(profileDir(name), { recursive: true }); + if (configSnapshot) { + writeFileSync(profileConfigFile(name), configSnapshot.contents, { + mode: configSnapshot.mode, + }); + } + }); + + for (const { account, value } of secretSnapshots) { + if (value.length === 0) continue; + runProfileRollbackStep(failures, `restore secret ${account}`, () => { + secrets.secretSet(account, value, name); + }); + } + + if (!profileExists(name)) { + failures.push({ + step: "verify source profile", + error: new Error(`Profile directory is missing: ${name}`), + }); + } + if (profileExists(quarantinedName)) { + failures.push({ + step: "verify quarantine cleanup", + error: new Error(`Quarantined profile directory still exists: ${quarantinedName}`), + }); + } + return failures; +} + export function renameProfile( source: string, target: string, secrets: ProfileSecretStore = PROFILE_SECRET_STORE, + operations: ProfileMutationOperations = PROFILE_MUTATION_OPERATIONS, ): void { assertSafeProfileName(source); assertSafeProfileName(target); ensureProfilesLayout(); - if (source === target) { - return; - } if (!profileExists(source)) { throw new ConfigurationError(`Profile not found: ${source}`); } + if (source === target) { + return; + } if (profileExists(target)) { throw new ConfigurationError(`Profile already exists: ${target}`); } - const active = getActiveProfileName(); - renameSync(profileDir(source), profileDir(target)); + const wasActive = getActiveProfileName() === source; + secrets.migrateProfileSecrets(source, PROFILE_SECRET_ACCOUNTS); + operations.renameDirectory(source, target); try { - secrets.moveProfileSecrets(source, target, [...PROFILE_SECRET_ACCOUNTS]); - } catch (error) { - if (profileExists(target) && !profileExists(source)) { - renameSync(profileDir(target), profileDir(source)); + if (wasActive) { + operations.setActive(target); } - throw error; - } - - if (active === source) { - setActiveProfile(target); + } catch (error) { + const rollbackFailures = rollbackProfileRename({ + source, + target, + wasActive, + operations, + }); + rethrowProfileMutationFailure( + error, + rollbackFailures, + `Renaming profile "${source}" to "${target}"`, + "Inspect both profiles before retrying.", + ); } } export function deleteProfile( name: string, secrets: ProfileSecretStore = PROFILE_SECRET_STORE, + operations: ProfileMutationOperations = PROFILE_MUTATION_OPERATIONS, ): void { assertSafeProfileName(name); ensureProfilesLayout(); @@ -461,11 +663,32 @@ export function deleteProfile( ); } - for (const account of PROFILE_SECRET_ACCOUNTS) { - secrets.secretDelete(account, name); + secrets.migrateProfileSecrets(name, PROFILE_SECRET_ACCOUNTS); + const secretSnapshots = snapshotProfileSecrets(name, secrets); + const configSnapshot = snapshotProfileConfig(name); + const quarantinedName = `.deleting-${randomBytes(8).toString("hex")}`; + operations.renameDirectory(name, quarantinedName); + try { + for (const account of PROFILE_SECRET_ACCOUNTS) { + secrets.secretDelete(account, quarantinedName); + } + operations.removeDirectory(quarantinedName); + } catch (error) { + const rollbackFailures = rollbackProfileDelete({ + name, + quarantinedName, + configSnapshot, + secretSnapshots, + secrets, + operations, + }); + rethrowProfileMutationFailure( + error, + rollbackFailures, + `Deleting profile "${name}"`, + "Inspect the profile before retrying.", + ); } - - rmSync(profileDir(name), { recursive: true, force: true }); } export type ConfigureCredentialStatus = { diff --git a/cli/src/lib/query-format.test.ts b/cli/src/lib/query-format.test.ts index cef6d3c..50c5980 100644 --- a/cli/src/lib/query-format.test.ts +++ b/cli/src/lib/query-format.test.ts @@ -130,6 +130,19 @@ describe("formatQueryCell", () => { expect(formatQueryCell("{}", { maxWidth: 10 })).toBe("{}"); }); + test("redacts sensitive keys inside JSON string cells", () => { + const output = formatQueryCell( + '{"password":"secret","nested":{"access_token":"token-value"},"id":9007199254740993,"name":"safe"}', + {}, + ); + + expect(output).toBe( + '{"password":"[REDACTED]","nested":{"access_token":"[REDACTED]"},"id":9007199254740993,"name":"safe"}', + ); + expect(output).not.toContain("secret"); + expect(output).not.toContain("token-value"); + }); + test("appends relative time to timestamps while keeping absolute form", () => { const timestamp = "2026-06-21T06:35:24.409Z"; const nowMs = Date.parse("2026-06-27T12:00:00.000Z"); diff --git a/cli/src/lib/query-format.ts b/cli/src/lib/query-format.ts index 1936fab..cd42a9f 100644 --- a/cli/src/lib/query-format.ts +++ b/cli/src/lib/query-format.ts @@ -13,7 +13,7 @@ import { type QueryDataType, } from "@/lib/query-column-types.ts"; import { pluralizeLabel } from "@/lib/pluralize.ts"; -import { redactPasswordFieldInText } from "@/lib/redact.ts"; +import { redactPasswordFieldInText, redactSensitiveJsonText } from "@/lib/redact.ts"; import { formatRelativeTimestamp, formatTimestampWithRelative } from "@/lib/relative-time.ts"; import { getTerminalWidth, @@ -98,8 +98,7 @@ function parseJsonStringValue(value: string): string | null { return null; } try { - JSON.parse(trimmed); - return trimmed; + return redactSensitiveJsonText(trimmed); } catch { return null; } @@ -192,14 +191,14 @@ function formatStringQueryCell( options: QueryCellOptions, colorize: boolean, ): string { - const sanitized = redactPasswordFieldInText(value); + const jsonText = parseJsonStringValue(value); + const sanitized = jsonText ?? redactPasswordFieldInText(value); if (sanitized.length === 0) { return colorize ? renderDisplayText([span('""', "subtle")]) : '""'; } const columnTypeMap = options.columnTypeMap ?? new Map(); const dataType = resolveCellDataType(sanitized, options.columnName, columnTypeMap); - const jsonText = parseJsonStringValue(sanitized); if (dataType === "timestamp" && isTimestampValue(sanitized)) { return formatTimestampQueryCell(sanitized, options, colorize); diff --git a/cli/src/lib/redact.test.ts b/cli/src/lib/redact.test.ts index e23c160..ce6814f 100644 --- a/cli/src/lib/redact.test.ts +++ b/cli/src/lib/redact.test.ts @@ -1,5 +1,9 @@ import { describe, expect, test } from "bun:test"; -import { MASKED_PASSWORD_PLACEHOLDER, redactPasswordFieldInText } from "@/lib/redact.ts"; +import { + MASKED_PASSWORD_PLACEHOLDER, + redactPasswordFieldInText, + redactSensitiveJsonText, +} from "@/lib/redact.ts"; describe("redactPasswordFieldInText", () => { test("redacts password:value without a space after the colon", () => { @@ -20,3 +24,38 @@ describe("redactPasswordFieldInText", () => { ); }); }); + +describe("redactSensitiveJsonText", () => { + const cases = [ + { + name: "nested objects inside arrays", + input: '[{"token":"secret","id":9007199254740993},{"safe":"value"}]', + expected: '[{"token":"[REDACTED]","id":9007199254740993},{"safe":"value"}]', + }, + { + name: "escaped sensitive keys", + input: '{"pass\\u0077ord":"secret","id":9007199254740993}', + expected: '{"pass\\u0077ord":"[REDACTED]","id":9007199254740993}', + }, + { + name: "structured sensitive values", + input: '{"client_secret":{"nested":[1,2]},"refresh_token":["a","b"],"safe":{"x":1}}', + expected: '{"client_secret":"[REDACTED]","refresh_token":"[REDACTED]","safe":{"x":1}}', + }, + { + name: "surrounding and structural whitespace", + input: ' \n { "password" : "secret" , "id" : 9007199254740993 } \t', + expected: ' \n { "password" : "[REDACTED]" , "id" : 9007199254740993 } \t', + }, + ] as const; + + for (const { name, input, expected } of cases) { + test(`redacts ${name} without changing other source text`, () => { + expect(redactSensitiveJsonText(input)).toBe(expected); + }); + } + + test("rejects malformed JSON", () => { + expect(() => redactSensitiveJsonText('{"password":"secret"')).toThrow(); + }); +}); diff --git a/cli/src/lib/redact.ts b/cli/src/lib/redact.ts index 03f4a63..bdc5f8b 100644 --- a/cli/src/lib/redact.ts +++ b/cli/src/lib/redact.ts @@ -41,6 +41,134 @@ export function redactSensitiveJsonValue(value: unknown): unknown { return value; } +class SourcePreservingJsonRedactor { + private position = 0; + + constructor(private readonly source: string) {} + + redact(): string { + const leadingWhitespace = this.takeWhitespace(); + const value = this.redactValue(false); + return leadingWhitespace + value + this.takeWhitespace(); + } + + private takeWhitespace(): string { + const start = this.position; + while (/\s/.test(this.source[this.position] ?? "")) this.position += 1; + return this.source.slice(start, this.position); + } + + private redactValue(shouldRedact: boolean): string { + if (shouldRedact) { + this.position = this.findValueEnd(this.position); + return '"[REDACTED]"'; + } + if (this.source[this.position] === "{") return this.redactObject(); + if (this.source[this.position] === "[") return this.redactArray(); + + const end = this.findValueEnd(this.position); + const value = this.source.slice(this.position, end); + this.position = end; + return value; + } + + private redactObject(): string { + let output = "{"; + this.position += 1; + output += this.takeWhitespace(); + + while (this.source[this.position] !== "}") { + const keyStart = this.position; + const keyEnd = this.findStringEnd(keyStart); + const rawKey = this.source.slice(keyStart, keyEnd); + const key = JSON.parse(rawKey) as string; + + output += rawKey; + this.position = keyEnd; + output += this.takeWhitespace(); + output += this.source[this.position] ?? ""; + this.position += 1; + output += this.takeWhitespace(); + output += this.redactValue(isSensitiveJsonKey(key)); + output += this.takeWhitespace(); + + if (this.source[this.position] === ",") { + output += ","; + this.position += 1; + output += this.takeWhitespace(); + } + } + + this.position += 1; + return `${output}}`; + } + + private redactArray(): string { + let output = "["; + this.position += 1; + output += this.takeWhitespace(); + + while (this.source[this.position] !== "]") { + output += this.redactValue(false); + output += this.takeWhitespace(); + if (this.source[this.position] === ",") { + output += ","; + this.position += 1; + output += this.takeWhitespace(); + } + } + + this.position += 1; + return `${output}]`; + } + + private findStringEnd(start: number): number { + let position = start + 1; + while (position < this.source.length) { + if (this.source[position] === "\\") { + position += 2; + continue; + } + if (this.source[position] === '"') return position + 1; + position += 1; + } + return this.source.length; + } + + private findValueEnd(start: number): number { + const firstCharacter = this.source[start]; + if (firstCharacter === '"') return this.findStringEnd(start); + if (firstCharacter !== "{" && firstCharacter !== "[") { + let position = start; + while (position < this.source.length && !/[\s,}\]]/.test(this.source[position] ?? "")) { + position += 1; + } + return position; + } + + const closingCharacters = [firstCharacter === "{" ? "}" : "]"]; + let position = start + 1; + while (position < this.source.length && closingCharacters.length > 0) { + const character = this.source[position]; + if (character === '"') { + position = this.findStringEnd(position); + continue; + } + if (character === "{") closingCharacters.push("}"); + if (character === "[") closingCharacters.push("]"); + if (character === closingCharacters.at(-1)) closingCharacters.pop(); + position += 1; + } + return position; + } +} + +/** Redact sensitive object fields while preserving all non-sensitive JSON source lexemes. */ +export function redactSensitiveJsonText(source: string): string { + JSON.parse(source); + return new SourcePreservingJsonRedactor(source).redact(); +} + export function truncateBodySnippet(body: string, maxLength = BODY_MAX_LENGTH): string { if (body.length <= maxLength) { return body; diff --git a/cli/src/lib/secrets.test.ts b/cli/src/lib/secrets.test.ts index 535fb6e..65d6b7a 100644 --- a/cli/src/lib/secrets.test.ts +++ b/cli/src/lib/secrets.test.ts @@ -3,6 +3,7 @@ import { mkdtempSync, rmSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { createFakeKeychain } from "@/test-utils/keychain.ts"; +import { getProfileId } from "@/lib/profile-store.ts"; import { secretExists, secretGet, secretSet } from "@/lib/secrets.ts"; let testHome = ""; @@ -56,8 +57,8 @@ describe("secretSet", () => { describe("secretDelete", () => { test("fails closed when Keychain deletion fails", () => { const keychain = createFakeKeychain(); - const account = "profile/default/api-key"; keychain.store.secretSet("api-key", "atm_test", "default"); + const account = `profile/${getProfileId("default")}/api-key`; keychain.failingDeletes.add(account); expect(() => keychain.store.secretDelete("api-key", "default")).toThrow( diff --git a/cli/src/lib/secrets.ts b/cli/src/lib/secrets.ts index 1f1094a..c7fd35b 100644 --- a/cli/src/lib/secrets.ts +++ b/cli/src/lib/secrets.ts @@ -5,7 +5,12 @@ import { credentialsFile, kvGet, kvSet, kvUnset } from "@/lib/config.ts"; import { CliError } from "@/lib/errors.ts"; import { logWarn } from "@/lib/log.ts"; import { readEnv } from "@/lib/env.ts"; -import { assertSafeProfileName, isFromEnvProfile } from "@/lib/profile-store.ts"; +import { + assertSafeProfileName, + ensureProfileId, + getProfileId, + isFromEnvProfile, +} from "@/lib/profile-store.ts"; type SecretBackend = "macos" | "file"; @@ -36,7 +41,7 @@ export type SecretStore = { secretGet(key: string, profileName: string): string; secretExists(key: string, profileName: string): boolean; secretDelete(key: string, profileName: string): void; - moveProfileSecrets(sourceProfile: string, targetProfile: string, keys: readonly string[]): void; + migrateProfileSecrets(profileName: string, keys: readonly string[]): void; secretStoreDisplay(): string; }; @@ -75,11 +80,15 @@ function requireSafeCredentialsFile(): void { } } -function resolveSecretKey(key: string, profileName: string): string { +function legacySecretKey(key: string, profileName: string): string { assertSafeProfileName(profileName); return `profile/${profileName}/${key}`; } +function stableSecretKey(key: string, profileId: string): string { + return `profile/${profileId}/${key}`; +} + export function createSecretStore( secretProcess: SecretProcess = SYSTEM_SECRET_PROCESS, ): SecretStore { @@ -122,6 +131,53 @@ export function createSecretStore( } } + function readStoredSecret(storageAccount: string): string { + if (secretBackend() === "macos") { + const result = secretProcess.spawnSync( + "security", + ["find-generic-password", "-s", "altertable", "-a", storageAccount, "-w"], + { encoding: "utf8" }, + ); + if (keychainLookupFailed(result)) { + throw new CliError(`Failed to read secret from macOS keychain (${storageAccount}).`); + } + const keychainValue = result.status === 0 ? String(result.stdout).trim() : ""; + if (keychainValue !== "") return keychainValue; + } + requireSafeCredentialsFile(); + return kvGet(credentialsFile(), storageAccount); + } + + function storedSecretExists(storageAccount: string): boolean { + if (secretBackend() === "macos") { + const result = secretProcess.spawnSync( + "security", + ["find-generic-password", "-s", "altertable", "-a", storageAccount], + { stdio: "ignore" }, + ); + if (result.status === 0) return true; + if (keychainLookupFailed(result)) { + throw new CliError(`Failed to inspect secret in macOS keychain (${storageAccount}).`); + } + } + requireSafeCredentialsFile(); + return kvGet(credentialsFile(), storageAccount) !== ""; + } + + function deleteStoredSecret(storageAccount: string): void { + if (secretBackend() === "macos") { + const result = secretProcess.spawnSync( + "security", + ["delete-generic-password", "-s", "altertable", "-a", storageAccount], + { stdio: "ignore" }, + ); + if (result.status !== 0 && result.status !== KEYCHAIN_ITEM_NOT_FOUND_STATUS) { + throw new CliError(`Failed to delete secret from macOS keychain (${storageAccount}).`); + } + } + kvUnset(credentialsFile(), storageAccount); + } + function secretSet( account: string, value: string, @@ -132,7 +188,7 @@ export function createSecretStore( // commands are refused in env mode; in-process caches are deliberately dropped. if (isFromEnvProfile(profileName)) return; - const storageAccount = resolveSecretKey(account, profileName); + const storageAccount = stableSecretKey(account, ensureProfileId(profileName)); const backend = secretBackend(); if (backend === "macos" && !options?.fromArgv) { secretSetMacos(storageAccount, value); @@ -148,100 +204,55 @@ export function createSecretStore( function secretGet(key: string, profileName: string): string { if (isFromEnvProfile(profileName)) return ""; - const secretKey = resolveSecretKey(key, profileName); - const backend = secretBackend(); - if (backend === "macos") { - const result = secretProcess.spawnSync( - "security", - ["find-generic-password", "-s", "altertable", "-a", secretKey, "-w"], - { encoding: "utf8" }, - ); - if (keychainLookupFailed(result)) { - throw new CliError(`Failed to read secret from macOS keychain (${secretKey}).`); - } - const keychainValue = result.status === 0 ? String(result.stdout).trim() : ""; - if (keychainValue !== "") return keychainValue; - requireSafeCredentialsFile(); - return kvGet(credentialsFile(), secretKey); + const profileId = getProfileId(profileName); + if (profileId) { + const value = readStoredSecret(stableSecretKey(key, profileId)); + if (value !== "") return value; } - - requireSafeCredentialsFile(); - return kvGet(credentialsFile(), secretKey); + return readStoredSecret(legacySecretKey(key, profileName)); } function secretExists(key: string, profileName: string): boolean { if (isFromEnvProfile(profileName)) return false; - const secretKey = resolveSecretKey(key, profileName); - const backend = secretBackend(); - if (backend === "macos") { - const result = secretProcess.spawnSync( - "security", - ["find-generic-password", "-s", "altertable", "-a", secretKey], - { stdio: "ignore" }, - ); - if (result.status === 0) return true; - if (keychainLookupFailed(result)) { - throw new CliError(`Failed to inspect secret in macOS keychain (${secretKey}).`); - } - requireSafeCredentialsFile(); - return kvGet(credentialsFile(), secretKey) !== ""; + const profileId = getProfileId(profileName); + if (profileId && storedSecretExists(stableSecretKey(key, profileId))) { + return true; } - - requireSafeCredentialsFile(); - return kvGet(credentialsFile(), secretKey) !== ""; + return storedSecretExists(legacySecretKey(key, profileName)); } function secretDelete(key: string, profileName: string): void { if (isFromEnvProfile(profileName)) return; - const secretKey = resolveSecretKey(key, profileName); - if (secretBackend() === "macos") { - const result = secretProcess.spawnSync( - "security", - ["delete-generic-password", "-s", "altertable", "-a", secretKey], - { stdio: "ignore" }, - ); - if (result.status !== 0 && result.status !== KEYCHAIN_ITEM_NOT_FOUND_STATUS) { - throw new CliError(`Failed to delete secret from macOS keychain (${secretKey}).`); - } - kvUnset(credentialsFile(), secretKey); - return; + const profileId = getProfileId(profileName); + if (profileId) { + deleteStoredSecret(stableSecretKey(key, profileId)); } - - kvUnset(credentialsFile(), secretKey); + deleteStoredSecret(legacySecretKey(key, profileName)); } - function moveProfileSecrets( - sourceProfile: string, - targetProfile: string, - keys: readonly string[], - ): void { - const prepared: Array<{ key: string; targetValue: string }> = []; - - try { - for (const key of keys) { - const sourceValue = secretGet(key, sourceProfile); - if (sourceValue.length === 0) continue; - prepared.push({ key, targetValue: secretGet(key, targetProfile) }); - secretSet(key, sourceValue, targetProfile); - } - } catch (error) { - for (const entry of prepared.toReversed()) { - try { - if (entry.targetValue.length > 0) { - secretSet(entry.key, entry.targetValue, targetProfile); - } else { - secretDelete(entry.key, targetProfile); - } - } catch { - // Best-effort rollback; preserve the original failure for the caller. + function migrateProfileSecrets(profileName: string, keys: readonly string[]): void { + const profileId = ensureProfileId(profileName); + const legacySecrets = keys.flatMap((key) => { + const value = readStoredSecret(legacySecretKey(key, profileName)); + return value === "" ? [] : [{ key, value }]; + }); + + for (const { key, value } of legacySecrets) { + const storageAccount = stableSecretKey(key, profileId); + if (!storedSecretExists(storageAccount)) { + if (secretBackend() === "macos") { + secretSetMacos(storageAccount, value); + } else { + kvSet(credentialsFile(), storageAccount, value); + chmodSync(credentialsFile(), 0o600); } } - throw error; } - - for (const { key } of prepared) secretDelete(key, sourceProfile); + for (const { key } of legacySecrets) { + deleteStoredSecret(legacySecretKey(key, profileName)); + } } function secretStoreDisplay(): string { @@ -253,7 +264,7 @@ export function createSecretStore( secretGet, secretExists, secretDelete, - moveProfileSecrets, + migrateProfileSecrets, secretStoreDisplay, }; } @@ -281,12 +292,8 @@ export function secretDelete(key: string, profileName: string): void { systemSecretStore.secretDelete(key, profileName); } -export function moveProfileSecrets( - sourceProfile: string, - targetProfile: string, - keys: readonly string[], -): void { - systemSecretStore.moveProfileSecrets(sourceProfile, targetProfile, keys); +export function migrateProfileSecrets(profileName: string, keys: readonly string[]): void { + systemSecretStore.migrateProfileSecrets(profileName, keys); } export function secretStoreDisplay(): string { diff --git a/tests/configure.test.ts b/tests/configure.test.ts index 251c90e..cc3af73 100644 --- a/tests/configure.test.ts +++ b/tests/configure.test.ts @@ -4,6 +4,17 @@ import { jsonMock, whoamiMock } from "./mock-http.ts"; const queryMock = [jsonMock("POST", "/query", {})]; +async function storedProfileSecret( + workspace: TestWorkspace, + account: string, + value: string, +): Promise { + const config = await workspace.readFile(workspace.defaultProfileConfig); + const profileId = /^profile_id=(.+)$/m.exec(config)?.[1]; + if (!profileId) throw new Error("Default profile ID is missing"); + return `profile/${profileId}/${account}=${value}\n`; +} + describe("altertable profile configure", () => { let workspace: TestWorkspace; @@ -20,16 +31,22 @@ describe("altertable profile configure", () => { await workspace.resetConfig(); expect((await workspace.runCommand("altertable profile configure --user u_blabla --password s_llll")).exitCode).toBe(0); expect(await workspace.readFile(workspace.defaultProfileConfig)).toContain("user=u_blabla\n"); - expect(await workspace.readFile(workspace.credentialsFile)).toContain("profile/default/lakehouse/password=s_llll\n"); + expect(await workspace.readFile(workspace.credentialsFile)).toContain( + await storedProfileSecret(workspace, "lakehouse/password", "s_llll"), + ); expect(await workspace.fileMode(workspace.credentialsFile)).toBe("600"); await workspace.resetConfig(); expect((await workspace.runCommand("altertable profile configure --basic-token dG9rZW4=")).exitCode).toBe(0); - expect(await workspace.readFile(workspace.credentialsFile)).toContain("profile/default/lakehouse/basic-token=dG9rZW4=\n"); + expect(await workspace.readFile(workspace.credentialsFile)).toContain( + await storedProfileSecret(workspace, "lakehouse/basic-token", "dG9rZW4="), + ); await workspace.resetConfig(); expect((await workspace.runCommand("altertable profile configure --api-key atm_prod --env production")).exitCode).toBe(0); - expect(await workspace.readFile(workspace.credentialsFile)).toContain("profile/default/api-key=atm_prod\n"); + expect(await workspace.readFile(workspace.credentialsFile)).toContain( + await storedProfileSecret(workspace, "api-key", "atm_prod"), + ); expect(await workspace.readFile(workspace.defaultProfileConfig)).toContain("api_key_env=production\n"); }); @@ -38,15 +55,19 @@ describe("altertable profile configure", () => { expect((await workspace.runCommand("altertable profile configure --user u1 --password p1")).exitCode).toBe(0); expect((await workspace.runCommand("altertable profile configure --api-key atm_x --env prod")).exitCode).toBe(0); expect(await workspace.readFile(workspace.defaultProfileConfig)).toContain("user=u1\n"); - expect(await workspace.readFile(workspace.credentialsFile)).toContain("profile/default/lakehouse/password=p1\n"); - expect(await workspace.readFile(workspace.credentialsFile)).toContain("profile/default/api-key=atm_x\n"); + expect(await workspace.readFile(workspace.credentialsFile)).toContain( + await storedProfileSecret(workspace, "lakehouse/password", "p1"), + ); + expect(await workspace.readFile(workspace.credentialsFile)).toContain( + await storedProfileSecret(workspace, "api-key", "atm_x"), + ); await workspace.resetConfig(); expect((await workspace.runCommand("altertable profile configure --api-key atm_x --env prod")).exitCode).toBe(0); expect((await workspace.runCommand("altertable profile configure --api-key atm_y --env staging")).exitCode).toBe(0); const credentials = await workspace.readFile(workspace.credentialsFile); expect(credentials).not.toContain("atm_x"); - expect(credentials).toContain("profile/default/api-key=atm_y\n"); + expect(credentials).toContain(await storedProfileSecret(workspace, "api-key", "atm_y")); expect(await workspace.readFile(workspace.defaultProfileConfig)).toContain("api_key_env=staging\n"); }); @@ -61,10 +82,14 @@ describe("altertable profile configure", () => { test("reads stdin secrets", async () => { await workspace.resetConfig(); expect((await workspace.runCommand("altertable profile configure --user alice --password-stdin", { stdin: "s_fromstdin" })).exitCode).toBe(0); - expect(await workspace.readFile(workspace.credentialsFile)).toContain("profile/default/lakehouse/password=s_fromstdin\n"); + expect(await workspace.readFile(workspace.credentialsFile)).toContain( + await storedProfileSecret(workspace, "lakehouse/password", "s_fromstdin"), + ); expect((await workspace.runCommand("altertable profile configure --api-key-stdin --env prod", { stdin: "atm_fromstdin" })).exitCode).toBe(0); - expect(await workspace.readFile(workspace.credentialsFile)).toContain("profile/default/api-key=atm_fromstdin\n"); + expect(await workspace.readFile(workspace.credentialsFile)).toContain( + await storedProfileSecret(workspace, "api-key", "atm_fromstdin"), + ); }); test("profile show reports auth without leaking secrets, and non-TTY configure requires flags", async () => {