mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-03 05:59:43 +02:00
fix(remote): authorize the attach wake first; tell the caller what happened to its bytes
Review round 3 on #439. - The attachRemoteSession branch of POST /api/sessions ran `ensureHostAwake` before the multi-user gates, so a non-admin could have any configured host's `wakeCommand` spawned (or a packet broadcast) and the request held for the wake budget, then be refused for the workingDir. The admin gate now comes first, before the host is even looked up; remote hosts are admin-only infrastructure everywhere else. Route test: wake spy empty, 403. - The non-wait input route answers `{buffered:true}` when the registry took the chunk and `{buffered:true, dropped:true}` when it was over the cap and is gone (`RemoteInputOutcome` gains 'dropped'); additive to the bare `{}`. - The send-and-wait path answers OPERATION_FAILED when the host never comes back, like create and attach, instead of writing into the stalled pane and reporting delivered:true plus a timeout. - The flush writes with `fromUser: true`, so a first prompt buffered through a wake can still name the tab. Docs: api-reference (input route), remote-sessions.md (two invariants), CLAUDE.md key pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QdGP4jUTjc9J2RYYykDrCG
This commit is contained in:
co-authored by
Claude Opus 5
parent
1040f6c489
commit
5bb489addb
+18
-12
@@ -81,7 +81,7 @@ export type RemoteInputAction = 'deliver' | 'probe' | 'buffer';
|
||||
* The caller-facing outcome of {@link RemoteWakeRegistry.handleInput}: either the
|
||||
* caller writes the bytes as usual, or the registry took ownership of them.
|
||||
*/
|
||||
export type RemoteInputOutcome = 'deliver' | 'buffered';
|
||||
export type RemoteInputOutcome = 'deliver' | 'buffered' | 'dropped';
|
||||
|
||||
/**
|
||||
* Decide what to do with an input chunk on an input route. Mirrors
|
||||
@@ -240,8 +240,8 @@ export interface WakeableSession {
|
||||
readonly remote: WakeableRemote | undefined;
|
||||
/** COD-108 reattach: respawns the local ssh pane, idempotently attaching the durable remote tmux. */
|
||||
reattachRemote(): Promise<boolean>;
|
||||
/** Write bytes to the session's pane. */
|
||||
writeViaMux(data: string): Promise<boolean>;
|
||||
/** Write bytes to the session's pane. `fromUser` marks input a person typed (or an agent sent for them). */
|
||||
writeViaMux(data: string, options?: { fromUser?: boolean }): Promise<boolean>;
|
||||
}
|
||||
|
||||
/** Injected IO so the registry holds no direct dependency on ssh/net/child_process in tests. */
|
||||
@@ -442,12 +442,12 @@ export class RemoteWakeRegistry {
|
||||
|
||||
if (action === 'deliver') return 'deliver';
|
||||
if (action === 'buffer') {
|
||||
this._enqueue(session.id, data);
|
||||
const queued = this._enqueue(session.id, data);
|
||||
// A buffered verdict with no wake in flight still has to DRIVE a wake (the
|
||||
// previous one failed and reset the probe state, or the ladder landed here
|
||||
// directly) — otherwise the bytes would sit in the buffer forever.
|
||||
if (state.waking == null && target) void this.wake(session);
|
||||
return 'buffered';
|
||||
return queued;
|
||||
}
|
||||
|
||||
// action === 'probe' — the throttle window elapsed, so one TCP connect is owed.
|
||||
@@ -455,9 +455,9 @@ export class RemoteWakeRegistry {
|
||||
state.reachable = remote ? await this.deps.probe(remote) : true;
|
||||
if (state.reachable) return 'deliver';
|
||||
|
||||
this._enqueue(session.id, data);
|
||||
const queued = this._enqueue(session.id, data);
|
||||
void this.wake(session);
|
||||
return 'buffered';
|
||||
return queued;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -749,17 +749,19 @@ export class RemoteWakeRegistry {
|
||||
return state.resolvedRemote ?? session.remote;
|
||||
}
|
||||
|
||||
private _enqueue(sessionId: string, data: string): void {
|
||||
/** Queue a chunk; `'dropped'` when it was over the cap and never entered the buffer. */
|
||||
private _enqueue(sessionId: string, data: string): 'buffered' | 'dropped' {
|
||||
const state = this._state(sessionId);
|
||||
const next = appendBoundedPending(state.pending, data);
|
||||
if (next === state.pending) {
|
||||
// Oversized chunk: dropped whole (see `appendBoundedPending`), so the buffer is
|
||||
// untouched and nothing is delivered as a fragment. Logged because the user's
|
||||
// paste is gone — the 200 the route returns cannot say so.
|
||||
// untouched and nothing is delivered as a fragment. Logged, and reported to the
|
||||
// route, which answers `dropped:true` — the user's paste is gone and a bare 200
|
||||
// could not say so.
|
||||
this.deps.log?.(
|
||||
`[RemoteWake] dropped a ${Buffer.byteLength(data)}-byte input chunk for session ${sessionId} — over the ${REMOTE_WAKE_PENDING_MAX_BYTES}-byte wake buffer, and a truncated paste must not be delivered as a fragment`
|
||||
);
|
||||
return;
|
||||
return 'dropped';
|
||||
}
|
||||
const before = state.pending.reduce((sum, chunk) => sum + Buffer.byteLength(chunk), 0);
|
||||
const after = next.reduce((sum, chunk) => sum + Buffer.byteLength(chunk), 0);
|
||||
@@ -767,6 +769,7 @@ export class RemoteWakeRegistry {
|
||||
this.deps.log?.(`[RemoteWake] pending buffer cap reached for session ${sessionId} — oldest input dropped`);
|
||||
}
|
||||
state.pending = next;
|
||||
return 'buffered';
|
||||
}
|
||||
|
||||
private async _flush(state: WakeState, session: WakeableSession): Promise<void> {
|
||||
@@ -779,7 +782,10 @@ export class RemoteWakeRegistry {
|
||||
// afterwards removed the NEXT chunk instead, so the drop-oldest bookkeeping lost a
|
||||
// chunk that was never written while the log line blamed the one that was.
|
||||
state.pending = state.pending.slice(1);
|
||||
const ok = await session.writeViaMux(chunk).catch(() => false);
|
||||
// `fromUser`: these bytes came through the input route as a person's prompt, so
|
||||
// they may name the tab — without it a session whose FIRST prompt was buffered
|
||||
// through a wake could never be auto-named.
|
||||
const ok = await session.writeViaMux(chunk, { fromUser: true }).catch(() => false);
|
||||
if (!ok) {
|
||||
// Drop the rest, and say so. Retaining it looked safer but was worse: the wake
|
||||
// still resolves and marks the host reachable, so the NEXT input takes the
|
||||
|
||||
@@ -888,6 +888,15 @@ export function registerSessionRoutes(
|
||||
// creation (owned durable sessions) is handled by the dedicated case-create
|
||||
// endpoint below, which #145 consolidated remote-host resolution into.
|
||||
if (body.attachRemoteSession) {
|
||||
// Remote hosts are admin-only infrastructure everywhere else (the list answers
|
||||
// `[]` to a non-admin; write and discovery routes are `adminOnly`), and the wake
|
||||
// below spawns the host's `wakeCommand` or broadcasts a packet. So the gate comes
|
||||
// FIRST — before the host is even looked up — or an unprivileged account could
|
||||
// invoke that executable for any configured `hostId` and only then be told the
|
||||
// workingDir was outside its workspace (reproduced upstream: wake spy fired, 403).
|
||||
if (isMultiUserMode() && !isAdmin(req)) {
|
||||
return createErrorResponse(ApiErrorCode.FORBIDDEN, 'Remote hosts are admin-only in multi-user mode');
|
||||
}
|
||||
const { hostId, remoteSessionName } = body.attachRemoteSession;
|
||||
const host = (await readRemoteHosts(CODEMAN_CONFIG_DIR)).find((item) => item.id === hostId);
|
||||
if (!host) return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Remote host not found');
|
||||
@@ -1645,12 +1654,26 @@ export function registerSessionRoutes(
|
||||
if (wantsWait) {
|
||||
// Send-and-wait keeps the response open anyway, so blocking on the wake is
|
||||
// simpler and more correct than buffering (buffering would break the wait).
|
||||
await remoteWake.ensureAwake(session);
|
||||
} else if ((await remoteWake.handleInput(session, inputStr)) === 'buffered') {
|
||||
// A host that never comes back is an error here, as on the create/attach
|
||||
// paths: writing into the stalled pane would answer `delivered:true` plus a
|
||||
// timeout, which is the combination the API docs send callers to the wrong
|
||||
// recovery for.
|
||||
if (!(await remoteWake.ensureAwake(session))) {
|
||||
return createErrorResponse(
|
||||
ApiErrorCode.OPERATION_FAILED,
|
||||
`${session.remote?.label ?? 'the remote host'} did not come back after a wake-on-LAN request — nothing was sent`
|
||||
);
|
||||
}
|
||||
} else {
|
||||
const outcome = await remoteWake.handleInput(session, inputStr);
|
||||
// The registry holds the bytes and flushes them in order once the pane is
|
||||
// reattached. The client's ACK is this 200 — a tagged retry is deduped
|
||||
// (`shouldApplyInput` above already consumed the seq), so nothing is lost.
|
||||
return {};
|
||||
// `buffered` is additive to the historical bare `{}`; `dropped` says the chunk
|
||||
// was over the wake buffer's cap and is GONE (a 200 with no field could not
|
||||
// tell delivered from buffered from dropped).
|
||||
if (outcome === 'buffered') return { buffered: true };
|
||||
if (outcome === 'dropped') return { buffered: true, dropped: true };
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user