Skip to content

Commit 9f1e63d

Browse files
Copilotedvilme
andauthored
Skip directory .env / python.envFile paths when resolving environment variables (#1865)
If `<project>/.env` was a directory, for example a virtual environment named `.env`, `getEnvironmentVariables()` tried to read it as a file and threw `EntryIsADirectory`. That broke terminal env injection and Pylance. The same happened when `python.envFile` pointed at a directory. The cause was the `fs-extra` `pathExists` check, which also returns true for directories. ### Changes - **`envVariableManager.ts`**: both `pathExists` checks are replaced with a private `isEnvFile` helper, which accepts only regular files: ```ts async function isEnvFile(filePath: string): Promise<boolean> { try { return (await fsapi.stat(filePath)).isFile(); } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === 'ENOENT' || code === 'ENOTDIR') { return false; } throw error; } } ``` - It uses `stat`, not `lstat`, so symlinks to regular files still load. Symlinks to directories are skipped. - Errors other than `ENOENT`/`ENOTDIR` (for example, `EACCES`) are still raised, not hidden. - `parseEnvFile` is never called on a skipped path. - **Tests** (`envVariableManager.unit.test.ts`): these call `getEnvironmentVariables` directly and cover: - a directory at either path - a valid file next to a directory - two regular files loading in the existing order - symlinks to a file or a directory, using real temp-dir symlinks that are skipped if the platform can't create them - missing paths and `ENOTDIR` being skipped, and `EACCES` being raised ### Not changed - **Precedence:** the order documented in `docs/README.md` stays the same: base → `python.envFile` → project `.env` → overrides. This PR does **not** implement the issue's separate request to have `python.envFile` suppress project `.env` discovery. - **Other behavior:** path selection, deduplication, merging, and URI overrides are untouched. - **Race:** this is a check before the read, so a path that changes type between the `stat` and the read can still fail. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com> Co-authored-by: Eduardo Villalpando Mello <eduardovil@microsoft.com>
1 parent cc8f6ac commit 9f1e63d

2 files changed

Lines changed: 236 additions & 2 deletions

File tree

‎src/features/execution/envVariableManager.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,23 @@ import { createFileSystemWatcher, getConfiguration } from '../../common/workspac
88
import type { PythonProjectManager } from '../projectManager';
99
import { mergeEnvVariables, parseEnvFile } from './envVarUtils';
1010

11+
/**
12+
* Returns true if the path resolves (following symlinks) to a regular file.
13+
* Missing paths and paths with a non-directory parent are treated as absent;
14+
* other errors (for example, permission errors) are propagated.
15+
*/
16+
async function isEnvFile(filePath: string): Promise<boolean> {
17+
try {
18+
return (await fsapi.stat(filePath)).isFile();
19+
} catch (error) {
20+
const code = (error as NodeJS.ErrnoException).code;
21+
if (code === 'ENOENT' || code === 'ENOTDIR') {
22+
return false;
23+
}
24+
throw error;
25+
}
26+
}
27+
1128
export interface EnvVarManager extends PythonEnvironmentVariablesApi, Disposable {}
1229

1330
export class PythonEnvVariableManager implements EnvVarManager {
@@ -50,7 +67,7 @@ export class PythonEnvVariableManager implements EnvVarManager {
5067
let envFilePath = config.get<string>('envFile');
5168
envFilePath = envFilePath ? path.normalize(resolveVariables(envFilePath, uri)) : undefined;
5269

53-
if (envFilePath && (await fsapi.pathExists(envFilePath))) {
70+
if (envFilePath && (await isEnvFile(envFilePath))) {
5471
const other = await parseEnvFile(Uri.file(envFilePath));
5572
env = mergeEnvVariables(env, other);
5673
}
@@ -59,7 +76,7 @@ export class PythonEnvVariableManager implements EnvVarManager {
5976
if (
6077
projectEnvFilePath &&
6178
projectEnvFilePath?.toLowerCase() !== envFilePath?.toLowerCase() &&
62-
(await fsapi.pathExists(projectEnvFilePath))
79+
(await isEnvFile(projectEnvFilePath))
6380
) {
6481
const other = await parseEnvFile(Uri.file(projectEnvFilePath));
6582
env = mergeEnvVariables(env, other);
Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
import * as assert from 'assert';
2+
import fsapi from 'fs-extra';
3+
import * as os from 'os';
4+
import * as path from 'path';
5+
import * as sinon from 'sinon';
6+
import { Uri } from 'vscode';
7+
import * as workspaceApis from '../../../common/workspace.apis';
8+
import * as envVarUtils from '../../../features/execution/envVarUtils';
9+
import { PythonEnvVariableManager } from '../../../features/execution/envVariableManager';
10+
import type { PythonProjectManager } from '../../../features/projectManager';
11+
12+
type StatResult = 'file' | 'directory' | NodeJS.ErrnoException;
13+
14+
function errnoError(code: string): NodeJS.ErrnoException {
15+
const error = new Error(code) as NodeJS.ErrnoException;
16+
error.code = code;
17+
return error;
18+
}
19+
20+
function pathKey(filePath: string): string {
21+
const normalizedPath = path.normalize(filePath);
22+
return process.platform === 'win32' ? normalizedPath.toLowerCase() : normalizedPath;
23+
}
24+
25+
suite('PythonEnvVariableManager.getEnvironmentVariables', () => {
26+
let sandbox: sinon.SinonSandbox;
27+
let parseEnvFileStub: sinon.SinonStub;
28+
let configuredEnvFile: string | undefined;
29+
let manager: PythonEnvVariableManager;
30+
31+
const projectRoot = path.resolve('/workspace/project');
32+
const projectUri = Uri.file(projectRoot);
33+
const projectEnvFile = path.normalize(path.join(projectRoot, '.env'));
34+
const configuredFile = path.normalize(path.resolve('/config/custom.env'));
35+
36+
function parsedPaths(): string[] {
37+
return parseEnvFileStub.getCalls().map((c) => pathKey((c.args[0] as Uri).fsPath));
38+
}
39+
40+
function stubStat(results: Map<string, StatResult>): void {
41+
const normalizedResults = new Map([...results].map(([p, result]) => [pathKey(p), result]));
42+
sandbox.stub(fsapi, 'stat').callsFake((async (p: string) => {
43+
const result = normalizedResults.get(pathKey(p)) ?? errnoError('ENOENT');
44+
if (result === 'file' || result === 'directory') {
45+
return { isFile: () => result === 'file' } as fsapi.Stats;
46+
}
47+
throw result;
48+
}) as unknown as typeof fsapi.stat);
49+
}
50+
51+
setup(() => {
52+
sandbox = sinon.createSandbox();
53+
configuredEnvFile = undefined;
54+
55+
const mockWatcher = {
56+
onDidCreate: () => ({ dispose: () => {} }),
57+
onDidChange: () => ({ dispose: () => {} }),
58+
onDidDelete: () => ({ dispose: () => {} }),
59+
dispose: () => {},
60+
};
61+
sandbox.stub(workspaceApis, 'createFileSystemWatcher').returns(mockWatcher as never);
62+
sandbox.stub(workspaceApis, 'getWorkspaceFolder').returns(undefined);
63+
sandbox.stub(workspaceApis, 'getWorkspaceFolders').returns(undefined);
64+
sandbox.stub(workspaceApis, 'getConfiguration').returns({
65+
get: (key: string) => (key === 'envFile' ? configuredEnvFile : undefined),
66+
} as never);
67+
68+
parseEnvFileStub = sandbox.stub(envVarUtils, 'parseEnvFile');
69+
parseEnvFileStub.callsFake(async (uri: Uri) => {
70+
if (pathKey(uri.fsPath) === pathKey(projectEnvFile)) {
71+
return { PROJECT_VAR: 'project', SHARED: 'from-project' };
72+
}
73+
return { CONFIG_VAR: 'config', SHARED: 'from-config' };
74+
});
75+
76+
const pm = {
77+
get: () => ({ name: 'project', uri: projectUri }),
78+
} as unknown as PythonProjectManager;
79+
manager = new PythonEnvVariableManager(pm);
80+
});
81+
82+
teardown(() => {
83+
manager.dispose();
84+
sandbox.restore();
85+
});
86+
87+
test('project .env directory with missing configured file does not throw and leaves base unchanged', async () => {
88+
configuredEnvFile = configuredFile;
89+
stubStat(new Map<string, StatResult>([[projectEnvFile, 'directory']]));
90+
const base = { BASE: 'base' };
91+
92+
const env = await manager.getEnvironmentVariables(projectUri, undefined, base);
93+
94+
assert.deepStrictEqual(env, { BASE: 'base' });
95+
assert.deepStrictEqual(parsedPaths(), []);
96+
});
97+
98+
test('configured env file that is a directory is skipped while project .env still loads', async () => {
99+
configuredEnvFile = configuredFile;
100+
stubStat(
101+
new Map<string, StatResult>([
102+
[configuredFile, 'directory'],
103+
[projectEnvFile, 'file'],
104+
]),
105+
);
106+
107+
const env = await manager.getEnvironmentVariables(projectUri, undefined, { BASE: 'base' });
108+
109+
assert.deepStrictEqual(parsedPaths(), [pathKey(projectEnvFile)]);
110+
assert.strictEqual(env.PROJECT_VAR, 'project');
111+
assert.strictEqual(env.CONFIG_VAR, undefined);
112+
});
113+
114+
test('valid configured file is loaded when project .env is a directory', async () => {
115+
configuredEnvFile = configuredFile;
116+
stubStat(
117+
new Map<string, StatResult>([
118+
[configuredFile, 'file'],
119+
[projectEnvFile, 'directory'],
120+
]),
121+
);
122+
123+
const env = await manager.getEnvironmentVariables(projectUri, undefined, { BASE: 'base' });
124+
125+
assert.deepStrictEqual(parsedPaths(), [pathKey(configuredFile)]);
126+
assert.strictEqual(env.CONFIG_VAR, 'config');
127+
assert.strictEqual(env.SHARED, 'from-config');
128+
});
129+
130+
test('when both are regular files, configured file is applied before project .env', async () => {
131+
configuredEnvFile = configuredFile;
132+
stubStat(
133+
new Map<string, StatResult>([
134+
[configuredFile, 'file'],
135+
[projectEnvFile, 'file'],
136+
]),
137+
);
138+
139+
const env = await manager.getEnvironmentVariables(projectUri, undefined, { BASE: 'base' });
140+
141+
assert.deepStrictEqual(parsedPaths(), [pathKey(configuredFile), pathKey(projectEnvFile)]);
142+
assert.strictEqual(env.SHARED, 'from-project');
143+
assert.strictEqual(env.PROJECT_VAR, 'project');
144+
});
145+
146+
test('missing paths and ENOTDIR are skipped without parsing', async () => {
147+
configuredEnvFile = configuredFile;
148+
stubStat(new Map<string, StatResult>([[configuredFile, errnoError('ENOTDIR')]]));
149+
const base = { BASE: 'base' };
150+
151+
const env = await manager.getEnvironmentVariables(projectUri, undefined, base);
152+
153+
assert.deepStrictEqual(env, { BASE: 'base' });
154+
assert.deepStrictEqual(parsedPaths(), []);
155+
});
156+
157+
test('unexpected stat errors such as EACCES are propagated', async () => {
158+
stubStat(new Map<string, StatResult>([[projectEnvFile, errnoError('EACCES')]]));
159+
160+
await assert.rejects(
161+
manager.getEnvironmentVariables(projectUri, undefined, { BASE: 'base' }),
162+
(err: NodeJS.ErrnoException) => err.code === 'EACCES',
163+
);
164+
assert.deepStrictEqual(parsedPaths(), []);
165+
});
166+
167+
suite('symlinks', () => {
168+
let tmpDir: string;
169+
170+
setup(async () => {
171+
tmpDir = await fsapi.mkdtemp(path.join(os.tmpdir(), 'envvarmgr-'));
172+
});
173+
174+
teardown(async () => {
175+
await fsapi.remove(tmpDir);
176+
});
177+
178+
async function trySymlink(target: string, link: string, type: 'file' | 'dir'): Promise<boolean> {
179+
try {
180+
await fsapi.symlink(target, link, type);
181+
return true;
182+
} catch {
183+
return false;
184+
}
185+
}
186+
187+
test('symlink to a regular file is accepted', async function () {
188+
const target = path.join(tmpDir, 'real.env');
189+
const link = path.join(tmpDir, 'link.env');
190+
await fsapi.writeFile(target, 'CONFIG_VAR=config\n');
191+
if (!(await trySymlink(target, link, 'file'))) {
192+
this.skip();
193+
}
194+
configuredEnvFile = link;
195+
196+
const env = await manager.getEnvironmentVariables(projectUri, undefined, { BASE: 'base' });
197+
198+
assert.deepStrictEqual(parsedPaths(), [pathKey(link)]);
199+
assert.strictEqual(env.CONFIG_VAR, 'config');
200+
});
201+
202+
test('symlink to a directory is skipped', async function () {
203+
const target = path.join(tmpDir, 'realdir');
204+
const link = path.join(tmpDir, 'linkdir');
205+
await fsapi.mkdir(target);
206+
if (!(await trySymlink(target, link, 'dir'))) {
207+
this.skip();
208+
}
209+
configuredEnvFile = link;
210+
211+
const env = await manager.getEnvironmentVariables(projectUri, undefined, { BASE: 'base' });
212+
213+
assert.deepStrictEqual(env, { BASE: 'base' });
214+
assert.deepStrictEqual(parsedPaths(), []);
215+
});
216+
});
217+
});

0 commit comments

Comments
 (0)