Repository navigation
Network/env follow-ups (#177) and runner-owned adapter deadline (#175) - #179
Conversation
… flag value), hard cap 30s (#175)
… PATH, HOME, XDG, PERL5DB, GCONV_PATH, JAVA opts, shell opts...) + parity tests (#177)
…/20 and 5f00::/16; run address lists on generated Go (#177)
… vets every resolved address (#177)
…ge (Proxy-Authorization, custom auth); refuse userinfo redirect targets (#177)
…ss-target mapped-literal cases (#177)
howlcipher
left a comment
There was a problem hiding this comment.
Assurance review: #179 @ 6da850e (closes #177, #175)
Verdict: ready to merge, no blockers. Every #177 item holds on core, gogen and Node, and the #175 cap is correct. go build ./... and the full go test ./... pass locally, and CI build is green.
Verified
- Range table. There's one source,
capability.SpecialPrefixes(destinations.go:16-21). gogen and JS render it, and parity tests compare each rendered table to the source. I ran an independent 38-blocked / 8-public probe on all 3 targets. Denied: CGNAT, 0/8, broadcast, ::7f00:1, ::a00:1, 64:ff9b:1::/48, 6to4 wrapping private, link-local, multicast and broadcast addresses, Teredo (including the inverted client), 198.18/15, 192.0.0/24, fec0::/10, plus loopback, RFC1918, ULA, link-local, NAT64 /96, ::, multicast and mapped forms. Allowed: 8.8.8.8, 2606:4700::1111, 1.1.1.1, ::ffff:8.8.8.8, ::808:808, and 6to4 wrapping public IPs. Teredo is refused outright, which also blocks public IPs inside it. That's an over-block by design and it's documented. - Cross-origin headers. The allowlist (network.go:468-486, gogen.go:963-972, javascript.go:650) keeps 7 safe headers and drops everything else, including non-canonical names like
x-api-key,X-AUTH-TOKENandproxy-authorization. Same-origin redirects keep custom and auth headers. Redirects to URLs with userinfo are refused on all 3 targets. - Env deny-list. It's single-sourced (process.go:94-138), case-insensitive, and has a parity test on each target. The independent
nettest.EnvRefusedlist catches anything dropped from the source table. All requested names and prefixes are refused. PATH can't be granted, and it's moot for choosing the program because argv must be an absolute path. - localhost. checkDial (network.go:285-310) denies localhost resolving to 169.254.169.254, 10/8, mapped 169.254 or a public address, and allows 127/8, ::1 and ::ffff:127/8. A fake-resolver test covers this on core and gogen, and JS checks and pins every resolved address.
- #175.
ParseAdapterDeadlinerefuses 0, negative and >30s.effectiveExecutionPolicyclamps to 30s, and a shorter runner deadline wins.
Mutations (my own, not in the PR's set): 17/19 caught. Survived: core and gogen header allowlist matched case-sensitively. This fails closed, because canonical safe names still match and a non-canonical safe header would only be dropped, so it's a test gap rather than a hole. Suggested fix: add one test with a non-canonical safe key.
Follow-ups (not blocking this PR)
- Exec isn't contained when the deadline hits (vm.go:2521-2531, existing code).
CommandContextplusWaitDelaywith no Setpgid or group kill. With a 500ms deadline,sh -c "setsid sleep & sleep"returned in about 600ms (good, no blocking), but both the setsid grandchild and the same-group background child kept running. This must be fixed before any live run grantsprocess:exec: set Setpgid, kill the group incmd.Cancel, and contain setsid escapees with a cgroup or tracking, unless the harness does that itself. AdapterDeadlineFlagisn't connected to any runner binary yet. It's only used in the library and tests. Please connect it in the #178 runner.- Gaps in the env deny-list. Code loading: OPENSSL_CONF, OPENSSL_MODULES/ENGINES, RUBYGEMS_GEMDEPS, GEM_PATH, PHPRC, R_PROFILE_USER, TCLLIBPATH, GUILE_LOAD_PATH, ERL_LIBS. Low risk: NETRC, CURL_HOME, WGETRC, NPM_CONFIG_, PIP_, GOPROXY/GOSUMDB/GOINSECURE, KUBECONFIG, GLIBC_TUNABLES, SSLKEYLOGFILE, MAKEFLAGS. Longer term, I'd switch
process:env=to a short allowlist, since a denylist will keep leaking.
Assurance (bot-assurance-0001)
howlcipher
left a comment
There was a problem hiding this comment.
COMMENT review at 6da850e (base e36bd2d). Verdict: merge-ready. No blockers; three non-blocking items below.
Gates (fresh checkout): gofmt -l empty (exit 0), go vet ./... exit 0, go test -count=1 ./... exit 0 (75 packages ok, including lock/arity 386/parity/freeze/pin tests and the Node-backed JS tests). No default flip, #90 stays Partial, tip-lock untouched.
Env: One table (CodeLoadingEnvNames + prefixes LD_/DYLD_/GIT_/BASH_FUNC_/LUA_), upper-cased before matching, so path/Path/git_ssh_command are refused. validEnvName rejects =, NUL and other non-identifier bytes. Every name on my list is covered except the proxy variables and TMPDIR (see nit 2).
Redirects: Cross-origin keeps only the 7 RedirectSafeHeaders (case-insensitive), so Authorization, Cookie, Proxy-Authorization and X-Api-Key are dropped; same-origin keeps everything. Go refuses userinfo whenever URL.User != nil, including @, :@ and %40 forms. Node checks every resolved address and rejects the set if any member fails; localhost must be loopback on all three targets. ::ffff:0:0/96 is decoded, and 3fff::/20 and 5f00::/16 are in the shared table.
Deadline: effectiveExecutionPolicy defaults to 1s and clamps the policy to 30s. ParseAdapterDeadline refuses 0, negative and >30s rather than clamping. The policy only comes from the runner ExecutionPolicy; no candidate or transport field reaches it. Artifact runs are unaffected (test covers it).
Mutations spot-checked (3/3 caught): dropping PATH from the table fails TestEnvGrantRefusesCodeLoading, TestGoFetchSecurity and TestJSCodeLoadingEnv; disabling the userinfo check fails TestFetchSecurity; removing the 30s clamp fails TestAdapterDeadlineRunnerOwned.
Breaking changes: all 7 are in change_log.md.
Non-blocking:
-adapter-deadlineisn't registered by any CLI:AdapterDeadlineFlagis only used in a test. Either wire it into the runner (main -run-bc / the hflive harness) or leave #175 open until that happens.- HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXY (and lowercase), NODE_USE_ENV_PROXY, GEM_HOME/GEM_PATH and TMPDIR aren't refused. A granted proxy var would route an allow-listed child's traffic, so consider adding them in a follow-up.
- JS userinfo parity: WHATWG gives
http://@h/andhttp://:@h/an empty username and password, so the JS check accepts them where Go refuses. They carry no credentials so it's harmless, but checking for@in the authority would match Go.
Closes #177. Closes #175.
Follow-ups from the #176 reviews (Lain, Motoko) and the #173 review (Motoko). Every fix has a negative test, and each was mutation-checked: I reverted or weakened the fix locally and confirmed the test failed.
#177
capability.CodeLoadingEnvNames+CodeLoadingEnvPrefixes, rendered into generated Go and JS. It addsGIT_*,BASH_FUNC_*,LUA_*,GCONV_PATH,PYTHONHOME,PERL5DB,PERLLIB,JAVA_TOOL_OPTIONS/_JAVA_OPTIONS/JDK_JAVA_OPTIONS,SHELLOPTS/BASHOPTS/PS4,HOME/XDG_CONFIG_HOME,PATH,GOFLAGS/GODEBUG,NODE_EXTRA_CA_CERTS, TLS trust stores and more. A parity test runs one independent list (nettest.EnvRefused/EnvAllowed) on core, generated Go and Node, and both rendered tables are compared with the capability tables.capability.RedirectSafeHeadersis stripped. That covers Proxy-Authorization and custom auth headers. Same-origin redirects keep all headers.allandfamily), not just the first one.[::ffff:a.b.c.d]: core, generated Go and Node now treat it as the same host asa.b.c.d(nettest.MappedGrantCases).::ffff:0:0/96is decoded, and3fff::/20and5f00::/16are added. A test checks that the generated Go and JS tables equalcapability.SpecialPrefixes, and the blocked and public address lists now run against generated Go too.#175: adds
vm.ExecutionPolicy.AdapterDeadline, which the runner owns. It defaults to 1s and is clamped atvm.MaxAdapterDeadline(30s). A shorterDeadlinestill wins.vm.AdapterDeadlineFlag/ParseAdapterDeadlinegive runners an-adapter-deadlineflag that rejects 0, negative and over-cap values. Artifact runs are unaffected.Breaking changes (see change_log.md):
process:env=HOME,PATH,GIT_*and the other listed names are now refused. Two existing tests that assertedHOMEwas accepted now assertLANGinstead.localhostthat resolves to a non-loopback address is refused.Pins, freeze, lock, arity and parity are unchanged. Local runs of gofmt, vet and
go test ./...are green. Journal: docs/journals/2026-10-13_network_env_followups_adapter_deadline.md