bug #52724 [Security] make secret required for DefaultLoginRateLimiter (RobertMe)

nicolas-grekas · nicolas-grekas · commit d3aa478f47ce · 2023-11-26T14:46:29.000+01:00
This PR was merged into the 6.4 branch. Discussion ---------- [Security] make secret required for DefaultLoginRateLimiter | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes/no | New feature? | no | Deprecations? | yes/no | Issues | | License | MIT This tickets results from the discussion here: #52469 (review) and `@nicolas`-grekas requested a PR for it. The `secret` parameter has been added in #51434 with a default value of `''` and a deprecation message that it is required / may not be empty. Which is fine and doesn't hurt backwards compatibility. The later ticket #52469 changes the deprecation into an exception, as it is undesirable that no secret is used (in any scenario). This leads to the unintended side effect that there is a BC breakage when a developer manually creates a `DefaultLoginRateLimiter` as it is now actually required to provide a (non empty) value due to the check and exception. Allowing the service / class to be used without providing the secret parameter, in a backwards compatible manner, but then still breaking the backwards compatibility by throwing due to the default value is confusing. So making the `secret` required makes more sense from a developer perspective as it is clear in that the parameter must be provided. Commits ------- ecbf0e9 [Security] make secret required for DefaultLoginRateLimiter
diff --git a/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php b/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php
@@ -34,7 +34,7 @@ final class DefaultLoginRateLimiter extends AbstractRequestRateLimiter
     /**
      * @param non-empty-string $secret A secret to use for hashing the IP address and username
      */
-    public function __construct(RateLimiterFactory $globalFactory, RateLimiterFactory $localFactory, #[\SensitiveParameter] string $secret = '')
+    public function __construct(RateLimiterFactory $globalFactory, RateLimiterFactory $localFactory, #[\SensitiveParameter] string $secret)
     {
         if (!$secret) {
             throw new InvalidArgumentException('A non-empty secret is required.');

Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@ final class DefaultLoginRateLimiter extends AbstractRequestRateLimiter`
`34`	`34`	`/**`
`35`	`35`	`* @param non-empty-string $secret A secret to use for hashing the IP address and username`
`36`	`36`	`*/`
`37`		`- public function __construct(RateLimiterFactory $globalFactory, RateLimiterFactory $localFactory, #[\SensitiveParameter] string $secret = '')`
	`37`	`+ public function __construct(RateLimiterFactory $globalFactory, RateLimiterFactory $localFactory, #[\SensitiveParameter] string $secret)`
`38`	`38`	`{`
`39`	`39`	`if (!$secret) {`
`40`	`40`	`throw new InvalidArgumentException('A non-empty secret is required.');`