fix(env): prevent duplicate environment variables and allow removing extra copies

Closes #8041

app/Http/Controllers/Api/ServicesController.php

@@ -573,8 +573,7 @@ class ServicesController extends Controller
                             $command = $value->after('SERVICE_')->beforeLast('_');
                             $generatedValue = generateEnvValue($command->value(), $service);
                         }
-                        EnvironmentVariable::create([
-                            'key' => $key,
+                        $service->environment_variables()->updateOrCreate(['key' => $key], [
                             'value' => $generatedValue,
                             'resourceable_id' => $service->id,
                             'resourceable_type' => $service->getMorphClass(),

app/Livewire/Project/Resource/Create.php

@@ -2,7 +2,6 @@
 
 namespace App\Livewire\Project\Resource;
 
-use App\Models\EnvironmentVariable;
 use App\Models\Service;
 use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
 use Livewire\Component;
@@ -106,8 +105,7 @@ class Create extends Component
                             $key = str()->before($value, '=');
                             $value = str(str()->after($value, '='));
                             if ($value) {
-                                EnvironmentVariable::create([
-                                    'key' => $key,
+                                $service->environment_variables()->updateOrCreate(['key' => $key], [
                                     'value' => $value,
                                     'resourceable_id' => $service->id,
                                     'resourceable_type' => $service->getMorphClass(),

app/Livewire/Project/Shared/EnvironmentVariable/All.php

@@ -1138,7 +1138,9 @@ class All extends Component
     private function handleSingleSubmit($data)
     {
         $data['key'] = ValidationPatterns::validatedEnvironmentVariableKey($data['key']);
-        $found = $this->resource->environment_variables()->where('key', $data['key'])->first();
+        $found = EnvironmentVariable::query()
+            ->forResourceKey($this->resource->getMorphClass(), $this->resource->id, (bool) ($data['is_preview'] ?? false), $data['key'])
+            ->exists();
         if ($found) {
             $this->dispatch('error', 'Environment variable already exists.');
 
@@ -1240,9 +1242,11 @@ class All extends Component
             $comment = is_array($data) ? ($data['comment'] ?? null) : null;
 
             $method = $isPreview ? 'environment_variables_preview' : 'environment_variables';
-            $found = $this->resource->$method()->where('key', $key)->first();
+            // A key can have copies from earlier duplicate saves. Each copy gets the saved value,
+            // so that a deployment uses this value whichever copy it reads last.
+            $copies = $this->resource->$method()->where('key', $key)->get();
 
-            if ($found) {
+            foreach ($copies as $found) {
                 if (! $found->is_shown_once && ! $found->is_multiline && formatEnvValueForDevView($found->value) !== null) {
                     $changed = false;
 
@@ -1267,7 +1271,9 @@ class All extends Component
                         $count++;
                     }
                 }
-            } else {
+            }
+
+            if ($copies->isEmpty()) {
                 $environment = new EnvironmentVariable;
                 $environment->key = $key;
                 $environment->value = $value;

app/Livewire/Project/Shared/EnvironmentVariable/Show.php

@@ -214,6 +214,15 @@ class Show extends Component
         );
     }
 
+    private function keyBelongsToAnotherVariable(): bool
+    {
+        if ($this->isSharedVariable) {
+            return false;
+        }
+
+        return $this->env->sameKeyVariables(ValidationPatterns::normalizeEnvironmentVariableKey($this->key))->exists();
+    }
+
     private function syncData(bool $toModel = false): void
     {
         if ($toModel) {
@@ -376,6 +385,13 @@ class Show extends Component
                 }
             }
 
+            if ($this->keyBelongsToAnotherVariable()) {
+                $this->key = $this->env->key;
+                $this->dispatch('error', 'Environment variable already exists.');
+
+                return;
+            }
+
             $submittedValue = $this->env->is_shown_once
                 ? (filled($this->newLockedValue) ? $this->newLockedValue : $this->env->value)
                 : $this->value;
@@ -586,8 +602,9 @@ class Show extends Component
         try {
             $this->authorize('delete', $this->env);
 
-            // Check if the variable is used in Docker Compose
-            if ($this->type === 'service' || $this->type === 'application' && $this->env->resourceable?->docker_compose) {
+            // A Compose variable must keep one row, but an extra copy of it can go.
+            $isDuplicate = ! $this->isSharedVariable && $this->env->sameKeyVariables()->exists();
+            if (! $isDuplicate && ($this->type === 'service' || $this->type === 'application' && $this->env->resourceable?->docker_compose)) {
                 [$isUsed, $reason] = $this->isEnvironmentVariableUsedInDockerCompose($this->env->key, $this->env->resourceable?->docker_compose);
 
                 if ($isUsed) {

app/Models/EnvironmentVariable.php

@@ -12,11 +12,13 @@ use Closure;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Eloquent\Casts\Attribute;
 use Illuminate\Database\Eloquent\Model;
+use Illuminate\Database\UniqueConstraintViolationException;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Gate;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Validation\ValidationException;
 use OpenApi\Attributes as OA;
+use PDOException;
 
 #[OA\Schema(
     description: 'Environment Variable model',
@@ -201,6 +203,62 @@ class EnvironmentVariable extends BaseModel
         return $this->belongsTo(Service::class);
     }
 
+    /**
+     * The variables of one resource and preview flag that have the given key.
+     */
+    public function scopeForResourceKey(Builder $query, string $resourceableType, int $resourceableId, bool $isPreview, string $key): Builder
+    {
+        return $query->where('resourceable_type', $resourceableType)
+            ->where('resourceable_id', $resourceableId)
+            ->where('is_preview', $isPreview)
+            ->where('key', $key);
+    }
+
+    /**
+     * The other variables of this resource and preview flag that have the given key (by default its own key).
+     */
+    public function sameKeyVariables(?string $key = null): Builder
+    {
+        return static::query()
+            ->forResourceKey($this->resourceable_type, $this->resourceable_id, (bool) $this->is_preview, $key ?? $this->key)
+            ->whereKeyNot($this->getKey());
+    }
+
+    /**
+     * The table has no unique index on resource and key, so two requests that create the same key at the
+     * same time (parallel Compose parses or saves) could both insert it. Inserts of one key wait for each
+     * other, and a second row is refused like a unique index refuses it, so firstOrCreate() returns the
+     * existing row.
+     */
+    protected function performInsert(Builder $query)
+    {
+        if ($this->resourceable_type === null || $this->resourceable_id === null) {
+            return parent::performInsert($query);
+        }
+
+        $connection = $this->getConnection();
+
+        return $connection->transaction(function () use ($connection, $query) {
+            if ($connection->getDriverName() === 'pgsql') {
+                // Held until the transaction commits, so the next insert of this key sees this row.
+                $connection->select('select pg_advisory_xact_lock(hashtextextended(?, 0))', [
+                    "environment_variables:{$this->resourceable_type}:{$this->resourceable_id}:".(int) (bool) $this->is_preview.":{$this->key}",
+                ]);
+            }
+
+            if ($this->sameKeyVariables()->exists()) {
+                throw new UniqueConstraintViolationException(
+                    $connection->getName(),
+                    'insert into "environment_variables"',
+                    [],
+                    new PDOException("Environment variable {$this->key} already exists."),
+                );
+            }
+
+            return parent::performInsert($query);
+        });
+    }
+
     public function scopeWithoutBuildpackControlVariables(Builder $query): Builder
     {
         foreach (self::BUILDPACK_CONTROL_VARIABLE_PREFIXES as $prefix) {

app/Services/ServerTransfer/ServerTransferImporter.php

@@ -1521,6 +1521,10 @@ class ServerTransferImporter
      */
     private function importEnvVars(array $variables, object $resource, bool $isPreview): void
     {
+        // Older sources can export duplicate keys of one resource; Docker Compose uses the last one.
+        $variables = collect($variables)
+            ->keyBy(fn (array $variable): string => ((bool) data_get($variable, 'is_preview', false) || $isPreview ? 'preview:' : 'production:').data_get($variable, 'key'));
+
         foreach ($variables as $variable) {
             EnvironmentVariable::withoutEvents(function () use ($variable, $resource, $isPreview) {
                 $uuid = filled(data_get($variable, 'uuid')) ? (string) data_get($variable, 'uuid') : new_public_id();

app/Traits/HasRepositoryDetection.php

@@ -5,7 +5,6 @@ namespace App\Traits;
 use App\Data\RepositoryDetectionResult;
 use App\Models\Application;
 use App\Models\ApplicationSetting;
-use App\Models\EnvironmentVariable;
 use App\Models\GiteaApp;
 use App\Models\GithubApp;
 use App\Models\GitlabApp;
@@ -144,8 +143,8 @@ trait HasRepositoryDetection
                 if (! preg_match('/^[A-Z_][A-Z0-9_]*$/i', (string) $key)) {
                     continue;
                 }
-                EnvironmentVariable::create([
-                    'key' => $key,
+                // The created hook of the application can already have added a default for this key.
+                $application->environment_variables()->updateOrCreate(['key' => $key], [
                     'value' => $value,
                     'resourceable_type' => $application->getMorphClass(),
                     'resourceable_id' => $application->id,

bootstrap/helpers/shared.php

@@ -3458,12 +3458,13 @@ function parseDockerComposeFile(Service|Application $resource, bool $isNew = fal
                                 }
                                 $savedService->save();
                             }
-                            EnvironmentVariable::create([
-                                'key' => $key,
-                                'value' => $fqdn,
+                            EnvironmentVariable::firstOrCreate([
+                                'key' => $key->value(),
                                 'resourceable_type' => get_class($resource),
                                 'resourceable_id' => $resource->id,
                                 'is_preview' => false,
+                            ], [
+                                'value' => $fqdn,
                                 'comment' => $envComments[$originalKey] ?? null,
                             ]);
                         }

tests/Feature/ApplicationDeploymentControlVarFilteringTest.php

@@ -12,6 +12,7 @@ use App\Models\Server;
 use App\Models\Team;
 use App\Models\User;
 use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Arr;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\DB;
 use Symfony\Component\Process\Process;
@@ -276,7 +277,7 @@ function makeDeploymentControlVarFixture(array $applicationAttributes = []): arr
 
 function createApplicationEnvironmentVariable(Application $application, array $attributes): EnvironmentVariable
 {
-    return EnvironmentVariable::create([
+    $attributes = [
         'resourceable_type' => Application::class,
         'resourceable_id' => $application->id,
         'is_preview' => false,
@@ -285,7 +286,13 @@ function createApplicationEnvironmentVariable(Application $application, array $a
         'is_multiline' => false,
         'is_literal' => false,
         ...$attributes,
-    ]);
+    ];
+
+    // The created hooks of the application already add defaults such as NIXPACKS_NODE_VERSION.
+    return EnvironmentVariable::updateOrCreate(
+        Arr::only($attributes, ['resourceable_type', 'resourceable_id', 'is_preview', 'key']),
+        $attributes,
+    );
 }
 
 function makeControlVarFilteringJob(Application $application, Server $server, array $overrides = []): array
@@ -1138,7 +1145,7 @@ it('creates missing Laravel defaults as preview variables for preview deployment
         ->and($fallbackPath->value)->toBe('/index.php')
         ->and($rootDir->is_preview)->toBeTrue()
         ->and($rootDir->value)->toBe('/app/public')
-        ->and($application->environment_variables()->whereIn('key', $keys)->pluck('value', 'key')->all())->toBe([
+        ->and($application->environment_variables()->whereIn('key', $keys)->pluck('value', 'key')->all())->toEqual([
             'NIXPACKS_PHP_ROOT_DIR' => '/app/web',
             'NIXPACKS_PHP_FALLBACK_PATH' => '/app.php',
         ])

tests/Feature/CloneApplicationEnvironmentVariablesTest.php

@@ -60,7 +60,7 @@ test('the migration keeps the most recently changed NIXPACKS_NODE_VERSION of eac
     $other = Application::factory()->create($attributes);
 
     // The rows an earlier clone_application() added next to the defaults of the created hooks.
-    $copies = collect([false, true])->map(fn (bool $isPreview) => EnvironmentVariable::query()->create([
+    $copies = collect([false, true])->map(fn (bool $isPreview) => insertDuplicateEnvironmentVariable([
         'key' => 'NIXPACKS_NODE_VERSION',
         'value' => '20',
         'is_preview' => $isPreview,

tests/Feature/EnvironmentVariable/EnvironmentVariableMultilineWhitespaceTest.php

@@ -18,8 +18,7 @@ beforeEach(function () {
 
 function createVariable(Application $application, string $value, bool $isMultiline = false): EnvironmentVariable
 {
-    $env = EnvironmentVariable::create([
-        'key' => 'TEST_VAR',
+    $env = $application->environment_variables()->updateOrCreate(['key' => 'TEST_VAR'], [
         'value' => $value,
         'is_multiline' => $isMultiline,
         'is_literal' => true,

tests/Feature/EnvironmentVariableDuplicateKeyTest.php

@@ -0,0 +1,148 @@
+<?php
+
+use App\Livewire\Project\Shared\EnvironmentVariable\All;
+use App\Livewire\Project\Shared\EnvironmentVariable\Show;
+use App\Models\Application;
+use App\Models\Environment;
+use App\Models\EnvironmentVariable;
+use App\Models\InstanceSettings;
+use App\Models\Project;
+use App\Models\Team;
+use App\Models\User;
+use Illuminate\Database\UniqueConstraintViolationException;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Livewire\Livewire;
+
+uses(RefreshDatabase::class);
+
+beforeEach(function () {
+    InstanceSettings::forceCreate(['id' => 0]);
+    $this->user = User::factory()->create();
+    $team = Team::factory()->create();
+    $team->members()->attach($this->user, ['role' => 'owner']);
+    $project = Project::factory()->create(['team_id' => $team->id]);
+    $environment = Environment::factory()->create(['project_id' => $project->id]);
+    $this->application = Application::factory()->create(['environment_id' => $environment->id]);
+
+    $this->actingAs($this->user);
+});
+
+function duplicateKeyTestVariable(Application $application, string $key, bool $isPreview = false, string $value = 'value'): EnvironmentVariable
+{
+    // Without events, so that the created hook adds no preview copy.
+    return EnvironmentVariable::withoutEvents(fn () => EnvironmentVariable::query()->forceCreate([
+        'uuid' => new_public_id(),
+        'key' => $key,
+        'value' => $value,
+        'is_preview' => $isPreview,
+        'resourceable_type' => Application::class,
+        'resourceable_id' => $application->id,
+    ]));
+}
+
+test('a second variable with the same key on one resource is refused', function () {
+    duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS');
+
+    expect(fn () => duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS'))
+        ->toThrow(UniqueConstraintViolationException::class);
+});
+
+test('the same key can exist for production and preview, and on other resources', function () {
+    $other = Application::factory()->create(['environment_id' => $this->application->environment_id]);
+
+    duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS');
+    duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS', isPreview: true);
+    duplicateKeyTestVariable($other, 'JOB_ATTEMPTS');
+
+    expect(EnvironmentVariable::query()->where('key', 'JOB_ATTEMPTS')->count())->toBe(3);
+});
+
+test('a create that loses the race to another request returns the row of that request', function () {
+    $existing = duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS', value: '3');
+
+    // firstOrCreate() goes this way when its lookup ran before the other request inserted the row.
+    $result = $this->application->environment_variables()->createOrFirst(['key' => 'JOB_ATTEMPTS'], ['value' => '5']);
+
+    expect($result->id)->toBe($existing->id)
+        ->and($this->application->environment_variables()->where('key', 'JOB_ATTEMPTS')->pluck('value')->all())->toBe(['3']);
+});
+
+test('renaming a variable to a key that the resource already has is refused', function () {
+    duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS', value: '3');
+    $renamed = duplicateKeyTestVariable($this->application, 'JOB_BACKOFF_DELAY', value: '1000');
+
+    Livewire::test(Show::class, ['env' => $renamed, 'type' => 'application'])
+        ->set('key', 'JOB_ATTEMPTS')
+        ->call('submit')
+        ->assertDispatched('error', 'Environment variable already exists.')
+        ->assertSet('key', 'JOB_BACKOFF_DELAY');
+
+    expect($renamed->fresh()->key)->toBe('JOB_BACKOFF_DELAY');
+});
+
+test('adding a preview variable whose key already exists for previews is refused', function () {
+    duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS', isPreview: true);
+
+    Livewire::test(All::class, ['resource' => $this->application])
+        ->call('submit', ['key' => 'JOB_ATTEMPTS', 'value' => '3', 'is_preview' => true])
+        ->assertDispatched('error', 'Environment variable already exists.');
+
+    expect($this->application->environment_variables_preview()->where('key', 'JOB_ATTEMPTS')->count())->toBe(1);
+});
+
+test('saving the developer view again does not add rows', function () {
+    $component = Livewire::test(All::class, ['resource' => $this->application])
+        ->call('loadEnvironmentVariables')
+        ->call('switch')
+        ->set('variables', "JOB_ATTEMPTS=3\nJOB_BACKOFF_DELAY=1000");
+
+    $component->call('submit')->call('submit')
+        ->set('variables', "JOB_ATTEMPTS=4\nJOB_BACKOFF_DELAY=1000")
+        ->call('submit');
+
+    expect($this->application->environment_variables()->whereIn('key', ['JOB_ATTEMPTS', 'JOB_BACKOFF_DELAY'])->pluck('value', 'key')->all())
+        ->toBe(['JOB_ATTEMPTS' => '4', 'JOB_BACKOFF_DELAY' => '1000'])
+        ->and($this->application->environment_variables()->whereIn('key', ['JOB_ATTEMPTS', 'JOB_BACKOFF_DELAY'])->count())->toBe(2);
+});
+
+test('saving the developer view gives every existing copy of a key the saved value', function () {
+    duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS', value: '3');
+    insertDuplicateEnvironmentVariable([
+        'key' => 'JOB_ATTEMPTS',
+        'value' => '5',
+        'resourceable_type' => Application::class,
+        'resourceable_id' => $this->application->id,
+    ]);
+
+    Livewire::test(All::class, ['resource' => $this->application])
+        ->call('loadEnvironmentVariables')
+        ->call('switch')
+        ->set('variables', 'JOB_ATTEMPTS=7')
+        ->call('submit');
+
+    expect($this->application->environment_variables()->where('key', 'JOB_ATTEMPTS')->pluck('value')->all())->toBe(['7', '7']);
+});
+
+test('an extra copy of a Compose variable can be deleted, but not its last row', function () {
+    $this->application->update([
+        'build_pack' => 'dockercompose',
+        'docker_compose' => "services:\n  app:\n    image: nginx\n    environment:\n      JOB_ATTEMPTS: \${JOB_ATTEMPTS}\n",
+    ]);
+    $first = duplicateKeyTestVariable($this->application, 'JOB_ATTEMPTS', value: '3');
+    $copy = insertDuplicateEnvironmentVariable([
+        'key' => 'JOB_ATTEMPTS',
+        'value' => '3',
+        'resourceable_type' => Application::class,
+        'resourceable_id' => $this->application->id,
+    ]);
+
+    Livewire::test(Show::class, ['env' => $copy, 'type' => 'application'])
+        ->call('delete')
+        ->assertDispatched('success', 'Environment variable deleted successfully.');
+
+    Livewire::test(Show::class, ['env' => $first->fresh(), 'type' => 'application'])
+        ->call('delete')
+        ->assertNotDispatched('success');
+
+    expect($this->application->environment_variables()->where('key', 'JOB_ATTEMPTS')->pluck('id')->all())->toBe([$first->id]);
+});

tests/Feature/RedisUsernameEnvironmentVariableTest.php

@@ -2,7 +2,6 @@
 
 use App\Actions\Database\StartRedis;
 use App\Models\Environment;
-use App\Models\EnvironmentVariable;
 use App\Models\InstanceSettings;
 use App\Models\PrivateKey;
 use App\Models\Project;
@@ -75,7 +74,7 @@ it('creates one REDIS_USERNAME variable and starts with unique environment entri
 it('removes duplicate REDIS_USERNAME variables and keeps the most recently changed one', function () {
     $database = create_standalone_redis($this->environment->id, $this->destination);
     $other = create_standalone_redis($this->environment->id, $this->destination);
-    $kept = EnvironmentVariable::query()->create([
+    $kept = insertDuplicateEnvironmentVariable([
         'key' => 'REDIS_USERNAME',
         'value' => 'admin',
         'resourceable_type' => StandaloneRedis::class,

tests/Feature/RepositoryScanSecurityTest.php

@@ -66,6 +66,22 @@ test('environment import stores valid keys and skips malformed keys', function (
     expect($application->environment_variables()->pluck('value', 'key')->all())->toBe(['VALID_KEY' => 'value']);
 });
 
+test('environment import updates a default variable instead of adding a second one', function () {
+    $application = Application::factory()->create(['build_pack' => 'nixpacks']);
+    $component = new class extends PublicGitRepository
+    {
+        public function importInto(Application $application): void
+        {
+            $this->importDetectedEnvironmentVariables($application);
+        }
+    };
+    $component->envImported = true;
+    $component->envExampleVars = ['NIXPACKS_NODE_VERSION' => '20'];
+    $component->importInto($application);
+
+    expect($application->environment_variables()->where('key', 'NIXPACKS_NODE_VERSION')->pluck('value')->all())->toBe(['20']);
+});
+
 class RepositoryScanStateTestComponent extends PublicGitRepository
 {
     public function mount() {}

tests/Pest.php

@@ -1,7 +1,9 @@
 <?php
 
+use App\Models\EnvironmentVariable;
 use App\Models\Server;
 use Illuminate\Process\PendingProcess;
+use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Process;
 use Illuminate\Support\Once;
 use Livewire\Features\SupportTesting\Testable;
@@ -181,3 +183,23 @@ function writtenProxyFile(array $lines, string $file): ?string
 
     return $content;
 }
+
+/**
+ * Inserts an environment variable next to one with the same key, like older versions could do.
+ * The model refuses such a row, so the insert skips it.
+ *
+ * @param  array<string, mixed>  $attributes
+ */
+function insertDuplicateEnvironmentVariable(array $attributes): EnvironmentVariable
+{
+    $id = DB::table('environment_variables')->insertGetId([
+        'uuid' => new_public_id(),
+        'is_preview' => false,
+        'created_at' => now(),
+        'updated_at' => now(),
+        ...$attributes,
+        'value' => encrypt((string) ($attributes['value'] ?? '')),
+    ]);
+
+    return EnvironmentVariable::query()->findOrFail($id);
+}