Merge branch 'jill/validate-secret-reference' into jill/migrate-to-connect-sdk

src/utils.test.ts

@@ -549,7 +549,7 @@ describe("loadSecrets when using Service Account", () => {
 
 	describe("secret reference validation", () => {
 		it("fails with clear message when a secret reference is invalid", async () => {
-			process.env.MY_SECRET = "op://invalid/ref/form";
+			process.env.MY_SECRET = "op://x";
 			(Secrets.validateSecretReference as jest.Mock).mockImplementationOnce(
 				() => {
 					throw new Error("invalid reference format");
@@ -572,7 +572,6 @@ describe("loadSecrets when using Service Account", () => {
 					}
 				},
 			);
-			mockResolve.mockResolvedValue("value1");
 
 			await expect(loadSecrets(false)).rejects.toThrow(
 				"Invalid secret reference(s): OTHER",
@@ -836,21 +835,21 @@ describe("findMatchingFieldAndFile", () => {
 describe("findSectionIdsByQuery", () => {
 	it("throws when sections is empty", () => {
 		expect(() => findSectionIdsByQuery([], "section-1")).toThrow(
-			/section section-1 could not be found/,
+			/Item has no sections; cannot resolve section "section-1"/,
 		);
 	});
 
 	it("throws when sections is null/undefined", () => {
 		expect(() =>
 			findSectionIdsByQuery(undefined as unknown as FullItem["sections"], "x"),
-		).toThrow(/could not be found/);
+		).toThrow(/Item has no sections; cannot resolve section "x"/);
 	});
 
-	it("returns section id when section matches by id", () => {
+	it("throws when section query matches no section", () => {
-		const sections = [{ id: "sec-1", label: "Section 1" }];
+		const sections = [{ id: "sec-1", label: "Other" }];
-		expect(
+		expect(() =>
-			findSectionIdsByQuery(sections as FullItem["sections"], "sec-1"),
+			findSectionIdsByQuery(sections as FullItem["sections"], "nonexistent"),
-		).toEqual(["sec-1"]);
+		).toThrow(/No section matching "nonexistent" found in specified item/);
 	});
 
 	it("returns section id when section matches by label", () => {
@@ -864,7 +863,7 @@ describe("findSectionIdsByQuery", () => {
 		const sections = [{ id: "sec-1", label: "Other" }];
 		expect(() =>
 			findSectionIdsByQuery(sections as FullItem["sections"], "nonexistent"),
-		).toThrow(/could not be found/);
+		).toThrow(/No section matching "nonexistent" found in specified item/);
 	});
 
 	it("returns multiple ids when multiple sections match", () => {

src/utils.ts

@@ -92,30 +92,8 @@ const getSecretFromConnectItem = async (
 		return fieldValue;
 	}
 
-	// If a file was found, get the content of the file (with retry on 503)
 	if (fileId) {
-		const maxAttempts = 3;
+		return getFileContentWithRetry(client, parsed.vault, parsed.item, fileId);
-		const retryDelayMs = 2000;
-		for (let attempt = 1; attempt <= maxAttempts; attempt++) {
-			try {
-				const content = await client.getFileContent(
-					parsed.vault,
-					parsed.item,
-					fileId,
-				);
-				return content;
-			} catch (err) {
-				const is503 =
-					err &&
-					typeof err === "object" &&
-					(err as Record<string, unknown>).statusCode === 503;
-				if (is503 && attempt < maxAttempts) {
-					await new Promise((resolve) => setTimeout(resolve, retryDelayMs));
-					continue;
-				}
-				throw err;
-			}
-		}
 	}
 
 	if (parsed.section) {
@@ -129,6 +107,33 @@ const getSecretFromConnectItem = async (
 	);
 };
 
+const getFileContentWithRetry = async (
+	client: OPConnect,
+	vaultId: string,
+	itemId: string,
+	fileId: string,
+): Promise<string> => {
+	const maxAttempts = 3;
+	const retryDelayMs = 2000;
+	for (let attempt = 1; attempt <= maxAttempts; attempt++) {
+		try {
+			return await client.getFileContent(vaultId, itemId, fileId);
+		} catch (err) {
+			// Retry on 503 errors as this can happen on multiple secret fetches
+			const is503 =
+				err !== null &&
+				typeof err === "object" &&
+				(err as Record<string, unknown>).statusCode === 503;
+			if (is503 && attempt < maxAttempts) {
+				await new Promise((r) => setTimeout(r, retryDelayMs));
+				continue;
+			}
+			throw err;
+		}
+	}
+	return "";
+};
+
 export const findSectionIdsByQuery = (
 	sections: FullItem["sections"],
 	sectionQuery: string | undefined,
@@ -136,7 +141,7 @@ export const findSectionIdsByQuery = (
 	// If no sections were returned with the item throw an error
 	if (!sections || sections.length === 0) {
 		throw new Error(
-			`section ${sectionQuery} could not be found in specified item`,
+			`Item has no sections; cannot resolve section "${sectionQuery}"`,
 		);
 	}
 
@@ -147,7 +152,7 @@ export const findSectionIdsByQuery = (
 	// If no sections were found with the given query throw an error
 	if (ids.length === 0) {
 		throw new Error(
-			`section ${sectionQuery} could not be found in specified item`,
+			`No section matching "${sectionQuery}" found in specified item`,
 		);
 	}
 
@@ -251,6 +256,19 @@ const findSingleMatch = <T>(matches: T[]): T | undefined => {
 	}
 	return matches[0];
 };
+
+const createConnectClient = (host: string, token: string): OPConnect => {
+	try {
+		return OnePasswordConnect({
+			// eslint-disable-next-line @typescript-eslint/naming-convention
+			serverURL: host,
+			token,
+		});
+	} catch (err) {
+		const message = err instanceof Error ? err.message : String(err);
+		throw new Error(`Connect authentication failed: ${message}`);
+	}
+};
 // #endregion
 
 // #region Shared helpers and auth
@@ -262,7 +280,7 @@ export const getEnvVarNamesWithSecretRefs = (): string[] =>
 	);
 
 const validateSecretRefs = (envNames: string[]): void => {
-	const invalid: string[] = [];
+	const invalid: { name: string; message: string }[] = [];
 
 	for (const envName of envNames) {
 		const ref = process.env[envName];
@@ -272,15 +290,18 @@ const validateSecretRefs = (envNames: string[]): void => {
 
 		try {
 			Secrets.validateSecretReference(ref);
-		} catch {
+		} catch (err) {
-			invalid.push(envName);
+			const message = err instanceof Error ? err.message : String(err);
+			invalid.push({ name: envName, message });
 		}
 	}
 
 	// Throw an error if any secret references are invalid
 	if (invalid.length > 0) {
-		const names = invalid.join(", ");
+		const details = invalid
-		throw new Error(`Invalid secret reference(s): ${names}`);
+			.map(({ name, message }) => `${name}: ${message}`)
+			.join("; ");
+		throw new Error(`Invalid secret reference(s): ${details}`);
 	}
 };
 
@@ -352,10 +373,10 @@ const fetchVaultId = async (
 	client: OPConnect,
 	vaultQuery: string,
 	ref: string,
-	cache: Map<string, string>,
+	vaultIdCache: Map<string, string>,
 ): Promise<string> => {
-	// Check if the vault ID is already cached
+	// Check if the vault ID is already cached to avoid unnecessary API calls
-	const cached = cache.get(vaultQuery);
+	const cached = vaultIdCache.get(vaultQuery);
 	if (cached !== undefined) {
 		return cached;
 	}
@@ -367,7 +388,7 @@ const fetchVaultId = async (
 		);
 	}
 
-	cache.set(vaultQuery, vault.id);
+	vaultIdCache.set(vaultQuery, vault.id);
 	return vault.id;
 };
 // #endregion
@@ -390,20 +411,8 @@ const loadSecretsViaConnect = async (
 		throw new Error(authErr);
 	}
 
-	// Authenticate with the Connect SDK
+	const client = createConnectClient(host, token);
-	let client;
+	const vaultIdCache = new Map<string, string>();
-	try {
-		client = OnePasswordConnect({
-			// eslint-disable-next-line @typescript-eslint/naming-convention
-			serverURL: host,
-			token,
-		});
-	} catch (err) {
-		const message = err instanceof Error ? err.message : String(err);
-		throw new Error(`Connect authentication failed: ${message}`);
-	}
-
-	const vaultIdByQuery = new Map<string, string>();
 
 	for (const envName of envs) {
 		const ref = process.env[envName];
@@ -419,7 +428,7 @@ const loadSecretsViaConnect = async (
 				client,
 				parsed.vault,
 				ref,
-				vaultIdByQuery,
+				vaultIdCache,
 			);
 			const item = await client.getItem(vaultId, parsed.item);