Skip to content

Commit fe63502

Browse files
villahernandez-codervillahernandez-coder
authored
fix(auth): prevent infinite auth loop from file contention, headless keyring, and supervisor state drops (#28341) (#29448)
1 parent 2fe7c2d commit fe63502

16 files changed

Lines changed: 546 additions & 66 deletions

File tree

‎packages/cli/index.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,26 @@ async function run() {
7676
env: newEnv,
7777
});
7878

79+
// Clear one-time auth override from supervisor environment after passing to child
80+
delete process.env['GEMINI_CLI_AUTH_OVERRIDE'];
81+
delete newEnv['GEMINI_CLI_AUTH_OVERRIDE'];
82+
7983
if (latestAdminSettings) {
8084
child.send({ type: 'admin-settings', settings: latestAdminSettings });
8185
}
8286

83-
child.on('message', (msg: { type?: string; settings?: unknown }) => {
84-
if (msg.type === 'admin-settings-update' && msg.settings) {
85-
latestAdminSettings = msg.settings;
86-
}
87-
});
87+
child.on(
88+
'message',
89+
(msg: { type?: string; settings?: unknown; authType?: string }) => {
90+
if (msg.type === 'admin-settings-update' && msg.settings) {
91+
latestAdminSettings = msg.settings;
92+
}
93+
if (msg.type === 'auth-selected-type' && msg.authType) {
94+
process.env['GEMINI_CLI_AUTH_OVERRIDE'] = msg.authType;
95+
newEnv['GEMINI_CLI_AUTH_OVERRIDE'] = msg.authType;
96+
}
97+
},
98+
);
8899

