Merge pull request #54 from weierophinney/hotfix/uri-xss-double-slash-test

Modify multiple slash in URI path detection

Author: dbu

CHANGELOG.md

@@ -1 +1,12 @@
 # Change Log
+
+## Added
+
+- Adds `UriIntegrationTest::testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS()`, `UriIntegrationTest::testStringRepresentationWithMultipleSlashes(array $test)`, and `RequestIntegrationTest::testGetRequestTargetInOriginFormNormalizesUriWithMultipleLeadingSlashesInPath()`.
+  These validate that a path containing multiple leading slashes is (a) represented with a single slash when calling `UriInterface::getPath()`, and (b) represented without changes when calling `UriInterface::__toString()`, including when calling `RequestInterface::getRequestTarget()` (which returns the path without the URI authority by default, to comply with origin-form).
+  This is done to validate mitigations for [CVE-2015-3257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3257).
+
+## Changed
+
+- Modifies `UriIntegrationTest::testPathWithMultipleSlashes()` to only validate multiple slashes in the middle of a path.
+  Multiple leading slashes are covered with the newly introduced tests.

src/RequestIntegrationTest.php

@@ -169,4 +169,25 @@ public function testUriPreserveHost_Host_Host()
         $request2 = $request->withUri($this->buildUri('http://www.bar.com/foo'), true);
         $this->assertEquals($host, $request2->getHeaderLine('host'));
     }
+
+    /**
+     * Tests that getRequestTarget(), when using the default behavior of
+     * displaying the origin-form, normalizes multiple leading slashes in the
+     * path to a single slash. This is done to prevent URL poisoning and/or XSS
+     * issues.
+     *
+     * @see UriIntegrationTest::testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS
+     */
+    public function testGetRequestTargetInOriginFormNormalizesUriWithMultipleLeadingSlashesInPath()
+    {
+        if (isset($this->skippedTests[__FUNCTION__])) {
+            $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
+        }
+
+        $url = 'http://example.org//valid///path';
+        $request = $this->request->withUri($this->buildUri($url));
+        $requestTarget = $request->getRequestTarget();
+
+        $this->assertSame('/valid///path', $requestTarget);
+    }
 }

src/UriIntegrationTest.php

@@ -233,11 +233,50 @@ public function testPathWithMultipleSlashes()
             $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
         }
 
-        $expected = 'http://example.org//valid///path';
+        $expected = 'http://example.org/valid///path';
         $uri = $this->createUri($expected);
 
         $this->assertInstanceOf(UriInterface::class, $uri);
+        $this->assertSame('/valid///path', $uri->getPath());
         $this->assertSame($expected, (string) $uri);
-        $this->assertSame('//valid///path', $uri->getPath());
+    }
+
+    /**
+     * Tests that getPath() normalizes multiple leading slashes to a single
+     * slash. This is done to ensure that when a path is used in isolation from
+     * the authority, it will not cause URL poisoning and/or XSS issues.
+     *
+     * @see https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3257
+     * @psalm-param array{expected: non-empty-string, uri: UriInterface} $test
+     */
+    public function testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS()
+    {
+        if (isset($this->skippedTests[__FUNCTION__])) {
+            $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
+        }
+
+        $expected = 'http://example.org//valid///path';
+        $uri = $this->createUri($expected);
+
+        $this->assertInstanceOf(UriInterface::class, $uri);
+        $this->assertSame('/valid///path', $uri->getPath());
+
+        return [
+            'expected' => $expected,
+            'uri' => $uri,
+        ];
+    }
+
+    /**
+     * Tests that the full string representation of a URI that includes multiple
+     * leading slashes in the path is presented verbatim (in contrast to what is
+     * provided when calling getPath()).
+     *
+     * @depends testGetPathNormalizesMultipleLeadingSlashesToSingleSlashToPreventXSS
+     * @psalm-param array{expected: non-empty-string, uri: UriInterface} $test
+     */
+    public function testStringRepresentationWithMultipleSlashes(array $test)
+    {
+        $this->assertSame($test['expected'], (string) $test['uri']);
     }
 }