From 35d68a792bc8788764b5ee086f3e17468be89fd0 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:02:21 +0200 Subject: [PATCH] Stop a rejected settings form flashing the credential it carried When validation fails, Laravel flashes the request's input into the session so the form can be repopulated. Its exclusion list is current_password, password and password_confirmation -- written for the login and password screens, and covering none of the credentials the system settings screens take. `dontFlash` did not appear anywhere in this repository. So every one of these went into the session in clear the moment its form was rejected: secret ExternalStorageSettingsController (S3 secret access key) key_file ExternalStorageSettingsController (GCS service account JSON) bind_password LdapSettingsController client_secret SocialLoginSettingsController, EmailSettingsController secret_key CaptchaSettingsController Each is stored with an `encrypted` cast, and config/session.php puts sessions in the database with `encrypt => false` -- so the rejected save wrote in clear into the same database the cast exists to protect. The sharpest one is key_file. serviceAccountKeyRule() exists to catch a paste that lost its last line, which makes "the request carrying a service account private key" and "the request that fails validation" the same request more often than not. dontFlash() merges rather than replaces, so the framework's three stay. The cost is that these five come back blank after a failed save. That is already what they do after a successful one -- every screen here treats them as write-only, and a blank means "keep what is stored" -- so the behaviour is now the same either way instead of only on success. Tests: one per field, each submitting a form that fails validation while carrying a secret, then reading the old input back the way the form would. All five fail against the unmodified bootstrap/app.php. A sixth pins that the framework's own three are still excluded, and each assertion checks a neighbouring non-secret field still comes back, so this cannot pass by flashing nothing at all. Note for the record: this is testable in the existing harness after all. phpunit.xml sets SESSION_DRIVER=array, but old input is written to the session whatever the driver backs it, so getOldInput() sees exactly what a database session would have stored. --- bootstrap/app.php | 27 ++++ .../Platform/SecretsNotFlashedTest.php | 118 ++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 tests/Feature/Platform/SecretsNotFlashedTest.php diff --git a/bootstrap/app.php b/bootstrap/app.php index 4bb5a5dd..6c2d2140 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -104,6 +104,33 @@ return Application::configure(basePath: dirname(__DIR__)) ]); }) ->withExceptions(function (Exceptions $exceptions) { + // Every credential this application stores encrypted, named again + // here so a failed validation does not write it back out in clear. + // + // A ValidationException flashes the request's input into the + // session so the form can be repopulated, minus this list. The + // framework's own three cover the login and password forms; none + // of the settings screens' credentials were on it, and + // config/session.php stores sessions in the database by default + // with `encrypt => false`. So a mistyped storage form put the + // secret access key in clear into the same database whose dump the + // `encrypted` cast exists to survive — and a service account key + // file, which is most likely to fail validation exactly when it + // was pasted incompletely, put a private key there. + // + // Merged with the framework's defaults rather than replacing them. + // The cost is that these fields come back blank after a failed + // save, which is what every one of these screens already does on a + // successful one: they are write-only, and a blank means "keep + // what is stored". + $exceptions->dontFlash([ + 'secret', // ExternalStorageSettingsController (S3) + 'key_file', // ExternalStorageSettingsController (GCS) + 'bind_password', // LdapSettingsController + 'client_secret', // SocialLoginSettingsController, EmailSettingsController + 'secret_key', // CaptchaSettingsController + ]); + // RFC 7807 for /api/* only. Everything else — web pages, Inertia // requests, the public share links — keeps Laravel's own handling // untouched, which is why this is scoped by path rather than by diff --git a/tests/Feature/Platform/SecretsNotFlashedTest.php b/tests/Feature/Platform/SecretsNotFlashedTest.php new file mode 100644 index 00000000..2c7184f6 --- /dev/null +++ b/tests/Feature/Platform/SecretsNotFlashedTest.php @@ -0,0 +1,118 @@ +admin = User::factory()->create(); +}); + +test('a rejected storage form does not flash the secret access key', function () { + // Fails on the missing bucket, which is required. + $this->actingAs($this->admin)->patch('/system/settings/storage', [ + 'active' => true, + 'provider' => 's3', + 'access_key' => 'AKIAEXAMPLE', + 'secret' => 'super-secret-access-key', + 'region' => 'us-east-1', + 'use_path_style' => false, + ])->assertSessionHasErrors('bucket'); + + expect(session()->getOldInput('secret'))->toBeNull() + // The rest of the form still comes back, or the screen would clear + // itself every time somebody mistypes one field. + ->and(session()->getOldInput('region'))->toBe('us-east-1'); +}); + +test('a rejected storage form does not flash the service account key file', function () { + // The sharpest case: serviceAccountKeyRule() is what catches a paste + // that lost its last line, so the request most likely to fail here is + // the one carrying a private key. + $key = json_encode([ + 'type' => 'service_account', + 'project_id' => 'example', + 'private_key_id' => 'abc', + 'private_key' => "-----BEGIN PRIVATE KEY-----\nMIIBVAIBADAN\n-----END PRIVATE KEY-----\n", + 'client_email' => 'svc@example.iam.gserviceaccount.com', + ]); + + $this->actingAs($this->admin)->patch('/system/settings/storage', [ + 'active' => true, + 'provider' => 'gcs', + 'key_file' => $key, + 'use_path_style' => false, + ])->assertSessionHasErrors('bucket'); + + expect(session()->getOldInput('key_file'))->toBeNull() + ->and(json_encode(session()->all()))->not->toContain('BEGIN PRIVATE KEY'); +}); + +test('a rejected ldap form does not flash the bind password', function () { + // Fails on the missing email_attribute, which is required. + $this->actingAs($this->admin)->patch('/system/settings/ldap', [ + 'active' => true, + 'port' => 636, + 'encryption' => 'ssl', + 'bind_dn' => 'cn=admin,dc=example,dc=test', + 'bind_password' => 'super-secret-bind-password', + 'name_attribute' => 'cn', + 'auto_provision' => false, + 'auto_approve' => false, + ])->assertSessionHasErrors('email_attribute'); + + expect(session()->getOldInput('bind_password'))->toBeNull() + ->and(session()->getOldInput('bind_dn'))->toBe('cn=admin,dc=example,dc=test'); +}); + +test('a rejected social login form does not flash the client secret', function () { + $this->actingAs($this->admin)->patch('/system/settings/social-login/google', [ + 'enabled' => true, + 'client_id' => '', + 'client_secret' => 'super-secret-client-secret', + ])->assertSessionHasErrors(); + + expect(session()->getOldInput('client_secret'))->toBeNull(); +}); + +test('a rejected captcha form does not flash the secret key', function () { + // Fails on the missing site_key, which turnstile requires. + $this->actingAs($this->admin)->patch('/system/settings/captcha', [ + 'provider' => 'turnstile', + 'secret_key' => 'super-secret-captcha-key', + 'on_login' => false, + 'on_registration' => false, + 'on_password_reset' => false, + 'on_public_comments' => false, + ])->assertSessionHasErrors('site_key'); + + expect(session()->getOldInput('secret_key'))->toBeNull() + ->and(session()->getOldInput('provider'))->toBe('turnstile'); +}); + +test('the framework defaults are kept, not replaced', function () { + // dontFlash() merges, so adding to it must not drop password. + $this->actingAs($this->admin)->put('/settings/password', [ + 'current_password' => 'wrong-password', + 'password' => 'a-brand-new-password', + 'password_confirmation' => 'a-brand-new-password', + ])->assertSessionHasErrors('current_password'); + + expect(session()->getOldInput('password'))->toBeNull() + ->and(session()->getOldInput('current_password'))->toBeNull(); +});