From 20f487607f65fffbc48e533bae345369ae2e7e38 Mon Sep 17 00:00:00 2001 From: Martin Krisell Date: Fri, 26 Apr 2024 16:07:32 +0200 Subject: [PATCH 1/5] Add support for previous app keys in signed URL verification --- .../Routing/RoutingServiceProvider.php | 5 +++- src/Illuminate/Routing/UrlGenerator.php | 19 ++++++++++++-- tests/Integration/Routing/UrlSigningTest.php | 26 +++++++++++++++++++ tests/Routing/RoutingUrlGeneratorTest.php | 24 +++++++++++------ 4 files changed, 63 insertions(+), 11 deletions(-) diff --git a/src/Illuminate/Routing/RoutingServiceProvider.php b/src/Illuminate/Routing/RoutingServiceProvider.php index 547e195e3866..8250338eee28 100755 --- a/src/Illuminate/Routing/RoutingServiceProvider.php +++ b/src/Illuminate/Routing/RoutingServiceProvider.php @@ -77,7 +77,10 @@ protected function registerUrlGenerator() }); $url->setKeyResolver(function () { - return $this->app->make('config')->get('app.key'); + $config = $this->app->make('config'); + + // Return all accepted app keys, but ensure current key is first for performance. + return [$config->get('app.key'), ...($config->get('app.previous_keys') ?? [])]; }); // If the route collection is "rebound", for example, when the routes stay diff --git a/src/Illuminate/Routing/UrlGenerator.php b/src/Illuminate/Routing/UrlGenerator.php index 3bfd6127d464..51c0afb7c3f4 100755 --- a/src/Illuminate/Routing/UrlGenerator.php +++ b/src/Illuminate/Routing/UrlGenerator.php @@ -368,6 +368,9 @@ public function signedRoute($name, $parameters = [], $expiration = null, $absolu ksort($parameters); $key = call_user_func($this->keyResolver); + if (is_array($key)) { + $key = $key[0]; + } return $this->route($name, $parameters + [ 'signature' => hash_hmac('sha256', $this->route($name, $parameters, $absolute), $key), @@ -455,9 +458,21 @@ public function hasCorrectSignature(Request $request, $absolute = true, array $i $original = rtrim($url.'?'.$queryString, '?'); - $signature = hash_hmac('sha256', $original, call_user_func($this->keyResolver)); + $keys = call_user_func($this->keyResolver); + if (!is_array($keys)) { + $keys = [$keys]; + } + + foreach ($keys as $key) { + $signature = hash_hmac('sha256', $original, $key); + $validSignature = hash_equals($signature, (string) $request->query('signature', '')); + + if ($validSignature) { + return true; + } + } - return hash_equals($signature, (string) $request->query('signature', '')); + return false; } /** diff --git a/tests/Integration/Routing/UrlSigningTest.php b/tests/Integration/Routing/UrlSigningTest.php index 66dd9a87cda5..f836066d26f9 100644 --- a/tests/Integration/Routing/UrlSigningTest.php +++ b/tests/Integration/Routing/UrlSigningTest.php @@ -354,6 +354,32 @@ public function testItCanGenerateMiddlewareDefinitionViaStaticMethod() $this->assertSame('Illuminate\Routing\Middleware\ValidateSignature:foo,bar', $signature); } + public function testUrlsSignedByPreviousAppKeysAreValidWhenAddedAsPreviousKeys() + { + Route::get('/foo/{id}', function (Request $request, $id) { + return $request->hasValidSignature() ? 'valid' : 'invalid'; + })->name('foo'); + + config(['app.key' => 'oldest-key']); + $oldestURL = URL::signedRoute('foo', ['id' => 1]); + + config(['app.key' => 'old-key']); + $oldURL = URL::signedRoute('foo', ['id' => 1]); + + config(['app.key' => 'new-key']); + $newUrl = URL::signedRoute('foo', ['id' => 1]); + + tap($this->get($oldestURL), fn ($response) => $this->assertSame('invalid', $response->original)); + tap($this->get($oldURL), fn ($response) => $this->assertSame('invalid', $response->original)); + tap($this->get($newUrl), fn ($response) => $this->assertSame('valid', $response->original)); + + config(['app.previous_keys' => ['old-key', 'oldest-key']]); + + tap($this->get($oldestURL), fn ($response) => $this->assertSame('valid', $response->original)); + tap($this->get($oldURL), fn ($response) => $this->assertSame('valid', $response->original)); + tap($this->get($newUrl), fn ($response) => $this->assertSame('valid', $response->original)); + } + protected function createValidateSignatureMiddleware(array $ignore) { return new class($ignore) extends ValidateSignature diff --git a/tests/Routing/RoutingUrlGeneratorTest.php b/tests/Routing/RoutingUrlGeneratorTest.php index 1d92d5e6e1b0..96ae839ac513 100755 --- a/tests/Routing/RoutingUrlGeneratorTest.php +++ b/tests/Routing/RoutingUrlGeneratorTest.php @@ -894,7 +894,7 @@ public function testSignedUrlWithKeyResolver() $request = Request::create('http://www.foo.com/') ); $url->setKeyResolver(function () { - return 'secret'; + return 'first-secret'; }); $route = new Route(['GET'], 'foo', ['as' => 'foo', function () { @@ -902,24 +902,32 @@ public function testSignedUrlWithKeyResolver() }]); $routes->add($route); - $request = Request::create($url->signedRoute('foo')); + $firstRequest = Request::create($url->signedRoute('foo')); - $this->assertTrue($url->hasValidSignature($request)); + $this->assertTrue($url->hasValidSignature($firstRequest)); $request = Request::create($url->signedRoute('foo').'?tempered=true'); $this->assertFalse($url->hasValidSignature($request)); $url2 = $url->withKeyResolver(function () { - return 'other-secret'; + return 'second-secret'; }); - $this->assertFalse($url2->hasValidSignature($request)); + $this->assertFalse($url2->hasValidSignature($firstRequest)); - $request = Request::create($url2->signedRoute('foo')); + $secondRequest = Request::create($url2->signedRoute('foo')); - $this->assertTrue($url2->hasValidSignature($request)); - $this->assertFalse($url->hasValidSignature($request)); + $this->assertTrue($url2->hasValidSignature($secondRequest)); + $this->assertFalse($url->hasValidSignature($secondRequest)); + + // Key resolver also supports multiple keys, for app key rotation via the config "app.previous_keys" + $url3 = $url->withKeyResolver(function () { + return ['first-secret', 'second-secret']; + }); + + $this->assertTrue($url3->hasValidSignature($firstRequest)); + $this->assertTrue($url3->hasValidSignature($secondRequest)); } public function testMissingNamedRouteResolution() From 281a97bbc797b8a2ca5c22ee8fb8a1915f84b80f Mon Sep 17 00:00:00 2001 From: Martin Krisell Date: Fri, 26 Apr 2024 16:21:57 +0200 Subject: [PATCH 2/5] Simplify array wrap and unwrap logic in UrlGenerator --- src/Illuminate/Routing/UrlGenerator.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Illuminate/Routing/UrlGenerator.php b/src/Illuminate/Routing/UrlGenerator.php index 51c0afb7c3f4..fdc1277c2e94 100755 --- a/src/Illuminate/Routing/UrlGenerator.php +++ b/src/Illuminate/Routing/UrlGenerator.php @@ -368,9 +368,9 @@ public function signedRoute($name, $parameters = [], $expiration = null, $absolu ksort($parameters); $key = call_user_func($this->keyResolver); - if (is_array($key)) { - $key = $key[0]; - } + + // If previous app keys are added for key rotation, pick the first one which is the current app key + $key = is_array($key) ? $key[0] : $key; return $this->route($name, $parameters + [ 'signature' => hash_hmac('sha256', $this->route($name, $parameters, $absolute), $key), @@ -458,10 +458,10 @@ public function hasCorrectSignature(Request $request, $absolute = true, array $i $original = rtrim($url.'?'.$queryString, '?'); + // For app key rotation, $keys may be an array of keys. Wrap a single key in an array + // to simplify logic and preserve backwards compatility with previous keyResolvers. $keys = call_user_func($this->keyResolver); - if (!is_array($keys)) { - $keys = [$keys]; - } + $keys = is_array($keys) ? $keys : [$keys]; foreach ($keys as $key) { $signature = hash_hmac('sha256', $original, $key); From a4b3bdfdd3d01ae6132d33c87f9d5171a03b9485 Mon Sep 17 00:00:00 2001 From: Martin Krisell Date: Fri, 26 Apr 2024 17:09:06 +0200 Subject: [PATCH 3/5] Fix style --- src/Illuminate/Routing/UrlGenerator.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Illuminate/Routing/UrlGenerator.php b/src/Illuminate/Routing/UrlGenerator.php index fdc1277c2e94..622e80d630b9 100755 --- a/src/Illuminate/Routing/UrlGenerator.php +++ b/src/Illuminate/Routing/UrlGenerator.php @@ -369,7 +369,7 @@ public function signedRoute($name, $parameters = [], $expiration = null, $absolu $key = call_user_func($this->keyResolver); - // If previous app keys are added for key rotation, pick the first one which is the current app key + // If previous app keys are added for key rotation, pick the first one which is the current app key. $key = is_array($key) ? $key[0] : $key; return $this->route($name, $parameters + [ @@ -458,7 +458,7 @@ public function hasCorrectSignature(Request $request, $absolute = true, array $i $original = rtrim($url.'?'.$queryString, '?'); - // For app key rotation, $keys may be an array of keys. Wrap a single key in an array + // For app key rotation, $keys may be an array of keys. Wrap a single key in an array // to simplify logic and preserve backwards compatility with previous keyResolvers. $keys = call_user_func($this->keyResolver); $keys = is_array($keys) ? $keys : [$keys]; From d34f213d3e7ed9281cf5a6633b519c70ef90451a Mon Sep 17 00:00:00 2001 From: Martin Krisell Date: Fri, 26 Apr 2024 17:11:54 +0200 Subject: [PATCH 4/5] Remove double whitespace in comment --- src/Illuminate/Routing/UrlGenerator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Routing/UrlGenerator.php b/src/Illuminate/Routing/UrlGenerator.php index 622e80d630b9..e14a782f81a5 100755 --- a/src/Illuminate/Routing/UrlGenerator.php +++ b/src/Illuminate/Routing/UrlGenerator.php @@ -459,7 +459,7 @@ public function hasCorrectSignature(Request $request, $absolute = true, array $i $original = rtrim($url.'?'.$queryString, '?'); // For app key rotation, $keys may be an array of keys. Wrap a single key in an array - // to simplify logic and preserve backwards compatility with previous keyResolvers. + // to simplify logic and preserve backwards compatility with previous keyResolvers. $keys = call_user_func($this->keyResolver); $keys = is_array($keys) ? $keys : [$keys]; From 9788d689a2aa450735ad8b5916658a0d90722aaa Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Thu, 9 May 2024 13:30:11 -0400 Subject: [PATCH 5/5] formatting --- .../Routing/RoutingServiceProvider.php | 1 - src/Illuminate/Routing/UrlGenerator.php | 20 +++++++++---------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/Illuminate/Routing/RoutingServiceProvider.php b/src/Illuminate/Routing/RoutingServiceProvider.php index 8250338eee28..e26202d85ac6 100755 --- a/src/Illuminate/Routing/RoutingServiceProvider.php +++ b/src/Illuminate/Routing/RoutingServiceProvider.php @@ -79,7 +79,6 @@ protected function registerUrlGenerator() $url->setKeyResolver(function () { $config = $this->app->make('config'); - // Return all accepted app keys, but ensure current key is first for performance. return [$config->get('app.key'), ...($config->get('app.previous_keys') ?? [])]; }); diff --git a/src/Illuminate/Routing/UrlGenerator.php b/src/Illuminate/Routing/UrlGenerator.php index e14a782f81a5..df4243313b5f 100755 --- a/src/Illuminate/Routing/UrlGenerator.php +++ b/src/Illuminate/Routing/UrlGenerator.php @@ -369,11 +369,12 @@ public function signedRoute($name, $parameters = [], $expiration = null, $absolu $key = call_user_func($this->keyResolver); - // If previous app keys are added for key rotation, pick the first one which is the current app key. - $key = is_array($key) ? $key[0] : $key; - return $this->route($name, $parameters + [ - 'signature' => hash_hmac('sha256', $this->route($name, $parameters, $absolute), $key), + 'signature' => hash_hmac( + 'sha256', + $this->route($name, $parameters, $absolute), + is_array($key) ? $key[0] : $key + ), ], $absolute); } @@ -458,16 +459,15 @@ public function hasCorrectSignature(Request $request, $absolute = true, array $i $original = rtrim($url.'?'.$queryString, '?'); - // For app key rotation, $keys may be an array of keys. Wrap a single key in an array - // to simplify logic and preserve backwards compatility with previous keyResolvers. $keys = call_user_func($this->keyResolver); + $keys = is_array($keys) ? $keys : [$keys]; foreach ($keys as $key) { - $signature = hash_hmac('sha256', $original, $key); - $validSignature = hash_equals($signature, (string) $request->query('signature', '')); - - if ($validSignature) { + if (hash_equals( + hash_hmac('sha256', $original, $key), + (string) $request->query('signature', '') + )) { return true; } }