89100
return new Promise<number>((resolve) => {
90101
child.on('error', (err) => {

‎packages/cli/src/config/settings.test.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,43 @@ describe('Settings Loading and Merging', () => {
261261
},
262262
);
263263

264+
it('should unconditionally override selectedType when GEMINI_CLI_AUTH_OVERRIDE is present and delete it from process.env', () => {
265+
(mockFsExistsSync as Mock).mockImplementation(
266+
(pathLike: fs.PathLike) =>
267+
path.normalize(pathLike.toString()) ===
268+
path.normalize(USER_SETTINGS_PATH),
269+
);
270+
(fs.readFileSync as Mock).mockImplementation(
271+
(pathDesc: fs.PathOrFileDescriptor) => {
272+
if (
273+
path.normalize(pathDesc.toString()) ===
274+
path.normalize(USER_SETTINGS_PATH)
275+
) {
276+
return JSON.stringify({
277+
security: {
278+
auth: {
279+
selectedType: AuthType.USE_GEMINI,
280+
},
281+
},
282+
});
283+
}
284+
return '{}';
285+
},
286+
);
287+
288+
vi.stubEnv('GEMINI_CLI_AUTH_OVERRIDE', AuthType.LOGIN_WITH_GOOGLE);
289+
290+
const settings = loadSettings(MOCK_WORKSPACE_DIR);
291+
292+
expect(settings.user.settings.security?.auth?.selectedType).toBe(
293+
AuthType.LOGIN_WITH_GOOGLE,
294+
);
295+
expect(settings.merged.security.auth.selectedType).toBe(
296+
AuthType.LOGIN_WITH_GOOGLE,
297+
);
298+
expect(process.env['GEMINI_CLI_AUTH_OVERRIDE']).toBeUndefined();
299+
});
300+
264301
it('should merge system, user and workspace settings, with system taking precedence over workspace, and workspace over user', () => {
265302
(mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => {
266303
const normP = path.normalize(p.toString());

‎packages/cli/src/config/settings.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -904,6 +904,35 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings {
904904
userSettings = userResult.settings;
905905
workspaceSettings = workspaceResult.settings;
906906

907+
// Support environment variable override from relaunch supervisor across exit code 199
908+
const envAuthOverride = process.env['GEMINI_CLI_AUTH_OVERRIDE'];
909+
if (envAuthOverride) {
910+
delete process.env['GEMINI_CLI_AUTH_OVERRIDE'];
911+
}
912+
const authOverride =
913+
envAuthOverride &&
914+
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
915+
Object.values(AuthType).includes(envAuthOverride as AuthType)
916+
? // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
917+
(envAuthOverride as AuthType)
918+
: undefined;
919+
if (authOverride) {
920+
if (!userSettings.security) {
921+
userSettings.security = {};
922+
}
923+
if (!userSettings.security.auth) {
924+
userSettings.security.auth = {};
925+
}
926+
userSettings.security.auth.selectedType = authOverride;
927+
if (!userOriginalSettings.security) {
928+
userOriginalSettings.security = {};
929+
}
930+
if (!userOriginalSettings.security.auth) {
931+
userOriginalSettings.security.auth = {};
932+
}
933+
userOriginalSettings.security.auth.selectedType = authOverride;
934+
}
935+
907936
// Support legacy theme names
908937
if (userSettings.ui?.theme === 'VS') {
909938
userSettings.ui.theme = DefaultLight.name;

‎packages/cli/src/ui/auth/AuthDialog.tsx‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
AuthType,
1919
clearCachedCredentialFile,
2020
type Config,
21+
debugLogger,
2122
} from '@google/gemini-cli-core';
2223
import { useKeypress } from '../hooks/useKeypress.js';
2324
import { AuthState } from '../types.js';
@@ -129,15 +130,24 @@ export function AuthDialog({
129130
} else {
130131
setAuthContext({});
131132
}
132-
await clearCachedCredentialFile();
133+
134+
const currentAuthType = settings.merged.security?.auth?.selectedType;
135+
if (currentAuthType && currentAuthType !== authType) {
136+
await clearCachedCredentialFile();
137+
}
133138

134139
settings.setValue(scope, 'security.auth.selectedType', authType);
135140
if (
136141
authType === AuthType.LOGIN_WITH_GOOGLE &&
137142
config.isBrowserLaunchSuppressed()
138143
) {
139144
setExiting(true);
140-
setTimeout(relaunchApp, 100);
145+
setTimeout(() => {
146+
void relaunchApp({ overrideAuthType: authType }).catch((err) => {
147+
debugLogger.error('Failed to trigger supervisor relaunch:', err);
148+
process.exit(1);
149+
});
150+
}, 100);
141151
return;
142152
}
143153

‎packages/cli/src/utils/commentJson.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,5 +366,38 @@ describe('commentJson', () => {
366366

367367
expect(updatedContent).toContain('// This should be preserved');
368368
});
369+
370+
it('should ignore dangerous keys (__proto__, constructor, prototype) to prevent prototype pollution', () => {
371+
const originalContent = `{
372+
"theme": "light"
373+
}`;
374+
375+
fs.writeFileSync(testFilePath, originalContent, 'utf-8');
376+
377+
const maliciousUpdates = JSON.parse(
378+
'{"__proto__": {"polluted": "yes"}, "theme": "dark"}',
379+
) as Record<string, unknown>;
380+
381+
updateSettingsFilePreservingFormat(testFilePath, maliciousUpdates);
382+
383+
// Verify prototype was not polluted
384+
expect(
385+
(Object.prototype as unknown as Record<string, unknown>)['polluted'],
386+
).toBeUndefined();
387+
388+
const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
389+
expect(updatedContent).toContain('"theme": "dark"');
390+
});
391+
392+
it('should handle ENOENT gracefully if file is missing during read', () => {
393+
const nonExistentPath = path.join(tempDir, 'missing.json');
394+
updateSettingsFilePreservingFormat(nonExistentPath, {
395+
theme: 'dark',
396+
});
397+
398+
expect(fs.existsSync(nonExistentPath)).toBe(true);
399+
const content = fs.readFileSync(nonExistentPath, 'utf-8');
400+
expect(JSON.parse(content)).toEqual({ theme: 'dark' });
401+
});
369402
});
370403
});

‎packages/cli/src/utils/commentJson.ts‎

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66

77
import * as fs from 'node:fs';
8+
import * as path from 'node:path';
89
import { parse, stringify } from 'comment-json';
910
import { coreEvents } from '@google/gemini-cli-core';
1011

@@ -13,25 +14,82 @@ import { coreEvents } from '@google/gemini-cli-core';
1314
*/
1415
type CommentedRecord = Record<string | symbol, unknown>;
1516

17+
function isDangerousKey(key: string): boolean {
18+
return key === '__proto__' || key === 'constructor' || key === 'prototype';
19+
}
20+
21+
let tempCounter = 0;
22+
23+
function writeAtomicSync(filePath: string, content: string): void {
24+
const dir = path.dirname(filePath);
25+
if (!fs.existsSync(dir)) {
26+
fs.mkdirSync(dir, { recursive: true });
27+
}
28+
const tempPath = path.join(
29+
dir,
30+
`.${path.basename(filePath)}.${process.pid}.${Date.now()}.${tempCounter++}.tmp`,
31+
);
32+
try {
33+
fs.writeFileSync(tempPath, content, 'utf-8');
34+
fs.renameSync(tempPath, filePath);
35+
} catch {
36+
try {
37+
if (fs.existsSync(tempPath)) {
38+
fs.unlinkSync(tempPath);
39+
}
40+
} catch {
41+
// ignore
42+
}
43+
// Fallback to direct write if rename fails
44+
fs.writeFileSync(filePath, content, 'utf-8');
45+
}
46+
}
47+
1648
/**
1749
* Updates a JSON file while preserving comments and formatting.
1850
*/
1951
export function updateSettingsFilePreservingFormat(
2052
filePath: string,
2153
updates: Record<string, unknown>,
2254
): void {
55+
const dirPath = path.dirname(filePath);
56+
if (!fs.existsSync(dirPath)) {
57+
fs.mkdirSync(dirPath, { recursive: true });
58+
}
59+
2360
if (!fs.existsSync(filePath)) {
24-
fs.writeFileSync(filePath, JSON.stringify(updates, null, 2), 'utf-8');
61+
writeAtomicSync(filePath, JSON.stringify(updates, null, 2));
2562
return;
2663
}
2764

28-
const originalContent = fs.readFileSync(filePath, 'utf-8');
29-
3065
let parsed: Record<string, unknown>;
3166
try {
32-
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
33-
parsed = parse(originalContent) as Record<string, unknown>;
34-
} catch (error) {
67+
const originalContent = fs.readFileSync(filePath, 'utf-8');
68+
if (!originalContent.trim()) {
69+
parsed = {};
70+
} else {
71+
const rawParsed: unknown = parse(originalContent);
72+
if (
73+
typeof rawParsed !== 'object' ||
74+
rawParsed === null ||
75+
Array.isArray(rawParsed)
76+
) {
77+
parsed = {};
78+
} else {
79+
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
80+
parsed = rawParsed as Record<string, unknown>;
81+
}
82+
}
83+
} catch (error: unknown) {
84+
if (
85+
error &&
86+
typeof error === 'object' &&
87+
'code' in error &&
88+
error.code === 'ENOENT'
89+
) {
90+
writeAtomicSync(filePath, JSON.stringify(updates, null, 2));
91+
return;
92+
}
3593
coreEvents.emitFeedback(
3694
'error',
3795
'Error parsing settings file. Please check the JSON syntax.',
@@ -43,7 +101,7 @@ export function updateSettingsFilePreservingFormat(
43101
const updatedStructure = applyUpdates(parsed, updates);
44102
const updatedContent = stringify(updatedStructure, null, 2);
45103

46-
fs.writeFileSync(filePath, updatedContent, 'utf-8');
104+
writeAtomicSync(filePath, updatedContent);
47105
}
48106

49107
/**
@@ -58,6 +116,9 @@ function preserveCommentsOnPropertyDeletion(
58116
container: Record<string, unknown>,
59117
propName: string,
60118
): void {
119+
if (isDangerousKey(propName)) {
120+
return;
121+
}
61122
const target = container as CommentedRecord;
62123
const beforeSym = Symbol.for(`before:${propName}`);
63124
const afterSym = Symbol.for(`after:${propName}`);
@@ -117,13 +178,19 @@ function applyKeyDiff(
117178
desired: Record<string, unknown>,
118179
): void {
119180
for (const existingKey of Object.getOwnPropertyNames(base)) {
181+
if (isDangerousKey(existingKey)) {
182+
continue;
183+
}
120184
if (!Object.prototype.hasOwnProperty.call(desired, existingKey)) {
121185
preserveCommentsOnPropertyDeletion(base, existingKey);
122186
delete base[existingKey];
123187
}
124188
}
125189

126190
for (const nextKey of Object.getOwnPropertyNames(desired)) {
191+
if (isDangerousKey(nextKey)) {
192+
continue;
193+
}
127194
const nextVal = desired[nextKey];
128195
const baseVal = base[nextKey];
129196

‎packages/cli/src/utils/processUtils.test.ts‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* SPDX-License-Identifier: Apache-2.0
55
*/
66

7-
import { vi } from 'vitest';
7+
import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest';
88
import {
99
RELAUNCH_EXIT_CODE,
1010
relaunchApp,
@@ -40,6 +40,68 @@ describe('processUtils', () => {
4040
expect(runExitCleanup).toHaveBeenCalledTimes(1);
4141
expect(processExit).toHaveBeenCalledWith(RELAUNCH_EXIT_CODE);
4242
});
43+
44+
it('should send auth override IPC message with callback if options.overrideAuthType is provided', async () => {
45+
const originalSend = process.send;
46+
const sendMock = vi.fn((_msg: unknown, cb?: () => void) => {
47+
if (cb) cb();
48+
return true;
49+
});
50+
Object.defineProperty(process, 'send', {
51+
value: sendMock,
52+
configurable: true,
53+
writable: true,
54+
});
55+
vi.stubEnv('VITEST', '');
56+
57+
try {
58+
await relaunchApp({ overrideAuthType: 'login_with_google' });
59+
expect(sendMock).toHaveBeenCalledWith(
60+
{
61+
type: 'auth-selected-type',
62+
authType: 'login_with_google',
63+
},
64+
expect.any(Function),
65+
);
66+
expect(processExit).toHaveBeenCalledWith(RELAUNCH_EXIT_CODE);
67+
} finally {
68+
Object.defineProperty(process, 'send', {
69+
value: originalSend,
70+
configurable: true,
71+
writable: true,
72+
});
73+
}
74+
});
75+
76+
it('should fall back to timeout if process.send callback is never called', async () => {
77+
vi.useFakeTimers();
78+
const originalSend = process.send;
79+
const sendMock = vi.fn(() => false);
80+
Object.defineProperty(process, 'send', {
81+
value: sendMock,
82+
configurable: true,
83+
writable: true,
84+
});
85+
vi.stubEnv('VITEST', '');
86+
87+
try {
88+
const relaunchPromise = relaunchApp({
89+
overrideAuthType: 'login_with_google',
90+
});
91+
await vi.advanceTimersByTimeAsync(500);
92+
await relaunchPromise;
93+
94+
expect(sendMock).toHaveBeenCalled();
95+
expect(processExit).toHaveBeenCalledWith(RELAUNCH_EXIT_CODE);
96+
} finally {
97+
Object.defineProperty(process, 'send', {
98+
value: originalSend,
99+
configurable: true,
100+
writable: true,
101+
});
102+
vi.useRealTimers();
103+
}
104+
});
43105
});
44106

45107
describe('SEA handling utilities', () => {

0 commit comments

Comments
 (0)