#428: Validate signed URLs regardless of t[] vs t[0] (#475)

* Change mock expectation to atLeastOnce()

The previous implementation assumed that we'd only want to retrieve our URLs once, but we'd prefer to read them several times.

* Add test for t[] vs t[0] handling for accessTokens

* Add helper class for flattening arrays in query strings

* #428: Fix issue with t[] vs t[0] in URLs breaking accessToken

As detailed in Issue #428, certain sites (Facebook) regenerates URLs on their side where arguments using the array syntax of PHP gets an index appended in the URL. This breaks our signed requests and ends up with Facebook receiving a 400 HTTP error response instead of the image.

By using the Http\QueryString class from Guzzle we can work around the issue by introducing two new URLs to the list of URLs we test when we check the signature. These URLs will have t[] replaced with t[0] (and up), or t[0], t[1], etc. replaced with t[]. This way we'll accept both possible changes.

In cases where parameters are intermingled (t[]=foo&b[]=bar&t[]=baz) this will not help, but I'm fairly certain we won't see any requests like that in the wild. In addition any request made as t[1]=foo&t[0]=bar is undefined (and might give unexpected results).

* Change helper class name and add build URL method

* Add guzzlehttp/psr7 dependency and update lockfile

* Rewrite index array converter for guzzlehttp\psr7.

The old version had been removed and deprecated, but by adding a helper
function to use `parse_url` data (which is what the Url class in
guzzlehttp\psr7 does internally), we can work around limitations in the
current one (as it _always_ escapes [] in query argument names, which is
suitable for URLs to be used, but not for us - we're validating the
content for the signature).

* phpdoc-fix

composer.json

@@ -22,7 +22,8 @@
     "symfony/http-foundation": "~2.7.3",
     "symfony/console": "~2.7.3",
     "ramsey/uuid": "~3.0.0",
