mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-15 22:33:05 -07:00
fix(core): remove redundant ENOENT fallback in getPlansDir to fix traversal vulnerability
This removes the insecure ENOENT fallback in `Storage.getPlansDir` that could be exploited to bypass the `isSubpath` check via symlinks. The fallback was unnecessary because the underlying `resolveToRealPath` function (via `robustRealpath`) was recently updated to gracefully handle and resolve symlinks for non-existent target paths.
This commit is contained in:
@@ -3275,6 +3275,9 @@ describe('Plans Directory Initialization', () => {
|
||||
debugMode: false,
|
||||
model: 'test-model',
|
||||
cwd: '/tmp/test',
|
||||
planSettings: {
|
||||
directory: 'plans',
|
||||
},
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -3386,7 +3389,8 @@ describe('Plans Directory Initialization', () => {
|
||||
|
||||
await config.initialize();
|
||||
|
||||
const plansDir = config.storage.getPlansDir();
|
||||
// Even if getPlansDir is called manually, it should NOT create the directory
|
||||
const plansDir = config.getPlansDir();
|
||||
expect(fs.mkdirSync).not.toHaveBeenCalled();
|
||||
expect(config.getWorkspaceContext().getDirectories()).not.toContain(
|
||||
plansDir,
|
||||
|
||||
@@ -2251,22 +2251,22 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
getPlansDir(): string {
|
||||
const plansDir = this.storage.getPlansDir(this.getActiveExtensionPlanDir());
|
||||
|
||||
if (this.initializedPlanDirs.has(plansDir)) {
|
||||
if (!this.planEnabled || this.initializedPlanDirs.has(plansDir)) {
|
||||
return plansDir;
|
||||
}
|
||||
|
||||
try {
|
||||
fs.mkdirSync(plansDir, { recursive: true });
|
||||
|
||||
let realPlansDir = plansDir;
|
||||
try {
|
||||
const resolved = resolveToRealPath(plansDir);
|
||||
if (resolved) {
|
||||
realPlansDir = resolved;
|
||||
}
|
||||
} catch {
|
||||
// Ignore failures in mock environments
|
||||
const realPlansDir = resolveToRealPath(plansDir);
|
||||
const realProjectRoot = resolveToRealPath(this.getTargetDir());
|
||||
|
||||
if (!isSubpath(realProjectRoot, realPlansDir)) {
|
||||
throw new Error(
|
||||
`Security violation: Resolved plan directory '${realPlansDir}' is outside the project root '${realProjectRoot}'.`,
|
||||
);
|
||||
}
|
||||
|
||||
this.workspaceContext.addDirectory(realPlansDir);
|
||||
this.initializedPlanDirs.add(plansDir);
|
||||
} catch (e: unknown) {
|
||||
|
||||
@@ -378,6 +378,28 @@ describe('Storage – additional helpers', () => {
|
||||
},
|
||||
expected: path.resolve(projectRoot, 'new-plans'),
|
||||
},
|
||||
{
|
||||
name: 'security escape via symbolic link with non-existent dir throws',
|
||||
customDir: 'link-to-outside/new-dir',
|
||||
setup: () => {
|
||||
vi.mocked(fs.realpathSync).mockImplementation((p: fs.PathLike) => {
|
||||
const pStr = p.toString();
|
||||
if (pStr.includes('link-to-outside/new-dir')) {
|
||||
const err = new Error('ENOENT') as NodeJS.ErrnoException;
|
||||
err.code = 'ENOENT';
|
||||
throw err;
|
||||
}
|
||||
if (pStr.includes('link-to-outside')) {
|
||||
return '/outside/project/root';
|
||||
}
|
||||
return pStr;
|
||||
});
|
||||
return () => vi.mocked(fs.realpathSync).mockRestore();
|
||||
},
|
||||
expected: '',
|
||||
expectedError:
|
||||
"Custom plans directory 'link-to-outside/new-dir' resolves to '/outside/project/root/new-dir', which is outside the project root '/tmp/project'.",
|
||||
},
|
||||
];
|
||||
|
||||
testCases.forEach(({ name, customDir, expected, expectedError, setup }) => {
|
||||
|
||||
@@ -325,24 +325,7 @@ export class Storage {
|
||||
if (customDir) {
|
||||
const resolvedPath = path.resolve(this.getProjectRoot(), customDir);
|
||||
const realProjectRoot = resolveToRealPath(this.getProjectRoot());
|
||||
let realResolvedPath = resolvedPath;
|
||||
|
||||
try {
|
||||
realResolvedPath = resolveToRealPath(resolvedPath);
|
||||
} catch (e: unknown) {
|
||||
if (
|
||||
!(
|
||||
e &&
|
||||
typeof e === 'object' &&
|
||||
'code' in e &&
|
||||
(e.code === 'ENOENT' || e.code === 'EISDIR')
|
||||
)
|
||||
) {
|
||||
throw e;
|
||||
}
|
||||
// Construct the fallback path safely against the real project root
|
||||
realResolvedPath = path.resolve(realProjectRoot, customDir);
|
||||
}
|
||||
const realResolvedPath = resolveToRealPath(resolvedPath);
|
||||
|
||||
if (!isSubpath(realProjectRoot, realResolvedPath)) {
|
||||
throw new Error(
|
||||
|
||||
Reference in New Issue
Block a user