mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-06-02 19:32:20 +00:00
security: rely on central validation for extracted paths instead of naive stripping
This commit is contained in:
@@ -347,25 +347,35 @@ describe('atCommandUtils', () => {
|
||||
}
|
||||
});
|
||||
|
||||
it('should sanitize paths against traversal (..)', async () => {
|
||||
it('should NOT strip traversal (..), but let central validation handle it', async () => {
|
||||
const traversalPath = 'src/../../etc/passwd';
|
||||
const absPath = path.resolve('/mock/root', traversalPath);
|
||||
|
||||
(mockConfig['validatePathAccess'] as Mock).mockImplementation((p) => {
|
||||
if (p === absPath) return 'Outside workspace';
|
||||
return null;
|
||||
});
|
||||
|
||||
const result = await resolveAtCommandPath(
|
||||
`FAIL ${traversalPath}`,
|
||||
mockConfig as unknown as Config,
|
||||
);
|
||||
// '..' should be stripped, resulting in 'src/etc/passwd' (which doesn't exist in our mock)
|
||||
expect(result.status).toBe('not_found');
|
||||
|
||||
// It should NOT be stripped. It should resolve to the absolute path and fail authorization.
|
||||
expect(result.status).toBe('unauthorized');
|
||||
if (result.status === 'unauthorized') {
|
||||
expect(result.absolutePath).toBe(absPath);
|
||||
}
|
||||
});
|
||||
|
||||
it('should sanitize paths against null bytes', async () => {
|
||||
it('should reject paths with null bytes via validatePath', async () => {
|
||||
const nullBytePath = 'src/index.ts\0.exe';
|
||||
const result = await resolveAtCommandPath(
|
||||
`FAIL ${nullBytePath}`,
|
||||
mockConfig as unknown as Config,
|
||||
);
|
||||
// Null byte and following should be stripped/handled.
|
||||
// Our implementation strips \0, so it becomes 'src/index.ts.exe'
|
||||
expect(result.status).toBe('not_found');
|
||||
// validatePath rejects strings with null bytes
|
||||
expect(result.status).toBe('invalid');
|
||||
});
|
||||
|
||||
it('should handle paths with slashes and extensions correctly', async () => {
|
||||
|
||||
@@ -173,21 +173,17 @@ function tryExtractPath(noisyString: string): string | null {
|
||||
const lineMatch = segmentToClean.match(/^(.+?):(\d+)(?::\d+)?$/);
|
||||
const pathOnly = lineMatch ? lineMatch[1] : segmentToClean;
|
||||
|
||||
// 3. Sanitize extracted path to prevent path traversal and null byte injection
|
||||
const cleanSegment = pathOnly.replace(/\0/g, '').replace(/\.\./g, '');
|
||||
|
||||
if (cleanSegment.length === 0) continue;
|
||||
|
||||
// Check if the cleaned segment is considered "valid" by our heuristics
|
||||
// (i.e. no control chars, no markers, etc.)
|
||||
if (validatePath(cleanSegment).isValid) {
|
||||
// 3. Validate the extracted segment using centralized heuristics.
|
||||
// We rely on validatePath and Config.validatePathAccess for robust checking
|
||||
// rather than naive string stripping which can be bypassed or corrupt valid names.
|
||||
if (validatePath(pathOnly).isValid) {
|
||||
// Prioritize segments that actually look like paths (have slashes or dots)
|
||||
if (
|
||||
cleanSegment.includes('/') ||
|
||||
cleanSegment.includes('\\') ||
|
||||
cleanSegment.includes('.')
|
||||
pathOnly.includes('/') ||
|
||||
pathOnly.includes('\\') ||
|
||||
pathOnly.includes('.')
|
||||
) {
|
||||
return cleanSegment;
|
||||
return pathOnly;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user