mvschwarz/openrig
1
0 Fork 0
mirror of https://github.com/mvschwarz/openrig.git synced 2026-11-07 22:58:42 -10:00
Code Issues Packages Projects Releases 1 Wiki Activity

fix(slice-21 FR-5b): check #3 file allowlist mirrors shipped decoder + Date-only daemon-start comment (OPR.0.3.2.21.FR-5)

Browse Source 
Forward-fix for the BLOCKING verdict on e5d61983
(qitem-20260602041334-55985aa9).

BLOCKER-1 — check #3 false-ok on relative paths.

Where it bit:
- The check used a locally-implemented parseAllowlistPairs that
  accepted any non-empty path including relative ones.
- allowlistPathCoversRoot called path.resolve(allowlistPath), which
  resolved a relative entry like "." into process.cwd() and
  reported coverage as ok when the actual file API rejected the
  same entry.
- Shipped decodeAllowlist at domain/files/path-safety.ts:60-80
  silently drops non-absolute pairs at line 71 before returning
  AllowlistRoot[]. The doctor was lying about file-API readiness.

Fix shape:
- workspace-doctor.ts now imports decodeAllowlist from
  domain/files/path-safety.ts and uses it as the canonical decoder
  for raw files.allowlist values. Adapts the returned
  AllowlistRoot[] (canonicalPath) to AllowlistEntry[] (path) for
  evidence-shape stability so test consumers and FR-5c daemon-
  route consumers see the same `{name, path}` shape.
- Local parseAllowlistPairs helper removed (no drift surface).
- AllowlistEntry JSDoc explicitly states path is canonical-absolute
  (post-decodeAllowlist + realpathSync fallback) so the
  parsedEntries injection contract is unambiguous.
- CheckFileAllowlistInput.parsedEntries JSDoc says raw
  named-pair strings should go through allowlistValue (not
  parsedEntries) so SettingsStore callers don't accidentally
  bypass the absolute-path gate.
- Fail message now says "no usable entries... non-absolute or
  malformed pairs are silently dropped to match the shipped file
  API" and fix-hint includes "absolute paths only" so operators
  understand the gate.
- allowlistPathCoversRoot now realpath-resolves workspaceRoot
  too (with path.resolve fallback) so coverage matches across
  macOS symlinks like /var/folders -> /private/var/folders that
  decodeAllowlist canonicalizes through.

Discriminator tests added (3):
- "returns fail when allowlist contains only relative paths" —
  workspace:. now returns fail with entryCount=0; mutation target
  is removing the path.isAbsolute gate in decodeAllowlist, which
  would let workspace:. pass through and the test fails.
- "returns fail when allowlist mixes a relative-only entry without
  absolute fallback" — workspace:relative/path/here returns fail
  with entryCount=0.
- "returns ok when allowlist mixes relative (dropped) with absolute
  (kept) entries covering root" — positive-guard so an over-eager
  fix doesn't trip whenever a relative-shape entry is present.

NON-BLOCKER cleanup — daemonStartTime comment now Date-only.

The CheckDaemonReloadInput.daemonStartTime field's JSDoc previously
said "epoch-ms or Date" but the type and implementation only
accepted Date. Updated comment to Date-only and added a hint for
FR-5c route consumers: capture once at the doctor route via
`new Date(Date.now() - process.uptime() * 1000)` and pass through.
Avoids the type-vs-comment drift surface guard called out.

Verification:
- packages/daemon tsc EXIT=0
- packages/daemon workspace-doctor.test.ts: 31/31 PASS
  (28 from FR-5a/b + 3 new BLOCKER-1 discriminators)
- Pre-commit staged-diff grep for forbidden-attribution pattern:
  ZERO matches
- Pre-commit FULL-CONTENT grep on every touched file (touch-event-
  scope rule): ZERO matches
- Commit body verification grep clean per banked discipline
- git diff --cached --stat: 2 files (+95 / -38)

Branch: slice-21-fr-5-rig-workspace-doctor off LOCAL main e154a5ce.
Chain: a4b4b67d (FR-5a, CLEAR) → e5d61983 (FR-5b, BLOCKED on this
verdict; the underlying false-ok-on-relative-paths is now closed
via this fix-up) → this commit (FR-5b fix-up + non-blocker
comment cleanup). LOCAL only, NO push.

Files:
- packages/daemon/src/domain/workspace/workspace-doctor.ts (+86 / -38)
- packages/daemon/test/workspace-doctor.test.ts (+47 / -1)
This commit is contained in:
mvschwarz committed 2026-06-01 21:17:03 -07:00
1 parent e5d61983ae
commit bc5ec05fce
2 files changed
+95 -38
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

