fix(security): adversarial review findings — hydrator RCE + name collision + CI dispatch idempotency - #43
Conversation
…sion, dispatch idempotency Adversarial review (via chittycanon-code-cardinal subagent) of the chittymarket refactor found three high-priority issues. This PR addresses them. ## Finding #5 (CRITICAL): hydrate-pointers.sh heredoc RCE sink Previous: fetched body interpolated into a triple-quoted Python literal via shell heredoc. Body containing `"""` or arbitrary Python could escape the literal and execute in CI with `contents: write` + `pull-requests: write` permissions. Fix: fetched content goes to a tempfile, then handed to Python via environment variable (`FETCHED_FILE`) — NOT shell interpolation. The Python block reads the file with .read_text(). No injection path. ## Finding #2 (HIGH): pointer URL is mutable and unpinned Previous: `prompt_url` could point at any URL; no allowlist; no content hash; Monday cron auto-PRs whatever upstream returned. Fix: - Allowlist of source hosts (currently: raw.githubusercontent.com only) - prompt_sha (SHA-256 hex) REQUIRED on every pointer; hydrator verifies actual fetched-content hash matches the pin before writing - --max-filesize 1048576 cap on curl to bound DoS surface - Added prompt_sha: c95066f87361ef9d4909b6bc0fad58f49c2acb424d7927a779bb3b9779db289c to plugins/chittyos-core/agents/chittyagent-schema.md (current chittyfoundation/chittyschema content hash) New exit codes: 3=disallowed-host, 4=missing-prompt-sha. ## Finding #3 (HIGH): CI didn't re-run dispatch.sh sync Previous: pre-commit drift hook ran dispatch.sh, but was local-only and trivially bypassed with `--no-verify`. The CI workflow ran lint + test + manifest-idempotency but NEVER ran `dispatch.sh sync` to check for projection/canonical divergence. Fix: new "Dispatch idempotency" step in validate-chittymarket.yml. Re-runs dispatch.sh sync; fails the PR if any canonical/ or plugins/ file changed. Closes the --no-verify bypass at PR time. ## Finding #1 (HIGH-LATENT): cross-kind name collision Previous: dispatch.sh's canonical lookup is first-match-wins by basename across kind-subdirs (agents → skills → commands → mcp → hooks). Two canonicals sharing a stem (canonical/agents/foo.md + canonical/skills/foo.md) would silently project only the first; the second becomes a no-op no-error orphan. Fix: new lint-plugins.sh section 8b cross-checks canonical basenames across all kind-subdirs and errors on any collision. ## NOT in this PR (deferred to follow-up) Findings still open from the adversarial review: - #4 No actual codex/openclaw runtime tests — needs runtime test harnesses - #6 Race conditions in dispatch.sh (no flock), no orphan cleanup, no enforcement for declared-but-unimplemented runtimes - #7 evidence-gate hole — only checked when capabilities.generated.json is in the staged diff - #8 YAML flow-style runtimes silently produce empty list Local: lint clear, 64/64 tests, manifest idempotent, dispatch idempotent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 43 minutes and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adda52b121
ℹ️ 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".
| echo "$collisions" | while IFS= read -r line; do | ||
| red " ERROR: canonical name collision: $line" | ||
| ERRORS=$((ERRORS + 1)) |
There was a problem hiding this comment.
The new cross-kind collision block prints errors but does not actually fail linting, because echo "$collisions" | while ... runs the loop in a subshell in Bash, so ERRORS=$((ERRORS + 1)) is lost when the loop exits. As a result, commits with canonical basename collisions can still pass scripts/lint-plugins.sh and CI despite the new guard, defeating the intended protection against dispatch shadowing.
Useful? React with 👍 / 👎.
| [ "$NO_SHA" -gt 0 ] && exit 4 | ||
| [ "$DISALLOWED" -gt 0 ] && exit 3 |
There was a problem hiding this comment.
The script now exits with 3/4 for disallowed hosts and missing prompt_sha, but the hydrate workflow only treats exit code 2 as failure and otherwise continues after set +e (.github/workflows/hydrate-pointers.yml, lines 39-43). That means these new security failures are silently treated as success in CI (no failure and no PR), so pointer integrity violations can go unnoticed.
Useful? React with 👍 / 👎.
| m = re.match(r"^---\n(.*?)\n---\n(.*)$", text, re.DOTALL) | ||
| if not m: | ||
| # Verify SHA-256. | ||
| actual_sha=$(shasum -a 256 "$tmp" | awk '{print $1}') |
There was a problem hiding this comment.
Hash verification is hard-coded to shasum -a 256, which is not guaranteed on all Linux environments (especially minimal containers where sha256sum is present but shasum is not). Because the script runs with set -euo pipefail, missing shasum causes an immediate non-semantic failure before the intended exit-code handling, making hydration fail even for valid pointers.
Useful? React with 👍 / 👎.
| run: | | ||
| chmod +x plugins/chittyagent-dispatch/scripts/dispatch.sh plugins/chittyagent-dispatch/scripts/adapters/*.sh | ||
| bash plugins/chittyagent-dispatch/scripts/dispatch.sh sync > /dev/null | ||
| if ! git diff --quiet -- canonical/ plugins/; then |
There was a problem hiding this comment.
The new idempotency gate relies on git diff --quiet -- canonical/ plugins/, which only detects changes to tracked files and ignores newly created untracked files. If dispatch.sh sync materializes missing projection/state files (for example after adding a canonical), this check can still pass even though regeneration produced filesystem drift, so CI can miss exactly the divergence this guard is meant to catch.
Useful? React with 👍 / 👎.
…/chittyschema#67 (#44) Upstream PR chittyfoundation/chittyschema#67 (merge 0f3b12e) renamed chittyconnect-concierge -> chittyagent-connect in the chittyschema-overlord agent body to align with chittymarket PR #29 family-prefix slug rename. New content sha256: e344f1df5383cd4b20621edc8b58c9325bb4c6895e19d74363c1a09cb2a5adb1 The hardened hydrator (PR #43) verified the upstream fetch matches this pin and refreshed the local cache. Local: lint clear, hydrator idempotent on re-run. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
fix(security): adversarial review findings — hydrator RCE, name collision, dispatch idempotency
Adversarial review (via chittycanon-code-cardinal subagent) of the chittymarket
refactor found three high-priority issues. This PR addresses them.
Finding #5 (CRITICAL): hydrate-pointers.sh heredoc RCE sink
Previous: fetched body interpolated into a triple-quoted Python literal via
shell heredoc. Body containing
"""or arbitrary Python could escape theliteral and execute in CI with
contents: write+pull-requests: writepermissions.
Fix: fetched content goes to a tempfile, then handed to Python via
environment variable (
FETCHED_FILE) — NOT shell interpolation. The Pythonblock reads the file with .read_text(). No injection path.
Finding #2 (HIGH): pointer URL is mutable and unpinned
Previous:
prompt_urlcould point at any URL; no allowlist; no contenthash; Monday cron auto-PRs whatever upstream returned.
Fix:
actual fetched-content hash matches the pin before writing
to plugins/chittyos-core/agents/chittyagent-schema.md (current
chittyfoundation/chittyschema content hash)
New exit codes: 3=disallowed-host, 4=missing-prompt-sha.
Finding #3 (HIGH): CI didn't re-run dispatch.sh sync
Previous: pre-commit drift hook ran dispatch.sh, but was local-only and
trivially bypassed with
--no-verify. The CI workflow ran lint + testdispatch.sh syncto check forprojection/canonical divergence.
Fix: new "Dispatch idempotency" step in validate-chittymarket.yml.
Re-runs dispatch.sh sync; fails the PR if any canonical/ or plugins/
file changed. Closes the --no-verify bypass at PR time.
Finding #1 (HIGH-LATENT): cross-kind name collision
Previous: dispatch.sh's canonical lookup is first-match-wins by basename
across kind-subdirs (agents → skills → commands → mcp → hooks). Two
canonicals sharing a stem (canonical/agents/foo.md + canonical/skills/foo.md)
would silently project only the first; the second becomes a no-op no-error
orphan.
Fix: new lint-plugins.sh section 8b cross-checks canonical basenames
across all kind-subdirs and errors on any collision.
NOT in this PR (deferred to follow-up)
Findings still open from the adversarial review:
enforcement for declared-but-unimplemented runtimes
is in the staged diff
Local: lint clear, 64/64 tests, manifest idempotent, dispatch idempotent.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com