Skip to content

Commit 94f78ad

Browse files
fix(auth): tighten Windows credentials ACL (#104) (#253)
* fix(auth): tighten Windows credentials ACL * docs(auth): document credential permission tightening
1 parent 36e5b14 commit 94f78ad

2 files changed

Lines changed: 103 additions & 2 deletions

File tree

src/lib/credentials.test.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { mkdtempSync, statSync, readFileSync, writeFileSync, existsSync, mkdirSync } from 'node:fs';
22
import { homedir, tmpdir } from 'node:os';
33
import { join } from 'node:path';
4-
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
55
import {
66
DEFAULT_PROFILE,
77
assertValidProfileName,
@@ -191,6 +191,45 @@ describe('ensureRestrictiveMode', () => {
191191
expect(() => ensureRestrictiveMode(credentialsPath)).not.toThrow();
192192
});
193193

194+
it('tightens the Windows ACL with icacls instead of POSIX chmod', () => {
195+
mkdirSync(tmpRoot, { recursive: true });
196+
writeFileSync(credentialsPath, 'data', { mode: 0o666 });
197+
const spawn = vi.fn(() => ({ status: 0, signal: null, output: [], pid: 123 })) as never;
198+
199+
ensureRestrictiveMode(credentialsPath, {
200+
platform: 'win32',
201+
env: { USERNAME: 'alice' } as NodeJS.ProcessEnv,
202+
spawnSync: spawn,
203+
});
204+
205+
expect(spawn).toHaveBeenCalledWith(
206+
'icacls',
207+
[credentialsPath, '/inheritance:r', '/grant:r', 'alice:F'],
208+
{
209+
shell: false,
210+
stdio: 'ignore',
211+
windowsHide: true,
212+
},
213+
);
214+
});
215+
216+
it('warns on Windows when credentials ACL tightening cannot run', () => {
217+
mkdirSync(tmpRoot, { recursive: true });
218+
writeFileSync(credentialsPath, 'data');
219+
const warnings: string[] = [];
220+
const spawn = vi.fn(() => ({ status: 0, signal: null, output: [], pid: 123 })) as never;
221+
222+
ensureRestrictiveMode(credentialsPath, {
223+
platform: 'win32',
224+
env: {} as NodeJS.ProcessEnv,
225+
spawnSync: spawn,
226+
warn: line => warnings.push(line),
227+
});
228+
229+
expect(spawn).not.toHaveBeenCalled();
230+
expect(warnings.join('\n')).toContain('credentials file permissions were not tightened');
231+
});
232+
194233
// POSIX-only premise: Windows has no 0644/0600 distinction to downgrade.
195234
it.skipIf(process.platform === 'win32')('downgrades over-permissive modes', () => {
196235
mkdirSync(tmpRoot, { recursive: true });

src/lib/credentials.ts

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
statSync,
88
writeFileSync,
99
} from 'node:fs';
10+
import { spawnSync, type SpawnSyncReturns } from 'node:child_process';
1011
import { homedir } from 'node:os';
1112
import { dirname, join } from 'node:path';
1213
import { localValidationError } from './errors.js';
@@ -61,6 +62,17 @@ export interface CredentialsOptions {
6162
path?: string;
6263
}
6364

65+
interface RestrictiveModeOptions {
66+
platform?: NodeJS.Platform;
67+
env?: NodeJS.ProcessEnv;
68+
spawnSync?: (
69+
command: string,
70+
args: readonly string[],
71+
options: { shell: false; stdio: 'ignore'; windowsHide: true },
72+
) => SpawnSyncReturns<Buffer>;
73+
warn?: (line: string) => void;
74+
}
75+
6476
const FILE_KEY_TO_FIELD: Record<string, keyof ProfileEntry> = {
6577
api_key: 'apiKey',
6678
api_url: 'apiUrl',
@@ -172,12 +184,62 @@ export function deleteProfile(profile: string, options: CredentialsOptions = {})
172184
return true;
173185
}
174186

175-
export function ensureRestrictiveMode(path: string): void {
187+
/**
188+
* Enforce restrictive access on the credentials file after atomic writes.
189+
* POSIX hosts use chmod(0600); Windows hosts use ACL tightening via icacls.
190+
*/
191+
export function ensureRestrictiveMode(path: string, options: RestrictiveModeOptions = {}): void {
176192
if (!existsSync(path)) return;
193+
if ((options.platform ?? process.platform) === 'win32') {
194+
ensureWindowsRestrictiveAcl(path, options);
195+
return;
196+
}
177197
const overpermissive = (statSync(path).mode & 0o077) !== 0;
178198
if (overpermissive) chmodSync(path, 0o600);
179199
}
180200

201+
/**
202+
* Restrict a Windows credentials file to the current user using icacls.
203+
* The command is invoked with an args array so credential paths are never shell-interpreted.
204+
*/
205+
function ensureWindowsRestrictiveAcl(path: string, options: RestrictiveModeOptions): void {
206+
const username = (options.env ?? process.env).USERNAME?.trim();
207+
if (!username) {
208+
warnWindowsAcl(
209+
'could not determine the Windows username; credentials file permissions were not tightened',
210+
options,
211+
);
212+
return;
213+
}
214+
215+
const run = options.spawnSync ?? spawnSync;
216+
const result = run('icacls', [path, '/inheritance:r', '/grant:r', `${username}:F`], {
217+
shell: false,
218+
stdio: 'ignore',
219+
windowsHide: true,
220+
});
221+
222+
if (result.error) {
223+
warnWindowsAcl(
224+
`icacls failed while tightening credentials file permissions: ${result.error.message}`,
225+
options,
226+
);
227+
return;
228+
}
229+
if (result.status !== 0) {
230+
warnWindowsAcl(
231+
`icacls exited with status ${result.status ?? 'unknown'}; credentials file permissions may be too broad`,
232+
options,
233+
);
234+
}
235+
}
236+
237+
/** Emit an explicit warning when Windows ACL tightening cannot be completed. */
238+
function warnWindowsAcl(message: string, options: RestrictiveModeOptions): void {
239+
const warn = options.warn ?? ((line: string) => process.stderr.write(`${line}\n`));
240+
warn(`[warning] ${message}`);
241+
}
242+
181243
function resolvePath(options: CredentialsOptions): string {
182244
return options.path ?? defaultCredentialsPath();
183245
}

0 commit comments

Comments
 (0)