fix(autonomy): add deny-by-default --credential-roots allowlist

[kyle-sexton]

Summary

The security-binding checker (plugins/autonomy/skills/setup/scripts/check-security-binding.mjs) recognized a probed host-credential path by static structural shape — exact home-anchored / fixed-system forms. A static checker cannot know an org's real credential locations, so for any such recognizer an adversarial reviewer can always craft a plausible-but-invented path (an invented home user, a mount that need not exist) whose failing read proves nothing while real host credentials go unrecognized. That is the non-convergence #549 documents.

This implements the operator-ratified Option A (deny-by-default; issue comment): a new --credential-roots <path,path,...> flag mirroring the already-landed --egress-hosts seam, with unconfigured default = deny-all. Credential side only — the egress seam (--egress-hosts, isNonExternalEgressHost, special-use ranges) is untouched.

Fix

• New --credential-roots flag, parsed exactly like --egress-hosts (comma-separated, trimmed; roots are not case-folded at parse time — normalizeHostPath folds case consistently at containment time, since the checker never touches the probing host's filesystem and POSIX case-sensitivity is not observable here).
• Deny-by-default containment replaces the structural recognizer. A filesystem credential entry counts as credential-absence evidence only when its recorded host-side expansion resolves — lexically, ..-safe, filesystem-independent (normalizeHostPath / pathUnderConfiguredRoot) — under one of the configured trusted roots. With no roots configured, every filesystem credential entry is untrusted and the level fails closed. Membership under a configured root is the sole test, dissolving the open-ended enumeration.
• Bounded closed sets kept, needing no allowlist: cloud-metadata-endpoint routes (isMetadataEndpoint) and single-segment well-known credential env tokens ($GITHUB_TOKEN, $GH_TOKEN). The expansion-coherence guard (credentialExpansionProblem: rooted, non-UNC, ephemeral-free, tail-consistent) is retained and now runs for every entry kind, so a concrete entry's host_expanded must repeat it verbatim.
• Segment-boundary containment (never a bare prefix — /home/run does not accept /home/runner-x); UNC / //-prefixed forms are refused so //etc/credentials cannot collapse onto a configured /etc root.
• Docs updated to the enforced behavior (no false guarantees): SKILL.md and templates/isolation-probe.md describe credential = deny-by-default while leaving the egress seam's accurate allowlist-with-fallback description intact; CHANGELOG.md + plugin.json bumped 0.8.0 → 0.9.0.
• Fixtures: pruned the corpus that tested the removed structural discrimination (which files/depths/anchors count); kept invented-home-user, host-home-mount, and dot-traversal as not-under-configured-root regressions; added unconfigured-roots (fail-closed), UNC-rejection, and metadata-expansion-mismatch regressions.

An independent security review (fresh-context subagent, rationale withheld) surfaced a UNC-collapse containment bypass and a concrete-entry coherence gap; both are fixed in the second commit with regression fixtures.

Verification

plugins/autonomy/skills/setup/scripts/check-security-binding.fixtures.test.sh (the repo's graded-fixture harness):

All 357 checks passed (100 fixtures graded, 0 quarantined).

Baseline before the change was All 394 checks passed (109 fixtures graded); the delta is the pruned structural-discrimination corpus (−12 fixtures + transcripts) plus 3 new deny-by-default / hardening fixtures. North-star behavior verified directly against the checker:

• (a) unconfigured — real $HOME/.ssh/id_rsa → /home/runner/.ssh/id_rsa with no --credential-roots ⇒ UNPROVEN, level fails closed (no --credential-roots configured).
• (b) under root — same path with --credential-roots /home/runner ⇒ Security binding OK (C1–C4 eligible).
• (c) not under root — /home/definitely-not-a-host-user/.ssh/id_rsa with --credential-roots /home/runner ⇒ does not resolve under any configured --credential-roots.

No egress-side fixture changed behavior.

Closes #549

Related

• feat(autonomy): guardrail matrix, security binding, setup slice (WP5) #377 — merged carrier that landed the --egress-hosts seam this mirrors
• babysit-prs: codify the decide-and-defer non-convergence resolution playbook (own-once, no duplication of safety.md) #375 — decide-and-defer playbook
• source-control: no read/consume-only posture — "safe" babysit tier and pull-request monitor write to GitHub autonomously #440 — report-only precedent
• Ratifying decision: autonomy(check-security-binding): static credential-root + egress-target validation cannot converge without a configured allowlist #549 (comment)

[claude]

Claude finished @kyle-sexton's task in 4m 44s —— View job

---

PR #947 Review: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather context (PR description, changed files)
• Review core checker logic (check-security-binding.mjs)
• Review test harness and fixtures
• Review documentation changes (SKILL.md, isolation-probe.md, CHANGELOG.md)
• Review plugin.json version bump
• Synthesize findings and post review

---

Summary

This PR replaces an open-ended structural recognizer for host credential locations with a deny-by-default allowlist (--credential-roots). The motivation is sound: static structural matching is non-convergent because an adversary can always craft a structurally valid but non-existent path whose failing read proves nothing. The architectural approach (seam-based, mirroring --egress-hosts) is correct for the same reason the egress seam was the right pattern there.

The implementation is well-executed. The path normalization logic is careful, the coherence guard correctly gates all entry types, and the key test regressions (dot-traversal, invented-home-user, host-home-mount, UNC bypass) are all preserved under the new model. Two minor issues worth addressing before merge.

---

Finding 1 — Misleading guidance in unrecognized-token error message

Severity: minor correctness / operator experience

check-security-binding.mjs line ~248

return `not a well-known credential env token (${...}); an org-specific token belongs on the configured --credential-roots allowlist, not recognized by name`;

The message tells operators that an org-specific token "belongs on the configured --credential-roots allowlist." But --credential-roots is a filesystem path list — there is no code path in credentialEntryProblem that would accept an env-token-form entry via root containment. A single-segment env token that isn't in CREDENTIAL_ENV_VARS is always rejected, regardless of what roots are configured.

An operator who reads this message and has a secret like $MY_ORG_TOKEN will look for a way to add it to --credential-roots and find none. The correct guidance is that they should probe the filesystem path where the token is stored (e.g. /run/secrets/my-org-token) and configure the containing directory in --credential-roots.

Suggested fix:

return `not a well-known credential env token (${[...CREDENTIAL_ENV_VARS]
  .map((name) => `$${name.toUpperCase()}`).join(", ")}); to prove absence of an org-specific injected secret, probe the host filesystem path where it is stored and configure its parent directory via --credential-roots`;

Fix this →

---

Finding 2 — Empty-roots edge case produces confusing error

Severity: minor UX

check-security-binding.mjs line ~1726

const credentialRoots =
  credentialRootsArg === null
    ? null
    : credentialRootsArg.split(",").map((root) => root.trim()).filter((root) => root.length > 0);

If an operator passes --credential-roots "," (commas only, no real paths), credentialRootsArg is "," (non-empty, so the usage check passes), but after split/trim/filter credentialRoots is []. The subsequent credentialEntryProblem call reaches pathUnderConfiguredRoot(expanded, []) → false and emits:

… does not resolve under any configured --credential-roots () — …

Empty parens rather than the clear "no --credential-roots configured" message an operator needs. The check if (credentialRoots === null) doesn't fire because the value is [], not null.

Suggested fix: treat an empty-after-parsing list the same as no-arg:

const credentialRootsParsed =
  credentialRootsArg === null
    ? null
    : credentialRootsArg.split(",").map((root) => root.trim()).filter((root) => root.length > 0);
const credentialRoots = credentialRootsParsed !== null && credentialRootsParsed.length === 0 ? null : credentialRootsParsed;

Or emit a usage error at parse time if the parsed list is empty. Either approach avoids the silent confusing behavior. Adding a fixture for this case would lock in the behavior.

Fix this →

---

What's correct

normalizeHostPath is sound. UNC rejection (//) at the top, drive-letter anchoring, segment-by-segment collapse with null-on-root-escape all cover the relevant attack surface. Worth noting the explicit design choice documented in the comment: the checker never reads the probing host's filesystem, so POSIX case-sensitivity is not observable and case-insensitive containment is strictly safe (it can only refuse to distinguish roots that differ only in case).

pathUnderConfiguredRoot enforces segment boundaries correctly. The normCandidate.startsWith(normRoot + "/") guard (or equality) means /home/runner-other never matches a /home/runner root. Root / is handled without introducing a bare-prefix vulnerability (any /-rooted path starts with /).

Coherence guard now runs for all entry types. In the old code, credentialExpansionProblem only fired after isRecognizedCredentialEntry returned true. The new code runs it inside credentialEntryProblem for every entry kind — metadata URL, env token, and filesystem path — which closes the gap where a concrete-entry coherence failure could only be caught post-recognition.

Dot-traversal is correctly denied under the new model. /root/../var/empty/.ssh/id_rsa with --credential-roots /root reaches normalizeHostPath, resolves to /var/empty/.ssh/id_rsa, which does not start with /root/. The denial is no longer an explicit dot-segment check but a consequence of normalization + containment — which is more robust (handles multi-hop traversal, mixed slashes, etc.).

UNC bypass is closed. normalizeHostPath("//etc/credentials") returns null immediately; pathUnderConfiguredRoot treats null candidate as no-match. A configured root of /etc cannot be collapsed onto by //etc/credentials.

Test manifest is consistently updated. Every fixture that involves credential path validation now has credential_roots set in the manifest and passed to the harness. The three preserved regression cases (invented-home-user, host-home-mount, dot-traversal) test exactly the deny cases described in the PR. The new credroots-unconfigured and unc-credpath fixtures cover the two headline behaviors.

Version bump and CHANGELOG entry are accurate. The 0.8.0 → 0.9.0 bump reflects a behavioral change to the checker's credential validation (not a strictly additive change), and the CHANGELOG entry precisely describes what changed and why.

---

Two minor issues (misleading operator guidance, empty-list UX confusion) — both safe to merge as-is since neither widens the security boundary, but the env-token message especially could trip up operators at wire time.

[claude]

Claude finished @kyle-sexton's task in 8m 28s —— View job

---

Security Review — PR #947: fix(autonomy): add deny-by-default --credential-roots allowlist

Scope: Files changed in this PR only, reviewed against the commit blob at d1442e1.

---

Methodology

Traced the full execution path of the new --credential-roots seam through:

• CLI argument parsing and validation
• normalizeHostPath (lexical normalization for containment testing)
• pathUnderConfiguredRoot (segment-boundary membership)
• credentialEntryProblem (deny-by-default dispatch)
• credentialExpansionProblem (coherence guard, unchanged but re-wired)
• Fixture corpus and manifest for regression coverage

Checked: path-traversal bypasses, UNC-collapse attacks, argument injection, env-token/metadata-URL escape from the roots check, operator misconfiguration foot-guns, and comparison/normalization inconsistencies.

---

Overall Assessment

The core implementation is sound. The deny-by-default posture is correctly implemented; normalizeHostPath handles ../. collapsing, UNC rejection, drive-letter anchoring, and lowercasing correctly; pathUnderConfiguredRoot applies a segment-boundary (not bare-prefix) match; and the coherence guard still runs for every entry kind before the roots check. No CRITICAL or IMPORTANT findings.

Two SUGGESTION-level findings follow.

---

SUGGESTION · CONFIRMED — pathUnderConfiguredRoot silently accepts all absolute paths when root is /

Severity: SUGGESTION | Confidence: CONFIRMED

pathUnderConfiguredRoot guards against an empty root (normRoot === "") and a null root, but does not guard against /:

// line 215
if (normRoot === null || normRoot === "") return false;
// line 217 — when normRoot === "/", this branch fires:
return normCandidate.startsWith(normRoot === "/" ? "/" : `${normRoot}/`);

normalizeHostPath("/") returns "/". With normRoot === "/" the condition becomes normCandidate.startsWith("/"), which is true for every absolute path. An operator who passes --credential-roots / (or --credential-roots /.) defeats the deny-by-default intent: any absolute host_expanded value would be accepted as credential-absence evidence, including invented paths (/home/nonexistent/.ssh/id_rsa) that the PR was specifically designed to reject.

The --credential-roots seam is operator-controlled, so this cannot be exploited without operator misconfiguration. However, the consequence of misconfiguring / is total silent bypass of the root-containment check, and there is no diagnostic warning. The code comments discuss case-insensitive containment ("can only ever REFUSE to distinguish…, never widen a root") but do not address the /-root case.

Suggested fix: Add a guard in pathUnderConfiguredRoot (or at parse time) that rejects or warns on normRoot === "/":

// In pathUnderConfiguredRoot:
if (normRoot === null || normRoot === "" || normRoot === "/") return false;

Or at parse time: emit a warning if any configured root normalizes to "/".

---

SUGGESTION · PLAUSIBLE — Concrete path entries with ../. segments pass the coherence check silently

Severity: SUGGESTION | Confidence: PLAUSIBLE

The old isRecognizedCredentialEntry function (removed in this PR) explicitly rejected any path entry containing "." or ".." segments for all entry kinds:

// old code (removed)
if (segments.some((segment) => segment === "." || segment === "..")) return false;

The new credentialExpansionProblem only checks expandedSegments for dot segments when the entry is home-anchored. For concrete (non-home-anchored) paths, the non-home-anchored branch at line 286–289 only checks verbatim equality:

if (!homeAnchored) {
  return normalizedExpanded === normalizedEntry
    ? null
    : "a concrete entry needs no host-side expansion, so host_expanded must repeat the entry verbatim";
}

This means a concrete path like /configured-root/subdir/../.ssh/id_rsa passes the coherence check (entry equals expanded verbatim). normalizeHostPath inside pathUnderConfiguredRoot then collapses the .. correctly, so the containment check still enforces the security invariant — the path is only accepted if the normalized form resolves under the configured root. The dot-traversal fixture confirms the escape-the-root case is rejected.

The gap is: a path that traverses within the root (e.g., /home/runner/subdir/../.ssh/id_rsa with root /home/runner) is silently accepted in non-canonical form. This is not a bypass (the probe genuinely tests /home/runner/.ssh/id_rsa via OS path resolution, and the outer existence check must succeed), but:

1. The error message for a partially-escaping traversal says "does not resolve under any configured --credential-roots" rather than "contains a dot-segment traversal," making obfuscation attempts harder to audit.
2. An operator reviewing a transcript with such a path might not immediately recognize the non-canonical form.
3. The behaviour change from the old code is undocumented.

Suggested fix: Add an explicit dot-segment check in the non-home-anchored branch of credentialExpansionProblem, mirroring the old rejection:

if (!homeAnchored) {
  // Dot segments in a concrete entry resolve elsewhere than the recorded path
  // claims — a traversal obscures the true probe target (the old code rejected
  // these outright; maintain that explicitness even though normalizeHostPath
  // would handle them correctly at containment time).
  const concreteSegments = normalizedEntry.split("/").filter(s => s.length > 0);
  if (concreteSegments.some(s => s === "." || s === "..")) {
    return "a concrete credential path entry must not contain dot-traversal segments (\".\", \"..\") — use the canonical path";
  }
  return normalizedExpanded === normalizedEntry
    ? null
    : "a concrete entry needs no host-side expansion, so host_expanded must repeat the entry verbatim";
}

---

No-issue areas (explicitly checked)

Area	Verdict
UNC collapse onto POSIX root (//etc/...)	Blocked in normalizeHostPath (returns null) and independently in old coherence guard
.. escaping the configured root	Blocked: normalizeHostPath collapses traversal; post-normalization path falls outside root → pathUnderConfiguredRoot returns false
Env-token bypass of credential roots check	Bounded closed set ($GITHUB_TOKEN, $GH_TOKEN); org-specific tokens rejected by name; verbatim expansion required
Metadata URL bypass of credential roots check	Bounded closed set (169.254.169.254 / metadata.google.internal, plain HTTP, known routes only); file://, HTTPS, off-port all rejected
Segment-boundary prefix attack (/home/runner-x accepted by root /home/runner)	Blocked: containment checks startsWith(normRoot + "/") not bare startsWith(normRoot)
Empty --credential-roots list (e.g. ",")	Produces [] after filter; roots.some(...) on empty array → false → all paths denied (fail-closed)
Relative-path configured root	Relative roots produce relative normRoot; absolute candidate paths don't start with a relative prefix → denied
Argument injection / shell concerns	None: value is split/compared as plain strings in Node.js; never passed to a shell
GitHub Actions patterns	No workflow files changed

[claude]

Claude finished @kyle-sexton's task in 4m 16s —— View job

---

PR #947 Review: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather context (PR description, changed files, prior review)
• Read core checker logic (check-security-binding.mjs)
• Check if prior findings were addressed in sync commits
• Review fixture manifest and test harness changes
• Review CI and documentation changes
• Synthesize findings and post review

---

What this sync adds

The sync adds one commit on top of the reviewed work:

ci: exclude the security-binding fixture manifest from machine-specific-paths — the fixture manifest pins /home/runner as a trusted --credential-roots value for test cases. The autonomy security-binding fixtures directory was already excluded from the machine-specific-paths hygiene scan (because its transcripts carry adversarial host paths by design), but the manifest was not. This commit adds the manifest to the exclusion list, consistent with the fixtures-dir exemption. The reasoning is sound: the manifest is part of the same test corpus. CI change is narrowly scoped and correct.

---

Prior findings: status

Both findings from the earlier review pass unchanged into the current HEAD.

---

Finding 1 — Misleading guidance in unrecognized-token error message (still open)

check-security-binding.mjs line 249

return `not a well-known credential env token (${[...CREDENTIAL_ENV_VARS]
  .map((name) => `$${name.toUpperCase()}`).join(", ")}); an org-specific token belongs on the configured --credential-roots allowlist, not recognized by name`;

--credential-roots is a filesystem path list. There is no code path in credentialEntryProblem that accepts an env-token-form entry via root containment — single-segment env tokens that aren't in CREDENTIAL_ENV_VARS are always rejected, regardless of what roots are configured. An operator with $MY_ORG_TOKEN will find no way to add it to --credential-roots. The same misleading claim also appears in the comment at line 118–119.

Correct guidance: probe the filesystem path where the injected secret is stored (e.g. /run/secrets/my-org-token) and configure its parent directory in --credential-roots.

Fix this →

---

Finding 2 — Empty-roots edge case produces confusing error (still open)

check-security-binding.mjs lines 1731–1734

const credentialRoots =
  credentialRootsArg === null
    ? null
    : credentialRootsArg.split(",").map((root) => root.trim()).filter((root) => root.length > 0);

When --credential-roots "," is passed: credentialRootsArg is "," (non-empty, so the usage check at line 1718 passes), but after split/trim/filter credentialRoots is []. The if (credentialRoots === null) guard at line 255 does not fire. pathUnderConfiguredRoot(expanded, []) returns false, producing:

… does not resolve under any configured --credential-roots () — …

Empty parens ([].join(", ") === ""), not the clear "no --credential-roots configured" message an operator needs. This is fail-closed (nothing is ever accepted), so not a security issue — but the silent empty-list behavior means the operator won't understand why their credential probe is being rejected.

Suggested fix: treat an empty-after-parsing list the same as null:

const credentialRootsParsed =
  credentialRootsArg === null
    ? null
    : credentialRootsArg.split(",").map((root) => root.trim()).filter((root) => root.length > 0);
const credentialRoots = credentialRootsParsed !== null && credentialRootsParsed.length === 0 ? null : credentialRootsParsed;

Fix this →

---

What's verified correct

The core security logic introduced in the first two commits is sound. Specific verifications:

• normalizeHostPath is sound. UNC rejection (//) at the top, drive-letter anchoring, dot-segment collapse with null-on-root-escape — a traversal can never produce a path that normalizes to something outside the expected subtree. /root/../var/empty/.ssh/id_rsa normalizes to /var/empty/.ssh/id_rsa via pure arithmetic, no filesystem read.

• pathUnderConfiguredRoot enforces segment boundaries. normCandidate.startsWith(normRoot + "/") (or equality) means /home/runner-other never matches a /home/runner root. The root / edge case is handled cleanly: normRoot === "/" uses startsWith("/") which accepts any POSIX-absolute path as expected.

• Empty-relative-root is silently ineffective, not dangerous. A root that normalizes to "" (e.g. .) hits the normRoot === "" guard and returns false — it never matches anything, strictly fail-closed.

• UNC bypass is closed. normalizeHostPath("//etc/credentials") → null; pathUnderConfiguredRoot treats a null candidate as no-match. A configured /etc root cannot be reached via //etc/credentials.

• Coherence guard now runs for all entry types. Previously credentialExpansionProblem only fired after recognition. Now it runs inside credentialEntryProblem for metadata URLs, env tokens, and filesystem paths alike — a concrete entry's host_expanded must repeat it verbatim.

• Test harness wired correctly. The one-line addition to check-security-binding.fixtures.test.mjs at line 92 correctly propagates entry.credential_roots as --credential-roots to the checker for each graded fixture that declares it.

• Documentation is accurate. SKILL.md and isolation-probe.md both correctly describe credential validation as deny-by-default with an explicit note that "with no roots configured every filesystem credential entry is untrusted and the level fails closed."

---

Two minor issues remain open from the prior review — the misleading env-token guidance (Finding 1) and the empty-list UX edge case (Finding 2). Neither widens the security boundary; both are safe to merge. Finding 1 is more likely to trip up a first-time operator.

[claude]

Claude finished @kyle-sexton's task in 3m 30s —— View job

---

Security Review — PR #947: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather PR context and prior review findings
• Read changed files (checker, fixtures, CI workflow)
• Check if prior security findings were addressed in sync commits
• Audit new code for security issues
• Post findings

---

Scope: Files changed in this PR, reviewed at commit 279042d. The sync commit is CI-only (.github/workflows/ci.yml exclusion tweak); the checker logic is unchanged from d1442e1.

Methodology: Traced the full --credential-roots execution path through CLI parsing → normalizeHostPath → pathUnderConfiguredRoot → credentialEntryProblem → credentialExpansionProblem, and reviewed the GitHub Actions workflow changes for injection / permission-widening risks.

Prior security review status: The prior review (run 29891463677) raised two SUGGESTION-level findings. Neither has been addressed in the sync commit. Both are re-confirmed below.

---

SUGGESTION · CONFIRMED — pathUnderConfiguredRoot accepts every absolute path when root is /

check-security-binding.mjs line 217

// line 215
if (normRoot === null || normRoot === "") return false;
// line 217 — when normRoot === "/":
return normCandidate.startsWith(normRoot === "/" ? "/" : `${normRoot}/`);

normalizeHostPath("/") returns "/". With normRoot === "/" the ternary evaluates to normCandidate.startsWith("/"), which is true for every absolute POSIX path. An operator who passes --credential-roots / (or --credential-roots /., which collapses identically) makes the checker accept any absolute host_expanded value as credential-absence evidence — including invented paths (/home/nonexistent/.ssh/id_rsa) that the deny-by-default design was specifically intended to reject. There is no diagnostic warning; the bypass is silent.

This is operator-controlled, so it cannot be exploited without misconfiguration, but the consequence of the mistake is total silent defeat of the deny-by-default posture with no error. The code comment discusses case-insensitive containment safety but does not address the /-root case.

Suggested fix (same as prior review):

// In pathUnderConfiguredRoot, extend the existing null/empty guard:
if (normRoot === null || normRoot === "" || normRoot === "/") return false;

Or emit a usage error at parse time if any configured root normalizes to "/".

---

SUGGESTION · PLAUSIBLE — Concrete paths with ../. segments pass the coherence check without explicit rejection

check-security-binding.mjs lines 286–289

if (!homeAnchored) {
  return normalizedExpanded === normalizedEntry
    ? null
    : "a concrete entry needs no host-side expansion, so host_expanded must repeat the entry verbatim";
}

A concrete path like /configured-root/subdir/../.ssh/id_rsa passes this check when host_expanded repeats the entry verbatim, because verbatim equality holds. normalizeHostPath collapses the .. correctly at containment time, so a traversal that escapes the configured root is still denied (the dot-traversal fixture confirms this). There is no security bypass.

What is absent is an explicit rejection with an informative error. A transcript containing a non-canonical concrete path (/root/subdir/../.ssh/id_rsa) receives "does not resolve under any configured --credential-roots" rather than "contains a dot-traversal segment." The old isRecognizedCredentialEntry rejected dot segments outright; the new code handles them only implicitly through normalization. This makes obfuscated traversal attempts harder to audit in a transcript corpus and leaves a behavior change from the old code undocumented.

Suggested fix:

if (!homeAnchored) {
  const concreteSegments = normalizedEntry.split("/").filter(s => s.length > 0);
  if (concreteSegments.some(s => s === "." || s === "..")) {
    return "a concrete credential path must not contain dot-traversal segments — use the canonical path";
  }
  return normalizedExpanded === normalizedEntry
    ? null
    : "a concrete entry needs no host-side expansion, so host_expanded must repeat the entry verbatim";
}

---

No-issue areas (explicitly checked in this pass)

Area	Verdict
UNC collapse onto POSIX root (//etc/...)	Blocked: normalizeHostPath returns null; pathUnderConfiguredRoot treats null candidate as no-match
.. escaping a configured root	Blocked: normalizeHostPath collapses traversal; normalized form falls outside root
Empty-roots after parse (--credential-roots "," → [])	Behavior is fail-closed (every path denied); error message has empty parens but the security posture is correct
Segment-boundary prefix attack (/home/runner-x vs root /home/runner)	Blocked: startsWith(normRoot + "/") not bare startsWith(normRoot)
Env-token bypass of roots check	Bounded closed set ($GITHUB_TOKEN, $GH_TOKEN); unrecognized names always rejected
Metadata URL bypass of roots check	Bounded closed set (known providers + routes; plain HTTP + default port only)
GitHub Actions workflow change (ci.yml)	Uses pull_request (not pull_request_target); all action refs SHA-pinned; no run: steps interpolating untrusted context; permissions: contents: read; the exclusion only widens the machine-paths hygiene skip for the manifest file — no security boundary affected
Argument injection in --credential-roots parsing	None: split/compare in Node.js, never passed to a shell

---

Two prior SUGGESTION-level findings remain open. Neither is a bypass without operator misconfiguration; neither introduces a new attack surface from outside. The /-root guard is the higher-priority fix since its consequence — total silent disable of deny-by-default — is severe relative to the ease of the mistake.

[kyle-sexton]

Classification — review + security-review passes (commit 279042d)

#	Source	Finding	Classification	Action
1	review	Misleading env-token guidance in error message (L249) + comment (L118-119) — tells operator to add a token to --credential-roots, but that flag is a filesystem-path list with no code path for env tokens	VALID (fix now)	Fixing on branch
2	review	--credential-roots "," parses to [], bypassing the "no roots configured" message, emits confusing empty-parens error	VALID (fix now)	Fixing on branch
3	security-review, CONFIRMED	--credential-roots / silently accepts every absolute path — total, no-warning defeat of the deny-by-default posture this PR exists to ship	VALID (fix now) — DEFER-FORBIDDEN: this is a fail-open path in the exact safety gate #549 was filed to close; shipping it uncaveated would also violate the CHANGELOG/SKILL.md's "fails closed" claim (rule 6e)	Fixing on branch
4	security-review, PLAUSIBLE	Concrete paths with ./.. segments pass the coherence check via verbatim-equality rather than an explicit dot-traversal rejection — not a bypass (normalization still denies the traversed path at containment time), just a less-specific audit message	VALID (defer) — no security impact, diagnostic-quality only	Filing follow-up issue, citing here

Findings 1-3 routed back to the branch for a fix; finding 4 gets a tracked follow-up (not blocking this PR).

[claude]

Claude finished @kyle-sexton's task in 1m 45s —— View job

---

PR #947 Review: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather PR context (latest commit fd3b5c7: "harden --credential-roots edge cases from review")
• Check the three open findings from prior review passes
• Read updated checker logic at key locations
• Check fixture additions for the /-root bypass fix
• Synthesize and post review

---

What this sync adds

The latest commit (fd3b5c7) directly addresses all three findings that Kyle classified as "fix now" in the review triage comment. One new fixture was added; no documentation or version-bump changes were needed.

---

Prior findings: status after fd3b5c7

Finding 1 — Misleading env-token guidance: ✅ FIXED

Comment (L117–121):

// *_token name proves nothing and is not recognized statically by name; probe
// the host FILESYSTEM path where the org's injected secret is stored (e.g.
// /run/secrets/my-org-token) and configure its parent in --credential-roots —
// the allowlist holds filesystem roots, not token names.

Error message (L255):

not a well-known credential env token (…); an org-specific token is not recognized by name — probe the host filesystem path where the injected secret is stored (e.g. /run/secrets/my-org-token) and configure its parent in --credential-roots

Both the comment and the operator-facing error message now correctly describe --credential-roots as a filesystem-roots list and redirect operators to probe the stored-secret path rather than trying to add a token name to the allowlist.

---

Finding 2 — Empty-roots edge case: ✅ FIXED

L1740–1745:

const credentialRootsParsed =
  credentialRootsArg === null
    ? null
    : credentialRootsArg.split(",").map((root) => root.trim()).filter((root) => root.length > 0);
const credentialRoots =
  credentialRootsParsed !== null && credentialRootsParsed.length === 0 ? null : credentialRootsParsed;

--credential-roots "," now produces credentialRoots = null, which routes to the "no --credential-roots configured" guidance rather than the confusing empty-parens rejection. The accompanying comment explains the intent. Fail-closed behavior is unchanged.

---

Finding 3 — /-root bypass: ✅ FIXED (with regression fixture)

L221–223:

if (normRoot === null || normRoot === "" || normRoot === "/") return false;
if (normCandidate === normRoot) return true;
return normCandidate.startsWith(`${normRoot}/`);

The /-root guard is in place and the now-dead normRoot === "/" ternary branch is removed — the prior startsWith(normRoot === "/" ? "/" : \${normRoot}/`)is replaced with the unconditionalstartsWith(`${normRoot}/`), which is correct since "/"` can no longer reach this branch.

The new fixture probe-evidence-credroots-root-slash.json is wired in the manifest as:

{
  "exit": 1,
  "credential_roots": "/",
  "findings_substrings": ["does not resolve under any configured --credential-roots"]
}

This locks in the behavior: --credential-roots / is no longer a silent bypass — it produces a denial, not an acceptance.

---

Finding 4 — Dot-segments in concrete paths (diagnostic quality): deferred as classified

No change in this commit; follow-up issue planned per triage classification. Correct — the security invariant is still enforced through normalization + containment.

---

Verification

All three security-relevant fixes are correct at the code level. The implementation choices are clean:

• The /-root guard is applied consistently inside pathUnderConfiguredRoot rather than at parse time, which is the right seam — the same normalization happens at containment time regardless of where the root originated.
• Removing the dead normRoot === "/" ternary branch is good housekeeping that removes a code path that could no longer fire.
• The empty-roots collapse to null faithfully preserves the "fail-closed either way" property while fixing the operator experience.

No new issues identified. All open findings from the prior review passes are resolved. Ready to merge.

[claude]

Claude finished @kyle-sexton's task in 3m 0s —— View job

---

Security Review — PR #947: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather PR context and prior review findings
• Read the latest commit diff (fd3b5c7 — "harden --credential-roots edge cases from review")
• Verify prior SUGGESTION findings were addressed
• Audit new code for security issues
• Post findings

---

Scope: The fd3b5c7 sync commit, reviewed against current HEAD. Three files changed: check-security-binding.mjs, check-security-binding.fixtures.test.manifest.json, and the new probe-evidence-credroots-root-slash.json fixture.

Methodology: Traced each prior finding through the new code to confirm closure; audited the edge-case handling in normalizeHostPath / pathUnderConfiguredRoot / credentialEntryProblem for any regression or new bypass introduced by the fixes.

---

Prior findings: all closed

Finding 1 — Misleading env-token guidance ✅ FIXED

check-security-binding.mjs lines 115–118 (comment) and line 255 (error message)

Both locations now correctly say "probe the host filesystem path where the org's injected secret is stored (e.g. /run/secrets/my-org-token) and configure its parent in --credential-roots." The --credential-roots flag is no longer misrepresented as accepting token names.

Finding 2 — Empty-roots edge case produces confusing error ✅ FIXED

check-security-binding.mjs lines 1740–1745

The two-variable split (credentialRootsParsed → credentialRoots = null when parsed list is []) is correct. --credential-roots "," now routes to the clear "no --credential-roots configured" message rather than an empty-parens denial.

Finding 3 — --credential-roots / silently accepts every absolute path ✅ FIXED

check-security-binding.mjs lines 216–223

The guard now reads:

if (normRoot === null || normRoot === "" || normRoot === "/") return false;

This is correct and complete:

• normalizeHostPath("/") returns "/", caught by normRoot === "/"
• normalizeHostPath("/.") collapses to "/", also caught
• The ternary that special-cased / into a bare startsWith("/") is removed; all non-blocked roots now use normCandidate.startsWith(\${normRoot}/`)`, which correctly requires a segment boundary

The new probe-evidence-credroots-root-slash.json fixture with manifest entry { "exit": 1, "credential_roots": "/", "findings_substrings": ["does not resolve under any configured --credential-roots"] } locks in the behavior.

---

No regression in other edge cases

Verified by tracing the modified code paths:

Case	Before fix	After fix
normRoot === "/" (configured root /)	Accepted every absolute path	Returned false — denied
normRoot === "/home/runner"	startsWith("/home/runner/") (correct)	Same — no change
normCandidate === normRoot equality	Checked after ternary	Same — checked after guard, so "/" equality never fires
credentialRoots = [] (empty-after-parse)	Reached roots.some(...) → false; confusing message	Now null — routes to "no --credential-roots configured"
credentialRoots = null (no arg)	"no --credential-roots configured"	Unchanged

The Windows drive-letter root case (e.g., --credential-roots C:/) is not caught by the normRoot === "/" guard, but it cannot accept anything via startsWith ("c:/foo" does not start with "c://") so it is effectively inert rather than a silent bypass — consistent with the existing behavior and not a regression.

---

No new security issues found

The changes in fd3b5c7 are narrowly scoped to the three prior findings. No new injection surfaces, access-control gaps, or path-normalization bypasses are introduced. The /-root guard is strictly more restrictive. The empty-list normalization is strictly fail-closed.

The deferred PLAUSIBLE finding (concrete paths with ../. segments in the non-home-anchored coherence branch) remains unaddressed by design per the classification comment — normalization at containment time still correctly denies any traversal that would escape a configured root, and the follow-up issue will track the diagnostic-quality gap. This is consistent with the stated defer decision and is not a security regression.

---

Summary: All three security findings from prior reviews are correctly fixed. No new issues introduced. Secure to merge.

[claude]

Claude finished @kyle-sexton's task in 2m 55s —— View job

---

PR #947 Final Review: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather context (PR description, prior review history, changed files)
• Review core checker logic at HEAD (fd3b5c7)
• Verify all prior findings are resolved
• Check fixture corpus and test harness
• Review documentation and version changes
• Synthesize findings and post final review

---

Status of all prior findings

This is the final review pass at fd3b5c7 ("harden --credential-roots edge cases from review"). All three findings Kyle classified as "fix now" are resolved. Finding 4 is deferred by design.

Finding 1 — Misleading env-token guidance: ✅ FIXED

Lines 118–121 (comment) and line 255 (error message):

// *_token name proves nothing and is not recognized statically by name; probe
// the host FILESYSTEM path where the org's injected secret is stored (e.g.
// /run/secrets/my-org-token) and configure its parent in --credential-roots —
// the allowlist holds filesystem roots, not token names.

Both the comment and the operator-facing error now correctly describe --credential-roots as a filesystem-root list and redirect operators to probe the stored-secret path rather than searching for a way to add a token name to the allowlist.

Finding 2 — Empty-roots edge case: ✅ FIXED

Lines 1740–1745:

const credentialRootsParsed =
  credentialRootsArg === null
    ? null
    : credentialRootsArg.split(",").map((root) => root.trim()).filter((root) => root.length > 0);
const credentialRoots =
  credentialRootsParsed !== null && credentialRootsParsed.length === 0 ? null : credentialRootsParsed;

--credential-roots "," now collapses to null, routing to the clear "no --credential-roots configured" message. The accompanying comment explains the intent. Fail-closed behavior is unchanged.

Finding 3 — --credential-roots / silently accepts every absolute path: ✅ FIXED + regression fixture

Line 221:

if (normRoot === null || normRoot === "" || normRoot === "/") return false;
if (normCandidate === normRoot) return true;
return normCandidate.startsWith(`${normRoot}/`);

The /-root guard is in place. The dead normRoot === "/" ternary branch is removed — the prior startsWith(normRoot === "/" ? "/" : \${normRoot}/`)is replaced with the unconditionalstartsWith(`${normRoot}/`), which is correct since "/"can no longer reach this branch. Paths likenormalizeHostPath("/.")also collapse to"/"` and are caught.

The new probe-evidence-credroots-root-slash.json fixture with manifest entry { "exit": 1, "credential_roots": "/", "findings_substrings": ["does not resolve under any configured --credential-roots"] } locks in the denial behavior at test time.

Finding 4 — Dot-segments in concrete paths (diagnostic quality): deferred by design

No change; follow-up issue planned per Kyle's triage classification. The security invariant is correctly enforced through normalizeHostPath + containment. The dot-traversal fixture continues to confirm that traversal-to-escape is denied.

---

Core logic verified

Area	Verdict
normalizeHostPath is sound	UNC rejection (// → null), drive-letter anchoring, ..-safe via null-on-root-escape. normalizeHostPath("/") → "/", caught by guard.
pathUnderConfiguredRoot enforces segment boundaries	Guards null/""/"/", then normCandidate === normRoot (exact match) or startsWith(normRoot + "/"). /home/runner-other never matches root /home/runner.
Coherence guard runs for all entry types	credentialExpansionProblem fires inside credentialEntryProblem for metadata URLs, env tokens, and filesystem paths alike.
Env-token and metadata-URL bypass is correct by design	Bounded closed sets; unrecognized names/routes always rejected; well-known tokens require verbatim host_expanded.
Empty-relative-root is inert, not a bypass	A root normalizing to "" (e.g. .) hits the normRoot === "" guard → false. Fail-closed.
UNC bypass is closed	normalizeHostPath("//etc/...") → null; pathUnderConfiguredRoot treats null candidate as no-match.
Test harness correctly wires credential_roots	Line 92 of the test MJS: if (entry.credential_roots) args.push("--credential-roots", entry.credential_roots).
Documentation is accurate	SKILL.md and isolation-probe.md both correctly describe credential validation as deny-by-default with explicit "fails closed" language.
Version bump and CHANGELOG entry are accurate	0.8.0 → 0.9.0 reflects a behavioral change (not additive); CHANGELOG entry is precise.

---

All prior findings are resolved. No new issues identified. Ready to merge.

[claude]

Claude finished @kyle-sexton's task in 5m 6s —— View job

---

Security Review — PR #947: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather PR context and prior review findings
• Read PR diff and core checker logic (check-security-binding.mjs) at HEAD fd3b5c7
• Audit path normalization and containment logic (normalizeHostPath, pathUnderConfiguredRoot)
• Audit credential entry dispatch and coherence guard
• Trace data flow from transcript fields through the validation pipeline
• Audit GitHub Actions workflow changes
• Verify all three prior findings are addressed
• Post findings

---

Scope: Files changed in this PR, reviewed independently at HEAD fd3b5c7005937207d664e88e002948a1d73a31e7.

Methodology: Full trace of the --credential-roots seam: CLI arg parsing → normalizeHostPath → pathUnderConfiguredRoot → credentialEntryProblem → credentialExpansionProblem, plus the transcript data-flow path (host_expanded → credentialPaths pairing) and the GitHub Actions workflow diff.

---

Overall assessment

The implementation is sound. No CRITICAL or IMPORTANT findings. All three findings from prior security review passes are correctly resolved in fd3b5c7.

---

Prior findings — status at HEAD

Finding from prior review: Misleading env-token guidance ✅ CONFIRMED FIXED

Lines 117–121 (comment) and line 255 (error message) both now correctly direct operators to probe the host filesystem path rather than claiming an env token can be added to --credential-roots.

Finding from prior review: Empty-roots edge case ✅ CONFIRMED FIXED

Lines 1740–1745: credentialRootsParsed set to null when the parsed list is [], routing --credential-roots "," to the clear "no --credential-roots configured" message rather than an empty-parens denial.

Finding from prior review: --credential-roots / silently accepts every absolute path ✅ CONFIRMED FIXED

Lines 216–224: the normRoot === "/" guard is in place; the prior startsWith(normRoot === "/" ? "/" : \${normRoot}/`)ternary is replaced with the unconditionalstartsWith(`${normRoot}/`). The new probe-evidence-credroots-root-slash.jsonfixture (manifest entry:"exit": 1, "credential_roots": "/"`) locks this in.

---

No-issue areas verified in this pass

Area	Verdict
UNC collapse (//etc/... onto POSIX root /etc)	Blocked: normalizeHostPath returns null at the lowered.startsWith("//") guard; pathUnderConfiguredRoot treats null candidate as no-match
.. escaping the configured root	Blocked: normalizeHostPath collapses traversal arithmetically; post-normalization path falls outside root → pathUnderConfiguredRoot returns false
Multi-hop .. traversal (e.g. /root/a/../../etc/passwd)	Blocked: the out.pop() loop correctly unwinds each hop; returning null if the stack underflows before the root anchor
/./ and embedded double-slash paths	Correctly collapsed: /./home/runner → /home/runner; /home//runner → /home/runner
Segment-boundary prefix attack (/home/runner-x vs root /home/runner)	Blocked: startsWith(\${normRoot}/`)— suffix/` is required; sibling never matches
normRoot === "/" (configured root /)	Blocked by the new explicit guard at line 221
Windows drive-letter root (C:/)	normalizeHostPath("C:/") returns "c:/" — NOT caught by the normRoot === "/" guard, but normCandidate.startsWith("c://") never fires for real Windows paths, so behavior is fail-closed (denies everything), not fail-open. Documented in prior review as existing behavior, not a regression.
Empty string root after normalization (e.g. .)	Caught by normRoot === "" guard → false
Env-token single-segment bypass of roots check	Bounded closed set ($GITHUB_TOKEN, $GH_TOKEN); unrecognized bare names always rejected
Metadata URL bypass of roots check	Bounded closed set (169.254.169.254 / metadata.google.internal, plain HTTP, known routes); file://, HTTPS, off-port, arbitrary hostnames all rejected
host_expanded comma-injection from transcript	Each positionally-paired entry independently runs the coherence guard (credentialExpansionProblem); concrete entries require verbatim equality, so injecting a second comma-delimited path that aliases a legitimate root requires the second entry to also independently pass containment. Length mismatch on any split is caught and rejected.
host_expanded null byte or control characters	normalizeHostPath doesn't strip control characters, but a null byte in position len(normRoot) prevents startsWith(normRoot + "/") from matching → fail-closed
Coherence guard scope (all entry kinds)	credentialExpansionProblem now runs for metadata URL, env token, and filesystem path alike — concrete entries (all three) must repeat host_expanded verbatim
Relative path as configured root	Relative normRoot never starts with /, so an absolute candidate path never matches → fail-closed
Missing --credential-roots argument value	args[i + 1] = undefined; !isNonEmptyString(undefined) = true → exits with usage error (exit 2)
Argument injection in --credential-roots parsing	Split/compare in Node.js as plain strings; never passed to a shell
GitHub Actions ci.yml workflow change	Only change: added fixture manifest to machine-specific-paths exclusion list. Trigger is pull_request (not pull_request_target); all action refs SHA-pinned; no run: step interpolates untrusted context; permissions: contents: read; exclusion pattern is safe pathspec syntax

---

Deferred finding (from prior review, no change expected)

The prior security review classified one PLAUSIBLE finding as DEFER: concrete paths with .. or . segments in the non-home-anchored branch of credentialExpansionProblem pass the verbatim-equality coherence check (since host_expanded must repeat the entry exactly, .. segments satisfy the equality), then are correctly handled at containment time by normalizeHostPath collapsing them. No bypass: a traversal that escapes the root after normalization falls outside containment and is denied. The gap is diagnostic only — the error says "does not resolve under any configured --credential-roots" rather than "contains a dot-traversal segment." This remains appropriately deferred.

---

Summary: No CRITICAL or IMPORTANT findings. All three prior security findings are correctly fixed and verified. The deny-by-default posture is correctly implemented and the /-root bypass regression is locked in by the new fixture. Secure to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd3b5c7005

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 3m 13s —— View job

---

PR #947 Review: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather context (PR description, prior reviews, new sync commit)
• Read the new commit diff (e9f7abf — "fail closed on case-mismatched and relative credential roots")
• Review changes to check-security-binding.mjs
• Review new fixture corpus changes
• Evaluate Codex findings addressed by this commit
• Synthesize findings and post final review

---

What this sync adds

The latest commit (e9f7abf) addresses the two Codex P2 findings from the automated review. No prior Claude findings are affected — all three were correctly resolved in fd3b5c7 and remain closed.

---

Codex Finding 1 — Preserve case when checking POSIX credential roots: ✅ FIXED

What changed: normalizeHostPath no longer unconditionally lowercases. The toLowerCase() on the full path is removed; only Windows drive-letter paths are folded (correctly, since Windows path matching is inherently case-insensitive). POSIX paths keep their case.

Why this is correct: The checker cannot observe the probing host's platform — process.platform reports the checker's host, not the probe's. On a case-sensitive host /var/Creds and /var/creds are different files; folding before containment would silently widen the allowlist to include a path the operator never trusted. Case-preserving comparison is the safe default: it can only refuse to match a case-mismatched candidate, never accept one the real host would reject. The commit comment explains this reasoning in detail.

Logic trace for the case-mismatch fixture (credential_roots: "/home/Runner", host_expanded: "/home/runner/.ssh/id_rsa"):

1. normalizeHostPath("/home/runner/.ssh/id_rsa") → /home/runner/.ssh/id_rsa (POSIX, case preserved)
2. normalizeHostPath("/home/Runner") → /home/Runner (POSIX, case preserved)
3. "/home/runner/.ssh/id_rsa".startsWith("/home/Runner/") → false (lowercase r ≠ uppercase R)
4. Result: denied → exit 1, "does not resolve under any configured --credential-roots" ✅

---

Codex Finding 2 — Reject relative credential root containment: ✅ FIXED

What changed: normalizeHostPath now returns null when the path has no POSIX root or Windows drive anchor (i.e., anchor === "" → return null instead of return out.join("/")). The function comment explains the rationale: a relative path names no stable host location — it resolves against an arbitrary working directory — so it must fail closed rather than produce a bare comparable string.

Why this is correct: The old behavior let a configured root secrets produce "secrets" as a normalized value, which would then contain candidate "secrets/not-real" via startsWith. On any host, secrets/not-real resolves relative to whatever $CWD is at runtime — it's not a stable credential store location. Returning null forces these through pathUnderConfiguredRoot's null-guard (normCandidate === null → false), ensuring they're always denied.

Logic trace for the relative-root fixture (credential_roots: "secrets", path: "secrets/not-real", host_expanded: "secrets/not-real"):

1. Coherence guard (credentialExpansionProblem): entry and expanded are lowercase-equal → passes (no coherence problem)
2. pathUnderConfiguredRoot("secrets/not-real", ["secrets"])
3. normalizeHostPath("secrets/not-real") → null (no anchor → anchor === "" → return null)
4. normCandidate === null → return false
5. Result: denied → exit 1, "does not resolve under any configured --credential-roots" ✅

---

No regression in previously correct areas

Area	Verdict
normalizeHostPath("/") still returns "/", caught by normRoot === "/" guard	✅ unchanged
normalizeHostPath("//etc/...") → null (UNC rejection)	✅ unchanged
normalizeHostPath("/home/runner/.ssh/../id_rsa") → /home/runner/id_rsa (dot-collapse)	✅ unchanged
normalizeHostPath("C:/Users/Alice") → c:/users/alice (drive-letter fold)	✅ unchanged, regex widened to [a-zA-Z] to accept uppercase driver letter before fold
normalizeHostPath("/home/runner/.ssh/id_rsa") → /home/runner/.ssh/id_rsa (POSIX case preserved)	✅ new behavior, correct
/-root guard in pathUnderConfiguredRoot (from prior fd3b5c7 fix)	✅ unaffected — still normRoot === "/"
Segment-boundary prefix (/home/runner-x vs root /home/runner)	✅ unaffected — still startsWith(normRoot + "/")
Empty-after-parse credentialRoots → null (from prior fd3b5c7 fix)	✅ unaffected

One subtle point confirmed: The coherence guard (credentialExpansionProblem, lines 298–299) still lowercases internally via entry.toLowerCase() and expanded.toLowerCase(). This is consistent and correct for its purpose — it only checks structural properties (tail-consistency, UNC rejection, ephemeral segment detection), not containment under a root. Containment is gated separately in pathUnderConfiguredRoot with the now-case-preserving normalizeHostPath. The two functions compose correctly.

---

Status of all findings across the full review history

#	Finding	Status at HEAD e9f7abf
1 (review)	Misleading env-token guidance (L255 + comment)	✅ Fixed in fd3b5c7
2 (review)	Empty-roots edge case → confusing empty-parens error	✅ Fixed in fd3b5c7
3 (security-review, CONFIRMED)	--credential-roots / silently accepts every absolute path	✅ Fixed in fd3b5c7 with regression fixture
4 (security-review, PLAUSIBLE)	Concrete paths with ../. segments — diagnostic quality only	Deferred by design (prior triage), follow-up issue planned
Codex P2	POSIX credential roots widen on case-sensitive hosts	✅ Fixed in e9f7abf with regression fixture
Codex P2	Relative credential root secrets contains secrets/not-real	✅ Fixed in e9f7abf with regression fixture

---

All open findings are resolved. No new issues introduced by this commit. Ready to merge.

[claude]

Claude finished @kyle-sexton's task in 4m 38s —— View job

---

Security Review — PR #947: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather PR context and prior review history
• Diff latest commit (e9f7abf — "fail closed on case-mismatched and relative credential roots")
• Read core checker at HEAD
• Verify prior open findings are addressed (Codex P2: case-sensitivity, relative roots)
• Audit new code for any introduced security issues
• Post findings

---

Scope: The e9f7abf sync commit, reviewed at HEAD. Five files changed: check-security-binding.mjs, the fixture manifest, the new ci-pool-a-l2-relative-credpath.json transcript, and the two updated fixture envelopes (probe-evidence-credroots-case-mismatch.json, probe-evidence-credroots-relative-root.json). No CI workflow changes in this commit.

Methodology: Full trace through normalizeHostPath → pathUnderConfiguredRoot for both Codex findings; audit of the coherence guard / containment interaction under the new case-handling rules; review of new fixtures for correctness.

---

Codex P2 findings — status at HEAD

Case-sensitivity on POSIX hosts ✅ FIXED

normalizeHostPath lines 190–217

Prior code called .toLowerCase() on the entire path before any normalization; this meant a configured root /var/Creds and a candidate /var/creds/not-real compared identically — a case-mismatched path was silently accepted on a case-sensitive POSIX host.

The fix:

const slashed = p.replaceAll("\\", "/");
// ...
const cased = driveMatch !== null ? slashed.toLowerCase() : slashed;

POSIX paths now preserve case throughout normalization. Windows drive-letter paths (case-insensitive on their host) are still folded. Since the checker cannot observe the probing host's platform, preserving case is the safe default — it can only ever refuse a case-mismatched candidate, never accept one the real host would reject.

The new probe-evidence-credroots-case-mismatch.json fixture (manifest: credential_roots: "/home/Runner") uses the existing ci-pool-a-l2-id-rsa.json transcript whose host_expanded is /home/runner/.ssh/id_rsa (lowercase r). Under the new code, normalizeHostPath("/home/runner/.ssh/id_rsa") → /home/runner/.ssh/id_rsa; root normalizeHostPath("/home/Runner") → /home/Runner; startsWith("/home/Runner/") → false → denied. Fixture expects exit: 1. Correct.

Relative credential root containment ✅ FIXED

normalizeHostPath line 204

Prior code reached if (anchor === "") return out.join("/") — an unanchored (relative) input returned a bare string. With --credential-roots secrets and a transcript host_expanded: "secrets/not-real", both normalized to relative strings and "secrets/not-real".startsWith("secrets/") evaluated true — a silent bypass.

The fix adds:

if (anchor === "") return null;

Relative paths (no / or drive-letter anchor) now return null. pathUnderConfiguredRoot already treats a null candidate as no-match at line 225. A relative root also normalizes to null, so roots.some(...) never fires for it. Both directions are independently denied.

The new probe-evidence-credroots-relative-root.json fixture (manifest: credential_roots: "secrets") uses ci-pool-a-l2-relative-credpath.json (transcript host_expanded: "secrets/not-real"). Both root and candidate normalize to null → denied. Fixture expects exit: 1. Correct.

---

No new security issues found

Explicitly verified:

Area	Verdict
Relative candidate with absolute root (e.g. host_expanded: "secrets/not-real", root /home/runner)	normCandidate = null → denied. Was already safe under old code because a relative string never starts with an absolute root; now explicitly null.
Drive-letter root C:/ (Windows drive root) — equivalent of POSIX / bypass	normalizeHostPath("C:/") → "c:/". normRoot === "/" guard doesn't fire, but startsWith("c://") is never true for real paths like "c:/users/...", so the root is inert rather than a bypass — fail-closed, not fail-open.
Interaction between case-insensitive coherence guard and case-sensitive containment	credentialExpansionProblem still calls .toLowerCase() on both entry and expanded for its internal checks (tail-consistency, dot-segment, UNC, rootedness). The coherence guard is strictly a pre-filter; any path it passes then faces the stricter case-preserving containment check. A case-mismatched host_expanded that passes coherence (e.g. /home/runner/.SSH/ID_RSA with tail /.ssh/id_rsa in lowercased form) is still accepted only if the case-preserved expanded value starts with the case-preserved configured root. No bypass is introduced; the pipeline is fail-closed for any root/candidate case mismatch.
UNC bypass, .. traversal, /-root bypass, empty-roots-after-parse, segment-boundary prefix	All previously verified and unchanged in this commit.
GitHub Actions CI	No workflow changes in e9f7abf.

---

Both Codex P2 findings are correctly fixed and regression-locked with new fixtures. No new security issues introduced. Secure to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9f7abf768

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 23s —— View job

---

PR #947 Review: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather context (PR description, prior review history, new sync commit)
• Read updated checker logic at HEAD (f4cf324)
• Review drive-root fix and new fixture
• Verify all prior findings remain resolved
• Synthesize findings and post review

---

What this sync adds

The latest commit (f4cf324) addresses the Codex P2 finding from the e9f7abf review: bare drive roots passed as --credential-roots C:/ could satisfy containment via the equality case and accept a failed drive-root read as L2 credential-absence evidence.

---

Codex P2 Finding — Bare drive root as credential root: ✅ FIXED + regression fixture

The vulnerability: normalizeHostPath("C:/") returns "c:/". The prior guard in pathUnderConfiguredRoot checked only normRoot === "/" — "c:/" is not POSIX /, so it slipped through. With both candidate and root normalizing to "c:/", normCandidate === normRoot → true, so the equality arm accepted a failed read of the entire drive as proof of credential absence.

The fix (check-security-binding.mjs line 235):

if (normRoot === null || normRoot === "" || normRoot === "/" || /^[a-z]:\/$/.test(normRoot)) return false;

/^[a-z]:\/$/.test("c:/") → true → returns false before the equality branch is reached. A narrower root such as C:/creds normalizes to "c:/creds", which does not match the regex (extra path segments after the slash), and continues correctly to the startsWith("c:/creds/") check.

Edge cases verified by tracing normalizeHostPath:

• C: (no trailing slash): driveMatch regex requires \/ after the drive letter → no match → anchor = "" → returns null → caught by the null guard. ✓
• C:// (double slash): body = /, leading empty segment skipped, out = [], returns "c:/" → caught by the drive-root regex. ✓
• C:/.: body = . → skipped, out = [], returns "c:/" → caught. ✓
• C:/..: out.length === 0 → returns null → caught by null guard. ✓

Regression fixture (probe-evidence-credroots-drive-root.json):

• Transcript: "path": "C:/", "host_expanded": "C:/"
• Manifest: credential_roots: "C:/", exit: 1, findings_substrings: ["does not resolve under any configured --credential-roots"]
• This fixture is load-bearing: before the fix, pathUnderConfiguredRoot("C:/", ["C:/"]) returns true via equality → level accepted as proven → exit 0. After the fix, the drive-root guard fires → false → level fails closed → exit 1. ✓

---

Status of all findings across the full review history

#	Source	Finding	Status at HEAD f4cf324
1	review	Misleading env-token guidance in error + comment	✅ Fixed in fd3b5c7
2	review	--credential-roots "," → [] → confusing empty-parens error	✅ Fixed in fd3b5c7
3	security-review, CONFIRMED	--credential-roots / silently accepts every absolute path	✅ Fixed in fd3b5c7 + regression fixture
4	security-review, PLAUSIBLE	Concrete paths with ../. segments — diagnostic quality only	Deferred by design, follow-up issue planned
Codex P2	e9f7abf	POSIX case folding widens the allowlist on case-sensitive hosts	✅ Fixed in e9f7abf + regression fixture
Codex P2	e9f7abf	Relative credential root secrets accepts secrets/not-real	✅ Fixed in e9f7abf + regression fixture
Codex P2	f4cf324	--credential-roots C:/ accepted via equality case	✅ Fixed in f4cf324 + regression fixture

---

What's correct at HEAD

The combined guard now closes all three "root that accepts everything" shapes in a single line:

normRoot === null || normRoot === "" || normRoot === "/" || /^[a-z]:\/$/.test(normRoot)

• null — UNC, relative paths, root-escape traversals
• "" — relative paths that normalize to empty (already covered by null, belt-and-suspenders)
• "/" — POSIX filesystem root
• /^[a-z]:\/$/.test(...) — Windows drive root (c:/, d:/, etc.)

The equality branch (normCandidate === normRoot) can only be reached by a non-root, non-UNC, non-relative, segment-bounded path — exactly the correct set of paths that are legitimate narrower roots.

All prior security verifications from the review history remain valid. No new issues introduced. Ready to merge.

[claude]

Claude finished @kyle-sexton's task in 3m 47s —— View job

---

Security Review — PR #947: fix(autonomy): add deny-by-default --credential-roots allowlist

• Gather PR context and prior review history
• Read latest commit diff (f4cf324 — "reject bare drive roots as credential roots")
• Audit normalizeHostPath / pathUnderConfiguredRoot for new and residual issues
• Audit credential entry dispatch and coherence guard
• Audit fixture changes and CI workflow
• Post findings

---

Scope: Files changed in this PR, reviewed at HEAD f4cf324a72c7cacc45e8a17e9f0c3757cd8c0b32. The latest commit touches check-security-binding.mjs, the fixture manifest, and one new transcript + fixture envelope.

Methodology: Full trace of the drive-root guard through normalizeHostPath → pathUnderConfiguredRoot → credentialEntryProblem; exhaustive edge-case inventory for the Windows path surface; coherence guard / containment interaction; CI workflow change.

---

Prior findings — all closed

Finding	Status
Misleading env-token guidance (error message + comment)	✅ Fixed in fd3b5c7
Empty-roots edge case → confusing empty-parens message	✅ Fixed in fd3b5c7
--credential-roots / silently accepts every absolute path	✅ Fixed in fd3b5c7
POSIX case folding widens the allowlist on case-sensitive hosts	✅ Fixed in e9f7abf
Relative root secrets contains relative candidate secrets/not-real	✅ Fixed in e9f7abf
Drive root C:/ accepted every path on the whole drive	✅ Fixed in f4cf324

---

No new security issues found

The single code change in f4cf324 extends the existing root-rejection guard in pathUnderConfiguredRoot:

if (normRoot === null || normRoot === "" || normRoot === "/" || /^[a-z]:\/$/.test(normRoot)) return false;

Exhaustive trace of the Windows-root surface at HEAD:

Input to normalizeHostPath as root	Normalized form	Guard result
C:/	c:/	/^[a-z]:\/$/.test("c:/") → rejected ✅
C:\\ (backslash)	c:/	Same ✅
C:/./ (dot-collapse)	c:/	Same ✅
C:// (double slash body)	c:/	Same ✅
C: (no slash)	null (no drive-regex match → no anchor)	normRoot === null → rejected ✅
C:/../ (traversal at root)	null (underflows stack)	normRoot === null → rejected ✅
\\server\share → //server/share	null (UNC prefix)	normRoot === null → rejected ✅
\\?\C:\ → //?/C:/	null (UNC prefix)	normRoot === null → rejected ✅
C:/creds (valid narrower root)	c:/creds	Regex doesn't match → accepted as a valid root ✅
Z:/ (other drive letter)	z:/	Regex matches → rejected ✅

The compound-root case (mix of valid and invalid roots in --credential-roots) also behaves correctly: someRoot.some(...) iterates all roots; an invalid root returns false for its iteration and doesn't block the valid one.

Coherence guard / containment interaction is still correct. credentialExpansionProblem lowercases for its structural checks (tail-consistency, dot-segments, ephemeral-segments, UNC rejection) — this is format validation only. Containment in pathUnderConfiguredRoot runs separately with normalizeHostPath preserving POSIX case, so case-preserving containment is unaffected by the lowercased coherence check.

probe-evidence-credroots-drive-root.json fixture is load-bearing. The manifest entry { "exit": 1, "credential_roots": "C:/", "findings_substrings": ["does not resolve under any configured --credential-roots"] } correctly locks in the denial. Trace: transcript has path: "C:/", host_expanded: "C:/" → coherence passes (verbatim equality holds on lowercased entry) → pathUnderConfiguredRoot("C:/", ["C:/"]) → normRoot = "c:/" → regex fires → returns false → finding reported → exit 1 ✅. The fixture exits 0 (fail-open, accepted as evidence) against the pre-fix code and exit 1 (denied) against the fix — it is a genuine regression test.

CI workflow change (f4cf324 is not in this commit — it appeared in 279042d): adding the fixture manifest to the existing machine-specific-paths exclusion list is narrowly scoped. The workflow uses pull_request (not pull_request_target); all action refs are SHA-pinned; no run: step interpolates untrusted context; permissions: contents: read is unchanged. The exclusion only widens the hygiene scanner's skip list — no security boundary is affected.

---

Summary: No CRITICAL, IMPORTANT, or SUGGESTION-level security findings. All six prior security findings across the review history are verified fixed. The drive-root guard is correct, complete, and regression-locked. Secure to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4cf324a72

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".