fix(core,cli): address review findings for plan dir resolution and security

This commit addresses several critical findings from the review bot:

- **Security:** Implemented defense-in-depth symlink resolution. Removed insecure string-based fallbacks in `Storage.getPlansDir` and added a mandatory `isSubpath` validation AFTER directory creation in `Config.getPlansDir` to prevent TOCTOU traversal attacks.
- **Architecture:** Fixed a race condition where active extension context was mutated synchronously in `AppContainer`, potentially corrupting concurrent background tasks. Mutation now occurs within the command execution pipeline.
- **Robustness:** Switched to canonical path checking for `plan` command detection to support aliases and subcommands.
- **Regressions:** Added a `planEnabled` guard to prevent unwanted directory creation when the planning feature is disabled.
- **Validation:** Added exhaustive unit tests covering sequential context switching, shared directory deduplication, and symlink security edge cases.
This commit is contained in:
Mahima Shanware
2026-04-06 21:09:47 +00:00
parent a5c2bf81f4
commit 6559fdbc31
5 changed files with 196 additions and 165 deletions
+32
View File
@@ -3380,6 +3380,38 @@ describe('Plans Directory Initialization', () => {
);
});
it('should deduplicate and cache when multiple extensions (or default) use the same directory', async () => {
vi.spyOn(fs, 'mkdirSync').mockReturnValue(undefined);
const config = new Config({
...baseParams,
plan: true,
});
await config.initialize();
// 1. Call for Default Plan Dir
const defaultDir = config.getPlansDir();
expect(fs.mkdirSync).toHaveBeenCalledTimes(1);
// 2. Mock an extension that happens to use the SAME directory string
vi.spyOn(
config as unknown as {
getActiveExtensionPlanDir: () => string | undefined;
},
'getActiveExtensionPlanDir',
).mockReturnValue(
'plans', // This will resolve to the same path as the default in our mock setup
);
const extDir = config.getPlansDir();
// It should be the same path
expect(extDir).toBe(defaultDir);
// It should NOT have called mkdirSync a second time
expect(fs.mkdirSync).toHaveBeenCalledTimes(1);
});
it('should NOT create plans directory or add it to workspace context when plan is disabled', async () => {
vi.spyOn(fs, 'mkdirSync').mockReturnValue(undefined);
const config = new Config({