diff --git a/app/Livewire/Project/Shared/SecretManagerLinks.php b/app/Livewire/Project/Shared/SecretManagerLinks.php index 283c9d4c33..afe4c85f31 100644 --- a/app/Livewire/Project/Shared/SecretManagerLinks.php +++ b/app/Livewire/Project/Shared/SecretManagerLinks.php @@ -2,7 +2,9 @@ namespace App\Livewire\Project\Shared; +use App\Livewire\Project\Shared\EnvironmentVariable\All; use App\Models\IntegrationToken; +use App\Support\ValidationPatterns; use Illuminate\Contracts\View\View; use Illuminate\Database\Eloquent\Builder; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; @@ -218,6 +220,12 @@ class SecretManagerLinks extends Component return; } + if (! ValidationPatterns::isValidEnvironmentVariableKey($key)) { + $this->dispatch('error', "{$key} is not a valid variable name."); + + return; + } + if ($this->resource->environment_variables()->where('key', $key)->exists()) { $this->dispatch('error', "A variable with the key {$key} already exists."); @@ -230,7 +238,7 @@ class SecretManagerLinks extends Component ]); $this->auditSecretManagerAction('reference_created', ['secret_key' => $key]); - $this->dispatch('refreshEnvs'); + $this->dispatch('refreshEnvs')->to(All::class); $this->dispatch('success', "Added {$key} as {{vault.{$key}}}."); } catch (\Throwable $e) { handleError($e, $this); @@ -246,21 +254,47 @@ class SecretManagerLinks extends Component return; } - $imported = $this->link->importMissingReferences(); + ['imported' => $imported, 'skipped' => $skipped] = $this->link->importMissingReferences(); $this->auditSecretManagerAction('references_imported', [ 'key_count' => count($imported), 'secret_keys' => $imported, + 'skipped_key_count' => count($skipped), ]); - $this->dispatch('refreshEnvs'); - $this->dispatch('success', $imported === [] - ? 'All remote keys already exist as variables.' - : 'Imported '.count($imported).' keys as {{vault.KEY}} references.'); + $this->dispatch('refreshEnvs')->to(All::class); + $this->dispatch('success', $this->importResultMessage($imported, $skipped)); } catch (\Throwable $e) { handleError($e, $this); } } + /** + * @param list $imported + * @param list $skipped + */ + private function importResultMessage(array $imported, array $skipped): string + { + if ($imported === [] && $skipped === []) { + return 'All remote keys already exist as variables.'; + } + + $message = $imported === [] + ? 'No keys imported.' + : 'Imported '.count($imported).' keys as {{vault.KEY}} references.'; + + if ($skipped === []) { + return $message; + } + + $shownNames = array_map(fn (string $key): string => e($key), array_slice($skipped, 0, 10)); + $moreCount = count($skipped) - count($shownNames); + $names = implode(', ', $shownNames).($moreCount > 0 ? " and {$moreCount} more" : ''); + + return $message.' '.(count($skipped) === 1 + ? "Skipped 1 key that is not a valid variable name: {$names}." + : 'Skipped '.count($skipped)." keys that are not valid variable names: {$names}."); + } + private function resetKeys(): void { $this->keys = []; @@ -297,6 +331,10 @@ class SecretManagerLinks extends Component return view('livewire.project.shared.secret-manager-links', [ 'selectedToken' => $this->selectedToken, 'filteredKeys' => $this->filteredKeys, + 'invalidKeys' => array_flip(array_filter( + $this->keys, + fn (string $key) => ! ValidationPatterns::isValidEnvironmentVariableKey($key), + )), ]); } } diff --git a/app/Models/SecretManagerLink.php b/app/Models/SecretManagerLink.php index 6d8f561db7..0e47cf63ed 100644 --- a/app/Models/SecretManagerLink.php +++ b/app/Models/SecretManagerLink.php @@ -5,6 +5,7 @@ namespace App\Models; use App\Services\DopplerService; use App\Services\InfisicalService; use App\Services\VaultService; +use App\Support\ValidationPatterns; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\MorphTo; @@ -81,9 +82,11 @@ class SecretManagerLink extends BaseModel /** * Create one {{vault.KEY}} reference variable per remote key that has no - * variable with that key yet. Only key names touch the database. + * variable with that key yet. Only key names touch the database. Remote + * names that are not valid variable keys (e.g. `db-password`) are skipped + * as they are, never renamed. * - * @return list The keys that were imported + * @return array{imported: list, skipped: list} */ public function importMissingReferences(): array { @@ -92,8 +95,17 @@ class SecretManagerLink extends BaseModel $existing = $this->resourceable->environment_variables()->pluck('key')->flip(); $imported = []; + $skipped = []; foreach ($keys as $key) { + $key = (string) $key; + + if (! ValidationPatterns::isValidEnvironmentVariableKey($key)) { + $skipped[] = $key; + + continue; + } + if (isset($existing[$key])) { continue; } @@ -105,7 +117,7 @@ class SecretManagerLink extends BaseModel $imported[] = $key; } - return $imported; + return ['imported' => $imported, 'skipped' => $skipped]; } /** Short human-readable description of the remote source for the UI. */ diff --git a/resources/views/livewire/project/shared/secret-manager-links.blade.php b/resources/views/livewire/project/shared/secret-manager-links.blade.php index 3fa283c9f8..84e21a3054 100644 --- a/resources/views/livewire/project/shared/secret-manager-links.blade.php +++ b/resources/views/livewire/project/shared/secret-manager-links.blade.php @@ -89,10 +89,16 @@ {{ $key }} {{ '{{vault.'.$key.'}'.'}' }} - - Add as variable - + @if (isset($invalidKeys[$key])) + + Not a valid variable name + + @else + + Add as variable + + @endif @empty
diff --git a/tests/Feature/SecretManagers/SecretManagerLinkTest.php b/tests/Feature/SecretManagers/SecretManagerLinkTest.php index 9b6ab4ff8c..d26f00c7c1 100644 --- a/tests/Feature/SecretManagers/SecretManagerLinkTest.php +++ b/tests/Feature/SecretManagers/SecretManagerLinkTest.php @@ -475,13 +475,30 @@ test('import creates reference variables for missing keys only', function () { $imported = $this->application->secretManagerLink->importMissingReferences(); - expect($imported)->toBe(['NEW_KEY']); + expect($imported)->toBe(['imported' => ['NEW_KEY'], 'skipped' => []]); $created = $this->application->environment_variables()->where('key', 'NEW_KEY')->firstOrFail(); expect($created->value)->toBe('{{vault.NEW_KEY}}') ->and($this->application->environment_variables()->where('key', 'EXISTING')->firstOrFail()->value)->toBe('local'); }); +test('import skips remote names that are not valid variable keys and imports the rest', function () { + Http::fake([ + 'https://example.com:8200/v1/secret/data/apps/web' => Http::response([ + 'data' => ['data' => ['ZED' => 'z', 'db-password' => 'p', 'API_KEY' => 'a']], + ]), + ]); + + $link = createSecretManagerLink('vault', ['mount' => 'secret', 'path' => 'apps/web'], ['base_url' => 'https://example.com:8200']); + + $result = $link->importMissingReferences(); + + expect($result)->toBe(['imported' => ['API_KEY', 'ZED'], 'skipped' => ['db-password']]) + ->and($this->application->environment_variables()->whereIn('key', ['API_KEY', 'ZED'])->pluck('value', 'key')->sortKeys()->all()) + ->toBe(['API_KEY' => '{{vault.API_KEY}}', 'ZED' => '{{vault.ZED}}']) + ->and($this->application->environment_variables()->where('key', 'db-password')->exists())->toBeFalse(); +}); + test('secret references are not marked as shared variables', function () { $secretRef = $this->application->environment_variables()->create([ 'key' => 'A', diff --git a/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php b/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php index 722dd63963..1900ddb588 100644 --- a/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php +++ b/tests/Feature/SecretManagers/SecretManagerLinksComponentTest.php @@ -1,5 +1,6 @@ assertDontSee('API_KEY'); }); -test('browse key actions encode apostrophes and backslashes', function () { +test('browse keys marks names that are not valid variable keys without an add action', function () { Http::fake([ 'https://api.doppler.com/v3/configs/config/secrets/download*' => Http::response([ + 'API_KEY' => 'a', + 'db-password' => 'b', "TEAM'S_KEY" => 'apostrophe-secret', - 'TEAM\\KEY' => 'backslash-secret', + 'HTML' => 'html-secret', ]), ]); $this->application->secretManagerLink()->create(['integration_token_id' => $this->token->id]); - $apostropheExpression = 'addReference('.Js::from("TEAM'S_KEY").')'; - $backslashExpression = 'addReference('.Js::from('TEAM\\KEY').')'; + Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) + ->call('loadKeys') + ->assertSeeHtml('wire:click="addReference('.Js::from('API_KEY').')"') + ->assertSee('db-password') + ->assertSee('Not a valid variable name') + ->assertDontSeeHtml('addReference('.Js::from('db-password').')') + ->assertDontSeeHtml('addReference('.Js::from("TEAM'S_KEY").')') + ->assertSeeHtml('<b>HTML</b>') + ->assertDontSeeHtml('HTML'); +}); + +test('add reference rejects a remote name that is not a valid variable key', function () { + Http::fake([ + 'https://api.doppler.com/v3/configs/config/secrets/download*' => Http::response([ + 'db-password' => 'b', + ]), + ]); + + $this->application->secretManagerLink()->create(['integration_token_id' => $this->token->id]); Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) ->call('loadKeys') - ->assertSeeHtml('wire:click="'.$apostropheExpression.'"') - ->assertSeeHtml('wire:target="'.$apostropheExpression.'"') - ->assertSeeHtml('wire:click="'.$backslashExpression.'"') - ->assertSeeHtml('wire:target="'.$backslashExpression.'"'); + ->call('addReference', 'db-password') + ->assertDispatched('error', 'db-password is not a valid variable name.') + ->assertNotDispatched('refreshEnvs'); + + expect($this->application->environment_variables()->where('key', 'db-password')->exists())->toBeFalse(); }); test('add reference creates a variable with a secret reference value', function () { @@ -212,7 +233,7 @@ test('add reference creates a variable with a secret reference value', function Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) ->call('loadKeys') ->call('addReference', 'DB_PASSWORD') - ->assertDispatched('refreshEnvs') + ->assertDispatchedTo(All::class, 'refreshEnvs') ->assertDispatched('success'); $created = $this->application->environment_variables()->where('key', 'DB_PASSWORD')->firstOrFail(); @@ -235,8 +256,8 @@ test('import all creates references for missing keys and skips existing ones', f Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) ->call('importAll') - ->assertDispatched('refreshEnvs') - ->assertDispatched('success'); + ->assertDispatchedTo(All::class, 'refreshEnvs') + ->assertDispatched('success', 'Imported 1 keys as {{vault.KEY}} references.'); expect($this->application->environment_variables()->where('key', 'NEW_KEY')->firstOrFail()->value) ->toBe('{{vault.NEW_KEY}}') @@ -248,6 +269,49 @@ test('import all creates references for missing keys and skips existing ones', f ->and($auditEvent->metadata['secret_keys'])->toBe('[REDACTED]'); }); +test('import all skips invalid remote names, imports the rest, and reports both', function () { + Http::fake([ + 'https://api.doppler.com/v3/configs/config/secrets/download*' => Http::response([ + 'API_KEY' => 'a', + 'db-password' => 'b', + 'ZED' => 'c', + ]), + ]); + + $this->application->secretManagerLink()->create(['integration_token_id' => $this->token->id]); + + Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) + ->call('importAll') + ->assertDispatchedTo(All::class, 'refreshEnvs') + ->assertDispatched('success', 'Imported 2 keys as {{vault.KEY}} references. Skipped 1 key that is not a valid variable name: db-password.') + ->assertNotDispatched('error'); + + expect($this->application->environment_variables()->whereIn('key', ['API_KEY', 'ZED'])->pluck('value', 'key')->sortKeys()->all()) + ->toBe(['API_KEY' => '{{vault.API_KEY}}', 'ZED' => '{{vault.ZED}}']) + ->and($this->application->environment_variables()->where('key', 'db-password')->exists())->toBeFalse(); + + $auditEvent = AuditEvent::query()->where('event', 'ui.application.secret_manager.references_imported')->sole(); + expect($auditEvent->metadata['key_count'])->toBe(2) + ->and($auditEvent->metadata['skipped_key_count'])->toBe(1); +}); + +test('import all reports skipped names when no remote name is a valid variable key', function () { + Http::fake([ + 'https://api.doppler.com/v3/configs/config/secrets/download*' => Http::response([ + 'db-password' => 'a', + 'x' => 'b', + ]), + ]); + + $this->application->secretManagerLink()->create(['integration_token_id' => $this->token->id]); + + Livewire::test(SecretManagerLinks::class, ['resource' => $this->application]) + ->call('importAll') + ->assertDispatched('success', 'No keys imported. Skipped 2 keys that are not valid variable names: <b>x</b>, db-password.'); + + expect($this->application->environment_variables()->whereIn('key', ['db-password', 'x'])->exists())->toBeFalse(); +}); + test('the source can be removed', function () { $this->application->secretManagerLink()->create(['integration_token_id' => $this->token->id]);