You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Four copies of the same escaping routine, and only one of them finished. internal/richtext had the good one; internal/auth, internal/config and internal/commands each carried their own, written before there was a shared home. The way this surfaced is the argument for closing it: one copy's doc comment had drifted into an unmatched parenthesis and a sentence that stopped mid-clause, and all four spelled the POSIX splice with a typographic quote — not a typo but gofmt rewriting the escape in doc-comment prose every time somebody fixed it. Fixing one copy taught nothing about the other three.
Four copies of an escaping routine is how one of them drifts, and the one that drifts is the one interpolating a value that came from the API. richtext.ShellQuote is now the only one, and every caller is on it.
The copies were not identical, and one caller had noticed
Checked before deleting, not assumed:
auth's was byte-for-byte the same function, including its shellActive helper.
commands' differed only in spelling — a compiled regexp where richtext walks runes — and agrees with it on every one of the 256 bytes and every rune up to U+2FFFF. Nothing those callers emit changed.
config's genuinely differed. It wrapped every value in single quotes whether or not it needed them, where richtext leaves a value that can mean nothing to a shell alone. So an ordinary path in the untrusted-config warnings, which used to read basecamp config trust '/home/x/.basecamp/config.json', now reads basecamp config trust /home/x/.basecamp/config.json. The same shell word, spelled shorter.
What the deleted copy was incidentally guaranteeing
Nothing, as it turns out — but the test standing over it claimed otherwise, and that is the part worth reading.
TestConfigSet_AuthorityKeyWarnsWithPath asserted the path came out surrounded by single quotes, with the message "path must be single-quoted for shell safety". It is the only caller in the repository that depended on config's copy rather than on quoting in general, and it failed on the move.
The assertion was weaker than its message. It ran against an ordinary temp-dir path with no apostrophe in it, so a routine that wrapped a value in quotes and never spliced an embedded one would have passed it unchanged — that is, the single mistake shell quoting exists to prevent was outside what the test could see. It now asks for the property the message was claiming: an ordinary path needs no quoting and stays readable, and a path carrying an apostrophe is spliced out and back in rather than merely wrapped. Mutating richtext.ShellQuote into a naive wrapper fails the new test and passed the old one.
So the emitted text changed, deliberately, and it is flagged here rather than left in the diff. The safety property did not change; it is checked now where before it was assumed.
One caller is not moved, and a fifth copy that was
connect_run.go still calls the old shellQuote name in internal/commands, because another change owns that file right now. The name survives there as a one-line delegation to richtext.ShellQuote — not a fourth implementation — and goes when that line moves.
The internal/connector recovery harness had a fifth copy inside a test file — a hand-rolled one, and the naive spelling: it wrapped unconditionally with no inert check. It builds the #!/bin/sh shim a fake agent is launched through, so it is not asserting against a fixed expected string and had no reason to spell its own. It is on richtext.ShellQuote now; internal/connector already imported richtext, and richtext has no internal dependencies at all, so there was no cycle to work around. The harness tests run that shim through a real shell, which is what confirms it.
Keeping the escape
The '\'' in the doc comment stays in an indented block, which gofmt does not reformat. Verified after writing, not assumed: gofmt -l is clean and the block is unchanged.
The real-shell test in internal/richtext now also covers the shapes the three deleted copies carried — a profile name, a config path with an apostrophe in a home directory, an opaque feed position — plus a tab. It stays behind //go:build unix because this repository builds for Windows and /bin/sh is not there; GOOS=windows go vet is clean for every package touched here.
internal/richtext.ShellQuote is now the only one. The copies in
internal/auth, internal/config and internal/commands are gone and their
callers moved to it.
auth's copy was byte-identical. commands' copy differed only in spelling — a
compiled regexp where richtext walks runes — and agrees with it on every
byte and every rune up to U+2FFFF, so nothing those callers emit changed.
config's copy did differ: it wrapped every value in single quotes whether or
not it needed them, so a path that can mean nothing to a shell now appears
bare in the untrusted-config warnings. The same shell word, spelled shorter.
A test asserted the old spelling as "shell safety", and that assertion was
weaker than it read: it passed for a wrapper that never spliced an embedded
quote at all. It now asks for the property instead — a path carrying an
apostrophe is spliced, not merely wrapped — and fails against such a
wrapper, which the one it replaces did not.
One call site is left on the old name: connect_run.go, which another change
owns. shellQuote there delegates to richtext.ShellQuote and goes when that
line moves.
Centralizes POSIX shell quoting in richtext.ShellQuote and migrates command, configuration, and authentication callers to it.
Changes:
Removes duplicate quoting implementations.
Updates emitted commands and warnings to use shared quoting.
Expands coverage for shell-sensitive values.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
"An inert path comes through unquoted" is a statement about POSIX paths. A
Windows path carries backslashes, which mean something to a shell, so
richtext.ShellQuote rightly quotes it and there is no bare path to assert —
the new trust-warning test would have failed on Windows, which this
repository builds for.
The Go suite runs on ubuntu-latest only, so nothing would have caught it:
the assertion would have stayed green here while being wrong for anyone
running the tests on Windows.
Split rather than deleted. The apostrophe case is the portable half and
stays in trust_test.go — no escape reaches inside single quotes on any
platform, so the splice has to happen everywhere. The bare-path case moves
to trust_unix_test.go behind //go:build unix, alongside the real-shell tests
in internal/richtext that are gated for the same reason. The commands-side
warning test asks richtext.ShellQuote for the expected spelling rather than
writing quotes out, so it means the same thing on both.
Copilot on #769.
The plain branch still depends on the spelling of the process temp root. If TMPDIR contains an apostrophe, absPath also contains one and richtext.ShellQuote(absPath) correctly splices it, so the raw-path substring assertion fails even though the warning is correct. The exact ShellQuote assertion above already covers the platform-dependent spelling; keep the extra splice checks only for the fixture that deliberately adds an apostrophe.
t.TempDir() is rooted under TMPDIR, and a valid POSIX TMPDIR may hold
apostrophes, spaces or anything else; macOS resolves /var through a symlink,
CI sandboxes use paths nobody would guess, and Windows spells the whole thing
differently. A shell-quoting test rooted in it asserts something about the
host rather than about the quoting — and the bare-path one would have failed
outright on a host whose temporary directory carries a quote.
Both config fixtures are now a relative path under a directory the test
changes into, so the expectation is an exact string that means the same
thing on every machine: `basecamp config trust repo/.basecamp/config.json`
and `basecamp config trust 'o'\''brien/config.json'`. Verified by running
them with TMPDIR set to a directory whose name carries an apostrophe, a
space, a non-ASCII character and a command substitution; the shape they
replace fails there.
The commands-side warning is about an absolute path and cannot be made
relative, so it drops the raw-substring assertion on the ordinary fixture —
which was the same dependency one step milder — and keeps comparing against
richtext.ShellQuote. The splice checks stay on the fixture that deliberately
carries an apostrophe.
Copilot on #769.
A one-line hand-rolled implementation in a test file, and the naive
spelling: it wrapped unconditionally with no inert check. It builds the
#!/bin/sh shim a fake agent is launched through, so it is generating a
script a real shell runs rather than asserting against a fixed expected
string — there was no reason for it to spell its own.
No cycle to work around: internal/connector already imports internal/
richtext, and richtext has no internal dependencies at all. The harness
tests execute that shim through /bin/sh, which is what confirms the move.
richtext.ShellQuote is now the only implementation in the repository. The
one remaining use of the old `shellQuote` name, in internal/commands, is a
forwarder to it and says so at its definition.
internal/harness/codex_unix_test.go spliced the quotes inline, with no
function around them, to put a temporary path into a script it runs through
sh. Searching for `func shellQuote` could never have found it — and an
inline expression is the copy most likely to drift, because it has no doc
comment to be wrong and nothing to grep for later. Converging an
implementation means grepping for the pattern, not the function.
No cycle: internal/richtext has no internal dependencies at all, so nothing
can import it into one.
The quoting there is load-bearing, and now proven so rather than assumed.
With TMPDIR set to a directory named `o'brien dir café $(id)`, passing the
path through unencoded fails the descendant test — the script writes its pid
somewhere the test does not look — and encoding it passes.
The rest of the sweep: richtext.go's HTML-entity replacements are a
different job, and the composer's `'a' 'b' 'c'` handling parses a drag
payload back apart rather than encoding anything for a shell.
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
authOAuth authenticationcommandsCLI command implementationstestsTests (unit and e2e)
2 participants
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.
github-actions
Bot
added
commands
Four copies of the same escaping routine, and only one of them finished.
internal/richtexthad the good one;internal/auth,internal/configandinternal/commandseach carried their own, written before there was a shared home. The way this surfaced is the argument for closing it: one copy's doc comment had drifted into an unmatched parenthesis and a sentence that stopped mid-clause, and all four spelled the POSIX splice with a typographic quote — not a typo but gofmt rewriting the escape in doc-comment prose every time somebody fixed it. Fixing one copy taught nothing about the other three.Four copies of an escaping routine is how one of them drifts, and the one that drifts is the one interpolating a value that came from the API.
richtext.ShellQuoteis now the only one, and every caller is on it.Originally tracked in Four shell-quoting implementations, one of them finished. The shared implementation and the gofmt problem came from The connector runs where it is started: no directory is a project's, whose own doc comment said converging the rest "is not this change's to do".
The copies were not identical, and one caller had noticed
Checked before deleting, not assumed:
shellActivehelper.richtextwalks runes — and agrees with it on every one of the 256 bytes and every rune up to U+2FFFF. Nothing those callers emit changed.richtextleaves a value that can mean nothing to a shell alone. So an ordinary path in the untrusted-config warnings, which used to readbasecamp config trust '/home/x/.basecamp/config.json', now readsbasecamp config trust /home/x/.basecamp/config.json. The same shell word, spelled shorter.What the deleted copy was incidentally guaranteeing
Nothing, as it turns out — but the test standing over it claimed otherwise, and that is the part worth reading.
TestConfigSet_AuthorityKeyWarnsWithPathasserted the path came out surrounded by single quotes, with the message "path must be single-quoted for shell safety". It is the only caller in the repository that depended on config's copy rather than on quoting in general, and it failed on the move.The assertion was weaker than its message. It ran against an ordinary temp-dir path with no apostrophe in it, so a routine that wrapped a value in quotes and never spliced an embedded one would have passed it unchanged — that is, the single mistake shell quoting exists to prevent was outside what the test could see. It now asks for the property the message was claiming: an ordinary path needs no quoting and stays readable, and a path carrying an apostrophe is spliced out and back in rather than merely wrapped. Mutating
richtext.ShellQuoteinto a naive wrapper fails the new test and passed the old one.So the emitted text changed, deliberately, and it is flagged here rather than left in the diff. The safety property did not change; it is checked now where before it was assumed.
One caller is not moved, and a fifth copy that was
connect_run.gostill calls the oldshellQuotename ininternal/commands, because another change owns that file right now. The name survives there as a one-line delegation torichtext.ShellQuote— not a fourth implementation — and goes when that line moves.The
internal/connectorrecovery harness had a fifth copy inside a test file — a hand-rolled one, and the naive spelling: it wrapped unconditionally with no inert check. It builds the#!/bin/shshim a fake agent is launched through, so it is not asserting against a fixed expected string and had no reason to spell its own. It is onrichtext.ShellQuotenow;internal/connectoralready importedrichtext, andrichtexthas no internal dependencies at all, so there was no cycle to work around. The harness tests run that shim through a real shell, which is what confirms it.Keeping the escape
The
'\''in the doc comment stays in an indented block, which gofmt does not reformat. Verified after writing, not assumed:gofmt -lis clean and the block is unchanged.The real-shell test in
internal/richtextnow also covers the shapes the three deleted copies carried — a profile name, a config path with an apostrophe in a home directory, an opaque feed position — plus a tab. It stays behind//go:build unixbecause this repository builds for Windows and/bin/shis not there;GOOS=windows go vetis clean for every package touched here.