mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 10:39:32 +01:00
fix: refuse to git fetch while the Docker case list cannot be read (release review)
readDockerCases() reads an unreadable or half-written docker-cases.json as [], so a fetch (which runs repository hooks, and runs unattended on the interval) could run inside a Docker workspace. The fetch paths now read it through a strict reader: the Fetch button answers a failed run (state 'error') and the scheduler skips the tick, undoes its picks and logs once. The status and diff routes are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
b89f15c12f
commit
1752d7ee2c
8 files changed
+221
-18
No files matched your search
@@ -28,6 +28,7 @@ import {
|
||||
persistDockerCaseClaudeSessionId,
|
||||
probeDockerCliVersion,
|
||||
readDockerCases,
|
||||
readDockerCasesOrThrow,
|
||||
readDockerHosts,
|
||||
resolveClaudeJsonSeedMount,
|
||||
resolveDockerClaudeArtifacts,
|
||||
@@ -70,6 +71,28 @@ describe('docker-hosts storage', () => {
|
||||
expect(await readDockerCases(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
/**
|
||||
* The git fetch paths read the list through the strict reader: the writer truncates and rewrites the file in
|
||||
* place, and a list read as `[]` there would let a fetch run git inside a Docker workspace.
|
||||
*/
|
||||
it('has a strict reader that rejects a list it cannot parse, where the plain reader returns []', async () => {
|
||||
expect(await readDockerCasesOrThrow(dir)).toEqual([]); // missing: no cases
|
||||
await writeDockerCases(dir, [CASE]);
|
||||
expect(await readDockerCasesOrThrow(dir)).toEqual([CASE]);
|
||||
const path = join(dir, 'docker-cases.json');
|
||||
for (const text of ['', '[{"name":"myproj","hostWorkspacePath":"/home/arkon/cas', '{"name":"myproj"}', 'null']) {
|
||||
writeFileSync(path, text);
|
||||
expect(await readDockerCases(dir)).toEqual([]);
|
||||
const err = await readDockerCasesOrThrow(dir).then(
|
||||
() => null,
|
||||
(e: Error) => e
|
||||
);
|
||||
expect(err?.message).toMatch(/not a valid JSON array/);
|
||||
// The parser's message quotes the file; the strict reader's never does.
|
||||
expect(err?.message).not.toContain('/home/arkon/cas');
|
||||
}
|
||||
});
|
||||
|
||||
it('persists the last Claude conversation id keyed by container name', async () => {
|
||||
await writeDockerCases(dir, [CASE, { ...CASE, name: 'other', container: 'custom-name' }]);
|
||||
await persistDockerCaseClaudeSessionId(dir, dockerContainerName(CASE.name), 'conv-1');
|
||||
|
||||
@@ -12,7 +12,8 @@
|
||||
* Port: N/A.
|
||||
*/
|
||||
import { mkdir, rm, writeFile } from 'node:fs/promises';
|
||||
import { dirname } from 'node:path';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { dirname, resolve } from 'node:path';
|
||||
import { describe, expect, it, vi } from 'vitest';
|
||||
import {
|
||||
GitFetchScheduler,
|
||||
@@ -279,6 +280,43 @@ describe('who runs', () => {
|
||||
expect(h.calls.length).toBe(1);
|
||||
});
|
||||
|
||||
/**
|
||||
* The Docker case list is rewritten in place, so a read can land on a half-written file. Read as "no Docker
|
||||
* workspaces", a tick would run git (and the repository's hooks) inside a folder a container can write to.
|
||||
*/
|
||||
it('fetches nothing when the Docker case list cannot be read, keeps the folders due, and logs it once', async () => {
|
||||
const h = harness([VIEW('a'), VIEW('b')]);
|
||||
let unreadable = true;
|
||||
h.ports.dockerWorkspaces = async () => {
|
||||
h.dockerReads++;
|
||||
if (unreadable) throw new Error('docker-cases.json is not a valid JSON array');
|
||||
return [];
|
||||
};
|
||||
const logged = vi.fn();
|
||||
h.ports.log = logged;
|
||||
expect(await h.scheduler.tick(0)).toBe(0);
|
||||
expect(await h.scheduler.tick(15_000)).toBe(0);
|
||||
expect(h.calls).toEqual([]);
|
||||
expect(h.dockerReads).toBe(2); // the picks were undone, so the next tick picked them again
|
||||
expect(logged).toHaveBeenCalledTimes(1);
|
||||
expect(h.scheduler.tracked()).toBe(0); // nothing left in flight or stamped
|
||||
|
||||
// The list reads again: both folders run at once, not an interval after the failed ticks.
|
||||
unreadable = false;
|
||||
expect(await h.scheduler.tick(30_000)).toBe(2);
|
||||
expect(h.calls.map((c) => c.cwd).sort()).toEqual(['/w/a', '/w/b']);
|
||||
// And their interval is timed from that run.
|
||||
expect(await h.scheduler.tick(30_000 + 299_000)).toBe(0);
|
||||
expect(await h.scheduler.tick(30_000 + 300_000)).toBe(2);
|
||||
});
|
||||
|
||||
it('is wired to the strict Docker reader the Fetch button uses', () => {
|
||||
const server = readFileSync(resolve(import.meta.dirname, '../src/web/server.ts'), 'utf8');
|
||||
const block = server.slice(server.indexOf('new GitFetchScheduler('));
|
||||
// defaultDockerWorkspaces is the strict one: test/routes/git-status-routes.test.ts pins that it rejects.
|
||||
expect(block.slice(0, block.indexOf('});'))).toContain('dockerWorkspaces: defaultDockerWorkspaces,');
|
||||
});
|
||||
|
||||
it('uses the module defaults for the limit and the timeout, because those settings are per device and never reach the server', async () => {
|
||||
const h = harness([VIEW('a')]);
|
||||
await h.scheduler.tick(0);
|
||||
|
||||
@@ -9,7 +9,8 @@ import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { createRouteTestHarness } from './_route-test-utils.js';
|
||||
import { registerGitStatusRoutes } from '../../src/web/routes/git-status-routes.js';
|
||||
import { defaultDockerWorkspaces, registerGitStatusRoutes } from '../../src/web/routes/git-status-routes.js';
|
||||
import { getDataDir } from '../../src/config/instance.js';
|
||||
import { clearGitStatusCache, runGit, type GitRunner } from '../../src/git-workspace-status.js';
|
||||
import { clearGitFetchLane, fetchWorkspaceOnce } from '../../src/git-fetch.js';
|
||||
|
||||
@@ -30,14 +31,18 @@ let dir: string;
|
||||
let session: Record<string, unknown>;
|
||||
|
||||
async function setup(
|
||||
opts: { git?: GitRunner; authUser?: { username: string; role: 'admin' | 'user' }; dockerWorkspaces?: string[] } = {}
|
||||
opts: {
|
||||
git?: GitRunner;
|
||||
authUser?: { username: string; role: 'admin' | 'user' };
|
||||
/** A fixed list, a reader of the test's own, or `'real'` for the route's default (the data dir's file). */
|
||||
dockerWorkspaces?: string[] | (() => Promise<string[]>) | 'real';
|
||||
} = {}
|
||||
) {
|
||||
const h = await createRouteTestHarness(
|
||||
(app, ctx) => registerGitStatusRoutes(app, ctx, opts.git, async () => opts.dockerWorkspaces ?? []),
|
||||
{
|
||||
authUser: opts.authUser,
|
||||
}
|
||||
);
|
||||
const docker = opts.dockerWorkspaces;
|
||||
const reader = docker === 'real' ? undefined : typeof docker === 'function' ? docker : async () => docker ?? [];
|
||||
const h = await createRouteTestHarness((app, ctx) => registerGitStatusRoutes(app, ctx, opts.git, reader), {
|
||||
authUser: opts.authUser,
|
||||
});
|
||||
session = h.ctx._session as unknown as Record<string, unknown>;
|
||||
session.workingDir = dir;
|
||||
return h;
|
||||
@@ -461,6 +466,58 @@ describe('POST /api/sessions/:id/git-fetch', () => {
|
||||
expect(calls).toEqual([]);
|
||||
});
|
||||
|
||||
/**
|
||||
* docker-cases.json is rewritten in place (truncate, then write), so a read can land on an empty or
|
||||
* half-written file. Read as "no Docker workspaces", a fetch would run git, and with it the repository's
|
||||
* hooks, inside a folder a container can write to.
|
||||
*/
|
||||
it('fetches nothing when the Docker case list cannot be read, and answers a failed run', async () => {
|
||||
git(dir, 'init', '-q', '-b', 'main');
|
||||
const { calls, runner } = recordingRunner();
|
||||
const { app, ctx } = await setup({
|
||||
git: runner,
|
||||
dockerWorkspaces: async () => {
|
||||
throw new Error('docker-cases.json is not a valid JSON array');
|
||||
},
|
||||
});
|
||||
const res = await app.inject({ method: 'POST', url: '/api/sessions/test-session-1/git-fetch' });
|
||||
expect(res.statusCode).toBe(200);
|
||||
expect(res.json().data).toMatchObject({ state: 'error', repos: [] });
|
||||
expect(res.json().data.error).toMatch(/Docker case list/);
|
||||
expect(calls).toEqual([]); // not even the resolver's rev-parse
|
||||
expect(session.gitFetch).toMatchObject({ trigger: 'click', state: 'error', repos: 0, failed: 0 });
|
||||
expect(ctx.broadcast).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('reads the real Docker case list strictly for a fetch, and leaves the status route as it was', async () => {
|
||||
git(dir, 'init', '-q', '-b', 'main');
|
||||
mkdirSync(getDataDir(), { recursive: true });
|
||||
const listPath = join(getDataDir(), 'docker-cases.json');
|
||||
try {
|
||||
writeFileSync(listPath, '[{"name":"c","hostWorkspacePath":"/x"');
|
||||
await expect(defaultDockerWorkspaces()).rejects.toThrow(/not a valid JSON array/);
|
||||
const { calls, fetches, runner } = recordingRunner();
|
||||
const { app } = await setup({ git: runner, dockerWorkspaces: 'real' });
|
||||
const fetched = await app.inject({ method: 'POST', url: '/api/sessions/test-session-1/git-fetch' });
|
||||
expect(fetched.json().data.state).toBe('error');
|
||||
expect(calls).toEqual([]);
|
||||
// The read-only status route still reads an unreadable list as none, as it always has.
|
||||
const status = await app.inject({ method: 'GET', url: '/api/sessions/test-session-1/git-status' });
|
||||
expect(status.json().data.state).toBe('ok');
|
||||
expect(fetches()).toEqual([]);
|
||||
|
||||
// A readable list that names this folder refuses the fetch as a Docker workspace, so the strict
|
||||
// reader is the one the route really reads.
|
||||
writeFileSync(listPath, JSON.stringify([{ name: 'c', hostWorkspacePath: realpathSync(dir) }]));
|
||||
expect(await defaultDockerWorkspaces()).toEqual([realpathSync(dir)]);
|
||||
const refused = await app.inject({ method: 'POST', url: '/api/sessions/test-session-1/git-fetch' });
|
||||
expect(refused.json().data).toMatchObject({ state: 'unsupported', reason: 'docker' });
|
||||
expect(fetches()).toEqual([]);
|
||||
} finally {
|
||||
rmSync(listPath, { force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('answers not-a-repo for a folder that is not a repository, running no fetch', async () => {
|
||||
const { fetches, runner } = recordingRunner();
|
||||
const { app } = await setup({ git: runner });
|
||||
|
||||
Reference in new issue
Block a user