mirror of https://github.com/garrytan/gstack.git
fix(browse): grant Windows ACLs by SID to avoid owner lockout
restrictFilePermissions/restrictDirectoryPermissions granted icacls permissions by bare username (os.userInfo().username). On machines where the username collides with another account object — most commonly when the machine name equals the username — LookupAccountName resolves the bare name to the machine's domain SID (S-1-5-21-... with no RID), producing an ACL that denies even the owner. ~/.gstack becomes inaccessible and browse fails with EPERM on mkdir. Resolve the current user's SID via %SystemRoot%\System32\whoami.exe (absolute path — a bare 'whoami' can hit GNU coreutils' whoami from the Git for Windows PATH, which rejects /user) and grant *<SID>:(F), which is unambiguous. Fall back to USERDOMAIN\USERNAME if SID resolution fails. Note: dist/browse.exe and dist/server-node.mjs embed this module, so a dist rebuild is required for the fix to take effect in releases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
11de390be1
commit
08a5c9378e
|
|
@ -42,6 +42,42 @@ import * as os from 'os';
|
||||||
|
|
||||||
let warnedOnce = false;
|
let warnedOnce = false;
|
||||||
|
|
||||||
|
let cachedGrantee: string | undefined;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Resolve the identity to use in icacls `/grant` arguments.
|
||||||
|
*
|
||||||
|
* Granting by bare username (`os.userInfo().username`) is ambiguous:
|
||||||
|
* LookupAccountName can resolve the name to the wrong principal when it
|
||||||
|
* collides with another account object — e.g. when the machine name equals
|
||||||
|
* the username (user "alice" on machine "ALICE"), the bare name can resolve
|
||||||
|
* to the machine's *domain* SID (S-1-5-21-… with no RID), producing an ACL
|
||||||
|
* that denies even the owner.
|
||||||
|
* A SID grant (`*S-1-5-21-…-1001`) is unambiguous, so we resolve the
|
||||||
|
* current user's SID via `whoami /user` and fall back to the fully
|
||||||
|
* qualified `DOMAIN\username` (still unambiguous) only if that fails.
|
||||||
|
*/
|
||||||
|
function getWindowsGrantee(): string {
|
||||||
|
if (cachedGrantee !== undefined) return cachedGrantee;
|
||||||
|
try {
|
||||||
|
// Absolute path: a bare `whoami` can resolve to GNU coreutils' whoami
|
||||||
|
// (Git for Windows / MSYS2 put usr/bin on PATH), which rejects /user.
|
||||||
|
const whoami = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\whoami.exe`;
|
||||||
|
const out = execFileSync(whoami, ['/user', '/fo', 'csv', '/nh'], {
|
||||||
|
encoding: 'utf8',
|
||||||
|
stdio: ['ignore', 'pipe', 'ignore'],
|
||||||
|
});
|
||||||
|
const sid = out.match(/"(S-1-[0-9-]+)"/)?.[1];
|
||||||
|
if (sid) {
|
||||||
|
cachedGrantee = `*${sid}`;
|
||||||
|
return cachedGrantee;
|
||||||
|
}
|
||||||
|
} catch { /* fall through to name-based grant */ }
|
||||||
|
const user = process.env.USERNAME || os.userInfo().username;
|
||||||
|
cachedGrantee = process.env.USERDOMAIN ? `${process.env.USERDOMAIN}\\${user}` : user;
|
||||||
|
return cachedGrantee;
|
||||||
|
}
|
||||||
|
|
||||||
function warnIcaclsFailure(fsPath: string, err: unknown): void {
|
function warnIcaclsFailure(fsPath: string, err: unknown): void {
|
||||||
if (warnedOnce) return;
|
if (warnedOnce) return;
|
||||||
warnedOnce = true;
|
warnedOnce = true;
|
||||||
|
|
@ -67,7 +103,7 @@ function warnIcaclsFailure(fsPath: string, err: unknown): void {
|
||||||
export function restrictFilePermissions(filePath: string): void {
|
export function restrictFilePermissions(filePath: string): void {
|
||||||
if (process.platform === 'win32') {
|
if (process.platform === 'win32') {
|
||||||
try {
|
try {
|
||||||
const user = os.userInfo().username;
|
const user = getWindowsGrantee();
|
||||||
execFileSync(
|
execFileSync(
|
||||||
'icacls',
|
'icacls',
|
||||||
[filePath, '/inheritance:r', '/grant:r', `${user}:(F)`],
|
[filePath, '/inheritance:r', '/grant:r', `${user}:(F)`],
|
||||||
|
|
@ -97,7 +133,7 @@ export function restrictFilePermissions(filePath: string): void {
|
||||||
export function restrictDirectoryPermissions(dirPath: string): void {
|
export function restrictDirectoryPermissions(dirPath: string): void {
|
||||||
if (process.platform === 'win32') {
|
if (process.platform === 'win32') {
|
||||||
try {
|
try {
|
||||||
const user = os.userInfo().username;
|
const user = getWindowsGrantee();
|
||||||
execFileSync(
|
execFileSync(
|
||||||
'icacls',
|
'icacls',
|
||||||
[dirPath, '/inheritance:r', '/grant:r', `${user}:(OI)(CI)(F)`],
|
[dirPath, '/inheritance:r', '/grant:r', `${user}:(OI)(CI)(F)`],
|
||||||
|
|
@ -155,3 +191,10 @@ export function mkdirSecure(dirPath: string): void {
|
||||||
export function __resetWarnedForTests(): void {
|
export function __resetWarnedForTests(): void {
|
||||||
warnedOnce = false;
|
warnedOnce = false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Expose the resolved icacls grantee. Test-only.
|
||||||
|
*/
|
||||||
|
export function __getWindowsGranteeForTests(): string {
|
||||||
|
return getWindowsGrantee();
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -23,6 +23,7 @@ import {
|
||||||
appendSecureFile,
|
appendSecureFile,
|
||||||
mkdirSecure,
|
mkdirSecure,
|
||||||
__resetWarnedForTests,
|
__resetWarnedForTests,
|
||||||
|
__getWindowsGranteeForTests,
|
||||||
} from '../src/file-permissions';
|
} from '../src/file-permissions';
|
||||||
|
|
||||||
let tmpDir: string;
|
let tmpDir: string;
|
||||||
|
|
@ -36,6 +37,20 @@ afterEach(() => {
|
||||||
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch { /* best-effort */ }
|
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch { /* best-effort */ }
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('getWindowsGrantee', () => {
|
||||||
|
test('on Windows, resolves an unambiguous grantee (SID or DOMAIN\\user)', () => {
|
||||||
|
if (process.platform !== 'win32') return;
|
||||||
|
// A bare-username grant is ambiguous when the username collides with
|
||||||
|
// another account object (e.g. machine name == username), which locks
|
||||||
|
// even the owner out. The grantee must be a SID (`*S-1-...`) or, as a
|
||||||
|
// fallback, a domain-qualified name — never a bare username.
|
||||||
|
const grantee = __getWindowsGranteeForTests();
|
||||||
|
const isSid = /^\*S-1-[0-9-]+$/.test(grantee);
|
||||||
|
const isQualified = grantee.includes('\\');
|
||||||
|
expect(isSid || isQualified).toBe(true);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('restrictFilePermissions', () => {
|
describe('restrictFilePermissions', () => {
|
||||||
test('on POSIX, sets file mode to 0o600', () => {
|
test('on POSIX, sets file mode to 0o600', () => {
|
||||||
if (process.platform === 'win32') return;
|
if (process.platform === 'win32') return;
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue