bug #52469 Check whether secrets are empty and mark them all as sensi…

…tive (nicolas-grekas) This PR was merged into the 6.4 branch. Discussion ---------- Check whether secrets are empty and mark them all as sensitive | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Some security-hardening. I made this throw a deprecation (as was already the case in some places) but I'd be fine making them throw an exception right away since an empty secret is a bold mistake. WDYT? (Webhook was experimental in 6.3 so it's fine throwing there I think.) Commits ------- 52ca699 Check whether secrets are empty and mark them all as sensitive
symfony · Nov 7, 2023 · cf5510d · cf5510d
2 parents f0fcc9f + 52ca699
commit cf5510d
Show file tree

Hide file tree

Showing 21 changed files with 74 additions and 34 deletions.
diff --git a/src/Symfony/Component/HttpFoundation/UriSigner.php b/src/Symfony/Component/HttpFoundation/UriSigner.php
@@ -12,8 +12,6 @@
 namespace Symfony\Component\HttpFoundation;
 
 /**
- * Signs URIs.
- *
  * @author Fabien Potencier <fabien@symfony.com>
  */
 class UriSigner
@@ -22,11 +20,14 @@ class UriSigner
     private string $parameter;
 
     /**
-     * @param string $secret    A secret
      * @param string $parameter Query string parameter to use
      */
     public function __construct(#[\SensitiveParameter] string $secret, string $parameter = '_hash')
     {
+        if (!$secret) {
+            throw new \InvalidArgumentException('A non-empty secret is required.');
+        }
+
         $this->secret = $secret;
         $this->parameter = $parameter;
     }

diff --git a/src/Symfony/Component/Mailer/Bridge/Brevo/Webhook/BrevoRequestParser.php b/src/Symfony/Component/Mailer/Bridge/Brevo/Webhook/BrevoRequestParser.php
@@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
         ]);
     }
 
-    protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
+    protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
     {
         $content = $request->toArray();
         if (

diff --git a/src/Symfony/Component/Mailer/Bridge/Mailgun/Webhook/MailgunRequestParser.php b/src/Symfony/Component/Mailer/Bridge/Mailgun/Webhook/MailgunRequestParser.php
@@ -17,6 +17,7 @@
 use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
 use Symfony\Component\HttpFoundation\RequestMatcherInterface;
 use Symfony\Component\Mailer\Bridge\Mailgun\RemoteEvent\MailgunPayloadConverter;
+use Symfony\Component\Mailer\Exception\InvalidArgumentException;
 use Symfony\Component\RemoteEvent\Event\Mailer\AbstractMailerEvent;
 use Symfony\Component\RemoteEvent\Exception\ParseException;
 use Symfony\Component\Webhook\Client\AbstractRequestParser;
@@ -37,8 +38,12 @@ protected function getRequestMatcher(): RequestMatcherInterface
         ]);
     }
 
-    protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
+    protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
     {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
+
         $content = $request->toArray();
         if (
             !isset($content['signature']['timestamp'])
@@ -60,7 +65,7 @@ protected function doParse(Request $request, string $secret): ?AbstractMailerEve
         }
     }
 
-    private function validateSignature(array $signature, string $secret): void
+    private function validateSignature(array $signature, #[\SensitiveParameter] string $secret): void
     {
         // see https://documentation.mailgun.com/en/latest/user_manual.html#webhooks-1
         if (!hash_equals($signature['signature'], hash_hmac('sha256', $signature['timestamp'].$signature['token'], $secret))) {

diff --git a/src/Symfony/Component/Mailer/Bridge/Mailjet/Webhook/MailjetRequestParser.php b/src/Symfony/Component/Mailer/Bridge/Mailjet/Webhook/MailjetRequestParser.php
@@ -37,7 +37,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
         ]);
     }
 
-    protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
+    protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
     {
         try {
             return $this->converter->convert($request->toArray());

diff --git a/src/Symfony/Component/Mailer/Bridge/Postmark/Webhook/PostmarkRequestParser.php b/src/Symfony/Component/Mailer/Bridge/Postmark/Webhook/PostmarkRequestParser.php
@@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
         ]);
     }
 
