diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c91dae7..42e6b1ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,13 @@ The published image tags: `latest` follows the most recent release below, ## Unreleased +**The browser filled your own login into client-secret fields.** Opening Admin → SSO, or a Google, +Microsoft or integration provider form, could put your mail address in the client ID and your +password in the secret field, one press of Save away from being stored. An earlier fix told Chrome +these are not the saved login, but six of the fields also set `autocomplete="off"` of their own, +which won — and Chrome ignores `off` on a password field. They now send `new-password` as intended, +and a test reads every form for the same mistake. + **Sign in through Authentik, Keycloak or any other OpenID Connect provider.** Enter the provider's issuer, client ID and secret under the new **Admin → SSO** and the login page offers a "Sign in with …" button above the password form, which stays; the `OIDC_*` variables do the same for an install diff --git a/src/Form/AccountType.php b/src/Form/AccountType.php index 4b8cffb7..6d9db5dd 100644 --- a/src/Form/AccountType.php +++ b/src/Form/AccountType.php @@ -67,7 +67,6 @@ class AccountType extends AbstractType 'attr' => [ 'placeholder' => '••••••••', 'class' => 'form-input', - 'autocomplete' => 'new-password', ] + PasswordManagerIgnore::SECRET, 'label' => 'Password', 'always_empty' => true, @@ -149,7 +148,6 @@ class AccountType extends AbstractType 'attr' => [ 'placeholder' => '••••••••', 'class' => 'form-input', - 'autocomplete' => 'new-password', ] + PasswordManagerIgnore::SECRET, 'label' => 'account.form.smtp.password', 'always_empty' => true, diff --git a/src/Form/Admin/OidcConfigType.php b/src/Form/Admin/OidcConfigType.php index 928041bb..bd493ccb 100644 --- a/src/Form/Admin/OidcConfigType.php +++ b/src/Form/Admin/OidcConfigType.php @@ -73,7 +73,6 @@ final class OidcConfigType extends AbstractType 'required' => false, 'mapped' => false, 'attr' => [ - 'autocomplete' => 'off', 'placeholder' => true === $hasSecret ? 'admin.integrations.secret_unchanged' : 'admin.integrations.secret_none', ] + PasswordManagerIgnore::SECRET, ]); diff --git a/src/Form/CalDavConnectType.php b/src/Form/CalDavConnectType.php index bb74ff74..e5808028 100644 --- a/src/Form/CalDavConnectType.php +++ b/src/Form/CalDavConnectType.php @@ -105,7 +105,6 @@ final class CalDavConnectType extends AbstractType 'required' => false, 'mapped' => false, 'attr' => [ - 'autocomplete' => 'off', 'placeholder' => true === $isNew ? '' : 'settings.integrations.field.secret_unchanged', ] + PasswordManagerIgnore::SECRET, ]); diff --git a/src/Form/Integration/IntegrationConnectType.php b/src/Form/Integration/IntegrationConnectType.php index 062ea207..03b9995a 100644 --- a/src/Form/Integration/IntegrationConnectType.php +++ b/src/Form/Integration/IntegrationConnectType.php @@ -69,7 +69,6 @@ final class IntegrationConnectType extends AbstractType 'required' => $isNew, 'mapped' => false, 'attr' => [ - 'autocomplete' => 'off', 'placeholder' => $isNew ? '' : 'settings.integrations.field.secret_unchanged', ] + PasswordManagerIgnore::SECRET, ]); diff --git a/src/Form/Integration/IntegrationProviderConfigType.php b/src/Form/Integration/IntegrationProviderConfigType.php index 08743cb8..5fd93790 100644 --- a/src/Form/Integration/IntegrationProviderConfigType.php +++ b/src/Form/Integration/IntegrationProviderConfigType.php @@ -69,7 +69,6 @@ final class IntegrationProviderConfigType extends AbstractType 'required' => false, 'mapped' => false, 'attr' => [ - 'autocomplete' => 'off', 'placeholder' => $config instanceof IntegrationProviderConfig && $config->hasClientSecret() ? 'admin.integrations.secret_unchanged' : 'admin.integrations.secret_none', diff --git a/src/Form/Integration/MailProviderConfigType.php b/src/Form/Integration/MailProviderConfigType.php index 6a4a3b2c..46cb3247 100644 --- a/src/Form/Integration/MailProviderConfigType.php +++ b/src/Form/Integration/MailProviderConfigType.php @@ -45,16 +45,17 @@ final class MailProviderConfigType extends AbstractType 'attr' => ['autocomplete' => 'off'] + PasswordManagerIgnore::ATTR, ]) // Not a password anyone signs in with — it is a value pasted out - // of a provider's console. `new-password` invited password managers - // to offer generation, and in a wizard that renders one provider at - // a time, to fill fields that were not on screen. There is no - // standard way to say "this is not a login", hence the vendor hints. + // of a provider's console. There is no standard way to say "this is + // not a login", hence the vendor hints for the extensions and + // `new-password` for the browser, both from PasswordManagerIgnore. + // No `autocomplete` of its own here: this array is the left side of + // the `+`, so an `off` written in it wins and the browser's opt-out + // never reaches the field — which is how it stayed unfixed once. ->add('clientSecret', PasswordType::class, [ 'label' => 'admin.integrations.field.client_secret', 'required' => false, 'mapped' => false, 'attr' => [ - 'autocomplete' => 'off', 'placeholder' => $config instanceof MailProviderConfig && $config->hasClientSecret() ? 'admin.integrations.secret_unchanged' : 'admin.integrations.secret_none', @@ -87,7 +88,6 @@ final class MailProviderConfigType extends AbstractType 'mapped' => false, 'help' => 'admin.integrations.field.push_token_help', 'attr' => [ - 'autocomplete' => 'off', 'placeholder' => $config instanceof MailProviderConfig && null !== $config->pushVerificationToken ? 'admin.integrations.secret_unchanged' : 'admin.integrations.secret_none', diff --git a/tests/Form/PasswordManagerIgnoreTest.php b/tests/Form/PasswordManagerIgnoreTest.php index 59145d5d..37741993 100644 --- a/tests/Form/PasswordManagerIgnoreTest.php +++ b/tests/Form/PasswordManagerIgnoreTest.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace App\Tests\Form; +use App\Entity\Oidc\OidcConfig; +use App\Form\Admin\OidcConfigType; use App\Form\Setup\FirstAdminType; use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase; use Symfony\Component\Form\Extension\Core\Type\FormType; @@ -102,6 +104,62 @@ final class PasswordManagerIgnoreTest extends KernelTestCase self::assertStringContainsString('data-form-type', $html); } + /** + * A secret field has to reach the browser as `new-password`. + * + * PasswordManagerIgnore::SECRET carries it, and six credential fields + * never sent it: each wrote `'autocomplete' => 'off'` in the array it + * unioned SECRET onto, and with `+` the left side wins. Chrome ignores + * `off` on a password field, so Admin → SSO took the administrator's own + * mail address as its client ID and their password as its client secret — + * the very autofill the constant was introduced to stop, in every form but + * the one it was reported from. + * + * Read from the source rather than rendered form by form: the mistake is in + * how the array is written, and a new form can make it without appearing + * in any list a test keeps. + */ + public function testNoSecretFieldOverridesTheBrowsersOptOut(): void + { + $offenders = []; + + foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator(\dirname(__DIR__, 2) . '/src/Form')) as $file) { + if (false === $file->isFile() || 'php' !== $file->getExtension()) { + continue; + } + + $source = (string) file_get_contents($file->getPathname()); + + // Every array literal that is the left side of `+ SECRET`. + preg_match_all('/\[(?:[^\[\]]|\[[^\[\]]*\])*\]\s*\+\s*PasswordManagerIgnore::SECRET/', $source, $matches); + + foreach ($matches[0] as $literal) { + if (1 === preg_match("/'autocomplete'\s*=>/", $literal)) { + $offenders[] = $file->getFilename(); + } + } + } + + self::assertSame([], $offenders, 'These forms set their own `autocomplete` on a field that takes PasswordManagerIgnore::SECRET; theirs wins and the browser fills in the saved login.'); + } + + public function testTheSingleSignOnSecretIsNotOfferedTheSavedLogin(): void + { + self::bootKernel(); + + $container = self::getContainer(); + + $form = $container->get(FormFactoryInterface::class) + ->create(OidcConfigType::class, new OidcConfig(), ['csrf_protection' => false]) + ->createView(); + + $html = $container->get(Environment::class) + ->createTemplate('{{ form_widget(field) }}') + ->render(['field' => $form['clientSecret']]); + + self::assertStringContainsString('autocomplete="new-password"', $html); + } + private static function renderField(string $field): string { $container = self::getContainer();