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.
This commit is contained in:
Andras Bacsai
2026-09-30 11:05:42 +02:00
parent 6a59008cda
commit e2c826a976
8 changed files with 102 additions and 11 deletions
+1
View File
@@ -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`.
+5 -1
View File
@@ -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
@@ -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
+4
View File
@@ -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 ?? [];
@@ -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([
@@ -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']);
+1 -1
View File
@@ -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 '));
+31 -6
View File
@@ -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 '<command>'`),
* 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()