Skip to content

Commit adf0d72

Browse files
authored
fix(vertex): warn when 'Google Cloud Credentials' field receives a file path (#294)
* fix(vertex): warn when 'Google Cloud Credentials' field receives a file path Users reported recurring "Error parsing JSON: Unexpected token 'C', \"C:\\Users\\..\"" spam on every Task creation. Root cause: pasting a filesystem path into the 'Google Cloud Credentials' field (which expects raw JSON content), instead of the sibling 'Google Cloud Key File Path' field. Google's own ecosystem uses GOOGLE_APPLICATION_CREDENTIALS for paths, so "credentials = path to credentials file" is a natural mental model that the field naming does not disambiguate. Three pieces: - gemini.ts and anthropic-vertex.ts: shape-check the field before parsing. If it looks like a path (Windows, POSIX absolute, ~/, ./), log a specific warning pointing at the Key File Path field and the GOOGLE_APPLICATION_CREDENTIALS env var. Skip the JSON.parse to avoid the generic 'Error parsing JSON' noise. The shared helper parseVertexJsonCredentials is exported from gemini.ts and reused from anthropic-vertex.ts so the Gemini-on-Vertex and Claude-on-Vertex paths behave identically. - safeJsonParse: add an optional context arg so logs identify their source instead of being anonymous. Backward-compatible. - Vertex.tsx: inline warning under the credentials field that fires on path-shaped input, naming the correct field to use instead. New i18n string propagated to all 18 locales. Auth behavior is unchanged for users who configure the fields correctly or use the env var. The only user-visible change is the new specific warning replacing the previous generic spam. * fix(vertex): bind parsed credentials before truthiness check + extract shared util Four review items from the PR review round: - Precedence bug at gemini.ts:225-233 and anthropic-vertex.ts:43-50: the truthiness check was on the raw vertexJsonCredentials field, so path-shaped input (which parseVertexJsonCredentials correctly returns undefined for) still entered the JSON branch with credentials: undefined instead of falling through to vertexKeyFile. Bind parseVertexJsonCredentials's result first and branch on it. - Extract parseVertexJsonCredentials from gemini.ts to a new utils/vertex-credentials.ts module. anthropic-vertex.ts previously imported from ./gemini, which pulled in the @google/genai runtime dependency unnecessarily. The shared util has no provider-specific imports. - a11y on the Vertex.tsx warning: added role="alert" and aria-live="polite" so screen readers announce the dynamic warning when it appears. - Mixed-input regression tests: added cases asserting that when vertexJsonCredentials is path-shaped AND vertexKeyFile is set, the GoogleAuth/GoogleGenAI client is constructed with the keyFile, not the (now undefined) credentials. Locks in the fallback contract. The pre-existing path-shape-only tests were also updated to match the corrected fallthrough: with no vertexKeyFile, neither GoogleAuth (anthropic-vertex) nor googleAuthOptions (gemini) is constructed. * fix(a11y): use role="status" for the Vertex credentials path warning role="alert" implies aria-live="assertive" by default, which mixed awkwardly with the explicit aria-live="polite" override (urgent role + don't-interrupt liveness). For an informational warning that fires as the user types into a settings field, role="status" is the conventional pattern — implicitly polite, semantically correct for a non-urgent dynamic message, and removes the role/live conflict. * fix(vertex): drop path preview from warning + extract shared path-shape predicate Three review items addressed: - vertex-credentials.ts: removed the dynamic preview of the user's input from the warning message — leaking usernames and directory names into extension logs is mild PII. The static message still identifies the correct field and the env var to use. - Extracted the path-shape predicate to @roo-code/types (packages/types/src/utils/looksLikeFilePath.ts) so the UI warning in Vertex.tsx and the runtime warning in parseVertexJsonCredentials share one implementation instead of drifting independently. Both call sites now import from the shared module; the predicate is pure, dep-free, regex-based, and covered by its own spec. - Vertex.spec.tsx: the <Trans> mock now resolves keys against the real webview-ui English settings.json so the test asserts the rendered warning text. A future translation regression that dropped the 'Google Cloud Key File Path' field name or the GOOGLE_APPLICATION_CREDENTIALS env-var mention from the English copy would now be caught. --------- Co-authored-by: 0xMink <260166390+0xMink@users.noreply.github.com>
1 parent cef0cc3 commit adf0d72

30 files changed

Lines changed: 624 additions & 13 deletions
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"zoo-code": patch
3+
---
4+
5+
Detect when the "Google Cloud Credentials" Vertex field has been given a filesystem path instead of the raw JSON contents of a service-account key file. The runtime now skips JSON.parse for path-shaped input, logs a single specific console warning naming the sibling "Google Cloud Key File Path" field and `GOOGLE_APPLICATION_CREDENTIALS` env var, and the settings UI shows an inline warning under the field while the input still looks like a path. Auth behavior is unchanged for correctly-configured users.
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// npx vitest run packages/core/src/message-utils/__tests__/safeJsonParse.spec.ts
2+
3+
import { safeJsonParse } from "../safeJsonParse.js"
4+
5+
describe("safeJsonParse", () => {
6+
let consoleErrorSpy: ReturnType<typeof vi.spyOn>
7+
8+
beforeEach(() => {
9+
consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
10+
})
11+
12+
afterEach(() => {
13+
consoleErrorSpy.mockRestore()
14+
})
15+
16+
it("returns the parsed value for valid JSON", () => {
17+
expect(safeJsonParse<{ a: number }>('{"a":1}')).toEqual({ a: 1 })
18+
expect(consoleErrorSpy).not.toHaveBeenCalled()
19+
})
20+
21+
it("returns the default value for null, undefined, or empty input", () => {
22+
expect(safeJsonParse<string>(null, "fallback")).toBe("fallback")
23+
expect(safeJsonParse<string>(undefined, "fallback")).toBe("fallback")
24+
expect(safeJsonParse<string>("", "fallback")).toBe("fallback")
25+
expect(consoleErrorSpy).not.toHaveBeenCalled()
26+
})
27+
28+
it("returns the default value and logs the generic message when no context is given (backward compatible)", () => {
29+
const result = safeJsonParse<{ a: number }>("not json", undefined)
30+
expect(result).toBeUndefined()
31+
expect(consoleErrorSpy).toHaveBeenCalledTimes(1)
32+
const message = consoleErrorSpy.mock.calls[0]?.[0]
33+
expect(message).toBe("Error parsing JSON:")
34+
})
35+
36+
it("includes the context label in the error log when provided", () => {
37+
const result = safeJsonParse<{ a: number }>("not json", undefined, "foo")
38+
expect(result).toBeUndefined()
39+
expect(consoleErrorSpy).toHaveBeenCalledTimes(1)
40+
const message = consoleErrorSpy.mock.calls[0]?.[0]
41+
expect(message).toBe("Error parsing JSON (foo):")
42+
})
43+
})

packages/core/src/message-utils/safeJsonParse.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,16 @@
33
*
44
* @param jsonString The string to parse
55
* @param defaultValue Value to return if parsing fails
6+
* @param context Optional label included in the error log so callers can be
7+
* identified when something other than valid JSON is supplied (e.g. a user
8+
* pasting a file path into a JSON field).
69
* @returns Parsed JSON object or defaultValue if parsing fails
710
*/
8-
export function safeJsonParse<T>(jsonString: string | null | undefined, defaultValue?: T): T | undefined {
11+
export function safeJsonParse<T>(
12+
jsonString: string | null | undefined,
13+
defaultValue?: T,
14+
context?: string,
15+
): T | undefined {
916
if (!jsonString) {
1017
return defaultValue
1118
}
@@ -14,7 +21,7 @@ export function safeJsonParse<T>(jsonString: string | null | undefined, defaultV
1421
return JSON.parse(jsonString) as T
1522
} catch (error) {
1623
// Log the error to the console for debugging.
17-
console.error("Error parsing JSON:", error)
24+
console.error(`Error parsing JSON${context ? ` (${context})` : ""}:`, error)
1825
return defaultValue
1926
}
2027
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
// npx vitest run src/__tests__/looksLikeFilePath.spec.ts
2+
3+
import { looksLikeFilePath } from "../utils/looksLikeFilePath.js"
4+
5+
describe("looksLikeFilePath", () => {
6+
describe("nullish, empty, and whitespace-only input", () => {
7+
it.each([
8+
["undefined", undefined],
9+
["null", null],
10+
["empty string", ""],
11+
["whitespace only", " \t\n "],
12+
])("returns false for %s", (_label, value) => {
13+
expect(looksLikeFilePath(value)).toBe(false)
14+
})
15+
})
16+
17+
describe("path-shaped input", () => {
18+
it.each([
19+
["Windows backslash path", "C:\\Users\\dev\\sa.json"],
20+
["Windows forward-slash path", "C:/Users/dev/sa.json"],
21+
["Windows drive lowercase", "d:\\creds.json"],
22+
["POSIX absolute path", "/home/dev/sa.json"],
23+
["POSIX absolute root /tmp", "/tmp/creds.json"],
24+
["POSIX home path", "~/sa.json"],
25+
["POSIX relative ./", "./sa.json"],
26+
["POSIX relative ../", "../secrets/sa.json"],
27+
])("returns true for %s", (_label, value) => {
28+
expect(looksLikeFilePath(value)).toBe(true)
29+
})
30+
31+
it("returns true after trimming surrounding whitespace", () => {
32+
expect(looksLikeFilePath(" /tmp/creds.json ")).toBe(true)
33+
expect(looksLikeFilePath("\tC:\\sa.json\n")).toBe(true)
34+
})
35+
})
36+
37+
describe("JSON-shaped or bare-token input", () => {
38+
it.each([
39+
["JSON object", '{"type":"service_account","client_email":"x@y.z"}'],
40+
["JSON array", "[1,2,3]"],
41+
["JSON with leading whitespace", ' {"type":"service_account"}'],
42+
["bare token", "not-json-and-not-a-path"],
43+
["service-account-style email", "sa@project.iam.gserviceaccount.com"],
44+
])("returns false for %s", (_label, value) => {
45+
expect(looksLikeFilePath(value)).toBe(false)
46+
})
47+
})
48+
})

packages/types/src/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,5 @@ export * from "./vscode.js"
3333
export * from "./worktree.js"
3434

3535
export * from "./providers/index.js"
36+
37+
export * from "./utils/looksLikeFilePath.js"
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Returns true when a string is shaped like a filesystem path (Windows
2+
// drive-letter, POSIX absolute, POSIX home, POSIX relative). Pure and
3+
// dependency-free so it can be shared between the extension runtime (e.g.
4+
// parseVertexJsonCredentials) and the webview UI (e.g. the Vertex settings
5+
// warning), guaranteeing both surfaces agree on what "looks like a path"
6+
// means.
7+
//
8+
// Returns false for nullish, empty, and whitespace-only input — neither
9+
// call site should warn in those cases.
10+
export function looksLikeFilePath(value: string | null | undefined): boolean {
11+
if (value == null) {
12+
return false
13+
}
14+
const trimmed = value.trim()
15+
if (!trimmed) {
16+
return false
17+
}
18+
return (
19+
/^[A-Za-z]:[\\/]/.test(trimmed) || // Windows: C:\... or C:/...
20+
trimmed.startsWith("/") || // POSIX absolute: /home/...
21+
trimmed.startsWith("~") || // POSIX home: ~/...
22+
trimmed.startsWith(".") // POSIX relative: ./... or ../...
23+
)
24+
}

0 commit comments

Comments
 (0)