Add a backtick to the hostile tmpdir axis and fix the remaining env-sensitive tests - #723
Merged
Conversation
…sensitivity failures (internal/cmd suite passes under backtick-hostile and normal TMPDIR on darwin; test+CI-lane change) The tmpdir axis in run-validate-in-validator.sh composed TMPDIR from a space, a literal $HOME, a --prefixed component, and ~80 padding characters, but carried no shape that a command-substitution-sensitive re-interpolation would run. Added a backtick to the "hostile $HOME" segment (now "hostile $HOME `id` --hostname/pppp..."). A backtick is a legal filename character, so mkdir -p still creates the path; it is only hostile to code that re-interpolates the path unquoted. The other three axes (workspace, root, uidmap) are byte-identical. Reproduced locally by pointing TMPDIR at that exact string and running `go test ./internal/... ./cmd/...` on darwin. Three tests failed, all already flagged out-of-scope by PR #720 and all failing the same way under any hostile TMPDIR, not specifically the backtick: - TestPrePushHookForwardsTransportAuthentication (git_hooks_test.go): wrote the fake-ssh script under fixture.homeDir. Git re-parses GIT_SSH_COMMAND's value as a shell command line, so a space in that path split it into two words. Moved the script to testkit.ExecFixtureDir(t), the merged helper for exactly this "git/cmd/shell actually invokes this" fixture shape, which is whitespace-free by construction. - TestHostedControlsVerifierScopesDummyTokenToGitHubCommands (hosted_controls_token_isolation_test.go): spliced the fixture's GO_BIN/GH_BIN/JQ_BIN paths directly into a double-quoted assignment inside generated shell script text (not a shell-quoted variable), so the backtick in the hostile root ran as a live command substitution when bash parsed the script, corrupting GO_BIN and falling back to the real go, which then legitimately failed to find ./cmd/workcell-citools under the fixture root. Moved just those three binaries (not the fixture root itself, which is only ever read through a quoted variable) to testkit.ExecFixtureDir(t). - TestVerifyReleaseArtifactPinsRequestedIdentities (release_verify_test.go): built a redirect ("> " + record) with an unquoted TempDir path. Wrapped it with the existing shQuote helper (release_outputs_verify_test.go), the established single-quoting idiom in this package for exactly this case. Triage rule matches the merged PRs: each of these three lived in TMPDIR incidentally rather than testing hostile-input handling, so they became TMPDIR-agnostic rather than keeping a hostile fixture. Full `go test ./internal/... ./cmd/... ./tests/...` passes on darwin both under this exact backtick-hostile TMPDIR and under a normal one. No other test in the suite regressed from the backtick: unlike the ~27 additional failures anticipated from prior analysis, none materialized in the Go suite on darwin (internal/host/release only fails when TMPDIR is nested under a home directory carrying a pre-existing macOS ACL, unrelated to this axis or the backtick, and was excluded by testing from a /tmp-rooted hostile path instead, matching how the real script roots TMPDIR under /tmp/workcell-home-<uid>). The containerized Linux hostile lane itself (docker, shellcheck, the installer scripts) is not exercised here and remains CI-only; this PR does not change its advisory continue-on-error status. gofmt -l ./internal: empty. go vet ./...: clean. go build ./...: clean.
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. |
Owner
Author
|
@codex review |
![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: 7f1a49fa79
ℹ️ 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".
…nd a backtick assertion (full internal/cmd suite passes under normal and backtick-hostile TMPDIR on darwin; test+CI-lane change) Two accepted findings from review of PR #723: 1. ExecFixtureDir (internal/testkit/exec_fixture_dir.go) rejected a candidate base only for whitespace, not for shell metacharacters. A writable, exec-capable XDG_RUNTIME_DIR containing a backtick or $ (no whitespace) would still be selected, and several migrated callers then splice the returned path into generated shell script text. Hardened the acceptance check to an allowlist (^[A-Za-z0-9_./-]+$) rather than a denylist of named metacharacters, since XDG_RUNTIME_DIR is ambient and not under this package's control. A rejected base now falls through to the next candidate. Verified by exporting XDG_RUNTIME_DIR to a writable, exec-capable directory whose path holds a literal backtick and no whitespace, and confirming ExecFixtureDir now skips it. As defense in depth, also quoted every splice of an ExecFixtureDir-derived path into generated shell script text with the existing ShellQuote helper, so a caller does not depend solely on the invariant above: internal/testkit/hosted_controls_token_isolation_test.go (GO_BIN/GH_BIN/JQ_BIN), internal/testkit/security_boundary_test.go (the malicious diff.external marker), internal/host/c3certify/certify_test.go (the hook marker), and three sites in internal/startupbench/bench_test.go (a shared operations log and two process-lifecycle markers). Splices that pass the path through an env var instead (GIT_SSH_COMMAND, BASH_ENV, WORKCELL_CACHEPROG_MARKER) were left alone: the value is read back with a runtime "${VAR}" expansion rather than re-parsed as literal script source, so quoting there is a no-op at best. 2. TestHostileAxesKeepTheShapesThatReproducedFindings (internal/testkit/local_docker_parity_test.go) asserted every earlier hostile shape of the tmpdir axis but had no assertion for the backtick PR #723 added, so a future edit could drop it silently. Added a strings.Contains(tmpdir, "`") assertion in the same style as the existing shape checks. Confirmed it fails without the backtick by reverting the axis line locally, running the test, and restoring the file. go test ./internal/testkit/ ./internal/host/c3certify/ ./internal/startupbench/ and the full ./internal/... ./cmd/... ./tests/... suite pass on darwin under both a normal TMPDIR and the exact backtick-hostile TMPDIR from PR #723. gofmt -l ./internal: empty. go vet ./...: clean. go build ./...: clean. bash -n on the axis script: clean.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.
Finishes the hostile-env lane: the tmpdir axis now includes a backtick, and the tests it exposed are fixed so all four hostile axes (tmpdir, workspace, root, uidmap) can go green.
The backtick
scripts/ci/run-validate-in-validator.shcomposed the tmpdir axis's TMPDIR from a space, a literal$HOME, a---prefixed component, and ~80 padding characters, but nothing that a command-substitution-sensitive re-interpolation would run. Added a backtick to the "hostile $HOME" segment. It's a legal filename character, somkdir -pstill creates the path; it's only hostile to code that re-interpolates the path unquoted. The other three axes are byte-identical.What broke, and the fix
Reproduced locally on darwin by pointing
TMPDIRat the exact backtick-hostile string and runninggo test ./internal/... ./cmd/.... Three tests failed — the same three flagged out-of-scope in PR #720, and (confirmed by testing without the backtick) they fail under any hostile TMPDIR shape, not specifically the backtick:TestPrePushHookForwardsTransportAuthentication(git_hooks_test.go): wrote a fake-ssh script under the fixture's home dir. Git re-parsesGIT_SSH_COMMAND's value as a shell command line, so a space in that path split it into two words. Moved the script totestkit.ExecFixtureDir(t).TestHostedControlsVerifierScopesDummyTokenToGitHubCommands(hosted_controls_token_isolation_test.go): splicedGO_BIN/GH_BIN/JQ_BINpaths directly into a double-quoted assignment inside generated shell script text, so the backtick in the hostile root ran as a live command substitution when bash parsed it, corruptingGO_BINand silently falling back to the realgo. Moved just those three fixture binaries totestkit.ExecFixtureDir(t).TestVerifyReleaseArtifactPinsRequestedIdentities(release_verify_test.go): built a shell redirect from an unquoted TempDir path. Wrapped it with the existingshQuotehelper already used for this inrelease_outputs_verify_test.go.Triage rule matches the merged PRs: each of these three lived in TMPDIR incidentally rather than testing hostile-input handling, so they became TMPDIR-agnostic instead of keeping a hostile fixture.
Verification
go test ./internal/... ./cmd/... ./tests/...passes on darwin both under this exact backtick-hostile TMPDIR and under a normal TMPDIR.gofmt -l ./internalis empty,go vet ./...andgo build ./...are clean.No other test regressed from the backtick itself — the ~27 additional failures anticipated from prior analysis did not materialize in the Go suite on darwin. (One package,
internal/host/release, does fail if TMPDIR is nested under a home directory carrying a pre-existing macOS ACL — an artifact of my local machine, unrelated to this axis or the backtick — so I validated from a/tmp-rooted hostile path instead, matching how the real script roots TMPDIR under/tmp/workcell-home-<uid>.)The containerized Linux hostile lane itself (docker, shellcheck, the installer scripts) is not exercised here and remains CI-only — that's what the hostile-env GitHub Actions job will confirm. This PR does not change the lane's advisory
continue-on-error: truestatus; promoting it to a required check is a follow-up for a separate decision.