Harden remote API security boundaries
This commit is contained in:
Bohdan Triapitsyn
@@ -1,21 +1,15 @@
|
||||
import { beforeEach, describe, expect, mock, test } from 'bun:test';
|
||||
|
||||
type ExecResult = { command: string; success: boolean; stdout?: string };
|
||||
|
||||
// Per-test controllable behaviour plus manual call tracking (the project's
|
||||
// tsconfig does not load bun-test's mock matcher types, so existing tests track
|
||||
// calls via plain arrays rather than `toHaveBeenCalled*`).
|
||||
let execImpl: (command: string, cwd: string) => ExecResult | Promise<ExecResult> = () => ({ command: '', success: false });
|
||||
let resolveRootImpl: (directory: string) => string | Promise<string> = (directory) => directory;
|
||||
let statusImpl: (directory: string) => { current: string } = () => ({ current: 'HEAD' });
|
||||
|
||||
const execCalls: Array<{ command: string; cwd: string }> = [];
|
||||
const resolveRootCalls: string[] = [];
|
||||
const statusCalls: string[] = [];
|
||||
|
||||
mock.module('@/lib/execCommands', () => ({
|
||||
execCommand: (command: string, cwd: string) => {
|
||||
execCalls.push({ command, cwd });
|
||||
return Promise.resolve(execImpl(command, cwd));
|
||||
},
|
||||
execCommands: () => Promise.resolve({ success: false, results: [] }),
|
||||
}));
|
||||
|
||||
@@ -24,28 +18,25 @@ mock.module('@/lib/gitApi', () => ({
|
||||
statusCalls.push(directory);
|
||||
return Promise.resolve(statusImpl(directory));
|
||||
},
|
||||
resolveGitPrimaryRoot: (directory: string) => {
|
||||
resolveRootCalls.push(directory);
|
||||
return Promise.resolve(resolveRootImpl(directory));
|
||||
},
|
||||
}));
|
||||
|
||||
const { getRootBranch, invalidateResolvedProjectRootCache } = await import('./worktreeStatus');
|
||||
|
||||
// Helper: a single `git rev-parse --absolute-git-dir --git-common-dir` reply.
|
||||
const revParse = (absoluteGitDir: string, commonDir: string): ExecResult => ({
|
||||
command: 'git rev-parse --absolute-git-dir --git-common-dir',
|
||||
success: true,
|
||||
stdout: `${absoluteGitDir}\n${commonDir}`,
|
||||
});
|
||||
|
||||
describe('worktreeStatus.getRootBranch', () => {
|
||||
beforeEach(() => {
|
||||
invalidateResolvedProjectRootCache();
|
||||
execCalls.length = 0;
|
||||
resolveRootCalls.length = 0;
|
||||
statusCalls.length = 0;
|
||||
execImpl = () => ({ command: '', success: false });
|
||||
resolveRootImpl = (directory) => directory;
|
||||
statusImpl = () => ({ current: 'HEAD' });
|
||||
});
|
||||
|
||||
test('derives root from absolute-git-dir and returns its branch', async () => {
|
||||
execImpl = () => revParse('/repo/.git', '.git');
|
||||
resolveRootImpl = () => '/repo';
|
||||
statusImpl = () => ({ current: 'main' });
|
||||
|
||||
expect(await getRootBranch('/repo')).toBe('main');
|
||||
@@ -53,39 +44,38 @@ describe('worktreeStatus.getRootBranch', () => {
|
||||
});
|
||||
|
||||
test('caches root resolution across repeated calls', async () => {
|
||||
execImpl = () => revParse('/repo/.git', '.git');
|
||||
resolveRootImpl = () => '/repo';
|
||||
statusImpl = () => ({ current: 'main' });
|
||||
|
||||
await getRootBranch('/repo');
|
||||
await getRootBranch('/repo');
|
||||
await getRootBranch('/repo');
|
||||
|
||||
// rev-parse runs once; the static root resolution is cached.
|
||||
expect(execCalls.length).toBe(1);
|
||||
expect(resolveRootCalls.length).toBe(1);
|
||||
});
|
||||
|
||||
test('dedupes concurrent resolutions of the same directory', async () => {
|
||||
execImpl = () => revParse('/repo/.git', '.git');
|
||||
resolveRootImpl = () => '/repo';
|
||||
statusImpl = () => ({ current: 'main' });
|
||||
|
||||
await Promise.all([getRootBranch('/repo'), getRootBranch('/repo'), getRootBranch('/repo')]);
|
||||
|
||||
expect(execCalls.length).toBe(1);
|
||||
expect(resolveRootCalls.length).toBe(1);
|
||||
});
|
||||
|
||||
test('invalidation forces re-resolution', async () => {
|
||||
execImpl = () => revParse('/repo/.git', '.git');
|
||||
resolveRootImpl = () => '/repo';
|
||||
statusImpl = () => ({ current: 'main' });
|
||||
|
||||
await getRootBranch('/repo');
|
||||
invalidateResolvedProjectRootCache('/repo');
|
||||
await getRootBranch('/repo');
|
||||
|
||||
expect(execCalls.length).toBe(2);
|
||||
expect(resolveRootCalls.length).toBe(2);
|
||||
});
|
||||
|
||||
test('falls back to the directory itself in a non-git folder', async () => {
|
||||
execImpl = () => ({ command: '', success: false });
|
||||
resolveRootImpl = (directory) => directory;
|
||||
statusImpl = () => ({ current: 'HEAD' });
|
||||
|
||||
expect(await getRootBranch('/plain')).toBe('HEAD');
|
||||
@@ -93,8 +83,7 @@ describe('worktreeStatus.getRootBranch', () => {
|
||||
});
|
||||
|
||||
test('resolves a linked worktree to its primary root and fetches that branch', async () => {
|
||||
// Worktree's own git dir lives under the primary repo's .git/worktrees.
|
||||
execImpl = () => revParse('/repo/.git/worktrees/wt', '/repo/.git');
|
||||
resolveRootImpl = () => '/repo';
|
||||
statusImpl = () => ({ current: 'main' });
|
||||
|
||||
// knownBranch is the *worktree* branch, which must NOT be returned for the root.
|
||||
@@ -103,10 +92,10 @@ describe('worktreeStatus.getRootBranch', () => {
|
||||
});
|
||||
|
||||
test('invalidation mid-flight does not let a stale resolve re-seed the cache', async () => {
|
||||
let releaseExec: (result: ExecResult) => void = () => {};
|
||||
execImpl = () =>
|
||||
new Promise<ExecResult>((resolve) => {
|
||||
releaseExec = resolve;
|
||||
let releaseResolve: (result: string) => void = () => {};
|
||||
resolveRootImpl = () =>
|
||||
new Promise<string>((resolve) => {
|
||||
releaseResolve = resolve;
|
||||
});
|
||||
statusImpl = () => ({ current: 'main' });
|
||||
|
||||
@@ -115,24 +104,24 @@ describe('worktreeStatus.getRootBranch', () => {
|
||||
// A worktree topology change invalidates the cache while the resolve runs.
|
||||
invalidateResolvedProjectRootCache();
|
||||
// Now let the original resolve settle — it must NOT populate the cache.
|
||||
releaseExec(revParse('/repo/.git', '.git'));
|
||||
releaseResolve('/repo');
|
||||
await pending;
|
||||
|
||||
execImpl = () => revParse('/repo/.git', '.git');
|
||||
resolveRootImpl = () => '/repo';
|
||||
await getRootBranch('/repo');
|
||||
|
||||
// Second call recomputes because the stale in-flight result was discarded.
|
||||
expect(execCalls.length).toBe(2);
|
||||
expect(resolveRootCalls.length).toBe(2);
|
||||
});
|
||||
|
||||
test('bounds the root cache by evicting the least-recently-used entry past the count cap', async () => {
|
||||
execImpl = (_command, cwd) => revParse(`${cwd}/.git`, '.git');
|
||||
resolveRootImpl = (directory) => directory;
|
||||
statusImpl = () => ({ current: 'main' });
|
||||
|
||||
for (let i = 0; i < 500; i += 1) {
|
||||
await getRootBranch(`/repo-${i}`);
|
||||
}
|
||||
const afterFill = execCalls.length;
|
||||
const afterFill = resolveRootCalls.length;
|
||||
expect(afterFill).toBe(500);
|
||||
|
||||
await getRootBranch('/repo-overflow');
|
||||
@@ -140,11 +129,11 @@ describe('worktreeStatus.getRootBranch', () => {
|
||||
await getRootBranch('/repo-499');
|
||||
|
||||
// /repo-overflow and evicted /repo-0 re-run; /repo-499 remains cached.
|
||||
expect(execCalls.length).toBe(afterFill + 2);
|
||||
expect(resolveRootCalls.length).toBe(afterFill + 2);
|
||||
});
|
||||
|
||||
test('uses knownBranch fast-path when the directory is its own root', async () => {
|
||||
execImpl = () => revParse('/repo/.git', '.git');
|
||||
resolveRootImpl = () => '/repo';
|
||||
|
||||
expect(await getRootBranch('/repo', { knownBranch: 'develop' })).toBe('develop');
|
||||
// No git status round-trip needed in the fast path.
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
import { getGitStatus } from '@/lib/gitApi';
|
||||
import { execCommand } from '@/lib/execCommands';
|
||||
import { getGitStatus, resolveGitPrimaryRoot } from '@/lib/gitApi';
|
||||
import type { WorktreeMetadata } from '@/types/worktree';
|
||||
|
||||
const normalizePath = (value: string): string => {
|
||||
@@ -13,39 +12,6 @@ const normalizePath = (value: string): string => {
|
||||
return replaced.replace(/\/+$/, '');
|
||||
};
|
||||
|
||||
const toAbsolutePath = (baseDir: string, maybeRelativePath: string): string => {
|
||||
const normalizedBase = normalizePath(baseDir);
|
||||
const normalizedInput = normalizePath(maybeRelativePath);
|
||||
if (!normalizedInput) return normalizedBase;
|
||||
if (normalizedInput.startsWith('/')) return normalizedInput;
|
||||
|
||||
const stack = normalizedBase.split('/').filter(Boolean);
|
||||
const parts = normalizedInput.split('/').filter(Boolean);
|
||||
for (const part of parts) {
|
||||
if (part === '.') continue;
|
||||
if (part === '..') {
|
||||
stack.pop();
|
||||
continue;
|
||||
}
|
||||
stack.push(part);
|
||||
}
|
||||
return `/${stack.join('/')}`;
|
||||
};
|
||||
|
||||
const derivePrimaryWorktreeRootFromGitDir = (gitDir: string): string | null => {
|
||||
const normalized = normalizePath(gitDir);
|
||||
if (!normalized) return null;
|
||||
if (normalized.endsWith('/.git')) {
|
||||
return normalized.slice(0, -'/.git'.length) || null;
|
||||
}
|
||||
const worktreesMarker = '/.git/worktrees/';
|
||||
const markerIndex = normalized.indexOf(worktreesMarker);
|
||||
if (markerIndex > 0) {
|
||||
return normalized.slice(0, markerIndex) || null;
|
||||
}
|
||||
return null;
|
||||
};
|
||||
|
||||
export async function getWorktreeStatus(worktreePath: string): Promise<WorktreeMetadata['status']> {
|
||||
const normalizedPath = normalizePath(worktreePath);
|
||||
const status = await getGitStatus(normalizedPath);
|
||||
@@ -118,38 +84,7 @@ export function invalidateResolvedProjectRootCache(directory?: string): void {
|
||||
}
|
||||
|
||||
const computeProjectRoot = async (directory: string): Promise<string> => {
|
||||
// A single `git rev-parse` invocation returns both paths (absolute-git-dir on
|
||||
// the first line, git-common-dir on the second), halving subprocess spawns
|
||||
// versus issuing the two queries separately. In a non-git directory the whole
|
||||
// command fails, mirroring the previous fall-through to `directory`.
|
||||
const result = await execCommand('git rev-parse --absolute-git-dir --git-common-dir', directory);
|
||||
if (!result.success) {
|
||||
return directory;
|
||||
}
|
||||
|
||||
const lines = (result.stdout || '')
|
||||
.split('\n')
|
||||
.map((line) => line.trim())
|
||||
.filter(Boolean);
|
||||
|
||||
const absoluteGitDir = normalizePath(lines[0] || '');
|
||||
if (absoluteGitDir) {
|
||||
const rootFromAbsoluteGitDir = derivePrimaryWorktreeRootFromGitDir(absoluteGitDir);
|
||||
if (rootFromAbsoluteGitDir) {
|
||||
return rootFromAbsoluteGitDir;
|
||||
}
|
||||
}
|
||||
|
||||
const rawCommonDir = normalizePath(lines[1] || '');
|
||||
if (rawCommonDir) {
|
||||
const commonDir = toAbsolutePath(directory, rawCommonDir);
|
||||
const rootFromCommonDir = derivePrimaryWorktreeRootFromGitDir(commonDir);
|
||||
if (rootFromCommonDir) {
|
||||
return rootFromCommonDir;
|
||||
}
|
||||
}
|
||||
|
||||
return directory;
|
||||
return resolveGitPrimaryRoot(directory).catch(() => directory);
|
||||
};
|
||||
|
||||
export const resolveProjectRoot = async (directory: string): Promise<string> => {
|
||||
|
||||
Reference in New Issue
Block a user