fix(secrets): skip remote secret names that are not valid variable keys

Import all stopped at the first remote secret whose name is not a valid environment variable key (for example db-password in Vault or Infisical). The key setter threw, so the import was partial, no audit entry was written, the variable list did not refresh, and the user saw only a generic error. Invalid names are now skipped and listed in the result toast and the audit entry, and the rest are imported. Browse marks invalid names and offers no add action for them, and adding one shows a clear error. The refreshEnvs event now goes only to the environment variable list.
2026-11-06 13:10:37 -04:00 · 2026-10-01 07:16:45 +02:00
parent 8144b1cbc1
commit 8d7469230c
5 changed files with 162 additions and 25 deletions
@@ -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<string>  $imported
+     * @param  list<string>  $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),
+            )),
        ]);
    }
 }
@@ -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<string> The keys that were imported
+     * @return array{imported: list<string>, skipped: list<string>}
     */
    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. */
@@ -89,10 +89,16 @@
                                                    <span class="font-mono text-[12px] text-black dark:text-fg">{{ $key }}</span>
                                                    <span class="font-mono text-[11px] text-neutral-400 dark:text-fg-dim">{{ '{{vault.'.$key.'}'.'}' }}</span>
                                                </div>
-                                                <x-forms.button wire:click="addReference({{ \Illuminate\Support\Js::from($key) }})"
-                                                    wire:target="addReference({{ \Illuminate\Support\Js::from($key) }})">
-                                                    Add as variable
-                                                </x-forms.button>
+                                                @if (isset($invalidKeys[$key]))
+                                                    <span class="shrink-0 text-[11px] text-neutral-500 dark:text-fg-dim">
+                                                        Not a valid variable name
+                                                    </span>
+                                                @else
+                                                    <x-forms.button wire:click="addReference({{ \Illuminate\Support\Js::from($key) }})"
+                                                        wire:target="addReference({{ \Illuminate\Support\Js::from($key) }})">
+                                                        Add as variable
+                                                    </x-forms.button>
+                                                @endif
                                            </div>
                                        @empty
                                            <div class="px-3 py-2 text-[12px] text-neutral-500 dark:text-fg-dim">
@@ -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',
@@ -1,5 +1,6 @@
 <?php

+use App\Livewire\Project\Shared\EnvironmentVariable\All;
 use App\Livewire\Project\Shared\EnvironmentVariable\Show;
 use App\Livewire\Project\Shared\SecretManagerLinks;
 use App\Models\Application;
@@ -179,25 +180,45 @@ test('browse keys shows key names only and search filters them', function () {
        ->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',
+            '<b>HTML</b>' => '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('&lt;b&gt;HTML&lt;/b&gt;')
+        ->assertDontSeeHtml('<b>HTML</b>');
+});
+
+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',
+            '<b>x</b>' => '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: &lt;b&gt;x&lt;/b&gt;, db-password.');
+
+    expect($this->application->environment_variables()->whereIn('key', ['db-password', '<b>x</b>'])->exists())->toBeFalse();
+});
+
 test('the source can be removed', function () {
    $this->application->secretManagerLink()->create(['integration_token_id' => $this->token->id]);