feat(plugin): add authenticated daemon lifecycle policy - #50
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const withinStore = resolved.startsWith(`${bunStoreReal}${path.sep}`); | ||
| const expectedTail = path.join("node_modules", ...segments); | ||
| if (!withinStore || !resolved.endsWith(`${path.sep}${expectedTail}`.replace(/^\//, path.sep))) { | ||
| if (!withinStore || !resolved.endsWith(expectedTail)) return null; | ||
| } |
There was a problem hiding this comment.
Correctness/security: resolveBunLink's fallback boundary check allows a non-canonical directory to pass certification.
const withinStore = resolved.startsWith(`${bunStoreReal}${path.sep}`);
const expectedTail = path.join("node_modules", ...segments);
if (!withinStore || !resolved.endsWith(`${path.sep}${expectedTail}`.replace(/^\//, path.sep))) {
if (!withinStore || !resolved.endsWith(expectedTail)) return null;
}
return { ok: true, layout: "bun_physical_link", packageDir: resolved };The outer check correctly requires a path-separator boundary before node_modules (${path.sep}${expectedTail}). But the inner fallback drops that boundary and checks resolved.endsWith(expectedTail) (no leading separator). Since expectedTail starts with the literal text "node_modules", any resolved path ending in .../somenode_modules/@pkg/... — i.e., a sibling directory whose name merely ends with node_modules, with no path-separator immediately before it — satisfies the inner endsWith check too, and the function still returns ok: true.
Concretely: if withinStore is true but the first (correct) check fails only because of the missing-separator issue, execution falls into the inner if, where !withinStore is false and !resolved.endsWith(expectedTail) is also false (loose match succeeds) — so the whole condition is false and the code does not return null; it falls through to return { ok: true, ... }. A directory literally named xnode_modules (or any prefix + node_modules) placed inside the trusted Bun store would be certified as bun_physical_link and its contents trusted for native-launcher execution, even though it isn't a real node_modules directory.
This is exactly the kind of boundary this function exists to enforce (see resolvePayloadPackageDir's doc comment: "No importer, cwd, global store, or unrelated ancestor is ever consulted"). It isn't covered by bootstrap.test.ts's existing Bun-link tests (only the canonical-layout happy path and a withinStore === false cross-install case are tested).
The .replace(/^\//, path.sep) on the same line is also dead code — it never changes anything on either POSIX (path.sep === "/", no-op replace) or Windows (the string never starts with / there).
Suggested fix: drop the inner fallback entirely and require the single strict check (withinStore && resolved.endsWith(${path.sep}${expectedTail})), or explicitly use path.sep-aware segment comparison instead of raw endsWith.
| const withinStore = resolved.startsWith(`${bunStoreReal}${path.sep}`); | ||
| const expectedTail = path.join("node_modules", ...segments); | ||
| if (!withinStore || !resolved.endsWith(`${path.sep}${expectedTail}`.replace(/^\//, path.sep))) { | ||
| if (!withinStore || !resolved.endsWith(expectedTail)) return null; | ||
| } |
There was a problem hiding this comment.
Bug: path-boundary check can be satisfied by a directory that merely ends in node_modules/<pkg>, without a preceding path separator.
The outer check correctly requires resolved.endsWith(sep + expectedTail). But when that fails, the fallback inner check drops the separator:
if (!withinStore || !resolved.endsWith(expectedTail)) return null;expectedTail is "node_modules/@pkg" with no leading separator, so String.prototype.endsWith will happily match a suffix like .../evilnode_modules/@pkg — no path-segment boundary is enforced. Since withinStore only checks that resolved is somewhere under the Bun store (a prefix check, not an exact-segment check), a maliciously/accidentally named sibling directory inside the store (e.g. xnode_modules/<pkg>) would be misclassified as a certified bun_physical_link layout — exactly the "certified physical layout" trust check this function exists to enforce.
Suggest computing the separator-prefixed tail once and using a single check:
const expectedTail = path.sep + path.join("node_modules", ...segments);
if (!withinStore || !resolved.endsWith(expectedTail)) return null;Worth adding a test case for a store-internal path that shares the node_modules/<pkg> suffix without the segment boundary — bootstrap.test.ts currently only covers a fully-foreign, out-of-store symlink.
| * contract's half-open supported daemon range. Publication metadata must | ||
| * never be passed here — only the handshake-retained value. | ||
| */ | ||
| export function evaluateDaemonCompatibility(authenticatedDaemonVer: string): CompatibilityVerdict { |
There was a problem hiding this comment.
The doc comment above says "Publication metadata must never be passed here — only the handshake-retained value," but that's enforced only by convention/naming, not by the type system: authenticatedDaemonVer is a bare string, and both AuthenticatedPeer.daemonVer and PublicationDiagnostics.daemonVer (types.ts) are also plain string fields with no nominal distinction.
A future caller can write evaluateCompatibility({ authenticatedDaemonVer: client.publication.daemonVer, ... }) — untrusted connection-file metadata — and it will type-check and compile cleanly, silently defeating the fencing guarantee this module's comments describe. Since this security property is exactly what the module exists to enforce, consider branding the authenticated value (e.g. a type AuthenticatedDaemonVer = string & { readonly __authenticated: unique symbol }, or accepting AuthenticatedPeer directly instead of unwrapping to a bare string) so the compiler — not just a comment — prevents publication data from reaching this path.
| this.native = NativeChannel.attach(this.options.descriptor as NativeDescriptor); | ||
| } | ||
| return { daemonVer: "shared-memory-test" }; | ||
| return { daemonVer: "shared-memory-test", daemonId: null }; |
There was a problem hiding this comment.
Correctness: the shared-memory SetupFrameChannel returns a placeholder identity that would flip McHostClient.authenticated away from the real handshake identity.
return { daemonVer: "shared-memory-test", daemonId: null };This PR adds McHostClient.authenticated (client.ts, the get authenticated() getter) and documents it as the value "Lifecycle policy must use ... never publication, for compatibility and fencing." That getter reads active.generation.daemonVer / authenticatedDaemonId straight from the currently active ConnectionGeneration.
But a TCP→shared-memory re-upgrade (client.ts's shadowAttempt/prepareCandidate path, R9-R11) constructs a new ConnectionGeneration whose channel.start() is this ShmFrameChannel.start(). Once that generation becomes active, connection.ts sets this.daemonVer = result.daemonVer and this.authenticatedDaemonId = result.daemonId from this hardcoded return value — i.e. daemonVer becomes the literal string "shared-memory-test" and daemonId becomes null, discarding the real value obtained during the original TCP handshake.
Concretely, the new evaluateDaemonCompatibility() (compatibility.ts) requires daemonVer to match mc-host/X.Y.Z; "shared-memory-test" does not, so any consumer that calls evaluateDaemonCompatibility(client.authenticated.daemonVer) after an shm upgrade would incorrectly report incompatible_daemon even though the daemon is healthy and was correctly authenticated moments earlier over TCP.
Today this is reachable only via the explicit test-profile shm provider (createExplicitShmTestProvider, gated on QUALIFIED_TEST_PROFILE), so it isn't hit in default production traffic yet — but it's the only existing implementation of the SetupFrameChannel interface for shm, nothing in the type system forces a future real implementation to carry the identity forward, and no test in client.test.ts's new "authenticated state retention" suite exercises client.authenticated across a transport re-upgrade.
| return value as string[]; | ||
| } | ||
|
|
||
| function parseCatalogResponse(parsed: Record<string, unknown>): CatalogSnapshot { |
There was a problem hiding this comment.
parseCatalogResponse hand-rolls the same closed-key-set check twice inline (here and again at the module-entry level a few lines down) instead of reusing mc-host-lifecycle/contract.ts's requireExactKeys(record, expected, what), which already implements this exact sort-and-compare pattern and is used 4x in that file. Both are new in this PR, so it's a same-PR duplication rather than pre-existing.
Also, a few lines up: OP_NAME_PATTERN = /^[a-z][a-z0-9._-]{0,63}$/ (line 1979) is a byte-for-byte structural clone of TRANSPORT_NAME_RE in transport-negotiation.ts:422 (same grammar, only the length cap differs) — same drift risk if the identifier grammar changes later and only one copy gets updated.
Consider hoisting a shared requireExactKeys/identifierPattern helper both modules import, so a future change to the validation semantics (e.g. reporting which key is unexpected) only needs one edit.
| export function redactLifecyclePath(value: string, sensitiveRoots: string[]): string { | ||
| let redacted = value; | ||
| for (const root of sensitiveRoots) { | ||
| if (redacted.startsWith(root)) { | ||
| redacted = `<data-root>${redacted.slice(root.length)}`; | ||
| } | ||
| } | ||
| return redacted; | ||
| } |
There was a problem hiding this comment.
Correctness: redactLifecyclePath matches on raw prefix, with no separator-boundary check, so it can both under- and over-redact.
export function redactLifecyclePath(value: string, sensitiveRoots: string[]): string {
let redacted = value;
for (const root of sensitiveRoots) {
if (redacted.startsWith(root)) {
redacted = `<data-root>${redacted.slice(root.length)}`;
}
}
return redacted;
}sensitiveRootsFor can yield e.g. dataRoot = "/home/u/.local/share". A diagnostic path for an unrelated sibling directory, /home/u/.local/share-backup/secret, startsWith the root even though it is not actually under it, so it gets redacted to <data-root>-backup/secret — a path that was never inside the admitted data root is now mislabeled as if it were. The reverse direction (missed redaction) isn't triggered by a missing separator here, but the function's contract ("Renderers must replace these prefixes before any path reaches human or JSON output (R35)") implies exact subtree matching, which this doesn't provide.
paths.test.ts's only coverage ("path redaction roots (R35)") checks an exact nested path and a wholly unrelated path (/elsewhere/file), not a same-prefix sibling, so this gap is untested.
Fix: require a separator (or exact equality) right after the root, e.g. redacted === root || redacted.startsWith(${root}${path.sep}).
| child.stdout?.on("data", (chunk: Buffer) => { | ||
| stdoutLen += chunk.length; | ||
| if (stdoutLen > MAX_STDOUT_BYTES) { | ||
| timedOut = false; | ||
| child.kill("SIGKILL"); | ||
| return; | ||
| } | ||
| stdoutChunks.push(chunk); | ||
| }); |
There was a problem hiding this comment.
Cleanup/correctness: an oversized-stdout kill is indistinguishable from an unrelated crash.
child.stdout?.on("data", (chunk: Buffer) => {
stdoutLen += chunk.length;
if (stdoutLen > MAX_STDOUT_BYTES) {
timedOut = false;
child.kill("SIGKILL");
return;
}
stdoutChunks.push(chunk);
});When stdout exceeds MAX_STDOUT_BYTES, the child is SIGKILLed. Since timedOut is explicitly false, runNativeLifecycle falls through past the timeout check to collected.signal !== null, which is true (SIGKILL), so the caller sees a generic NativeLaunchError("signal_exit", ...). policy.ts's launchFailure then maps signal_exit to a bare internal_error reason.
This means a native binary that emits legitimately-shaped but slightly-too-long output (e.g. after a future format change, or a check list that grows past today's bounds) is reported identically to an unrelated native crash — the real cause (oversized output, a MAX_STDOUT_BYTES policy decision made entirely client-side) is silently discarded rather than surfaced as its own failure code (e.g. a dedicated output_too_large). There's no test in native-launcher.test.ts exercising this specific path, so the current classification isn't locked in by a test either.
| const admission = admitLifecycleFilesystem(root, this.admissionIo); | ||
| if (!admission.ok) { | ||
| const state = preNativeState(classifyPreNativeRoots(root)); | ||
| return { | ||
| ok: false, | ||
| result: localResult(command, false, state, "unsupported_filesystem"), | ||
| }; | ||
| } | ||
| return { ok: true, root }; | ||
| } | ||
|
|
||
| private async mutatingCommand(command: "start" | "stop" | "restart"): Promise<DaemonResultV1> { | ||
| const preflight = this.preflight(command); | ||
| if (!preflight.ok) return preflight.result; | ||
| const platform = checkPlatform(this.platformReaders); | ||
| if (!platform.ok) { | ||
| const state = preNativeState(classifyPreNativeRoots(preflight.root)); | ||
| return localResult(command, false, state, "unsupported_platform"); |
There was a problem hiding this comment.
Altitude/fragility: preflight() hardcodes the failure reason instead of reading it from the callee.
const admission = admitLifecycleFilesystem(root, this.admissionIo);
if (!admission.ok) {
const state = preNativeState(classifyPreNativeRoots(root));
return {
ok: false,
result: localResult(command, false, state, "unsupported_filesystem"),
};
}
...
const platform = checkPlatform(this.platformReaders);
if (!platform.ok) {
const state = preNativeState(classifyPreNativeRoots(preflight.root));
return localResult(command, false, state, "unsupported_platform");
}Both FilesystemAdmission and PlatformGate are typed as tagged unions that already carry a .reason (and .detail) describing exactly why they failed — but preflight()/mutatingCommand() never read admission.reason or platform.reason; they just hardcode the one reason string each union currently has exactly one failing variant for. This is correct only because both unions happen to have a single failure variant today. If admitLifecycleFilesystem or checkPlatform ever grows a second failure reason (e.g. splitting noexec from remote-fs, as the TODO-shaped UNSUPPORTED_FS_TYPES set in paths.ts suggests might happen), this code keeps reporting the old reason for the new case, with no compiler error and no test forcing an update, since the callee's own reason is discarded rather than threaded through.
| * contract's half-open supported daemon range. Publication metadata must | ||
| * never be passed here — only the handshake-retained value. | ||
| */ | ||
| export function evaluateDaemonCompatibility(authenticatedDaemonVer: string): CompatibilityVerdict { |
There was a problem hiding this comment.
Altitude: the authenticated-vs-publication security separation is enforced only by naming convention, not by the type system.
export function evaluateDaemonCompatibility(authenticatedDaemonVer: string): CompatibilityVerdict {The doc comment above this function (and the module doc comment) says "Publication metadata must never be passed here — only the handshake-retained value," and mc-host-client/client.ts introduces exactly this distinction with AuthenticatedPeer.daemonVer vs. PublicationDiagnostics.daemonVer. But both are plain string fields, and this function's parameter is typed as plain string. Nothing in TypeScript's structural type system stops a future caller from passing client.publication?.daemonVer (untrusted, connection-file-sourced) where client.authenticated?.daemonVer (handshake-verified) belongs — the two are interchangeable at the type level despite being described as security-critically different. Since evaluateCompatibility/evaluateDaemonCompatibility aren't wired into any caller yet in this diff, there's no test that would catch this mix-up either. Consider a nominal wrapper type (e.g. a branded AuthenticatedDaemonVer string, or accepting the whole AuthenticatedPeer) so a publication-sourced string can't type-check here.
| const stat = fstatSync(fd); | ||
| if (!stat.isFile()) throw invalid("retained bootstrap is not a regular file"); | ||
| if (stat.nlink !== 1) throw invalid("retained bootstrap is not single-link"); | ||
| if (stat.uid !== process.getuid?.()) |
There was a problem hiding this comment.
Correctness (low severity): process.getuid?.() returning undefined is misreported as a "foreign owner" rather than an unsupported-platform condition.
if (stat.uid !== process.getuid?.())
throw invalid("retained bootstrap has a foreign owner");process.getuid is undefined on non-POSIX runtimes (e.g. Windows). There, process.getuid?.() evaluates to undefined, and since stat.uid is always a number, the comparison is always true, so this function throws native_payload_invalid ("foreign owner") for every input regardless of actual ownership. This fails closed (not exploitable), but it misclassifies a platform-support gap as a corrupted/hijacked payload. checkPlatform gates Linux/macOS earlier in policy.ts's call chain, but revalidateRetainedBootstrap/stageBootstrap have no such gate themselves and could be invoked directly (as the exported, independently-tested functions they are) with a misleading error on such a runtime.
| function parseCatalogResponse(parsed: Record<string, unknown>): CatalogSnapshot { | ||
| const keys = Object.keys(parsed).sort(); | ||
| const expected = ["generation", "modules", "op", "subc_ops"]; | ||
| if (keys.length !== expected.length || keys.some((key, i) => key !== expected[i])) { | ||
| throw malformedCatalog("unexpected top-level shape"); |
There was a problem hiding this comment.
Altitude/design: catalogSnapshot's new closed-shape validation is a forward-compatibility hazard for catalog.list versioning.
const keys = Object.keys(parsed).sort();
const expected = ["generation", "modules", "op", "subc_ops"];
if (keys.length !== expected.length || keys.some((key, i) => key !== expected[i])) {
throw malformedCatalog("unexpected top-level shape");
}The previous catalogList() only checked that modules was an array (parsed.modules ?? []), tolerating unknown/missing fields. This replacement (and the identical per-module key-set check a few lines down) rejects the entire response the moment the host adds any new top-level or per-module field — a normal, otherwise backward-compatible server-side rollout pattern. Every existing client would immediately start throwing malformed_control_response on catalog.list (and lose route.open capability discovery with it) the moment a newer host ships an additive field, unless client and host releases are always deployed in lockstep. If that lockstep is intentional (schema is meant to be pinned exactly), consider stating that explicitly in the doc comment; if not, an unknown top-level/module field should be ignored rather than fatal.
| return localResult( | ||
| command, | ||
| false, | ||
| state === "stopped" ? "stopped" : "wedged", |
There was a problem hiding this comment.
Simplification: redundant ternary — state can only ever be "stopped" or "wedged" here.
case "timeout":
return localResult(
command,
false,
state === "stopped" ? "stopped" : "wedged",
command === "stop" ? "shutdown_timeout" : "startup_timeout",
);state is assigned a few lines above via preNativeState(classifyPreNativeRoots(root)), and preNativeState (contract.ts) is typed to return exactly "stopped" | "wedged" — there is no third value. state === "stopped" ? "stopped" : "wedged" is therefore always equal to state itself; the ternary can be replaced with plain state.
| const roles = record.roles; | ||
| if (!Array.isArray(roles) || roles.length > MAX_CATALOG_ROLES) { | ||
| throw malformedCatalog("roles is not a bounded array"); |
There was a problem hiding this comment.
Cleanup/consistency: roles is bounded but not shape-validated, unlike every other field in this same strict parser.
const roles = record.roles;
if (!Array.isArray(roles) || roles.length > MAX_CATALOG_ROLES) {
throw malformedCatalog("roles is not a bounded array");
}module_id, module_version, and control_ops (via requireOpArray) are all validated element-by-element with strict type/shape checks. roles only gets an array-and-length check — its elements can be anything (objects, numbers, nested arrays) and pass straight through into CatalogEntry.roles. This is consistent with the existing roles: unknown[] type, but it sits oddly next to this function's own doc comment ("Any duplicate, missing field, unknown field, or out-of-bounds value is a terminal malformed_control_response — never a cast") and the closed, strict treatment of every sibling field. If roles is meant to stay a deliberately-opaque bag, worth a one-line comment saying so; otherwise it's a gap in the "never a cast" guarantee this function otherwise provides.
|
Review summary Reviewed the new mc-host-lifecycle module and the mc-host/mc-host-client authenticated-peer/catalog changes. Overall this is careful, well-tested hardening work (retained-bootstrap revalidation, capacity preflight, and the Rust-side XDG_DATA_HOME/HOME absolute-path checks are solid). Left inline comments on the highest-confidence issues; note some of these overlap with an earlier automated pass on this PR, since two review passes ran concurrently here. Worth fixing:
Minor/cleanup:
Nice test coverage overall on the Rust-side absolute-path hardening and the new lifecycle modules happy paths - the main gap is boundary/adversarial cases for the new trust checks (resolveBunLink above being the clearest example). |
1 participant
Summary
Stack
PR 4 of 10. Base:
stack/mc-host-03-native-runtime.Validation
Post-Deploy Monitoring & Validation
Watch authentication, compatibility, native payload, and storage-readiness reason codes for one release cycle. Roll back if explicit clients start daemons or managed calls send bodies before storage readiness. Owner: plugin maintainers.