From aa2592b17c9437389bcdeb654f40a18fc99ff30a Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 2 Oct 2026 16:01:00 +0200 Subject: [PATCH] fix(security): stop leaking locked shared variables and proxy secrets - References to locked (shown once) shared variables keep their reference text in the UI, copy action, and API instead of the locked value. - Remove the saved proxy configuration and validation logs from nested servers (for example destination.server) in API responses unless the token can read sensitive data. - The services list API now applies removeSensitiveData to every service. Co-Authored-By: Claude Opus 5.5 --- .../Controllers/Api/ServicesController.php | 10 +- .../Shared/EnvironmentVariable/Show.php | 5 +- app/Models/EnvironmentVariable.php | 95 ++++++++------- bootstrap/helpers/api.php | 71 ++++++++++- .../LockedSharedVariableReferenceApiTest.php | 76 ++++++++++++ .../NestedServerProxyConfigurationApiTest.php | 108 +++++++++++++++++ .../LockedSharedVariableReferenceTest.php | 112 ++++++++++++++++++ 7 files changed, 423 insertions(+), 54 deletions(-) create mode 100644 tests/Feature/Api/LockedSharedVariableReferenceApiTest.php create mode 100644 tests/Feature/Api/NestedServerProxyConfigurationApiTest.php create mode 100644 tests/Feature/LockedSharedVariableReferenceTest.php diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 988f380ea9..1737357c2c 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -272,13 +272,13 @@ class ServicesController extends Controller : []; foreach ($projects as $project) { - $services->push($project->services()->with($serviceRelations)->get()); - } - foreach ($services as $service) { - $service = $this->removeSensitiveData($service); + $services = $services->merge( + $project->services()->with($serviceRelations)->get() + ->map(fn (Service $service) => $this->removeSensitiveData($service)) + ); } - return response()->json($services->flatten()); + return response()->json($services->values()); } #[OA\Post( diff --git a/app/Livewire/Project/Shared/EnvironmentVariable/Show.php b/app/Livewire/Project/Shared/EnvironmentVariable/Show.php index 6aee8fceba..191c7b8223 100644 --- a/app/Livewire/Project/Shared/EnvironmentVariable/Show.php +++ b/app/Livewire/Project/Shared/EnvironmentVariable/Show.php @@ -184,6 +184,7 @@ class Show extends Component return $this->env->get_real_environment_variables_with_server( $this->env->resolveReferencedValue(), $this->env->resourceable, + revealLockedSharedVariables: false, ); } @@ -250,7 +251,9 @@ class Show extends Component $this->is_shared = (bool) ($this->env->is_shared ?? false); if ($this->is_shared) { - $this->real_value = $this->env->real_value; + $this->real_value = $this->env instanceof ModelsEnvironmentVariable + ? $this->env->displayRealValue() + : $this->env->real_value; $this->is_really_required = $this->is_required && blank($this->real_value); } else { $this->real_value = null; diff --git a/app/Models/EnvironmentVariable.php b/app/Models/EnvironmentVariable.php index d9323278e1..5d2ccd9e0e 100644 --- a/app/Models/EnvironmentVariable.php +++ b/app/Models/EnvironmentVariable.php @@ -184,44 +184,56 @@ class EnvironmentVariable extends BaseModel public function realValue(): Attribute { return Attribute::make( - get: function () { - if (! $this->relationLoaded('resourceable')) { - $this->load('resourceable'); - } - $resource = $this->resourceable; - if (! $resource) { - return null; - } - - // Load relationships needed for shared variable resolution - if (! $resource->relationLoaded('environment')) { - $resource->load('environment'); - } - if (! $resource->relationLoaded('server') && method_exists($resource, 'server')) { - $resource->load('server'); - } - if (! $resource->relationLoaded('destination') && method_exists($resource, 'destination')) { - $resource->load('destination.server'); - } - - $real_value = $this->get_real_environment_variables($this->value, $resource); - - // Skip escaping for valid JSON objects/arrays to prevent quote corruption (see #6160) - if (json_validate($real_value) && (str_starts_with($real_value, '{') || str_starts_with($real_value, '['))) { - return $real_value; - } - - if ($this->is_literal || $this->is_multiline) { - $real_value = '\''.$real_value.'\''; - } else { - $real_value = escapeEnvVariables($real_value); - } - - return $real_value; - } + get: fn () => $this->resolveRealValue(), ); } + /** + * Resolved value for display and copy. References to locked shared variables keep their + * reference text, so the UI never reveals a value that is only shown once. + */ + public function displayRealValue(): ?string + { + return $this->resolveRealValue(revealLockedSharedVariables: false); + } + + private function resolveRealValue(bool $revealLockedSharedVariables = true): ?string + { + if (! $this->relationLoaded('resourceable')) { + $this->load('resourceable'); + } + $resource = $this->resourceable; + if (! $resource) { + return null; + } + + // Load relationships needed for shared variable resolution + if (! $resource->relationLoaded('environment')) { + $resource->load('environment'); + } + if (! $resource->relationLoaded('server') && method_exists($resource, 'server')) { + $resource->load('server'); + } + if (! $resource->relationLoaded('destination') && method_exists($resource, 'destination')) { + $resource->load('destination.server'); + } + + $real_value = $this->get_real_environment_variables_internal($this->value, $resource, null, $revealLockedSharedVariables); + + // Skip escaping for valid JSON objects/arrays to prevent quote corruption (see #6160) + if (json_validate($real_value) && (str_starts_with($real_value, '{') || str_starts_with($real_value, '['))) { + return $real_value; + } + + if ($this->is_literal || $this->is_multiline) { + $real_value = '\''.$real_value.'\''; + } else { + $real_value = escapeEnvVariables($real_value); + } + + return $real_value; + } + protected function isReallyRequired(): Attribute { return Attribute::make( @@ -267,9 +279,9 @@ class EnvironmentVariable extends BaseModel return preg_match('/^{{\s*(?:'.$types.')\..*}}$/s', trim($this->value)) === 1; } - public function get_real_environment_variables_with_server(?string $environment_variable = null, $resource = null, $server = null) + public function get_real_environment_variables_with_server(?string $environment_variable = null, $resource = null, $server = null, bool $revealLockedSharedVariables = true) { - return $this->get_real_environment_variables_internal($environment_variable, $resource, $server); + return $this->get_real_environment_variables_internal($environment_variable, $resource, $server, $revealLockedSharedVariables); } public function getResolvedValueWithServer($server = null) @@ -351,12 +363,7 @@ class EnvironmentVariable extends BaseModel ->first()?->value ?? $value; } - private function get_real_environment_variables(?string $environment_variable = null, $resource = null) - { - return $this->get_real_environment_variables_internal($environment_variable, $resource); - } - - private function get_real_environment_variables_internal(?string $environment_variable = null, $resource = null, $serverOverride = null) + private function get_real_environment_variables_internal(?string $environment_variable = null, $resource = null, $serverOverride = null, bool $revealLockedSharedVariables = true) { if (is_null($environment_variable) || $environment_variable === '' || is_null($resource)) { return $environment_variable; @@ -396,7 +403,7 @@ class EnvironmentVariable extends BaseModel ->where('team_id', $resource->team()->id) ->where("{$type}_id", $id) ->first(); - if ($found) { + if ($found && ($revealLockedSharedVariables || ! $found->is_shown_once)) { $environment_variable = str($environment_variable)->replace("{{{$sharedEnv}}}", $found->value); } } diff --git a/bootstrap/helpers/api.php b/bootstrap/helpers/api.php index 2c33ba12ab..ee89f52c6f 100644 --- a/bootstrap/helpers/api.php +++ b/bootstrap/helpers/api.php @@ -7,6 +7,7 @@ use App\Enums\StaticImageTypes; use App\Models\Application; use App\Models\ApplicationSetting; use App\Models\Environment; +use App\Models\EnvironmentVariable; use App\Models\StandaloneDocker; use App\Models\SwarmDocker; use App\Rules\ManualWebhookSecret; @@ -16,6 +17,7 @@ use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Model; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; +use Illuminate\Support\Collection as SupportCollection; use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Validator; use Illuminate\Validation\Rule; @@ -42,7 +44,7 @@ function serializeApiResponse($data) { if ($data instanceof Collection) { return $data->map(function ($d) { - $d = collect($d)->sortKeys(); + $d = collect(apiEnvironmentVariableDisplayValue($d))->sortKeys(); $created_at = data_get($d, 'created_at'); $updated_at = data_get($d, 'updated_at'); if ($created_at) { @@ -67,10 +69,10 @@ function serializeApiResponse($data) $d = $d->prepend($d['id'], 'id'); } - return $d; + return removeServerProxySecretsFromApiPayload($d); }); } else { - $d = collect($data)->sortKeys(); + $d = collect(apiEnvironmentVariableDisplayValue($data))->sortKeys(); $created_at = data_get($d, 'created_at'); $updated_at = data_get($d, 'updated_at'); if ($created_at) { @@ -95,10 +97,71 @@ function serializeApiResponse($data) $d = $d->prepend($d['id'], 'id'); } - return $d; + return removeServerProxySecretsFromApiPayload($d); } } +/** + * Serialize an environment variable with its display value, so a reference to a + * locked shared variable keeps its reference text instead of the locked value. + */ +function apiEnvironmentVariableDisplayValue(mixed $data): mixed +{ + if (! $data instanceof EnvironmentVariable || in_array('real_value', $data->getHidden(), true)) { + return $data; + } + + return [...$data->toArray(), 'real_value' => $data->displayRealValue()]; +} + +/** + * Remove the saved proxy configuration and validation logs from every + * serialized server in an API payload, including nested ones such as + * `destination.server`, unless the token can read sensitive data. + * The saved proxy configuration often contains DNS provider API tokens. + * + * @template T + * + * @param T $payload + * @return T + */ +function removeServerProxySecretsFromApiPayload(mixed $payload): mixed +{ + if (request()->attributes->get('can_read_sensitive', false) === true) { + return $payload; + } + + return removeServerProxySecrets($payload); +} + +/** + * @template T + * + * @param T $value + * @return T + */ +function removeServerProxySecrets(mixed $value): mixed +{ + if (! is_array($value) && ! $value instanceof SupportCollection) { + return $value; + } + + $proxy = $value['proxy'] ?? null; + if (is_array($proxy)) { + unset($proxy['last_saved_proxy_configuration']); + $value['proxy'] = $proxy; + unset($value['validation_logs']); + } + + foreach ($value as $key => $item) { + if (is_array($item) || $item instanceof SupportCollection) { + $value[$key] = removeServerProxySecrets($item); + } + } + + return $value; +} + /** * Re-expose a model's `$hidden` sensitive fields when the current API request * carries the `read:sensitive` or `root` token ability (set by the diff --git a/tests/Feature/Api/LockedSharedVariableReferenceApiTest.php b/tests/Feature/Api/LockedSharedVariableReferenceApiTest.php new file mode 100644 index 0000000000..93224102a3 --- /dev/null +++ b/tests/Feature/Api/LockedSharedVariableReferenceApiTest.php @@ -0,0 +1,76 @@ + InstanceSettings::firstOrCreate(['id' => 0], ['is_api_enabled' => true])); + Server::flushIdentityMap(); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + session(['currentTeam' => $this->team]); + + $this->bearerToken = $this->user->createToken('test-token', ['read', 'read:sensitive'])->plainTextToken; + + $server = Server::factory()->create(['team_id' => $this->team->id]); + $destination = StandaloneDocker::query()->where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $this->team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + $this->application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => StandaloneDocker::class, + ]); +}); + +function createLockedSharedReferenceForApiTest(bool $locked): void +{ + SharedEnvironmentVariable::create([ + 'key' => 'DB_SECRET', + 'value' => 'locked-shared-secret', + 'type' => 'team', + 'team_id' => test()->team->id, + 'is_shown_once' => $locked, + ]); + EnvironmentVariable::create([ + 'key' => 'DATABASE_PASSWORD', + 'value' => '{{team.DB_SECRET}}', + 'resourceable_type' => Application::class, + 'resourceable_id' => test()->application->id, + ]); +} + +it('does not reveal a locked shared variable through a reference in the application envs API', function () { + createLockedSharedReferenceForApiTest(locked: true); + + $response = $this->withHeaders(['Authorization' => 'Bearer '.$this->bearerToken]) + ->getJson("/api/v1/applications/{$this->application->uuid}/envs"); + + $response->assertOk(); + $env = collect($response->json())->firstWhere('key', 'DATABASE_PASSWORD'); + expect($env['real_value'])->toBe('{{team.DB_SECRET}}') + ->and($response->getContent())->not->toContain('locked-shared-secret'); +}); + +it('still resolves an unlocked shared variable reference in the application envs API', function () { + createLockedSharedReferenceForApiTest(locked: false); + + $response = $this->withHeaders(['Authorization' => 'Bearer '.$this->bearerToken]) + ->getJson("/api/v1/applications/{$this->application->uuid}/envs"); + + $response->assertOk(); + expect(collect($response->json())->firstWhere('key', 'DATABASE_PASSWORD')['real_value'])->toBe('locked-shared-secret'); +}); diff --git a/tests/Feature/Api/NestedServerProxyConfigurationApiTest.php b/tests/Feature/Api/NestedServerProxyConfigurationApiTest.php new file mode 100644 index 0000000000..f6325ccaf8 --- /dev/null +++ b/tests/Feature/Api/NestedServerProxyConfigurationApiTest.php @@ -0,0 +1,108 @@ + $team]); + $token = $user->createToken('nested-proxy-test', $abilities); + DB::table('personal_access_tokens')->where('id', $token->accessToken->id)->update([ + 'team_id' => $team->id, + ]); + + return $token->plainTextToken; +} + +beforeEach(function () { + Server::flushIdentityMap(); + InstanceSettings::query()->delete(); + $settings = new InstanceSettings; + $settings->id = 0; + $settings->save(); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + session(['currentTeam' => $this->team]); + + $this->server = Server::factory()->create(['team_id' => $this->team->id]); + $this->server->proxy->set('last_saved_proxy_configuration', "services:\n traefik:\n environment:\n - ".NESTED_PROXY_SECRET."\n"); + $this->server->save(); + + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); + $destination = $this->server->standaloneDockers()->firstOrFail(); + + $this->application = Application::create([ + 'name' => 'nested-proxy-app', + 'git_repository' => 'https://github.com/test/test', + 'git_branch' => 'main', + 'build_pack' => 'nixpacks', + 'ports_exposes' => '3000', + 'environment_id' => $this->environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + + $this->database = StandalonePostgresql::create([ + 'name' => 'nested-proxy-db', + 'postgres_user' => 'postgres', + 'postgres_password' => 'password', + 'postgres_db' => 'app', + 'image' => 'postgres:16-alpine', + 'environment_id' => $this->environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + + $this->service = Service::factory()->create([ + 'server_id' => $this->server->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'environment_id' => $this->environment->id, + ]); +}); + +dataset('endpoints with nested server data', [ + 'applications list' => [fn () => '/api/v1/applications'], + 'application detail' => [fn () => "/api/v1/applications/{$this->application->uuid}"], + 'databases list' => [fn () => '/api/v1/databases'], + 'database detail' => [fn () => "/api/v1/databases/{$this->database->uuid}"], + 'services list' => [fn () => '/api/v1/services'], + 'service detail' => [fn () => "/api/v1/services/{$this->service->uuid}"], + 'resources list' => [fn () => '/api/v1/resources'], +]); + +test('read token does not receive the saved proxy configuration of a nested server', function (Closure $endpoint) { + $token = nestedProxyApiToken($this->user, $this->team, ['read']); + + $response = $this->withToken($token)->getJson($endpoint->call($this)); + + $response->assertOk(); + expect($response->getContent()) + ->not->toContain('nested-proxy-config-secret') + ->not->toContain('last_saved_proxy_configuration'); +})->with('endpoints with nested server data'); + +test('read sensitive token still receives the saved proxy configuration of a nested server', function (Closure $endpoint) { + $token = nestedProxyApiToken($this->user, $this->team, ['read', 'read:sensitive']); + + $response = $this->withToken($token)->getJson($endpoint->call($this)); + + $response->assertOk(); + expect($response->getContent())->toContain('nested-proxy-config-secret'); +})->with('endpoints with nested server data'); diff --git a/tests/Feature/LockedSharedVariableReferenceTest.php b/tests/Feature/LockedSharedVariableReferenceTest.php new file mode 100644 index 0000000000..974623cbdc --- /dev/null +++ b/tests/Feature/LockedSharedVariableReferenceTest.php @@ -0,0 +1,112 @@ + 0]); + Server::flushIdentityMap(); + + $this->user = User::factory()->create(); + $this->team = Team::factory()->create(); + $this->team->members()->attach($this->user, ['role' => 'owner']); + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); + $this->server = Server::factory()->create(['team_id' => $this->team->id]); + $destination = StandaloneDocker::query()->where('server_id', $this->server->id)->firstOrFail(); + $this->application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'destination_id' => $destination->id, + 'destination_type' => StandaloneDocker::class, + ]); + + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); +}); + +function createSharedVariableForLockTest(string $type, bool $locked): SharedEnvironmentVariable +{ + $owner = match ($type) { + 'team' => [], + 'project' => ['project_id' => test()->project->id], + 'environment' => ['project_id' => test()->project->id, 'environment_id' => test()->environment->id], + 'server' => ['server_id' => test()->server->id], + }; + + return SharedEnvironmentVariable::create([ + 'key' => 'DB_SECRET', + 'value' => 'review-fix-e-shared-secret', + 'type' => $type, + 'team_id' => test()->team->id, + 'is_shown_once' => $locked, + ...$owner, + ]); +} + +function createReferencingVariableForLockTest(string $value): EnvironmentVariable +{ + return EnvironmentVariable::create([ + 'key' => 'DATABASE_PASSWORD', + 'value' => $value, + 'resourceable_type' => Application::class, + 'resourceable_id' => test()->application->id, + ]); +} + +test('copying a reference does not reveal a locked shared variable', function (string $type) { + createSharedVariableForLockTest($type, locked: true); + + Livewire::test(Show::class, ['env' => createReferencingVariableForLockTest("{{{$type}.DB_SECRET}}"), 'type' => 'application']) + ->call('copyValue') + ->assertReturned("{{{$type}.DB_SECRET}}"); +})->with(['team', 'project', 'environment', 'server']); + +test('copying an embedded reference does not reveal a locked shared variable', function () { + createSharedVariableForLockTest('team', locked: true); + + Livewire::test(Show::class, ['env' => createReferencingVariableForLockTest('postgres://app:{{team.DB_SECRET}}@db'), 'type' => 'application']) + ->call('copyValue') + ->assertReturned('postgres://app:{{team.DB_SECRET}}@db'); +}); + +test('the resolved value in the editor does not reveal a locked shared variable', function (string $type) { + createSharedVariableForLockTest($type, locked: true); + + $component = Livewire::test(Show::class, ['env' => createReferencingVariableForLockTest("{{{$type}.DB_SECRET}}"), 'type' => 'application']) + ->call('loadValues'); + + expect($component->get('real_value'))->not->toContain('review-fix-e-shared-secret') + ->and(json_encode($component->snapshot))->not->toContain('review-fix-e-shared-secret'); +})->with(['team', 'project', 'environment', 'server']); + +test('the editor and copy still resolve an unlocked shared variable', function () { + createSharedVariableForLockTest('team', locked: false); + + Livewire::test(Show::class, ['env' => createReferencingVariableForLockTest('{{team.DB_SECRET}}'), 'type' => 'application']) + ->call('loadValues') + ->assertSet('real_value', 'review-fix-e-shared-secret') + ->call('copyValue') + ->assertReturned('review-fix-e-shared-secret'); +}); + +test('deployments still receive the value of a locked shared variable', function (string $type) { + createSharedVariableForLockTest($type, locked: true); + $env = createReferencingVariableForLockTest("{{{$type}.DB_SECRET}}"); + + expect($env->getResolvedValueWithServer($this->server))->toBe('review-fix-e-shared-secret') + ->and($env->get_real_environment_variables_with_server($env->value, $this->application, $this->server))->toBe('review-fix-e-shared-secret') + ->and($env->fresh()->real_value)->toBe('review-fix-e-shared-secret'); +})->with(['team', 'project', 'environment', 'server']);