Skip to content

fix fish ProxyCommand shell - #450

Closed
GGonryun wants to merge 2 commits into
mainfrom
miguelcampos/cus-359-p0-cli-expects-bash-shell
Closed

fix fish ProxyCommand shell#450
GGonryun wants to merge 2 commits into
mainfrom
miguelcampos/cus-359-p0-cli-expects-bash-shell

Conversation

@GGonryun

@GGonryun GGonryun commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Problem

p0 ssh to AWS fails for users whose login shell is fish, while the same request
works under bash. OpenSSH runs a ProxyCommand as $SHELL -c "exec <command>",
and fish sources config.fish on every fish -c. If those startup files set
AWS_* — directly or via direnv/aws-vault helpers — they replace the session
credentials the CLI injected, so aws ssm start-session signs with the wrong
identity and AWS rejects the connection. bash doesn't read .bashrc
non-interactively, so bash users never see it; zsh's .zshenv is the same hazard.

Verified locally: a probe $SHELL logs -c exec <command> when ssh runs a
ProxyCommand, and env AWS_ACCESS_KEY_ID=INJECTED fish -c 'echo $AWS_ACCESS_KEY_ID'
prints the config.fish value, while /bin/sh -c prints INJECTED.

Status: N/A

Change

Sets SHELL=/bin/sh in the environment of the ssh/scp child the CLI spawns, so
OpenSSH runs the generated ProxyCommand under a shell that reads no startup files.

Only the ssh client children, and only on POSIX — Windows OpenSSH doesn't use
$SHELL for ProxyCommand. The ssh-proxy flow, where the CLI is itself the
ProxyCommand and spawns the provider binary with shell: false, was never affected.
This sets $SHELL on a child process only: the user's own shell and their remote
login shell are untouched. No credentials are written to disk or put on a command
line.

Check off any of the following areas of code that are modified:

  • authentication / authorization
  • workflow
  • APIs
  • lifecycle SDK
  • datastore abstractions
  • none of the above

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Documentation update
  • Dependency update
  • Performance improvement
  • Security fix

Validation

  • Testing: Four new unit tests cover the pinned value under fish, an identical
    child env under fish and bash, and $SHELL left alone both on Windows and for
    the ssh-proxy child. The two positive tests were confirmed to fail with the fix
    disabled. tsc --noEmit and eslint pass. Full suite: 302 pass; the 17 failures
    in src/commands/__tests__/ssh.test.ts and
    src/commands/shared/__tests__/ssh-resolve.test.ts reproduce unchanged at the
    base commit and are unrelated. Not yet run end to end against a real AWS target
    from a fish shell
    — that check should happen before merge.
  • Blast radius: env of the ssh/scp children on macOS and Linux. Windows and
    ssh-proxy unchanged. The only other ssh feature reading $SHELL is
    LocalCommand, which the CLI doesn't use.
  • Risks & mitigations: The ProxyCommand is CLI-generated, so there is nothing
    user-authored for a shell to interpret. /bin/sh exists on every supported POSIX
    platform and is OpenSSH's own fallback when $SHELL is unset.

Rollout / rollback

N/A — client-side, ships with the next release. Rollback is a revert.

Tracking (optional)

CUS-359, CX-464

@GGonryun GGonryun changed the title pin ProxyCommand shell and classify terminal StartSession failures fix fish ProxyCommand shell and classify terminal StartSession failures Jul 30, 2026
…ailures (CX-464)

OpenSSH runs ProxyCommand/Match exec via $SHELL -c; unlike sh -c, fish -c
sources the user's config.fish, which can override the AWS_* credentials p0
injects for `aws ssm start-session`, causing fish-only auth failures we
couldn't reproduce locally. Pin SHELL=/bin/sh in the spawned child's env.

Separately, a hard StartSession rejection (bad signature, credentials
mangled before the call, expired/unrecognized tokens) was indistinguishable
from access still propagating: the broad "Connection closed" pattern
matched the ssh kex fallout of any ProxyCommand death, so it retried
blindly for 30s and reported the generic "did not propagate" message,
hiding the real error. Add an optional terminalAccessPatterns hook that
aborts the retry loop immediately and surfaces a classified message via
AWS's new connectionErrorMessage.
@GGonryun
GGonryun force-pushed the miguelcampos/cus-359-p0-cli-expects-bash-shell branch from 8d07ddd to 9f3d114 Compare July 30, 2026 17:29
@GGonryun GGonryun self-assigned this Jul 30, 2026
@GGonryun
GGonryun requested review from Jenn738 and Copilot and removed request for Copilot July 30, 2026 17:33
@GGonryun
GGonryun force-pushed the miguelcampos/cus-359-p0-cli-expects-bash-shell branch from 9f3d114 to 289e6b3 Compare July 30, 2026 17:45
@GGonryun
GGonryun marked this pull request as ready for review July 30, 2026 17:46
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d1066af4-9dae-4f5a-ad14-b197eaf01443

📥 Commits

Reviewing files that changed from the base of the PR and between d9b5f79 and 289e6b3.

📒 Files selected for processing (2)
  • src/plugins/ssh/__tests__/index.test.ts
  • src/plugins/ssh/index.ts

Comment @coderabbitai help to get the list of available commands.

@GGonryun GGonryun changed the title fix fish ProxyCommand shell and classify terminal StartSession failures fix fish ProxyCommand shell Jul 30, 2026
@GGonryun GGonryun closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant