Repository navigation
[security] - Classify gh and apt functional egress, site the macOS and keyring helpers, drop the sandbox punkt fetch - #1646
Conversation
…d keyring helpers, drop the sandbox punkt fetch Codex round eleven on #1637, all four findings confirmed, plus what a token-level sweep of every in-boundary shell surface turned up beside them: - gh's GitHub traffic is functional egress, not only suppressed telemetry: new category-3 component `gh-api` sited on the setup script's `gh auth status` / `gh auth login --with-token` and the agentic `gh pr view` / `gh pr create` argv, beside the category-1 telemetry row (the git / git-remote precedent). SECURITY.md's category-3 sentence names it. Scenario 48 removes both auth-login branches and expects the WARN. - The generated Task Scheduler rows pinned a filename and a which(): cmd.exe now pins the COMSPEC command construction and the `/c <launcher>` argument, powershell the executable resolver and the `-File wrapper` argv. COMSPEC is a path override of the same interpreter (the UV_BIN shape), so it stays a fixed component. Scenario 49 swaps the COMSPEC construction and expects the WARN. - macOS setup/uninstall helpers and the Linux keyring CLI had no rows: expect, osascript, pbcopy, pbpaste, lsof, sw_vers and secret-tool join the local OS tooling row with invocation sites; CYCLAW_SECRET_TOOL is a path override, not a caller-chosen command. The same sweep found the Dockerfile's apt-get (new category-3 row `apt-install`) and its groupadd/useradd (local tooling). Scenarios 50 and 51 remove the expect invocation and the secret-tool lookup. - The sandbox audit's `nltk.download('punkt')` step was obsolete (the stemmer uses PorterStemmer only, no punkt, per SECURITY.md) and would have reached the NLTK data host on a cold cache: removed from .claude/skills/CyClaw-Sandbox/verify.sh; the core-libs row says so. - SKILL.md step 5 records the token-level sweep method and the path-override rule so the next round starts from a complete list. Checks: strict checker 0/0 (44 rows, 95 components, 89 sites / 39 binaries, 4 dynamic sites), verify.sh ALL PASS (55), doc-sync 0 drift, ruff clean, bash -n on both verify scripts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012semJHbJjTtdTy3zrhUnR3
|
PR is a complete fill of |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cf3fced6f
ℹ️ 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".
| The shell sweep is token-level, not name-level: take the first word of | ||
| every command position (after `if`, `!`, `command -v`, `|`, `&&`, `$(` | ||
| and leading `VAR=value` assignments) in every in-boundary `.sh`, the | ||
| Dockerfile's `RUN` lines and the compose file, drop shell builtins, | ||
| coreutils and the file's own functions, and classify what is left. |
There was a problem hiding this comment.
Implement the claimed token-level shell sweep
When an in-boundary shell script starts a new executable, T13 never parses that script as described here; it only checks manifests, the hard-coded KNOWN_EXTERNAL_COMPONENTS, and markers for already-declared sites. In an isolated checkout, adding wget https://example.invalid/probe to scripts/ensure-githooks.sh still made check_otel.py --strict exit 0 with zero warnings, so new unclassified network egress can silently pass the advertised strict classification check. Add the token-level discovery step (and an add-new-binary mutation test), or remove this coverage claim.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
Proposed changes
Follow-up to #1637, which the owner's auto-merge landed at 08:21 ET on 2026-10-13 while Codex's eleventh review pass was still running against its last head. That pass posted four P2 findings on the merged PR; this PR carries the one commit that answers them, cherry-picked onto fresh main (the merged branch is left as it was: no history rewrite). All four findings were verified against the source and confirmed, and a token-level sweep of every in-boundary shell surface found more of the same shape beside them, so the fix is the complete set rather than the four instances.
gh's GitHub traffic is functional egress, not only suppressed telemetry. The setup script's
gh auth status/gh auth login --with-tokenand the agenticgh pr view/gh pr createargv were sited only under the category-1GH_TELEMETRYrow, so the inventory reported the auth flow and the PR operations purely as telemetry that is switched off. New category-3 componentgh-apiwith its own row and sites, beside the telemetry row, the git / git-remote precedent.SECURITY.md's category-3 sentence names it.verify.shscenario 48 removes bothgh auth loginbranches and expects the WARN on the new row.The generated Task Scheduler rows pinned a filename and a
which().utils/win_schtasks.py'scmd.exerow matched only the.cmdfilename construction and thepowershellrow onlyshutil.which("powershell"), so replacing either generated child command left strict T13 green.cmd.exenow pinscommand=os.environ.get("COMSPEC", "cmd.exe")and the/c <launcher>argument;powershellpins thepowershell/pwshresolver, theps = powershell or powershell_executable()selection and the-File wrapper_pathargv.COMSPECis the OS-owned variable naming where the same interpreter lives, theUV_BINshape the owner ruled on in #1637, so it stays a fixed component rather than a dynamic row. Scenario 49 swaps the construction for a literal and expects the WARN.The macOS setup helpers and the Linux keyring CLI had no rows. Codex named
/usr/bin/expect,osascript,pbcopy/pbpasteand theCYCLAW_SECRET_TOOLpath. Rather than site those four, I swept the first word of every command position (afterif,!,command -v, pipes,&&,$(and leadingVAR=valueassignments) in every in-boundary.sh, the Dockerfile'sRUNlines and the compose file, minus builtins, coreutils and each file's own functions. That also surfacedlsof,sw_vers, and the Dockerfile'sapt-get,groupaddanduseradd. The local tools join the category-4 local-OS-tooling row with invocation-literal sites (expecton the heredoc and itsspawn -noecho /usr/bin/security add-generic-password;osascripton both AppleScript runs;pbcopy/pbpasteon the copy, the readback and the clear;lsofon the listener lookup in setup-keys and uninstall;sw_vers -productVersionin three scripts;secret-tooloncommand -v, lookup, store and clear).apt-getis a new category-3 rowapt-install(Debian mirror access at image-build time, version-pinned, nothing at runtime).CYCLAW_SECRET_TOOLis a path override of the same binary, sosecret-toolis fixed, not dynamic. Scenarios 50 and 51 remove the expect invocation and the keyring lookup. SKILL.md step 5 records the token-level method and the path-override rule so the next sweep starts from the full list.The sandbox audit fetched NLTK punkt on every run.
.claude/skills/CyClaw-Sandbox/verify.sh(inside the swept boundary) rannltk.download('punkt')andnltk.download('punkt_tab')unconditionally, which reaches the NLTK data host on a cold cache. The stemmer usesPorterStemmeronly,SECURITY.mdlists anynltk.download()as a security regression, and the core-libs row's evidence says punkt is avoided by design, so the step was removed rather than classified; the row records the removal. The.codex/skills/Cyclaw-Sandbox/verify.shmirror has the same line; it is agent tooling outside the inventory boundary the owner drew in #1637 and was not touched here.Invariant / Governance Impact: none of the six invariants or I6 are touched. The diff is the checker's inventory data (two new rows, nineteen new site rows, eleven new component names), its self-test (four scenarios), the skill text, one sentence of
SECURITY.md's egress classification, and one obsolete line removed from the sandbox audit script. No production map, kill value or scrub name changed;CONTRACT_DIGESTis unaffected; no core file moved.Agent-instruction surface (the pre-commit security gate flagged this, correctly): the skill files steer later agent sessions. Every row's text is my own paraphrase of what the cited file does; no web, issue or tool text was copied in. The gate's secure-by-design question about a new config input points at the
COMSPECandCYCLAW_SECRET_TOOLmentions in the checker and self-test: both are documented in rows as path overrides read by existing scripts, and the checker itself reads neither.Types of changes
Scope note: Infrastructure / CI (the
verify-skillschecker's egress inventory and its self-test), one sentence ofSECURITY.md, and one obsolete download line removed from the sandbox audit script; no server runtime path, no operator script changed.Benefits / why
Risks to monitor
utils/(only the checker's view ofutils/win_schtasks.pychanged),gate*.py,graph.py, or the launchers the server depends on changed. A regression in the checker is a reporting regression, not a telemetry leak.lsof,pbcopyorosascriptline trips a T13 WARN that must be re-pointed. Signal:verify-skills (otel-hardening)going yellow on an unrelated PR.verify-skills (CyClaw-Sandbox)leg is the check.verify.shcopies now differ..codex/skills/Cyclaw-Sandbox/verify.shstill has the punkt line; it is outside the boundary by the owner's decision, and this PR says so rather than editing a Codex-agent file. If the owner wants the mirror cleaned too, that is a one-line follow-up.verify-skillsiscontinue-on-errorinci.yml, so a redcheck_otel.pyleg would not block a merge. Pre-existing; the green reported below is the only gate this PR actually has.Checklist
docs/THREAT_MODEL.md,INVARIANTS.mdandSECURITY.md(and any relevant Phase docs)GROK_API_KEY=dummy pytest tests/ -q --tb=short, andbash .claude/skills/CyClaw-Sandbox/verify.shfor core RAG/agentic paths) and passes with no regressions — deliberately not run, per CLAUDE.md §5's verification policy; CI runs the suites. The checks that exercise this diff were run directly and are listed under Further comments.SECURITY.md's egress classification updatedFurther comments
Checks run directly on the pushed head (all green):
python3 .claude/skills/otel-hardening/check_otel.py --strict --as-of 2026-10-13→ 0 failures, 0 warnings; 44 inventory rows, 95 declared components resolve, 89 fixed launcher sites (39 binaries) and 4 dynamic sites carry their invocation markers.bash .claude/skills/otel-hardening/verify.sh→ ALL PASS (55 mutation scenarios; 48–51 are new). Run on the identical file set before the cherry-pick onto main, whose only additional change since the merged head is two documentation files.python3 .claude/skills/doc-sync/doc_sync.py→ 0 drift items, re-run on the new branch.ruff check --select E,F,I,B,C4,UP,S --ignore E501on the checker → clean;bash -non bothverify.shfiles;bash scripts/check-pr-template.shon this body → OK.macos/*.sh,scripts/*.sh,windows/, the Dockerfile,docker-compose.yml,cyclaw-gotchas/driver.shandCyClaw-Sandbox/*.sh, plus a curated grep for forty common macOS/Linux tool names over the same surfaces and the PowerShell and Python files beside them.Only CI has run: the pytest suites and the 80% coverage gate (this diff adds no Python under coverage sources), and the
verify-skills (CyClaw-Sandbox)leg that exercises the sandbox script the punkt line was removed from.Not run, and why: the four telemetry pytest files the skill's step 4 names; they pin
utils/telemetry_kill.py's maps, the Dockerfile/compose delivery and the ONNX seam, none of which this PR touches. No operator script changed, so none was executed.Review threads: the four Codex threads this PR answers live on the merged #1637; each has a reply pointing here and is resolved there.
Body layout note:
pr-template-check.ymlrequires theLast updatedstamp to be the body's last non-blank line, so the Claude Code attribution block sits just above it rather than after it.Suggested merge order of open PRs
Trial merge verified: the branch is one commit on top of main @ b3771da (cherry-picked clean on 2026-10-13); nothing else open touches these files.
This PR · Safe ✅
Impact: Checker inventory and self-test, skill text, one
SECURITY.mdsentence, one line removed from the sandbox audit script; no server runtime path. Independent, only open PR.Action: Merge when CI is green.
ELI5
The big guest-list PR (#1637) merged while the reviewer bot was still looking at it, and its last look found four more things. Instead of opening the merged PR back up, this small one carries the answer.
First,
ghlogging into GitHub and opening pull requests is real GitHub traffic the project does on purpose, so it now has its own "on purpose" entry next to the existing "we switched its telemetry off" entry, the same waygit clonegot its own entry last time. Second, the entries for the generated Windows scheduled tasks pointed at the file name and a lookup rather than the line that actually builds the command, so they have been re-pointed. Third, the Mac setup scripts run a handful of small helper programs (one that types a password into the Keychain prompt, one that fills the browser's key box, the clipboard, the port-checker, the version reader) and the Linux build uses a keyring tool; none of them were on the list. Rather than add only the ones the bot named, I listed every command word in every setup script and found a few more the same way, including the container build's package installer. Fourth, the sandbox test script was still downloading a language-toolkit file the project stopped needing long ago, which the security notes call a regression; that line is gone.What could still go wrong: more entries means more chances an innocent edit to a setup script makes the checker grumble until someone re-points a line. The sandbox script lost a step it should not have needed; if it turns out it did, the sandbox CI leg says so. Nothing in the running server changed.
🤖 Generated with Claude Code
https://claude.ai/code/session_012semJHbJjTtdTy3zrhUnR3
Last updated: 2026-10-13 05:00 ET