feat(command-assist): one-line installer for remote shell integration - #308
Merged
Conversation
Adds RemoteInstallerIntegrationTests, which pastes the one-line installer command
into a real Git Bash with HOME redirected to a temp dir and checks the filesystem
afterwards: snippet written byte-for-byte, loader line added/deduped, hand-placed
loader line recognized, decode-failure reporting, the resulting interactive shell's
OSC 133 lifecycle, and that nothing leaks into the calling shell.
Fixes a wording defect the run exposed in nova-install.sh: the "loader line already
there" message dropped the word "present", diverging from the design doc's specified
wording ("already present - unchanged").
Review finding: the sh-under-bash coverage for the fish installer never exercised the fish-syntax one-liner wrapper itself, so a quoting/precedence defect there could ship undetected. Adds FishOneLiner_RunsUnderRealFish, skipped when fish is absent (always on Windows).
…ippet Adds the third and final remote installer variant. Payload decode is pure .NET (Convert.FromBase64String + GZipStream), no external tools needed - the reason this variant motivated the whole design, since a Windows remote's `cat` is Get-Content and can never satisfy the old cat > file recipe. The installer runs via the call operator (child scope, never dot-sourced) and writes with an explicit no-BOM UTF8Encoding rather than Set-Content -Encoding utf8NoBOM, which Windows PowerShell 5.1 lacks.
…ain snippet secondary Settings' "Remote shell integration" row now offers Copy installer (the one-line BuildInstallerCommand paste) as the primary action and Copy plain snippet as the secondary one, wired through a shared selected-shell helper. BuildInstallInstructions' XML remark is rewritten to record why the old "can't read a base64 blob before running" objection was reversed rather than deleted. Docs' Install section now documents the one-liner flow instead of the manual cat > file + rc-edit recipe. No remote host was available in this environment, so the manual step of pasting the installer into a real SSH session was not performed - it is listed as an outstanding manual check in the task report.
…file edit CopyRemoteShellIntegrationInstallerAsync's status message was one sentence for all three shells, claiming the installer "adds the loader line to your config file if it isn't already there." False for fish: its installer writes into conf.d, which fish auto-sources, and never touches an rc file - contradicting the installer's own printed output and this same commit's docs, which already special-case fish. Branch on RemoteShellIntegrationSnippets.GetLoaderLine(shell), already null for fish for exactly this reason, instead of naming fish or switching on the enum. Non-fish shells keep the same sentence with "rc file" in place of "config file", matching the docs' vocabulary.
… a trailing newline Whole-branch review of the remote installer feature found a real bug: nova-install.sh's printf >> and nova-install.ps1's Add-Content both append the loader line directly at EOF, so an rc file/profile whose last byte isn't a newline gets the loader glued onto the user's last line - on a remote host, while the installer still reports success, and re-running can never self-heal since the grep/Select-String marker check still matches. Both installers now ensure the file ends in a newline before appending. Also closes four cheap gaps the same review flagged: a stale comment overstating what Installer_LeavesNothingBehindInTheCallingShell actually probes, no coverage of the zsh and "unknown shell" dispatch arms, no real-pwsh parse check of the generated PowerShell one-liner, and a .gitattributes rule that didn't reach nova-install.ps1 one directory down from its siblings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four review blockers on the one-line remote shell-integration installer. The rc append was never status-checked in the sh or the PowerShell installer. A rc file the user cannot write - root-owned, chattr +i, a read-only $HOME, a full disk - makes >> and Add-Content fail while the script carries on to print "added loader line" over the top of the real error, so the user is told it worked, gets no marks, and goes looking in the right file for a line that was never written. Both appends in each installer are now checked; on failure they say so, print the loader line to place by hand, and exit non-zero. The same fix had already been made for fish, without a test - hence the tests below. The mktemp fallback in the bash/zsh one-liner is gone. mktemp gives 0600 and O_EXCL; /tmp/nova-si.$$ gives neither, $$ is visible in ps, and > follows symlinks and does not change an existing file's mode. Another local user can pre-create the path and rewrite it between the redirect and sh "$__nova_t", or aim it at ~/.bashrc and let the redirect truncate that. A safe fallback needs set -C plus an unpredictable name, which is not worth the machinery; mktemp failing now fails the install with a message naming mktemp. Truncation. A decode failure used to be reported as "this host needs base64 and gzip" whatever caused it, including a payload that arrived short. The bash/zsh and pwsh arms now carry their payload length and check it before decoding, and the decode branch is chosen on the pipeline's exit status rather than on the temp file being non-empty - gzip -dc writes what it inflated before failing, so a corrupt payload could leave a broken installer that [ -s ] waved through and sh ran. What that check does and does not cover is worth being exact about, because the obvious reading is wrong. It catches bytes lost from the MIDDLE of the line while the tail still arrives: a flaky link, a multiplexer dropping a chunk of a paste. It does not catch a canonical-mode tty cut and cannot. N_TTY_BUF_SIZE is 4096 and the line is 8688 bytes, but the payload literal opens at byte 9 and closes at 7966, so a cut at 4096 always lands inside the quoted blob and takes the closing quote with it - bash answers "unexpected EOF while looking for matching '", pwsh "The string is missing the terminator: '.", and none of our code runs. A test now feeds the first 4095 bytes of the real one-liner to bash and pins that behaviour, so nobody re-reads the guard as covering it. fish carries no length check. Its line is 3680 bytes and fits under 4096, so it is the one arm a tty cannot truncate - and a check costs ~320 bytes of exactly the headroom that makes that true. It keeps every other guard bash has. The idempotency guard matched the marker anywhere in the rc file, including inside a comment, so a rc file containing only "# I disabled nova-shell-integration on purpose" got "already present - unchanged" and no loader line. Both installers now anchor to a non-comment occurrence. Also: the fish one-liner had no error handling at all and now matches its bash sibling; base64 -d falls back to -D for pre-Ventura macOS via a one-shot probe; pwsh's & $__nova_t moved outside the decode's try, where a terminating error from the installer script itself was being reported as "the payload did not unpack"; the PowerShell installer names the resolved profile path instead of printing the literal string $PROFILE, and its unreachable post-WriteAllText Test-Path is a try/catch; the class doc's payload sizes were wrong by 15-35% and are now stated as a property with the measurement left to a test; comments that narrated the diff rather than the code are rewritten; the docs page gets an honest paragraph on pasting an opaque blob, a section on the 4096-byte limit describing the symptom you actually get, and the new messages in troubleshooting. Tests: failed-rc-append for sh and pwsh (output, by-hand line, exit status), comment-only mention for both, a 4095-byte truncated paste, the payload-length check and its agreement with the real payload, fish's sub-4096 length as the reason it has no check, no predictable temp path, the base64 -D probe, decode-on-status, fish parity, and a length tripwire. FishOneLiner_RunsUnderRealFish no longer passes on a false success claim. The bash integration harness runs the one-liner from a file rather than through bash -c: Git Bash's MSYS runtime caps a reconstructed argv just under 8192 characters, and the length check pushed the line from 7721 to 8688, past it.
|
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.
Replaces the copy-a-300-line-snippet flow for remote shell integration with a one-line
installer. Settings now offers "Copy installer" first and keeps the plain snippet as the
secondary path.
The first eight commits are pre-existing work from an earlier session. The last commit is
this round's review response - four blockers plus follow-ups, detailed below.
How it works
The one-liner inlines a gzip+base64 payload, decodes it to a temp file, and runs it with
sh/&. Three arms: bash+zsh, fish, PowerShell.Nothing is fetched over the network. The payload is fully inlined - no
curl, no URL,no supply-chain surface. That is the whole security story, and it is also the whole
mitigation: a user cannot verify that the 7 KB blob on their clipboard corresponds to the
reviewable sources in
assets/shell-integration/install/. The threat model indocs/command-assist/RemoteShellIntegration.mdnow says so plainly, and points at"Copy plain snippet" as the out for anyone who wants to read what they are pasting.
Injection is structurally impossible rather than escaped away: the only interpolated value
is the base64 blob, whose alphabet is
[A-Za-z0-9+/=], so it cannot contain a quote,$,backtick,
;, newline, or$(. No host name, path, or other user-controlled value reachesthe generated command at all. Verified independently in all three shells, including fish's
distinct single-quote rules and PowerShell's backtick escape.
Neither installer needs an exec bit or a non-
noexec/tmp-sh <file>and& <file>both read rather than execute.
Review blockers found and fixed
This branch was reviewed before merge. Four blockers, each reproduced with evidence, each
re-verified against that same reproduction after the fix.
1. Both installers claimed success when the rc append had failed.
printf ... >>andAdd-Contenthad their status unchecked, so with a root-owned,chattr +i, or read-onlyrc file the output read
nova: added loader line to ~/.bashrcand exited 0. This is thesame bug an earlier commit on this branch already fixed for fish - the other two arms were
never checked. The asymmetry that gives it away: the snippet write was checked, only the rc
append was not. Now both appends are status-checked; on failure the installer says it could
not write, prints the loader line for the user to add by hand, and exits non-zero.
2. The
mktempfallback was a local code-execution hole.mktemp 2>/dev/null || printf /tmp/nova-si.%s "$$"-mktempgives 0600 andO_EXCL, thefallback gives neither.
/tmp/nova-si.<pid>is predictable ($$is visible inps), and>follows symlinks and does not change an existing file's mode or owner. On a shared host,another local user pre-creates the path 0666, waits for a paste on a box where
mktempisabsent (busybox) or fails (unwritable
$TMPDIR, a common hardening setting), and overwritesthe contents between the redirect and
sh "$__nova_t". A symlink variant needs no race atall: point it at
~/.bashrcand the redirect destroys the rc file. The fallback is deleted;mktempfailing now fails the install cleanly. A safe fallback would needset -Cplus anunpredictable name - rejected as more machinery than the case is worth, and recorded as such.
3. The "one line" is 7.7 KB, and the failure was misdiagnosed.
N_TTY_BUF_SIZEis 4096and applies whenever ICANON is on -
docker exec -it <c> sh, busybox/Alpineash,dashas
/bin/sh, serial consoles, pwsh without PSReadLine. Interactive bash/zsh/fish use rawmode via readline/ZLE, which is why this passed manual testing. Truncated, the decode failed
and the installer reported
this host needs base64 and gzipon a host that had both,sending users to install coreutils on a production container.
Now each of bash/zsh and pwsh embeds its payload length and compares before decoding, and
the decode branch keeps the base64/gzip message - accurate, because truncation has been
ruled out first. Also folded in: the branch is now the pipeline's exit status rather than
[ -s ], which closes a related hole wheregzip -dcwrites output before failing, so apartially inflated payload passed the size check and got executed - writing a truncated
snippet and appending a loader line that every future shell would then source.
Honest limit, and it is written into the code and the docs rather than glossed: the
length check catches mid-stream byte loss (a flaky link, a multiplexer dropping a paste
chunk). It cannot catch a canonical-mode tail cut. The opening quote sits at byte 9 and the
closing quote at 7966, so a 4096-byte cut always lands inside the payload literal and the
shell fails first with
unexpected EOF while looking for matching '- none of our coderuns. A test now feeds the first 4095 bytes to a real bash and pins exactly that.
4. The idempotency guard was a bare substring match. A
.bashrccontaining only# I disabled nova-shell-integration on purposemade the installer report "already present"and write nothing - so a user who had previously commented the line out could never install.
Now anchored to a non-comment occurrence (
^[^#]*), which also correctly tolerates anindented loader line inside an
ifblock.Also in the fix commit
(mktemp check, decode-status branch, failure messages).
(3680 bytes, 416 to spare); adding the check cost 324 bytes and left only 92, putting the
arm on the edge of the very failure the check exists to report. A test asserts both the
bound and the absence of the check, so the reasoning is enforced rather than just written
down.
$PROFILEinstead of the resolved path.& $__nova_tmoved out of the decodetry, so an error from the installer script is nolonger reported as "the payload did not unpack".
Test-Pathafter[IO.File]::WriteAllText(which throws) became a realtry/catch.
base64 -dis the GNU spelling; older macOS only accepts-D. Now probed once, with thebare letter stored so fish's
setnever sees an option-shaped value.cannot rot, pointing at the test that holds the measurement.
than the 300-line paste this replaced") rewritten to state the reasoning as it stands.
Testing
RemoteShellIntegrationInstallerTests30,RemoteInstallerIntegrationTests20 passed /2 skipped,
RemoteShellIntegrationSnippetTests44,PaneRemoteShellIntegrationTests17.Clean build, 0 errors, no warnings in any changed file.
The integration tests really do run real shells -
bash,fish,pwshviaProcess,asserting on files on disk, with the last one going through a real PTY and the production
OSC 133 parser. Every test scopes
HOME/-ProfilePath/-DestDirto a per-test tempdirectory; none can touch a real dotfile.
Two skips, both explicit rather than silent:
FishOneLiner_RunsUnderRealFish(no fish onWindows - it runs on Linux CI, which installs it) and a read-only-rc variant that self-skips
by trying the append rather than guessing the platform.
New guard tests are mutation-checked. Note for anyone repeating that:
copy /bpreservesthe source file's mtime, so a restore-from-backup can leave the file older than the compiled
DLL, MSBuild skips the recompile, and a
--no-buildrun re-executes the mutant binary whilethe source on disk is correct - a way to certify a mutant as clean. Touch the file and
rebuild.
.gitattributeswas verified, not assumed:git check-attrandgit ls-files --eolconfirm
text eol=lfandi/lf w/lfon all three installers - LF in the index and in theWindows working tree, checked as bytes. Without that, every installer would arrive with
bad interpreter: /bin/bash^M.Not verified
wholly unexercised shell path locally. Linux CI runs it.
and the ICANON limit is well established, but nothing was pasted into a live ICANON shell.
[...]::new()needs 5.0+) andExecutionPolicy Restrictedblocking& $__nova_t. Worth a troubleshooting line later.base64 -D- the probe is written but no macOS was available to run it.they may overflow (~443px). Flagged deliberately rather than blind-fixed - it wants eyes,
not a guess.
SettingsWindow.axamllayout is otherwise untouched by the fix commit.Note on CI weight
These tests carry
[Trait("Category","ShellIntegration")]and so land in the App.Tests lane,which
ci.ymlmarks non-blocking (#81, Avalonia headless deadlock). So the only realverification of a security-sensitive artifact runs in a job that cannot fail the build.
Pre-existing structure, not introduced here, but it changes what "the tests pass" is worth
on this particular PR.