-    protected function doParse(Request $request, string $secret): ?AbstractMailerEvent
+    protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?AbstractMailerEvent
     {
         $payload = $request->toArray();
         if (

diff --git a/src/Symfony/Component/Mailer/Bridge/Sendgrid/Webhook/SendgridRequestParser.php b/src/Symfony/Component/Mailer/Bridge/Sendgrid/Webhook/SendgridRequestParser.php
@@ -17,6 +17,7 @@
 use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
 use Symfony\Component\HttpFoundation\RequestMatcherInterface;
 use Symfony\Component\Mailer\Bridge\Sendgrid\RemoteEvent\SendgridPayloadConverter;
+use Symfony\Component\Mailer\Exception\InvalidArgumentException;
 use Symfony\Component\RemoteEvent\Event\Mailer\AbstractMailerEvent;
 use Symfony\Component\RemoteEvent\Exception\ParseException;
 use Symfony\Component\Webhook\Client\AbstractRequestParser;
@@ -86,12 +87,12 @@ protected function doParse(Request $request, string $secret): ?AbstractMailerEve
      *
      * @see https://docs.sendgrid.com/for-developers/tracking-events/getting-started-event-webhook-security-features
      */
-    private function validateSignature(
-        string $signature,
-        string $timestamp,
-        string $payload,
-        string $secret,
-    ): void {
+    private function validateSignature(string $signature, string $timestamp, string $payload, #[\SensitiveParameter] string $secret): void
+    {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
+
         $timestampedPayload = $timestamp.$payload;
 
         // Sendgrid provides the verification key as base64-encoded DER data. Openssl wants a PEM format, which is a multiline version of the base64 data.

diff --git a/src/Symfony/Component/Mailer/Transport/Smtp/Auth/CramMd5Authenticator.php b/src/Symfony/Component/Mailer/Transport/Smtp/Auth/CramMd5Authenticator.php
@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Mailer\Transport\Smtp\Auth;
 
+use Symfony\Component\Mailer\Exception\InvalidArgumentException;
 use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
 
 /**
@@ -41,6 +42,10 @@ public function authenticate(EsmtpTransport $client): void
      */
     private function getResponse(#[\SensitiveParameter] string $secret, string $challenge): string
     {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
+
         if (\strlen($secret) > 64) {
             $secret = pack('H32', md5($secret));
         }

diff --git a/src/Symfony/Component/Notifier/Bridge/Twilio/Webhook/TwilioRequestParser.php b/src/Symfony/Component/Notifier/Bridge/Twilio/Webhook/TwilioRequestParser.php
@@ -25,7 +25,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
         return new MethodRequestMatcher('POST');
     }
 
-    protected function doParse(Request $request, string $secret): ?SmsEvent
+    protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?SmsEvent
     {
         // Statuses: https://www.twilio.com/docs/sms/api/message-resource#message-status-values
         // Payload examples: https://www.twilio.com/docs/sms/outbound-message-logging

diff --git a/src/Symfony/Component/Notifier/Bridge/Vonage/Webhook/VonageRequestParser.php b/src/Symfony/Component/Notifier/Bridge/Vonage/Webhook/VonageRequestParser.php
@@ -16,6 +16,7 @@
 use Symfony\Component\HttpFoundation\RequestMatcher\IsJsonRequestMatcher;
 use Symfony\Component\HttpFoundation\RequestMatcher\MethodRequestMatcher;
 use Symfony\Component\HttpFoundation\RequestMatcherInterface;
+use Symfony\Component\Notifier\Exception\InvalidArgumentException;
 use Symfony\Component\RemoteEvent\Event\Sms\SmsEvent;
 use Symfony\Component\Webhook\Client\AbstractRequestParser;
 use Symfony\Component\Webhook\Exception\RejectWebhookException;
@@ -30,8 +31,12 @@ protected function getRequestMatcher(): RequestMatcherInterface
         ]);
     }
 
-    protected function doParse(Request $request, string $secret): ?SmsEvent
+    protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?SmsEvent
     {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
+
         // Signed webhooks: https://developer.vonage.com/en/getting-started/concepts/webhooks#validating-signed-webhooks
         if (!$request->headers->has('Authorization')) {
             throw new RejectWebhookException(406, 'Missing "Authorization" header.');
@@ -70,7 +75,7 @@ protected function doParse(Request $request, string $secret): ?SmsEvent
         return $event;
     }
 
-    private function validateSignature(string $jwt, string $secret): void
+    private function validateSignature(string $jwt, #[\SensitiveParameter] string $secret): void
     {
         $tokenParts = explode('.', $jwt);
         if (3 !== \count($tokenParts)) {

diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php
@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Security\Core\Authentication\Token;
 
+use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
@@ -32,12 +33,12 @@ public function __construct(UserInterface $user, string $firewallName, #[\Sensit
     {
         parent::__construct($user->getRoles());
 
-        if (empty($secret)) {
-            throw new \InvalidArgumentException('$secret must not be empty.');
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
         }
 
-        if ('' === $firewallName) {
-            throw new \InvalidArgumentException('$firewallName must not be empty.');
+        if (!$firewallName) {
+            throw new InvalidArgumentException('$firewallName must not be empty.');
         }
 
         $this->firewallName = $firewallName;

diff --git a/src/Symfony/Component/Security/Core/Signature/SignatureHasher.php b/src/Symfony/Component/Security/Core/Signature/SignatureHasher.php
@@ -12,6 +12,7 @@
 namespace Symfony\Component\Security\Core\Signature;
 
 use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
+use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
 use Symfony\Component\Security\Core\Signature\Exception\ExpiredSignatureException;
 use Symfony\Component\Security\Core\Signature\Exception\InvalidSignatureException;
 use Symfony\Component\Security\Core\User\UserInterface;
@@ -37,6 +38,10 @@ class SignatureHasher
      */
     public function __construct(PropertyAccessorInterface $propertyAccessor, array $signatureProperties, #[\SensitiveParameter] string $secret, ExpiredSignatureStorage $expiredSignaturesStorage = null, int $maxUses = null)
     {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
+
         $this->propertyAccessor = $propertyAccessor;
         $this->signatureProperties = $signatureProperties;
         $this->secret = $secret;

diff --git a/src/Symfony/Component/Security/Http/Authenticator/RememberMeAuthenticator.php b/src/Symfony/Component/Security/Http/Authenticator/RememberMeAuthenticator.php
@@ -19,6 +19,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
 use Symfony\Component\Security\Core\Exception\CookieTheftException;
+use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
 use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
 use Symfony\Component\Security\Core\Exception\UserNotFoundException;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
@@ -51,6 +52,10 @@ class RememberMeAuthenticator implements InteractiveAuthenticatorInterface
 
     public function __construct(RememberMeHandlerInterface $rememberMeHandler, #[\SensitiveParameter] string $secret, TokenStorageInterface $tokenStorage, string $cookieName, LoggerInterface $logger = null)
     {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
+
         $this->rememberMeHandler = $rememberMeHandler;
         $this->secret = $secret;
         $this->tokenStorage = $tokenStorage;

diff --git a/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php b/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php
@@ -14,6 +14,7 @@
 use Symfony\Component\HttpFoundation\RateLimiter\AbstractRequestRateLimiter;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\RateLimiter\RateLimiterFactory;
+use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
 use Symfony\Component\Security\Http\SecurityRequestAttributes;
 
 /**
@@ -35,10 +36,10 @@ final class DefaultLoginRateLimiter extends AbstractRequestRateLimiter
      */
     public function __construct(RateLimiterFactory $globalFactory, RateLimiterFactory $localFactory, #[\SensitiveParameter] string $secret = '')
     {
-        if ('' === $secret) {
-            trigger_deprecation('symfony/security-http', '6.4', 'Calling "%s()" with an empty secret is deprecated. A non-empty secret will be mandatory in version 7.0.', __METHOD__);
-            // throw new \Symfony\Component\Security\Core\Exception\InvalidArgumentException('A non-empty secret is required.');
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
         }
+
         $this->globalFactory = $globalFactory;
         $this->localFactory = $localFactory;
         $this->secret = $secret;

diff --git a/src/Symfony/Component/Webhook/Client/AbstractRequestParser.php b/src/Symfony/Component/Webhook/Client/AbstractRequestParser.php
@@ -22,7 +22,7 @@
  */
 abstract class AbstractRequestParser implements RequestParserInterface
 {
-    public function parse(Request $request, string $secret): ?RemoteEvent
+    public function parse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent
     {
         $this->validate($request);
 
@@ -41,7 +41,7 @@ public function createRejectedResponse(string $reason): Response
 
     abstract protected function getRequestMatcher(): RequestMatcherInterface;
 
-    abstract protected function doParse(Request $request, string $secret): ?RemoteEvent;
+    abstract protected function doParse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent;
 
     protected function validate(Request $request): void
     {

diff --git a/src/Symfony/Component/Webhook/Client/RequestParser.php b/src/Symfony/Component/Webhook/Client/RequestParser.php
@@ -41,7 +41,7 @@ protected function getRequestMatcher(): RequestMatcherInterface
         ]);
     }
 
-    protected function doParse(Request $request, string $secret): RemoteEvent
+    protected function doParse(Request $request, #[\SensitiveParameter] string $secret): RemoteEvent
     {
         $body = $request->toArray();
 
@@ -60,7 +60,7 @@ protected function doParse(Request $request, string $secret): RemoteEvent
         );
     }
 
-    private function validateSignature(HeaderBag $headers, string $body, $secret): void
+    private function validateSignature(HeaderBag $headers, string $body, #[\SensitiveParameter] string $secret): void
     {
         $signature = $headers->get($this->signatureHeaderName);
         $event = $headers->get($this->eventHeaderName);

diff --git a/src/Symfony/Component/Webhook/Client/RequestParserInterface.php b/src/Symfony/Component/Webhook/Client/RequestParserInterface.php
@@ -28,7 +28,7 @@ interface RequestParserInterface
      *
      * @throws RejectWebhookException When the payload is rejected (signature issue, parse issue, ...)
      */
-    public function parse(Request $request, string $secret): ?RemoteEvent;
+    public function parse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent;
 
     public function createSuccessfulResponse(): Response;
 

diff --git a/src/Symfony/Component/Webhook/Server/HeaderSignatureConfigurator.php b/src/Symfony/Component/Webhook/Server/HeaderSignatureConfigurator.php
@@ -13,6 +13,7 @@
 
 use Symfony\Component\HttpClient\HttpOptions;
 use Symfony\Component\RemoteEvent\RemoteEvent;
+use Symfony\Component\Webhook\Exception\InvalidArgumentException;
 use Symfony\Component\Webhook\Exception\LogicException;
 
 /**
@@ -26,8 +27,12 @@ public function __construct(
     ) {
     }
 
-    public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void
+    public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void
     {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
+
         $opts = $options->toArray();
         $headers = $opts['headers'];
         if (!isset($opts['body'])) {

diff --git a/src/Symfony/Component/Webhook/Server/HeadersConfigurator.php b/src/Symfony/Component/Webhook/Server/HeadersConfigurator.php
@@ -25,7 +25,7 @@ public function __construct(
     ) {
     }
 
-    public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void
+    public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void
     {
         $options->setHeaders([
             $this->eventHeaderName => $event->getName(),

diff --git a/src/Symfony/Component/Webhook/Server/JsonBodyConfigurator.php b/src/Symfony/Component/Webhook/Server/JsonBodyConfigurator.php
@@ -25,7 +25,7 @@ public function __construct(
     ) {
     }
 
-    public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void
+    public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void
     {
         $body = $this->serializer->serialize($event->getPayload(), 'json');
         $options->setBody($body);

diff --git a/src/Symfony/Component/Webhook/Server/RequestConfiguratorInterface.php b/src/Symfony/Component/Webhook/Server/RequestConfiguratorInterface.php
@@ -19,5 +19,5 @@
  */
 interface RequestConfiguratorInterface
 {
-    public function configure(RemoteEvent $event, string $secret, HttpOptions $options): void;
+    public function configure(RemoteEvent $event, #[\SensitiveParameter] string $secret, HttpOptions $options): void;
 }
diff --git a/src/Symfony/Component/Webhook/Subscriber.php b/src/Symfony/Component/Webhook/Subscriber.php
@@ -11,12 +11,18 @@
 
 namespace Symfony\Component\Webhook;
 
+use Symfony\Component\Webhook\Exception\InvalidArgumentException;
+
 class Subscriber
 {
     public function __construct(
         private readonly string $url,
-        #[\SensitiveParameter] private readonly string $secret,
+        #[\SensitiveParameter]
+        private readonly string $secret,
     ) {
+        if (!$secret) {
+            throw new InvalidArgumentException('A non-empty secret is required.');
+        }
     }
 
     public function getUrl(): string