mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
Send new-password on the secret fields that were overriding it
Reported from Admin → SSO: Chrome put the administrator's own mail address in the client ID and their password in the client secret. PasswordManagerIgnore::SECRET exists to stop exactly that, and carries autocomplete="new-password". Six credential fields wrote 'autocomplete' => 'off' in the array they unioned SECRET onto, and with `+` the left side wins — so the earlier fix reached the Admin → AI token field, which spreads the constant, and none of the others. Chrome ignores `off` on a password field. The new single sign-on form copied the pattern. The fields no longer set an autocomplete of their own. The two mail account password fields set new-password twice and lose the duplicate, so one rule holds everywhere. A test reads every form for an array that sets `autocomplete` and is unioned with SECRET, because the mistake is in how the array is written and a new form can make it without being in any list. Shown to fail with the line put back. Not run: the full suite. tests/Form passes apart from the one test that needs a database, which this run did not have. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
09c503c61c
commit
cc7b00311e
8 files changed
+71
-12
No files matched your search
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
]);
|
||||
|
||||
@@ -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,
|
||||
]);
|
||||
|
||||
@@ -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,
|
||||
]);
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in new issue
Block a user