Repository: laravel/framework (php)

Issue:
Signed URLs containing empty querystring values will not validate
- Laravel Version: 8.41.0
- PHP Version: 7.4.16
- Database Driver & Version: n/a

### Description:

If a url contains querystring parameters with empty values, the generated signed url will not validate. 

### Steps To Reproduce:

```php
<?php

// routes/web.php

use Illuminate\Http\Request;
use Illuminate\Support\Facades\Route;
use Illuminate\Support\Facades\URL;

Route::get('/', function () {
    $url = URL::signedRoute('signature-required', [
        'param1',
        'param2' => '',
    ]);

    echo '<a href="'.e($url).'">'.e($url).'</a>';
});

Route::get('/signature-required', function (Request $request) {
    if ($request->hasValidSignature()) {
        echo 'Valid';
    } else {
        echo 'Not Valid';
    }
})->name('signature-required');
```

This will generate a url that looks like this:
http://localhost:8000/signature-required?param2=&signature=6f4e4fe0dfa152bd0c2276a4180a3ec5259e42ae7ae6e14a2d19e1a2a9bb7b9c&param1

Not sure what possible use cases there are for empty string values, perhaps one just wants to check for the presence of a parameter? Of course, the workaround is to not use empty values. But since the signed url has been created, one would at least not expect that it wouldn't validate.


Pull request:
[8.x] Improve signed url signature verification
This was done in collaboration with @Krisell 

This fixes an issue where a signed url that contains a querystring parameter with no value or empty string value will always be invalid. Additionally, a valid signed url will continue to be valid even after tampering with the url by appending empty querystring values. 

This is because the validation code attempts to rebuild the original url for comparison using a combination of the normalized `$request->query()` array and `Arr::query()`. `$request-query()` is not able to differentiate between parameters with no value or empty string values. They all return as null. Furthermore, `Arr::query()` will strip away any nulls. The rebuilt url will not be the same as the original url and therefore fails validation.

Since the signature was created with the original url, we can simply get the original url by stripping the signature parameter (using regex) from the raw querystring. 

The following were some of our considerations on the fix:
* Ensure that parameters and values containing the string `signature` will not break our regex
* Verified that it is safe to use `$request->server->get('QUERY_STRING')`. According to http://www.faqs.org/rfcs/rfc3875.html (Section 4.1.7), QUERY_STRING variable must always be provided by the server and we can expect it will always exist. 

The signing code was unaffected and therefore this fix is backwards compatible with any already generated signed urls. We also added tests to:
* Verify our regex pattern for detecting signature works in certain edge cases
* Querystring parameters with null or empty string values are parsed correctly



Patch:
diff --git a/src/Illuminate/Routing/UrlGenerator.php b/src/Illuminate/Routing/UrlGenerator.php
index e8e09bd568..a8203dd414 100755
--- a/src/Illuminate/Routing/UrlGenerator.php
+++ b/src/Illuminate/Routing/UrlGenerator.php
@@ -388,9 +388,9 @@ public function hasCorrectSignature(Request $request, $absolute = true)
     {
         $url = $absolute ? $request->url() : '/'.$request->path();
 
-        $original = rtrim($url.'?'.Arr::query(
-            Arr::except($request->query(), 'signature')
-        ), '?');
+        $querystring = ltrim(preg_replace('/(^|&)signature=[^&]+/', '', $request->server->get('QUERY_STRING')), '&');
+
+        $original = rtrim($url.'?'.$querystring, '?');
 
         $signature = hash_hmac('sha256', $original, call_user_func($this->keyResolver));
 


Test patch:
diff --git a/tests/Integration/Routing/UrlSigningTest.php b/tests/Integration/Routing/UrlSigningTest.php
index 8c31100de2..ca8e7b19e9 100644
--- a/tests/Integration/Routing/UrlSigningTest.php
+++ b/tests/Integration/Routing/UrlSigningTest.php
@@ -59,6 +59,76 @@ public function testSignedUrlWithUrlWithoutSignatureParameter()
         $this->assertSame('invalid', $this->get('/foo/1')->original);
     }
 
+    public function testSignedUrlWithNullParameter()
+    {
+        Route::get('/foo/{id}', function (Request $request, $id) {
+            return $request->hasValidSignature() ? 'valid' : 'invalid';
+        })->name('foo');
+
+        $this->assertIsString($url = URL::signedRoute('foo', ['id' => 1, 'param']));
+        $this->assertSame('valid', $this->get($url)->original);
+    }
+
+    public function testSignedUrlWithEmptyStringParameter()
+    {
+        Route::get('/foo/{id}', function (Request $request, $id) {
+            return $request->hasValidSignature() ? 'valid' : 'invalid';
+        })->name('foo');
+
+        $this->assertIsString($url = URL::signedRoute('foo', ['id' => 1, 'param' => '']));
+        $this->assertSame('valid', $this->get($url)->original);
+    }
+
+    public function testSignedUrlWithMultipleParameters()
+    {
+        Route::get('/foo/{id}', function (Request $request, $id) {
+            return $request->hasValidSignature() ? 'valid' : 'invalid';
+        })->name('foo');
+
+        $this->assertIsString($url = URL::signedRoute('foo', ['id' => 1, 'param1' => 'value1', 'param2' => 'value2']));
+        $this->assertSame('valid', $this->get($url)->original);
+    }
+
+    public function testSignedUrlWithSignatureTextInKeyOrValue()
+    {
+        Route::get('/foo/{id}', function (Request $request, $id) {
+            return $request->hasValidSignature() ? 'valid' : 'invalid';
+        })->name('foo');
+
+        $this->assertIsString($url = URL::signedRoute('foo', ['id' => 1, 'custom-signature' => 'signature=value']));
+        $this->assertSame('valid', $this->get($url)->original);
+    }
+
+    public function testSignedUrlWithAppendedNullParameterInvalid()
+    {
+        Route::get('/foo/{id}', function (Request $request, $id) {
+            return $request->hasValidSignature() ? 'valid' : 'invalid';
+        })->name('foo');
+
+        $this->assertIsString($url = URL::signedRoute('foo', ['id' => 1]));
+        $this->assertSame('invalid', $this->get($url.'&appended')->original);
+    }
+
+    public function testSignedUrlParametersParsedCorrectly()
+    {
+        Route::get('/foo/{id}', function (Request $request, $id) {
+            return $request->hasValidSignature()
+                && intval($id) === 1
+                && $request->has('paramEmpty')
+                && $request->has('paramEmptyString')
+                && $request->query('paramWithValue') === 'value'
+                ? 'valid'
+                : 'invalid';
+        })->name('foo');
+
+        $this->assertIsString($url = URL::signedRoute('foo', ['id' => 1,
+            'paramEmpty',
+            'paramEmptyString' => '',
+            'paramWithValue' => 'value',
+        ]));
+        $this->assertSame('valid', $this->get($url)->original);
+    }
+
     public function testSignedMiddleware()
     {
         Route::get('/foo/{id}', function (Request $request, $id) {