-    "mongodb/mongodb": "~1.0.0"
+    "mongodb/mongodb": "~1.0.0",
+    "guzzlehttp/psr7": "~1.3.0"
   },
   "require-dev": {
     "symfony/filesystem": "~2.8.3",

library/Imbo/EventListener/AccessToken.php

@@ -12,7 +12,9 @@
 
 use Imbo\EventManager\EventInterface,
     Imbo\Http\Request\Request,
-    Imbo\Exception\RuntimeException;
+    Imbo\Exception\RuntimeException,
+    Imbo\Helpers\Urls,
+    GuzzleHttp\Psr7;
 
 /**
  * Access token event listener
@@ -141,6 +143,10 @@ public function checkAccessToken(EventInterface $event) {
         $uris = [$request->getRawUri(), $request->getUriAsIs()];
         $privateKey = $event->getAccessControl()->getPrivateKey($request->getPublicKey());
 
+        // append uris with [] expanded or [0] reduced
+        $uris[] = $this->getUnescapedAlternativeURL($request->getRawUri());
+        $uris[] = $this->getEscapedAlternativeURL($request->getRawUri());
+
         // See if we should modify the protocol for the incoming request
         $protocol = $config['authentication']['protocol'];
         if ($protocol === 'both') {
@@ -170,6 +176,88 @@ public function checkAccessToken(EventInterface $event) {
         throw new RuntimeException('Incorrect access token', 400);
     }
 
+    /**
+     * Helper method to generate an alternative form of an URL, where array indices have either
+     * been added or removed. foo[] is transformed into foo[0], while foo[0] is transformed into foo[].
+     *
+     * The result for URLs with both formats is undefined, or for URLs that intermingle their parameters,
+     * i.e. t[]=foo&b[]=bar&t[]=baz
+     *
+     * This was introduced because of differences between the URLs generated by the different clients, and
+     * because Facebook (at least) generates URLs were []s in URL arguments are expanded to [0] when
+     * requested from the backend. Since we sign our URLs, this breaks the token generation and thus breaks
+     * URLs when Facebook attempts to retrieve them.
+     *
+     * @param string $url The URL to generate the alternative form of
+     * @param int $encoding The encoding to use - from GuzzleHttp\Psr7
+     * @return string
+     */
+    protected function getAlternativeURL($url, $encoding = PHP_QUERY_RFC3986) {
+        $urlParts = parse_url($url);
+
+        if (!isset($urlParts['query'])) {
+            return $url;
+        }
+
+        $queryString = $urlParts['query'];
+        $fixKeyPattern = '#\[[0-9]+\]$#';
+
+        $parsed = Psr7\parse_query($queryString);
+        $newArguments = array();
+
+        foreach ($parsed as $key => $value) {
+            $fixedKey = preg_replace($fixKeyPattern, '', $key);
+
+            // if the key came out different, we're talking about a t[x] format - so we store those
+            // to allow for the correct sequence when regenerating the URL further below.
+            if ($fixedKey != $key) {
+                $fixedKey .= '[]';
+
+                if (!isset($newArguments[$fixedKey])) {
+                    $newArguments[$fixedKey] = array();
+                }
+
+                $newArguments[$fixedKey][] = $value;
+            } else if (is_array($value) && substr($key, -2) == '[]') {
+                // if the value is an array, and we have the [] format already, we expand the keys
+                foreach ($value as $innerKey => $innerValue) {
+                    // remove [] from the key and append the inner array key
+                    $indexedKey = substr($key, 0, -2) . '[' . $innerKey . ']';
+                    $newArguments[$indexedKey] = $innerValue;
+                }
+            } else {
+                $newArguments[$key] = $value;
+            }
+        }
+
+        $urlParts['query'] = Psr7\build_query($newArguments, $encoding);
+        $url = Urls::buildFromParseUrlParts($urlParts);
+
+        return $url;
+    }
+
+    /**
+     * Generate an unescaped, alternative version of an url.
+     *
+     * @see AccessToken::getAlternativeURL()
+     * @param $url string The URL to generate the alternative version of
+     * @return string
+     */
+    protected function getUnescapedAlternativeURL($url) {
+        return $this->getAlternativeURL($url, false);
+    }
+
+    /**
+     * Generate an escaped, alternative version of an url.
+     *
+     * @see AccessToken::getAlternativeURL()
+     * @param $url string The URL to generate the alternative version of
+     * @return string
+     */
+    protected function getEscapedAlternativeURL($url) {
+        return $this->getAlternativeURL($url, PHP_QUERY_RFC3986);
+    }
+
     /**
      * Check if the request is whitelisted
      *
@@ -216,4 +304,4 @@ private function isWhitelisted(Request $request) {
         // All transformations in the request are whitelisted
         return true;
     }
-}
+}

library/Imbo/Helpers/Urls.php

@@ -0,0 +1,50 @@
+<?php
+/**
+ * This file is part of the Imbo package
+ *
+ * (c) Christer Edvartsen <cogo@starzinger.net>
+ *
+ * For the full copyright and license information, please view the LICENSE file that was
+ * distributed with this source code.
+ */
+
+namespace Imbo\Helpers;
+
+/**
+ * Helper class for useful functions for building/manipulating URLs
+ *
+ * @author Mats Lindh <mats@lindh.no>
+ * @package Core\Helpers
+ */
+class Urls {
+    /**
+     * Generate a URL from an array with similar structure as returned from parse_url.
+     *
+     * @param array $parts An array in the format produced from parse_url
+     * @return string
+     */
+    public static function buildFromParseUrlParts(array $parts) {
+        $url = '';
+
+        $url .= isset($parts['scheme']) ? $parts['scheme'] : 'http';
+        $url .= '://';
+
+        if (isset($parts['user'])) {
+            $url .= $parts['user'];
+
+            if (isset($parts['pass'])) {
+                $url .= ':' . $parts['pass'];
+            }
+
+            $url .= '@';
+        }
+
+        $url .= isset($parts['host']) ? $parts['host'] : '';
+        $url .= isset($parts['port']) ? ':' . $parts['port'] : '';
+        $url .= isset($parts['path']) ? $parts['path'] : '';
+        $url .= isset($parts['query']) ? '?' . $parts['query'] : '';
+        $url .= isset($parts['fragment']) ? '#' . $parts['fragment'] : '';
+
+        return $url;
+    }
+}

tests/phpunit/ImboUnitTest/EventListener/AccessTokenTest.php

@@ -232,6 +232,26 @@ public function getAccessTokens() {
                 'private key',
                 false
             ],
+            // Test that URLs with t[0] and t[] can use the same accessToken
+            [
+                'http://imbo/users/imbo/images/foobar?t%5B0%5D=maxSize%3Awidth%3D400%2Cheight%3D400&t%5B1%5D=crop%3Ax%3D50%2Cy%3D50%2Cwidth%3D50%2Cheight%3D50',
+                '1ae7643a70e68377502c30ba54d0ffbfedd67a1f3c4b3f038a42c0ed17ad3551',
+                'foo',
+                true
+            ],
+            [
+                'http://imbo/users/imbo/images/foobar?t%5B%5D=maxSize%3Awidth%3D400%2Cheight%3D400&t%5B%5D=crop%3Ax%3D50%2Cy%3D50%2Cwidth%3D50%2Cheight%3D50',
+                '1ae7643a70e68377502c30ba54d0ffbfedd67a1f3c4b3f038a42c0ed17ad3551',
+                'foo',
+                true
+            ],
+            // and that they still break if something else is changed
+            [
+                'http://imbo/users/imbo/images/foobar?g%5B%5D=maxSize%3Awidth%3D400%2Cheight%3D400&t%5B%5D=crop%3Ax%3D50%2Cy%3D50%2Cwidth%3D50%2Cheight%3D50',
+                '1ae7643a70e68377502c30ba54d0ffbfedd67a1f3c4b3f038a42c0ed17ad3551',
+                'foo',
+                false
+            ],
         ];
     }
 
@@ -246,8 +266,8 @@ public function testThrowsExceptionOnIncorrectToken($url, $token, $privateKey, $
 
         $this->query->expects($this->once())->method('has')->with('accessToken')->will($this->returnValue(true));
         $this->query->expects($this->once())->method('get')->with('accessToken')->will($this->returnValue($token));
-        $this->request->expects($this->once())->method('getRawUri')->will($this->returnValue(urldecode($url)));
-        $this->request->expects($this->once())->method('getUriAsIs')->will($this->returnValue($url));
+        $this->request->expects($this->atLeastOnce())->method('getRawUri')->will($this->returnValue(urldecode($url)));
+        $this->request->expects($this->atLeastOnce())->method('getUriAsIs')->will($this->returnValue($url));
 
         $this->accessControl->expects($this->once())->method('getPrivateKey')->will($this->returnValue($privateKey));