packages/daemon/src/domain/workspace/workspace-doctor.ts
+51 -35
View File
@@ -19,6 +19,7 @@
import * as fs from "node:fs";
import * as path from "node:path";
import { decodeAllowlist } from "../files/path-safety.js";
export type CheckStatus = "ok" | "warn" | "fail";
@@ -152,8 +153,17 @@ export function checkMissionsFolder(opts: CheckMissionsFolderInput): DoctorCheck
};
}
/**
* Canonical-decoded allowlist entry as the shipped file API sees it
* (post `decodeAllowlist`): absolute path, realpath-canonicalized
* when reachable. Relative paths are silently dropped by the canonical
* decoder before they reach this shape — see check #3 for the
* usable-vs-raw entry-count semantics.
*/
export interface AllowlistEntry {
name: string;
/** Canonical absolute path (post-decodeAllowlist + realpathSync
* fallback). Equivalent to the file API's `AllowlistRoot.canonicalPath`. */
path: string;
}
@@ -163,9 +173,13 @@ export interface CheckFileAllowlistInput {
* (comma-separated `name:/abs/path` pairs, or empty string). */
allowlistValue: string;
allowlistSource: WorkspaceRootSource;
/** Pre-decoded entries; if omitted the check decodes
* allowlistValue locally with the same comma + colon parser
* ConfigStore.parseNamedPairs uses. */
/** Pre-decoded entries; if omitted the check decodes allowlistValue
* via the canonical `decodeAllowlist` from
* domain/files/path-safety.ts so the doctor reports exactly what
* the shipped file API at /api/files/* accepts. Pre-decoded
* entries MUST be in canonical form (absolute path); raw
* named-pair strings from SettingsStore should go through
* allowlistValue + the in-function decode, not parsedEntries. */
parsedEntries?: AllowlistEntry[];
}
@@ -177,20 +191,28 @@ export interface CheckFileAllowlistInput {
* env OPENRIG_FILES_ALLOWLIST, default `workspace:${workspaceRoot}`)
* is a CONFIGSTORE STRING KEY holding comma-separated `name:/abs/path`
* pairs. There is no per-workspace file. Check #3 verifies the
* resolved value parses to at least one valid entry, with a warning
* when no allowlist entry covers the workspaceRoot (the operator's
* file surface won't include workspace files).
* resolved value decodes to at least one USABLE entry (canonical,
* absolute) covering the workspaceRoot — the same usable-root
* semantics the shipped file API at /api/files/* enforces via
* `decodeAllowlist` in domain/files/path-safety.ts:60-80 (silently
* drops non-absolute paths at line 71).
*
* Decoding via the canonical decoder (not a local re-implementation)
* guarantees the doctor's verdict matches the actual file-API
* behavior — `workspace:.` or `workspace:relative-root` evaluates
* to zero usable entries here, mirroring the file API's silent skip.
*/
export function checkFileAllowlist(opts: CheckFileAllowlistInput): DoctorCheck {
const { workspaceRoot, allowlistValue, allowlistSource } = opts;
const entries = opts.parsedEntries ?? parseAllowlistPairs(allowlistValue);
const entries: AllowlistEntry[] = opts.parsedEntries
?? decodeAllowlist(allowlistValue).map((r) => ({ name: r.name, path: r.canonicalPath }));
if (entries.length === 0) {
return {
check: "file_allowlist_sane",
status: "fail",
message: `files.allowlist resolved to no valid entries (raw='${allowlistValue}', source=${allowlistSource})`,
message: `files.allowlist resolved to no usable entries (raw='${allowlistValue}', source=${allowlistSource}); non-absolute or malformed pairs are silently dropped to match the shipped file API`,
fixHint:
"set OPENRIG_FILES_ALLOWLIST or run `rig config set files.allowlist workspace:<workspaceRoot>` so the file surface has a readable root",
"set OPENRIG_FILES_ALLOWLIST or run `rig config set files.allowlist workspace:<absoluteWorkspaceRoot>` so the file surface has a readable root (absolute paths only)",
evidence: { allowlistValue, allowlistSource, entryCount: 0 },
};
}
@@ -199,9 +221,9 @@ export function checkFileAllowlist(opts: CheckFileAllowlistInput): DoctorCheck {
return {
check: "file_allowlist_sane",
status: "warn",
message: `files.allowlist has ${entries.length} entr${entries.length === 1 ? "y" : "ies"} but none cover workspace root '${workspaceRoot}'`,
message: `files.allowlist has ${entries.length} usable entr${entries.length === 1 ? "y" : "ies"} but none cover workspace root '${workspaceRoot}'`,
fixHint:
"add a `workspace:<workspaceRoot>` entry to files.allowlist (or set OPENRIG_FILES_ALLOWLIST) so workspace files are read-allowed",
"add a `workspace:<absoluteWorkspaceRoot>` entry to files.allowlist (or set OPENRIG_FILES_ALLOWLIST) so workspace files are read-allowed",
evidence: {
allowlistValue,
allowlistSource,
@@ -214,33 +236,24 @@ export function checkFileAllowlist(opts: CheckFileAllowlistInput): DoctorCheck {
return {
check: "file_allowlist_sane",
status: "ok",
message: `files.allowlist has ${entries.length} entr${entries.length === 1 ? "y" : "ies"} covering workspace root`,
message: `files.allowlist has ${entries.length} usable entr${entries.length === 1 ? "y" : "ies"} covering workspace root`,
evidence: { allowlistValue, allowlistSource, entryCount: entries.length, entries },
};
}
/** Local-only decoder, byte-identical to ConfigStore.parseNamedPairs.
* Duplicated (rather than imported from cli) to keep this domain
* module daemon-package-only with no cli cross-package import. */
function parseAllowlistPairs(raw: string): AllowlistEntry[] {
if (!raw || !raw.trim()) return [];
const out = new Map<string, string>();
for (const pair of raw.split(",")) {
const trimmed = pair.trim();
if (!trimmed) continue;
const colon = trimmed.indexOf(":");
if (colon === -1) continue;
const name = trimmed.slice(0, colon).trim();
const rawPath = trimmed.slice(colon + 1).trim();
if (!name || !rawPath) continue;
out.set(name, rawPath);
}
return Array.from(out.entries()).map(([name, p]) => ({ name, path: p }));
}
function allowlistPathCoversRoot(allowlistPath: string, workspaceRoot: string): boolean {
const normEntry = path.resolve(allowlistPath);
const normRoot = path.resolve(workspaceRoot);
// Canonical entry paths from decodeAllowlist are already absolute
// + realpath-resolved. workspaceRoot we resolve + try-to-realpath
// here so callers can pass an un-canonical workspace path and
// coverage still matches when the entry was realpath'd through a
// symlink (e.g. macOS `/var/folders/...` → `/private/var/...`).
const normEntry = allowlistPath;
let normRoot: string;
try {
normRoot = fs.realpathSync(workspaceRoot);
} catch {
normRoot = path.resolve(workspaceRoot);
}
if (normEntry === normRoot) return true;
const rel = path.relative(normEntry, normRoot);
return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel));
@@ -288,8 +301,11 @@ export function checkDaemonWorkspace(opts: CheckDaemonWorkspaceInput): DoctorChe
export interface CheckDaemonReloadInput {
/** Path to the ConfigStore config file on disk. */
configFilePath: string;
/** Daemon process start time (epoch-ms or Date). Compared to config
* file mtime; mtime > startTime → stale daemon. */
/** Daemon process start time as a Date. Callers typically capture
* this once at daemon-startup (e.g.
* `new Date(Date.now() - process.uptime() * 1000)` at the doctor
* route handler) and pass it through. Compared to config-file
* mtime; mtime > startTime → stale daemon. */
daemonStartTime: Date;
}
packages/daemon/test/workspace-doctor.test.ts
+44 -3
View File
@@ -213,14 +213,55 @@ describe("FR-5 check #3 — file allowlist sane", () => {
});
// Pre-decoded entries path: pre-parsed entries should bypass the
// local parser and be honored verbatim. Discriminator: pass an
// entry shape that the parser couldn't have produced.
// canonical decoder and be honored verbatim. Discriminator: pass an
// entry shape that the decoder couldn't have produced.
it("uses pre-decoded entries when provided", () => {
const result = checkFileAllowlist({
workspaceRoot: dir,
allowlistValue: "raw-value-different-from-entries",
allowlistSource: "default",
parsedEntries: [{ name: "ws", path: dir }],
parsedEntries: [{ name: "ws", path: fs.realpathSync(dir) }],
});
expect(result.status).toBe("ok");
expect(result.evidence?.entryCount).toBe(1);
});
// GUARD BLOCKER-1 discriminator (qitem-20260602041334-55985aa9):
// a relative path like `workspace:.` must NOT silently resolve into
// process.cwd() and return ok. The shipped file API's decodeAllowlist
// silently drops non-absolute paths at path-safety.ts:71; the doctor
// must mirror that or it lies about the workspace being read-ready.
it("returns fail when allowlist contains only relative paths (mirrors shipped file API drop)", () => {
const result = checkFileAllowlist({
workspaceRoot: dir,
allowlistValue: "workspace:.",
allowlistSource: "file",
});
expect(result.status).toBe("fail");
expect(result.message).toContain("no usable entries");
expect(result.fixHint).toContain("absolute paths only");
expect(result.evidence?.entryCount).toBe(0);
});
it("returns fail when allowlist mixes a relative-only entry without absolute fallback", () => {
const result = checkFileAllowlist({
workspaceRoot: dir,
allowlistValue: "workspace:relative/path/here",
allowlistSource: "env",
});
expect(result.status).toBe("fail");
expect(result.evidence?.entryCount).toBe(0);
});
// Positive-guard discriminator: a malformed relative entry should
// be dropped but a valid absolute companion should still produce ok.
// This protects against an over-eager fix where relative-presence
// causes the whole check to fail.
it("returns ok when allowlist mixes relative (dropped) with absolute (kept) entries covering root", () => {
const result = checkFileAllowlist({
workspaceRoot: dir,
allowlistValue: `bad:./relative,workspace:${dir}`,
allowlistSource: "env",
});
expect(result.status).toBe("ok");
expect(result.evidence?.entryCount).toBe(1);