Merge pull request #7 from godaddy/fix/security-hardening

fix: security hardening across auth, API, keychain, and error output

Author: wcole1-godaddy

src/cli/commands/api.ts

@@ -9,6 +9,7 @@ import {
 	parseFieldsEffect,
 	parseHeadersEffect,
 	readBodyFromFileEffect,
+	sanitizeResponseHeaders,
 } from "../../core/api";
 import { authLoginEffect, getTokenInfoEffect } from "../../core/auth";
 import { AuthenticationError, ValidationError } from "../../effect/errors";
@@ -318,7 +319,9 @@ const apiCommand = Command.make(
 					method,
 					status: response.status,
 					status_text: response.statusText,
-					headers: config.include ? response.headers : undefined,
+					headers: config.include
+						? sanitizeResponseHeaders(response.headers)
+						: undefined,
 					data: output ?? null,
 				},
 				apiRequestActions,

src/core/api.ts

@@ -16,6 +16,60 @@ import { type Environment, envGetEffect, getApiUrl } from "./environment";
 // Minimum seconds before expiry to consider token valid for a request
 const TOKEN_EXPIRY_BUFFER_SECONDS = 30;
 
+// Header names (lowercased) that must be redacted from debug output and
+// the --include envelope to prevent leaking tokens, cookies, or secrets.
+const SENSITIVE_HEADER_PARTS = [
+	"authorization",
+	"cookie",
+	"set-cookie",
+	"token",
+	"secret",
+	"api-key",
+	"apikey",
+	"x-auth",
+] as const;
+
+function isSensitiveHeader(headerName: string): boolean {
+	const lower = headerName.toLowerCase();
+	return SENSITIVE_HEADER_PARTS.some((part) => lower.includes(part));
+}
+
+/**
+ * Return a copy of headers with sensitive values replaced by "[REDACTED]".
+ */
+export { sanitizeHeaders as sanitizeResponseHeaders };
+
+function sanitizeHeaders(
+	headers: Record<string, string>,
+): Record<string, string> {
+	const sanitized: Record<string, string> = {};
+	for (const [key, value] of Object.entries(headers)) {
+		sanitized[key] = isSensitiveHeader(key) ? "[REDACTED]" : value;
+	}
+	return sanitized;
+}
+
+/**
+ * Redact values whose keys look like they contain secrets.
+ */
+function redactSensitiveBodyFields(body: string): string {
+	try {
+		const parsed = JSON.parse(body);
+		if (typeof parsed !== "object" || parsed === null) return body;
+		const redacted: Record<string, unknown> = {};
+		for (const [key, value] of Object.entries(parsed)) {
+			const lower = key.toLowerCase();
+			const isSensitive = SENSITIVE_HEADER_PARTS.some((part) =>
+				lower.includes(part),
+			) || lower.includes("password") || lower.includes("credential");
+			redacted[key] = isSensitive ? "[REDACTED]" : value;
+		}
+		return JSON.stringify(redacted);
+	} catch {
+		return body;
+	}
+}
+
 export type HttpMethod = "GET" | "POST" | "PUT" | "PATCH" | "DELETE";
 
 export interface ApiRequestOptions {
@@ -265,13 +319,12 @@ export function apiRequestEffect(
 
 		if (debug) {
 			console.error(`> ${method} ${url}`);
-			for (const [key, value] of Object.entries(requestHeaders)) {
-				const displayValue =
-					key.toLowerCase() === "authorization" ? "Bearer [REDACTED]" : value;
-				console.error(`> ${key}: ${displayValue}`);
+			const sanitizedRequestHeaders = sanitizeHeaders(requestHeaders);
+			for (const [key, value] of Object.entries(sanitizedRequestHeaders)) {
+				console.error(`> ${key}: ${value}`);
 			}
 			if (requestBody) {
-				console.error(`> Body: ${requestBody}`);
+				console.error(`> Body: ${redactSensitiveBodyFields(requestBody)}`);
 			}
 			console.error("");
 		}
@@ -302,7 +355,8 @@ export function apiRequestEffect(
 
 		if (debug) {
 			console.error(`< ${response.status} ${response.statusText}`);
-			for (const [key, value] of Object.entries(responseHeaders)) {
+			const sanitizedResponseHeaders = sanitizeHeaders(responseHeaders);
+			for (const [key, value] of Object.entries(sanitizedResponseHeaders)) {
 				console.error(`< ${key}: ${value}`);
 			}
 			console.error("");
@@ -340,7 +394,9 @@ export function apiRequestEffect(
 
 		// Check for error status codes
 		if (!response.ok) {
-			const errorMessage =
+			// Internal message includes the raw server payload for debugging;
+			// userMessage is a safe, generic description shown to users/agents.
+			const internalDetail =
 				typeof data === "object" && data !== null
 					? JSON.stringify(data)
 					: String(data || response.statusText);
@@ -349,7 +405,7 @@ export function apiRequestEffect(
 			if (response.status === 401) {
 				return yield* Effect.fail(
 					new AuthenticationError({
-						message: `Authentication failed (401): ${errorMessage}`,
+						message: `Authentication failed (401): ${internalDetail}`,
 						userMessage:
 							"Your session has expired or is invalid. Run 'godaddy auth login' to re-authenticate.",
 					}),
@@ -360,7 +416,7 @@ export function apiRequestEffect(
 			if (response.status === 403) {
 				return yield* Effect.fail(
 					new AuthenticationError({
-						message: `Access denied (403): ${errorMessage}`,
+						message: `Access denied (403): ${internalDetail}`,
 						userMessage:
 							"You don't have permission to access this resource. Check your account permissions.",
 					}),
@@ -369,7 +425,7 @@ export function apiRequestEffect(
 
 			return yield* Effect.fail(
 				new NetworkError({
-					message: `API error (${response.status}): ${errorMessage}`,
+					message: `API error (${response.status}): ${internalDetail}`,
 					userMessage: `API request failed with status ${response.status}: ${response.statusText}`,
 				}),
 			);

src/core/auth.ts

@@ -25,8 +25,21 @@ import {
 } from "./token-store";
 
 const PORT = 7443;
+const AUTH_HOST = "127.0.0.1";
+const AUTH_TIMEOUT_MS = 120_000;
 const DEFAULT_OAUTH_SCOPES = "apps.app-registry:read apps.app-registry:write";
 
+/**
+ * Escape a string for safe embedding in HTML text content.
+ */
+function escapeHtml(text: string): string {
+	return text
+		.replace(/&/g, "&")
+		.replace(/</g, "&lt;")
+		.replace(/>/g, "&gt;")
+		.replace(/"/g, "&quot;");
+}
+
 export interface AuthResult {
 	success: boolean;
 	accessToken?: string;
@@ -217,10 +230,11 @@ export function authLoginEffect(options?: {
 									"Content-Type": "text/html",
 								});
 								res.end(
-									`<html><body><h1>Authentication Failed</h1><p>${errorMessage}</p></body></html>`,
+									`<html><body><h1>Authentication Failed</h1><p>${escapeHtml(errorMessage)}</p></body></html>`,
 								);
 								reject(err);
 							} finally {
+								clearTimeout(authTimeout);
 								if (server) server.close();
 							}
 						} else {
@@ -234,7 +248,20 @@ export function authLoginEffect(options?: {
 						reject(err);
 					});
 
-					server.listen(PORT, () => {
+					// Auto-close after timeout if the user never completes the flow
+					const authTimeout = setTimeout(() => {
+						if (server) {
+							server.close();
+							server = null;
+						}
+						reject(
+							new Error(
+								"Login timed out. The authentication flow was not completed.",
+							),
+						);
+					}, AUTH_TIMEOUT_MS);
+
+					server.listen(PORT, AUTH_HOST, () => {
 						const actualPort = (server?.address() as import("net").AddressInfo)
 							?.port;
 						if (!actualPort) {

src/effect/layers/keychain-native.ts

@@ -288,6 +288,9 @@ function createWindowsKeychain(): KeychainService {
 
 	return {
 		async setPassword(service, account, password) {
+			assertSafePSValue(service, "service");
+			assertSafePSValue(account, "account");
+			assertSafePSValue(password, "password");
 			// Remove existing entry first (PasswordVault throws on duplicate)
 			const removeScript = `
 				${vaultInit}
@@ -307,6 +310,8 @@ function createWindowsKeychain(): KeychainService {
 		},
 
 		async getPassword(service, account) {
+			assertSafePSValue(service, "service");
+			assertSafePSValue(account, "account");
 			const script = `
 				${vaultInit}
 				try {
@@ -325,6 +330,8 @@ function createWindowsKeychain(): KeychainService {
 		},
 
 		async deletePassword(service, account) {
+			assertSafePSValue(service, "service");
+			assertSafePSValue(account, "account");
 			const script = `
 				${vaultInit}
 				try {
@@ -339,6 +346,7 @@ function createWindowsKeychain(): KeychainService {
 		},
 
 		async findCredentials(service) {
+			assertSafePSValue(service, "service");
 			const script = `
 				${vaultInit}
 				try {
@@ -370,9 +378,33 @@ function createWindowsKeychain(): KeychainService {
 	};
 }
 
-/** Escape single quotes for PowerShell string literals. */
+/**
+ * Escape a value for embedding inside PowerShell single-quoted strings.
+ *
+ * PowerShell single-quoted strings only interpret '' as a literal '.
+ * However, we must also strip NUL bytes (which can truncate strings in
+ * some contexts) and reject values containing characters that could break
+ * out of the quoting context if the script template is ever changed to
+ * use double-quotes or here-strings.
+ */
 function escapePS(value: string): string {
-	return value.replace(/'/g, "''");
+	// Strip NUL bytes that could truncate the string
+	const stripped = value.replace(/\0/g, "");
+	// In PowerShell single-quoted strings, only ' needs escaping (as '').
+	return stripped.replace(/'/g, "''");
+}
+
+/**
+ * Validate that a value is safe for PowerShell interpolation.
+ * Rejects values containing newlines or control characters that could
+ * escape the single-line script template.
+ */
+function assertSafePSValue(value: string, label: string): void {
+	if (/[\r\n]/.test(value)) {
+		throw new Error(
+			`Unsafe ${label} for credential storage: contains newline characters`,
+		);
+	}
 }
 
 // ---------------------------------------------------------------------------

src/services/applications.ts

@@ -175,7 +175,8 @@ export const releaseInput = type({
 });
 
 /**
- * Extract a user-friendly error message from a GraphQL ClientError
+ * Extract the internal error detail from a GraphQL ClientError.
+ * This may include server-side messages and extension codes.
  */
 function extractGraphQLError(err: unknown): string {
 	if (err instanceof ClientError) {
@@ -189,6 +190,29 @@ function extractGraphQLError(err: unknown): string {
 	return "An unexpected error occurred";
 }
 
+/**
+ * Return a safe, generic user-facing message for a GraphQL error.
+ * Avoids leaking internal server details in the CLI JSON envelope.
+ */
+function safeGraphQLUserMessage(err: unknown): string {
+	if (err instanceof ClientError) {
+		const status = err.response.status;
+		if (status === 401 || status === 403)
+			return "Authentication failed. Run 'godaddy auth login'.";
+		if (status === 404)
+			return "The requested resource was not found.";
+		if (status && status >= 500)
+			return "The server encountered an error. Please try again later.";
+		// For 4xx with GraphQL-level error messages, allow the first message
+		// through since these are validation-style errors the user can act on.
+		const graphqlErrors = err.response.errors;
+		if (graphqlErrors?.length && graphqlErrors[0].message) {
+			return graphqlErrors[0].message;
+		}
+	}
+	return "An unexpected error occurred";
+}
+
 export function createApplicationEffect(
 	input: typeof applicationInput.infer,
 	{ accessToken }: { accessToken: string | null },
@@ -225,7 +249,7 @@ export function createApplicationEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -258,7 +282,7 @@ export function updateApplicationEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -290,7 +314,7 @@ export function getApplicationEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -322,7 +346,7 @@ export function getApplicationAndLatestReleaseEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -370,7 +394,7 @@ export function createReleaseEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -402,7 +426,7 @@ export function enableApplicationEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -434,7 +458,7 @@ export function disableApplicationEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -465,7 +489,7 @@ export function listApplicationsEffect({
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});
@@ -497,7 +521,7 @@ export function archiveApplicationEffect(
 			catch: (err) =>
 				new NetworkError({
 					message: extractGraphQLError(err),
-					userMessage: extractGraphQLError(err),
+					userMessage: safeGraphQLUserMessage(err),
 				}),
 		});
 	});