From 62246d2fad8f25b388511d231cc9807b6d92d95c Mon Sep 17 00:00:00 2001 From: Christian Gunderman Date: Fri, 15 May 2026 11:10:48 -0700 Subject: [PATCH] chore: revert out of scope changes and fix GH_TOKEN allowlist --- .../src/ui/hooks/atCommandProcessor.test.ts | 20 ---------- .../cli/src/ui/hooks/atCommandProcessor.ts | 12 ++---- .../services/environmentSanitization.test.ts | 3 +- .../src/services/environmentSanitization.ts | 37 +++++++++---------- packages/core/src/utils/paths.test.ts | 13 ------- packages/core/src/utils/paths.ts | 5 +-- packages/test-utils/tsconfig.json | 2 +- packages/vscode-ide-companion/tsconfig.json | 2 +- 8 files changed, 25 insertions(+), 69 deletions(-) diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts index 8076d8a2be..ca2ecf7bc1 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts @@ -17,7 +17,6 @@ import { handleAtCommand, escapeAtSymbols, unescapeLiteralAt, - checkPermissions, } from './atCommandProcessor.js'; import { FileDiscoveryService, @@ -1540,23 +1539,4 @@ describe('unescapeLiteralAt', () => { const input = 'user@example.com and @scope/pkg'; expect(unescapeLiteralAt(escapeAtSymbols(input))).toBe(input); }); - - describe('checkPermissions', () => { - it('should handle ENAMETOOLONG gracefully in checkPermissions', async () => { - const longPath = 'a'.repeat(5000); - const query = `@${longPath}`; - - const localMockConfig = { - getTargetDir: () => '.', - validatePathAccess: () => true, - getResourceRegistry: () => ({ - findResourceByUri: () => undefined, - }), - } as unknown as Config; - - // checkPermissions should not throw ENAMETOOLONG - const permissions = await checkPermissions(query, localMockConfig); - expect(permissions).toEqual([]); - }); - }); }); diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index 38cb541f1b..512fe952ba 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -188,15 +188,9 @@ export async function checkPermissions( const pathName = part.content.substring(1); if (!pathName) continue; - let resolvedPathName: string; - try { - resolvedPathName = resolveToRealPath( - path.resolve(config.getTargetDir(), pathName), - ); - } catch { - // If path resolution fails (e.g. ENAMETOOLONG), skip this path - continue; - } + const resolvedPathName = resolveToRealPath( + path.resolve(config.getTargetDir(), pathName), + ); if (config.validatePathAccess(resolvedPathName, 'read')) { if (await fileExists(resolvedPathName)) { diff --git a/packages/core/src/services/environmentSanitization.test.ts b/packages/core/src/services/environmentSanitization.test.ts index cdee3330b9..da0d14e5dd 100644 --- a/packages/core/src/services/environmentSanitization.test.ts +++ b/packages/core/src/services/environmentSanitization.test.ts @@ -372,14 +372,13 @@ describe('getSecureSanitizationConfig', () => { it('should not filter out variables from allowed list that match NEVER_ALLOWED_NAME_PATTERNS', () => { const requestedConfig = { - allowedEnvironmentVariables: ['SAFE_VAR', 'MY_SECRET_TOKEN', 'GH_TOKEN'], + allowedEnvironmentVariables: ['SAFE_VAR', 'MY_SECRET_TOKEN'], }; const config = getSecureSanitizationConfig(requestedConfig); expect(config.allowedEnvironmentVariables).toContain('SAFE_VAR'); expect(config.allowedEnvironmentVariables).toContain('MY_SECRET_TOKEN'); - expect(config.allowedEnvironmentVariables).toContain('GH_TOKEN'); }); it('should deduplicate variables in allowed and blocked lists', () => { diff --git a/packages/core/src/services/environmentSanitization.ts b/packages/core/src/services/environmentSanitization.ts index da116d3a86..0bd8bdbfa3 100644 --- a/packages/core/src/services/environmentSanitization.ts +++ b/packages/core/src/services/environmentSanitization.ts @@ -14,11 +14,9 @@ export function sanitizeEnvironment( processEnv: NodeJS.ProcessEnv, config: EnvironmentSanitizationConfig, ): NodeJS.ProcessEnv { - // Enable strict sanitization in GitHub actions. const isStrictSanitization = !!processEnv['GITHUB_SHA'] || processEnv['SURFACE'] === 'Github'; - // Always sanitize when in GitHub actions. if (!config.enableEnvironmentVariableRedaction && !isStrictSanitization) { return { ...processEnv }; } @@ -152,28 +150,38 @@ function shouldRedactEnvironmentVariable( key = key.toUpperCase(); value = value?.toUpperCase(); - // User overrides take precedence. if (allowedSet?.has(key)) { return false; } + + if (key.startsWith('GEMINI_CLI_')) { + return false; + } + + if (value) { + for (const pattern of NEVER_ALLOWED_VALUE_PATTERNS) { + if (pattern.test(value)) { + return true; + } + } + } + + if (key.startsWith('GIT_CONFIG_')) { + return false; + } + if (blockedSet?.has(key)) { return true; } - // These are never redacted. - if ( - ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES.has(key) || - key.startsWith('GEMINI_CLI_') - ) { + if (ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES.has(key)) { return false; } - // These are always redacted. if (NEVER_ALLOWED_ENVIRONMENT_VARIABLES.has(key)) { return true; } - // If in strict mode (e.g. GitHub Action), and not explicitly allowed, redact it. if (isStrictSanitization) { return true; } @@ -184,15 +192,6 @@ function shouldRedactEnvironmentVariable( } } - // Redact if the value looks like a key/cert. - if (value) { - for (const pattern of NEVER_ALLOWED_VALUE_PATTERNS) { - if (pattern.test(value)) { - return true; - } - } - } - return false; } diff --git a/packages/core/src/utils/paths.test.ts b/packages/core/src/utils/paths.test.ts index 4d08c85548..bb2801a9ad 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/packages/core/src/utils/paths.test.ts @@ -602,19 +602,6 @@ describe('resolveToRealPath', () => { /Infinite recursion detected/, ); }); - - it('should handle ENAMETOOLONG gracefully', () => { - const longPath = path.resolve('/' + 'a'.repeat(5000)); - - vi.spyOn(fs, 'realpathSync').mockImplementation(() => { - const err = new Error('ENAMETOOLONG') as NodeJS.ErrnoException; - err.code = 'ENAMETOOLONG'; - throw err; - }); - - // Should return the path itself if realpathSync fails with ENAMETOOLONG - expect(resolveToRealPath(longPath)).toBe(longPath); - }); }); describe('makeRelative', () => { diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index 064db93a74..d80e0df20a 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -460,10 +460,7 @@ function robustRealpath(p: string, visited = new Set()): string { lstatError && typeof lstatError === 'object' && 'code' in lstatError && - (lstatError.code === 'ENOENT' || - lstatError.code === 'EISDIR' || - lstatError.code === 'ENAMETOOLONG' || - lstatError.code === 'EINVAL') + (lstatError.code === 'ENOENT' || lstatError.code === 'EISDIR') ) ) { throw lstatError; diff --git a/packages/test-utils/tsconfig.json b/packages/test-utils/tsconfig.json index 3bd258dbc0..ee9b84b1b4 100644 --- a/packages/test-utils/tsconfig.json +++ b/packages/test-utils/tsconfig.json @@ -2,7 +2,7 @@ "extends": "../../tsconfig.json", "compilerOptions": { "outDir": "dist", - "lib": ["DOM", "DOM.Iterable", "ES2023"], + "lib": ["DOM", "DOM.Iterable", "ES2021"], "composite": true, "types": ["node"] }, diff --git a/packages/vscode-ide-companion/tsconfig.json b/packages/vscode-ide-companion/tsconfig.json index eee49476a5..f135706485 100644 --- a/packages/vscode-ide-companion/tsconfig.json +++ b/packages/vscode-ide-companion/tsconfig.json @@ -3,7 +3,7 @@ "module": "NodeNext", "moduleResolution": "NodeNext", "target": "ES2022", - "lib": ["ES2023", "dom"], + "lib": ["ES2022", "dom"], "sourceMap": true, /* * skipLibCheck is necessary because the a2a-server package depends on