From e2c826a9765f33c4361dab7841e3aede7637ea26 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:05:42 +0200 Subject: [PATCH] fix: run SSH wrapper via sh and scope secret tokens to resource team SSH: sshd runs the remote command with the user's login shell, which can be fish or csh/tcsh. Those shells cannot parse POSIX `if ... fi`, so the bash/sh fallback wrapper now runs inside `sh -c "..."`. Unit tests run the wrapper under every login shell that is installed and check that it stays one simple sh command. Secret managers: the SecretManagerLinks component now lists and resolves tokens by the resource's team instead of the session team. A team switch in another tab can no longer link a token from a different team. SecretManagerLink::fetchSecrets() also throws when the linked token does not belong to the resource's team, so no request is sent with it. Also adds a lesson note: remove the SSH mux file before live SSH or login-shell tests. --- .ai/lessons.md | 1 + app/Helpers/SshMultiplexingHelper.php | 6 ++- .../Project/Shared/SecretManagerLinks.php | 18 +++++++-- app/Models/SecretManagerLink.php | 4 ++ .../SecretManagers/SecretManagerLinkTest.php | 20 ++++++++++ .../SecretManagerLinksComponentTest.php | 25 +++++++++++++ tests/Feature/SshMultiplexingLockTest.php | 2 +- tests/Unit/SshMultiplexingDisableTest.php | 37 ++++++++++++++++--- 8 files changed, 102 insertions(+), 11 deletions(-) diff --git a/.ai/lessons.md b/.ai/lessons.md index cefe025782..040431f38c 100644 --- a/.ai/lessons.md +++ b/.ai/lessons.md @@ -7,6 +7,7 @@ - Use the same regression test before and after the production change so the result shows the behavior difference. - Several dev instances can run from one checkout (see `./scripts/dev urls`). After you add a migration, run it on each running instance of that checkout, or those instances fail on the new code. - The checkout is shared with other sessions. When the full suite fails, rerun each failing file alone and compare with a clean `git archive HEAD` copy before you connect a failure to your change. +- `sshd` keeps the login shell of an open multiplexed SSH connection. Call `SshMultiplexingHelper::removeMuxFile()` before a live test of SSH or login-shell behavior. - Redirect browser test output to a file (`> /tmp/x.log 2>&1`); piping it (`| tail`) hangs because the Playwright server keeps the pipe open. - Call `visit()` directly in each `tests/v4/Browser` test body; Pest does not mark a test that only uses helper-wrapped `visit()` as a browser test, so it fails with `sendText() on null`. diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index f3d519b0bd..2d7289d43a 100644 --- a/app/Helpers/SshMultiplexingHelper.php +++ b/app/Helpers/SshMultiplexingHelper.php @@ -250,9 +250,13 @@ class SshMultiplexingHelper .$delimiter; } + /** + * sshd runs this with the SSH user's login shell, which can be fish or csh/tcsh. They cannot + * parse POSIX `if ... fi`, so the logic runs in `sh`; bash is preferred, Alpine may lack it. + */ private static function remoteShellCommand(): string { - return 'if command -v bash >/dev/null 2>&1; then exec bash -se; else exec sh -se; fi'; + return 'sh -c "if command -v bash >/dev/null 2>&1; then exec bash -se; else exec sh -se; fi"'; } public static function getConnectionTimeout(Server $server): int diff --git a/app/Livewire/Project/Shared/SecretManagerLinks.php b/app/Livewire/Project/Shared/SecretManagerLinks.php index c0641b56c5..283c9d4c33 100644 --- a/app/Livewire/Project/Shared/SecretManagerLinks.php +++ b/app/Livewire/Project/Shared/SecretManagerLinks.php @@ -4,6 +4,7 @@ namespace App\Livewire\Project\Shared; use App\Models\IntegrationToken; use Illuminate\Contracts\View\View; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Livewire\Component; @@ -41,8 +42,7 @@ class SecretManagerLinks extends Component private function loadData(): void { $this->link = $this->resource->secretManagerLink()->with('integrationToken')->first(); - $this->availableTokens = IntegrationToken::ownedByCurrentTeam() - ->whereIn('provider', IntegrationToken::SECRET_MANAGER_PROVIDERS) + $this->availableTokens = $this->resourceTeamTokens() ->get() ->filter(fn (IntegrationToken $token) => in_array('secrets', $token->capabilities ?? [], true)) ->values(); @@ -59,7 +59,19 @@ class SecretManagerLinks extends Component return null; } - return $this->availableTokens->firstWhere('uuid', $this->integration_token_uuid); + return $this->resourceTeamTokens()->where('uuid', $this->integration_token_uuid)->get() + ->first(fn (IntegrationToken $token) => in_array('secrets', $token->capabilities ?? [], true)); + } + + /** + * Secret manager tokens of the resource's team. The session team can differ: a user can + * switch teams in another tab while this component is still open. + */ + private function resourceTeamTokens(): Builder + { + return IntegrationToken::query() + ->where('team_id', $this->resource->team()?->id) + ->whereIn('provider', IntegrationToken::SECRET_MANAGER_PROVIDERS); } protected function rules(): array diff --git a/app/Models/SecretManagerLink.php b/app/Models/SecretManagerLink.php index 34e4e90d12..6d8f561db7 100644 --- a/app/Models/SecretManagerLink.php +++ b/app/Models/SecretManagerLink.php @@ -46,6 +46,10 @@ class SecretManagerLink extends BaseModel public function fetchSecrets(): array { $token = $this->integrationToken; + $teamId = $this->resourceable?->team()?->id; + if ($teamId === null || (int) $token->team_id !== (int) $teamId) { + throw new \RuntimeException('The secret manager token does not belong to the team of this resource.'); + } $settings = $this->settings ?? []; $metadata = $token->metadata ?? []; diff --git a/tests/Feature/SecretManagers/SecretManagerLinkTest.php b/tests/Feature/SecretManagers/SecretManagerLinkTest.php index 362130c728..9b6ab4ff8c 100644 --- a/tests/Feature/SecretManagers/SecretManagerLinkTest.php +++ b/tests/Feature/SecretManagers/SecretManagerLinkTest.php @@ -150,6 +150,26 @@ test('a doppler link fetches secrets with the stored token', function () { && str_contains($request->url(), 'project=proj')); }); +test('a link never fetches secrets with a token of another team', function () { + Http::fake(); + $otherToken = IntegrationToken::query()->create([ + 'team_id' => Team::factory()->create()->id, + 'provider' => 'doppler', + 'name' => 'Other team Doppler', + 'token' => 'dp.st.other', + 'capabilities' => ['secrets'], + ]); + // For example a link saved before this check, or written by another code path. + $link = $this->application->secretManagerLink()->create([ + 'integration_token_id' => $otherToken->id, + 'settings' => ['project' => 'proj', 'config' => 'prd'], + ]); + + expect(fn () => $link->fetchSecrets()) + ->toThrow(RuntimeException::class, 'The secret manager token does not belong to the team of this resource.'); + Http::assertNothingSent(); +}); + test('a vault link uses the base url and namespace from the token metadata', function () { Http::fake([ 'https://example.com:8200/v1/kv/data/apps/web' => Http::response([ diff --git a/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php b/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php index 28a8707b43..722dd63963 100644 --- a/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php +++ b/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php @@ -263,6 +263,31 @@ test('the source can be removed', function () { ]); }); +test('only tokens of the resource team can be linked, also after a team switch', function () { + $otherTeam = Team::factory()->create(); + $otherTeam->members()->attach($this->user->id, ['role' => 'member']); + $otherToken = IntegrationToken::query()->create([ + 'team_id' => $otherTeam->id, + 'provider' => 'doppler', + 'name' => 'Other team Doppler', + 'token' => 'dp.st.other', + 'capabilities' => ['secrets'], + ]); + + $component = Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]); + // The user switches to the other team in another browser tab; the next save reloads the token list. + session(['currentTeam' => $otherTeam]); + $component->set('integration_token_uuid', $this->token->uuid) + ->set('integration_token_uuid', $otherToken->uuid); + + expect($component->get('availableTokens')->pluck('id')->all())->toBe([$this->token->id]); + $this->assertDatabaseMissing('secret_manager_links', ['integration_token_id' => $otherToken->id]); + $this->assertDatabaseHas('secret_manager_links', [ + 'resourceable_id' => $this->application->id, + 'integration_token_id' => $this->token->id, + ]); +}); + test('members without update permission cannot save a source', function () { $member = User::factory()->create(); $this->team->members()->attach($member->id, ['role' => 'member']); diff --git a/tests/Feature/SshMultiplexingLockTest.php b/tests/Feature/SshMultiplexingLockTest.php index 272156fbd2..dd0ce783b2 100644 --- a/tests/Feature/SshMultiplexingLockTest.php +++ b/tests/Feature/SshMultiplexingLockTest.php @@ -153,7 +153,7 @@ it('adds mux options to ssh commands only after the explicit master is ready', f ->toContain('-o ControlMaster=auto') ->toContain("-o ControlPath=/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}") ->toContain('-o ControlPersist=3600') - ->toContain("'if command -v bash >/dev/null 2>&1; then exec bash -se; else exec sh -se; fi' << \\") + ->toContain("'sh -c \"if command -v bash >/dev/null 2>&1; then exec bash -se; else exec sh -se; fi\"' << \\") ->not->toContain('<< $delimiter'); Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); diff --git a/tests/Unit/SshMultiplexingDisableTest.php b/tests/Unit/SshMultiplexingDisableTest.php index 52155aad2a..472431da3f 100644 --- a/tests/Unit/SshMultiplexingDisableTest.php +++ b/tests/Unit/SshMultiplexingDisableTest.php @@ -15,14 +15,39 @@ use Tests\TestCase; */ class SshMultiplexingDisableTest extends TestCase { - public function test_remote_shell_prefers_bash_and_falls_back_to_sh() + /** + * sshd runs the remote command with the SSH user's login shell (`$SHELL -c ''`), + * which can be fish or csh/tcsh. The wrapper must work in each of them. + */ + public function test_remote_shell_runs_the_script_from_stdin_in_every_login_shell() { - $reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'remoteShellCommand'); + $wrapper = (new \ReflectionMethod(SshMultiplexingHelper::class, 'remoteShellCommand'))->invoke(null); + $tested = []; - $this->assertSame( - 'if command -v bash >/dev/null 2>&1; then exec bash -se; else exec sh -se; fi', - $reflection->invoke(null) - ); + foreach (['sh', 'bash', 'dash', 'zsh', 'fish', 'tcsh', 'csh'] as $loginShell) { + $path = trim((string) shell_exec('command -v '.escapeshellarg($loginShell).' 2>/dev/null')); + if ($path === '') { + continue; + } + + $output = shell_exec('echo "echo remote-ok" | '.escapeshellarg($path).' -c '.escapeshellarg($wrapper).' 2>&1'); + $this->assertSame('remote-ok', trim((string) $output), "Login shell {$loginShell} could not run the wrapper."); + $tested[] = $loginShell; + } + + $this->assertContains('sh', $tested); + } + + /** + * fish and csh/tcsh cannot parse POSIX `if ...; then ...; fi`. A single `sh -c "..."` command + * with only double quotes (it is wrapped in single quotes on the ssh command line) is a simple + * command that every login shell can run. + */ + public function test_remote_shell_is_one_simple_sh_command() + { + $wrapper = (new \ReflectionMethod(SshMultiplexingHelper::class, 'remoteShellCommand'))->invoke(null); + + $this->assertMatchesRegularExpression('/^sh -c "[^"\'$`!]*"$/', $wrapper); } public function test_generate_ssh_command_accepts_disable_multiplexing_parameter()