mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-04 22:49:41 +02:00
fix(files): fail closed on remote symlinks, guard PUT for remote cases, bound ssh fan-out
Follow-up to #421 (remote-case file reads over ssh), addressing the review. Symlink escape on a host without `readlink -f` (blocker). The probe's portable fallback canonicalized only the directory chain and returned the final component unresolved, so on macOS < 12.3 `ws/notes.txt -> ~/.ssh/id_rsa` came back as `.../ws/notes.txt` (with the target's size), passed every containment and blocklist check that runs on `realPath`, and `cat` followed the link. The fallback now walks the directory chain with `cd -P`/`pwd -P` and follows the LAST component with plain `readlink` for a bounded number of hops, and anything it cannot fully resolve (a loop, a readlink failure, the hop cap) is reported with an `x` marker that parses as null, i.e. 404. It never returns the unresolved string. Measured on a real /bin/sh with `readlink -f` shadowed: the pre-fix script reports `/ws/notes.txt`, the fixed one `/secret/id_rsa`; both branches (native and fallback) now agree. `PUT /api/sessions/:id/file-content` never had the remote guard the PR described. It sits ahead of `validateSessionFilePath`, which resolves against the LOCAL filesystem, because with a same-named directory on the Codeman host (an sshfs mount of the remote tree, the documented stop-gap) the write landed on the local twin while the viewer believed it edited the remote file. ssh fan-out is bounded. `src/remote-ssh-limiter.ts` is a document-conversion-limiter-shaped semaphore (default 4, env `CODEMAN_MAX_REMOTE_FILE_SSH`) around every probe and buffered read; the attachment-history list resolves its whole history in ONE batched probe (`probeRemoteAttachmentHistory`, threaded into `registerExternalAttachment({remoteProbes})` so the guards run unchanged) instead of one handshake per entry; and probes chunk at 40 paths because the whole script is one argv string. Terminal output in a remote session is written on the remote host, so a prompt-injected agent printing hundreds of `codeman://attach` links forked one ssh per link, each holding a 20 s timeout, and a 100-entry history re-listed on every attachment:detected tripped OpenSSH's default MaxStartups. Streams are deliberately not counted (one per browser request, held for a whole playback, and gated behind a counted probe anyway). Smaller items from the same review: probe records are NUL-terminated and index-keyed after a leading NUL (a newline in a filename can no longer shift the alignment, and the banner is fenced off without last-N-lines guessing); size comes from `stat -c %s || stat -f %z`; the three IO functions refuse under VITEST instead of opening a connection; an unreachable host now reads as unknown (missing: false) for detected AND external history entries, where external used to fold its 502 into missing; a client that aborted during the guard probe has its body's ssh child reaped (`reply.raw.destroyed` is checked before the close listener is attached); `describeExecError` never returns Node's `Command failed: <ssh line>` message, which carried the identity path and the probe script into a 502 body; and the docs note that `isSensitivePath`'s three home-anchored entries resolve against the Codeman host's home, not the remote one. Tests: the probe script runs on a real /bin/sh with a `readlink` shim that rejects `-f` (the escape, a relative chain through a symlinked directory, a loop, a newline filename, banner chatter that itself looks like a record), the limiter's cap and FIFO order, and route tests for the PUT guard (local twin untouched, no connection), the single batched history probe, the unreachable-host alignment and the aborted-client reap. All four route tests fail against the pre-fix file-routes.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
@@ -542,7 +542,9 @@ describe('file routes in a remote (SSH) case', () => {
|
||||
externalPath: outsidePath,
|
||||
},
|
||||
];
|
||||
mockedProbePaths.mockResolvedValue([fileProbe(outsidePath, 42), dirProbe]);
|
||||
mockedProbePaths.mockImplementation(async (_remote, paths) =>
|
||||
paths.map((path) => (path === outsidePath ? fileProbe(outsidePath, 42) : path === REMOTE_DIR ? dirProbe : null))
|
||||
);
|
||||
|
||||
const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${sessionId}/attachments` });
|
||||
|
||||
@@ -568,7 +570,15 @@ describe('file routes in a remote (SSH) case', () => {
|
||||
relativePath: 'out.png',
|
||||
},
|
||||
];
|
||||
mockedProbePaths.mockResolvedValue([fileProbe(`${REMOTE_DIR}/out.png`, 7), dirProbe]);
|
||||
mockedProbePaths.mockImplementation(async (_remote, paths) =>
|
||||
paths.map((path) =>
|
||||
path === `${REMOTE_DIR}/out.png`
|
||||
? fileProbe(`${REMOTE_DIR}/out.png`, 7)
|
||||
: path === REMOTE_DIR
|
||||
? dirProbe
|
||||
: null
|
||||
)
|
||||
);
|
||||
|
||||
const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${sessionId}/attachments` });
|
||||
|
||||
@@ -577,5 +587,150 @@ describe('file routes in a remote (SSH) case', () => {
|
||||
expect(item.size).toBe(7);
|
||||
expect(item.rawUrl).toContain('file-raw');
|
||||
});
|
||||
|
||||
describe('the history list probes the whole history in ONE ssh round trip', () => {
|
||||
// One connection per entry (up to ATTACHMENT_HISTORY_LIMIT, re-run on every
|
||||
// attachment:detected while the drawer is open) tripped OpenSSH's default
|
||||
// MaxStartups 10:30:100, which drops most of a burst that size.
|
||||
const history = () => [
|
||||
{
|
||||
id: 'hist-a',
|
||||
sessionId,
|
||||
fileName: 'out.png',
|
||||
extension: 'png',
|
||||
attachmentType: 'image' as const,
|
||||
size: 1,
|
||||
mtimeMs: 1,
|
||||
timestamp: 1,
|
||||
source: 'detected' as const,
|
||||
relativePath: 'out.png',
|
||||
},
|
||||
{
|
||||
id: 'hist-b',
|
||||
sessionId,
|
||||
fileName: 'shot.png',
|
||||
extension: 'png',
|
||||
attachmentType: 'image' as const,
|
||||
size: 1,
|
||||
mtimeMs: 1,
|
||||
timestamp: 1,
|
||||
source: 'external' as const,
|
||||
externalPath: outsidePath,
|
||||
},
|
||||
{
|
||||
id: 'hist-c',
|
||||
sessionId,
|
||||
fileName: 'gone.png',
|
||||
extension: 'png',
|
||||
attachmentType: 'image' as const,
|
||||
size: 1,
|
||||
mtimeMs: 1,
|
||||
timestamp: 1,
|
||||
source: 'detected' as const,
|
||||
relativePath: 'gone.png',
|
||||
},
|
||||
];
|
||||
|
||||
it('issues a single batched probe covering every entry plus the workspace root', async () => {
|
||||
harness.ctx._session.attachmentHistory = history();
|
||||
mockedProbePaths.mockImplementation(async (_remote, paths) =>
|
||||
paths.map((path) =>
|
||||
path === `${REMOTE_DIR}/out.png`
|
||||
? fileProbe(path, 7)
|
||||
: path === outsidePath
|
||||
? fileProbe(outsidePath, 42)
|
||||
: path === REMOTE_DIR
|
||||
? dirProbe
|
||||
: null
|
||||
)
|
||||
);
|
||||
|
||||
const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${sessionId}/attachments` });
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
expect(mockedProbePaths).toHaveBeenCalledTimes(1);
|
||||
const [, probed] = mockedProbePaths.mock.calls[0];
|
||||
expect([...probed].sort()).toEqual(
|
||||
[REMOTE_DIR, `${REMOTE_DIR}/gone.png`, `${REMOTE_DIR}/out.png`, outsidePath].sort()
|
||||
);
|
||||
// (ids are re-minted for external entries by the sanitizer, so key on the name)
|
||||
const items = JSON.parse(res.body).data.items as Array<{ fileName: string; missing: boolean; size: number }>;
|
||||
expect(items.map((item) => [item.fileName, item.missing, item.size])).toEqual([
|
||||
['out.png', false, 7],
|
||||
['shot.png', false, 42],
|
||||
['gone.png', true, 1],
|
||||
]);
|
||||
});
|
||||
|
||||
it('reports every entry as unknown (missing: false), detected AND external alike, when the host is unreachable', async () => {
|
||||
harness.ctx._session.attachmentHistory = history();
|
||||
mockedProbePaths.mockRejectedValue(new RemoteFileAccessError('remote host testhost unreachable: timed out'));
|
||||
|
||||
const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${sessionId}/attachments` });
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const items = JSON.parse(res.body).data.items as Array<{ id: string; missing: boolean }>;
|
||||
// The two branches used to disagree here: detected kept missing:false while
|
||||
// external's 502 was folded into missing:true.
|
||||
expect(items.map((item) => item.missing)).toEqual([false, false, false]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('PUT /api/sessions/:id/file-content', () => {
|
||||
// The remote guard has to come BEFORE the local path validation: with a
|
||||
// directory of the same absolute name on this host (an sshfs mount of the remote
|
||||
// tree, the documented stop-gap for #415) the write would land on the local twin
|
||||
// while the viewer believes it edited the remote file.
|
||||
let shadowRoot: string;
|
||||
let shadowFile: string;
|
||||
|
||||
beforeEach(() => {
|
||||
shadowRoot = mkdtempSync(join(tmpdir(), 'codeman-remote-put-'));
|
||||
shadowFile = join(shadowRoot, 'notes.txt');
|
||||
writeFileSync(shadowFile, 'LOCAL TEXT');
|
||||
harness.ctx._session.workingDir = shadowRoot;
|
||||
harness.ctx._session.remote = { ...remote, remotePath: shadowRoot };
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(shadowRoot, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('answers 400 for a remote case and never touches the local file of the same name', async () => {
|
||||
const res = await harness.app.inject({
|
||||
method: 'PUT',
|
||||
url: `/api/sessions/${sessionId}/file-content`,
|
||||
payload: { path: 'notes.txt', content: 'OVERWRITTEN', baseHash: 'whatever', force: true },
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(400);
|
||||
expect(JSON.parse(res.body).error).toMatch(/not supported for files in a remote/);
|
||||
expect(readFileSync(shadowFile, 'utf8')).toBe('LOCAL TEXT');
|
||||
expect(mockedProbePaths).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('a client that gives up during the guard probe', () => {
|
||||
it('still has its ssh body child reaped', async () => {
|
||||
// The probe is an ssh round trip; a client that aborted during it has already
|
||||
// closed the response, so a `close` listener attached afterwards never fires.
|
||||
const controller = new AbortController();
|
||||
mockedProbePaths.mockImplementation(async () => {
|
||||
controller.abort();
|
||||
await new Promise((resolveDelay) => setTimeout(resolveDelay, 20));
|
||||
return [fileProbe(`${REMOTE_DIR}/img.png`, 9), dirProbe];
|
||||
});
|
||||
|
||||
await harness.app
|
||||
.inject({ method: 'GET', url: `/api/sessions/${sessionId}/file-raw?path=img.png`, signal: controller.signal })
|
||||
.catch(() => undefined);
|
||||
await new Promise((resolveDelay) => setTimeout(resolveDelay, 50));
|
||||
|
||||
// The body WAS opened (the route ran to completion against an already-closed
|
||||
// response), which is exactly the window the guard covers.
|
||||
expect(mockedCreateReadStream).toHaveBeenCalledTimes(1);
|
||||
expect(closeSpy).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user