From a229095af1f70726036c1247a6ee5914b257577d Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 21 Feb 2018 22:35:31 +0100 Subject: [PATCH 1/4] Make Request strict Signed-off-by: Roeland Jago Douma --- lib/private/AppFramework/Http/Request.php | 112 ++++++++++++---------- lib/public/IRequest.php | 41 ++++---- 2 files changed, 80 insertions(+), 73 deletions(-) diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index 975c4255d5..2294a47104 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -1,4 +1,5 @@ inputStream = $stream; - $this->items['params'] = array(); + $this->items['params'] = []; $this->secureRandom = $secureRandom; $this->config = $config; $this->csrfTokenManager = $csrfTokenManager; @@ -149,7 +150,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { foreach($this->allowedKeys as $name) { $this->items[$name] = isset($vars[$name]) ? $vars[$name] - : array(); + : []; } $this->items['parameters'] = array_merge( @@ -175,8 +176,8 @@ class Request implements \ArrayAccess, \Countable, IRequest { * Countable method * @return int */ - public function count() { - return count(array_keys($this->items['parameters'])); + public function count(): int { + return \count(array_keys($this->items['parameters'])); } /** @@ -199,13 +200,15 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param string $offset The key to lookup * @return boolean */ - public function offsetExists($offset) { + public function offsetExists($offset): bool { return isset($this->items['parameters'][$offset]); } /** - * @see offsetExists - */ + * @see offsetExists + * @param string $offset + * @return mixed + */ public function offsetGet($offset) { return isset($this->items['parameters'][$offset]) ? $this->items['parameters'][$offset] @@ -213,15 +216,18 @@ class Request implements \ArrayAccess, \Countable, IRequest { } /** - * @see offsetExists - */ + * @see offsetExists + * @param string $offset + * @param mixed $value + */ public function offsetSet($offset, $value) { throw new \RuntimeException('You cannot change the contents of the request object'); } /** - * @see offsetExists - */ + * @see offsetExists + * @param string $offset + */ public function offsetUnset($offset) { throw new \RuntimeException('You cannot change the contents of the request object'); } @@ -284,7 +290,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @return bool */ public function __isset($name) { - if (in_array($name, $this->allowedKeys, true)) { + if (\in_array($name, $this->allowedKeys, true)) { return true; } return isset($this->items['parameters'][$name]); @@ -305,9 +311,9 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param string $name * @return string */ - public function getHeader($name) { + public function getHeader(string $name): string { - $name = strtoupper(str_replace(array('-'),array('_'),$name)); + $name = strtoupper(str_replace(['-'], ['_'],$name)); if (isset($this->server['HTTP_' . $name])) { return $this->server['HTTP_' . $name]; } @@ -340,7 +346,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param mixed $default If the key is not found, this value will be returned * @return mixed the content of the array */ - public function getParam($key, $default = null) { + public function getParam(string $key, $default = null) { return isset($this->parameters[$key]) ? $this->parameters[$key] : $default; @@ -351,7 +357,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * (as GET or POST) or throuh the URL by the route * @return array the array with all parameters */ - public function getParams() { + public function getParams(): array { return $this->parameters; } @@ -359,7 +365,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * Returns the method of the request * @return string the method of the request (POST, GET, etc) */ - public function getMethod() { + public function getMethod(): string { return $this->method; } @@ -368,7 +374,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param string $key the key that will be taken from the $_FILES array * @return array the file in the $_FILES element */ - public function getUploadedFile($key) { + public function getUploadedFile(string $key) { return isset($this->files[$key]) ? $this->files[$key] : null; } @@ -377,7 +383,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param string $key the key that will be taken from the $_ENV array * @return array the value in the $_ENV element */ - public function getEnv($key) { + public function getEnv(string $key) { return isset($this->env[$key]) ? $this->env[$key] : null; } @@ -386,7 +392,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param string $key the key that will be taken from the $_COOKIE array * @return string the value in the $_COOKIE element */ - public function getCookie($key) { + public function getCookie(string $key) { return isset($this->cookies[$key]) ? $this->cookies[$key] : null; } @@ -435,7 +441,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { // 'application/json' must be decoded manually. if (strpos($this->getHeader('Content-Type'), 'application/json') !== false) { $params = json_decode(file_get_contents($this->inputStream), true); - if($params !== null && count($params) > 0) { + if($params !== null && \count($params) > 0) { $this->items['params'] = $params; if($this->method === 'POST') { $this->items['post'] = $params; @@ -449,12 +455,12 @@ class Request implements \ArrayAccess, \Countable, IRequest { && strpos($this->getHeader('Content-Type'), 'application/x-www-form-urlencoded') !== false) { parse_str(file_get_contents($this->inputStream), $params); - if(is_array($params)) { + if(\is_array($params)) { $this->items['params'] = $params; } } - if (is_array($params)) { + if (\is_array($params)) { $this->items['parameters'] = array_merge($this->items['parameters'], $params); } $this->contentDecoded = true; @@ -465,7 +471,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * Checks if the CSRF check was correct * @return bool true if CSRF check passed */ - public function passesCSRFCheck() { + public function passesCSRFCheck(): bool { if($this->csrfTokenManager === null) { return false; } @@ -494,7 +500,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * * @return bool */ - private function cookieCheckRequired() { + private function cookieCheckRequired(): bool { if ($this->getHeader('OCS-APIREQUEST')) { return false; } @@ -510,7 +516,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * * @return array */ - public function getCookieParams() { + public function getCookieParams(): array { return session_get_cookie_params(); } @@ -520,7 +526,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param string $name * @return string */ - protected function getProtectedCookieName($name) { + protected function getProtectedCookieName(string $name): string { $cookieParams = $this->getCookieParams(); $prefix = ''; if($cookieParams['secure'] === true && $cookieParams['path'] === '/') { @@ -537,7 +543,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @return bool * @since 9.1.0 */ - public function passesStrictCookieCheck() { + public function passesStrictCookieCheck(): bool { if(!$this->cookieCheckRequired()) { return true; } @@ -557,7 +563,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @return bool * @since 9.1.0 */ - public function passesLaxCookieCheck() { + public function passesLaxCookieCheck(): bool { if(!$this->cookieCheckRequired()) { return true; } @@ -575,7 +581,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * If `mod_unique_id` is installed this value will be taken. * @return string */ - public function getId() { + public function getId(): string { if(isset($this->server['UNIQUE_ID'])) { return $this->server['UNIQUE_ID']; } @@ -595,11 +601,11 @@ class Request implements \ArrayAccess, \Countable, IRequest { * Do always use this instead of $_SERVER['REMOTE_ADDR'] * @return string IP address */ - public function getRemoteAddress() { + public function getRemoteAddress(): string { $remoteAddress = isset($this->server['REMOTE_ADDR']) ? $this->server['REMOTE_ADDR'] : ''; $trustedProxies = $this->config->getSystemValue('trusted_proxies', []); - if(is_array($trustedProxies) && in_array($remoteAddress, $trustedProxies)) { + if(\is_array($trustedProxies) && \in_array($remoteAddress, $trustedProxies)) { $forwardedForHeaders = $this->config->getSystemValue('forwarded_for_headers', [ 'HTTP_X_FORWARDED_FOR' // only have one default, so we cannot ship an insecure product out of the box @@ -625,7 +631,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param string $type * @return bool */ - private function isOverwriteCondition($type = '') { + private function isOverwriteCondition(string $type = ''): bool { $regex = '/' . $this->config->getSystemValue('overwritecondaddr', '') . '/'; $remoteAddr = isset($this->server['REMOTE_ADDR']) ? $this->server['REMOTE_ADDR'] : ''; return $regex === '//' || preg_match($regex, $remoteAddr) === 1 @@ -637,7 +643,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * and load balancers * @return string Server protocol (http or https) */ - public function getServerProtocol() { + public function getServerProtocol(): string { if($this->config->getSystemValue('overwriteprotocol') !== '' && $this->isOverwriteCondition('protocol')) { return $this->config->getSystemValue('overwriteprotocol'); @@ -671,7 +677,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * * @return string HTTP protocol. HTTP/2, HTTP/1.1 or HTTP/1.0. */ - public function getHttpProtocol() { + public function getHttpProtocol(): string { $claimedProtocol = strtoupper($this->server['SERVER_PROTOCOL']); $validProtocols = [ @@ -680,7 +686,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { 'HTTP/2', ]; - if(in_array($claimedProtocol, $validProtocols, true)) { + if(\in_array($claimedProtocol, $validProtocols, true)) { return $claimedProtocol; } @@ -692,10 +698,10 @@ class Request implements \ArrayAccess, \Countable, IRequest { * reverse proxies * @return string */ - public function getRequestUri() { + public function getRequestUri(): string { $uri = isset($this->server['REQUEST_URI']) ? $this->server['REQUEST_URI'] : ''; if($this->config->getSystemValue('overwritewebroot') !== '' && $this->isOverwriteCondition()) { - $uri = $this->getScriptName() . substr($uri, strlen($this->server['SCRIPT_NAME'])); + $uri = $this->getScriptName() . substr($uri, \strlen($this->server['SCRIPT_NAME'])); } return $uri; } @@ -705,7 +711,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @throws \Exception * @return string Path info */ - public function getRawPathInfo() { + public function getRawPathInfo(): string { $requestUri = isset($this->server['REQUEST_URI']) ? $this->server['REQUEST_URI'] : ''; // remove too many leading slashes - can be caused by reverse proxy configuration if (strpos($requestUri, '/') === 0) { @@ -727,16 +733,16 @@ class Request implements \ArrayAccess, \Countable, IRequest { list($path, $name) = \Sabre\Uri\split($scriptName); if (!empty($path)) { if($path === $pathInfo || strpos($pathInfo, $path.'/') === 0) { - $pathInfo = substr($pathInfo, strlen($path)); + $pathInfo = substr($pathInfo, \strlen($path)); } else { throw new \Exception("The requested uri($requestUri) cannot be processed by the script '$scriptName')"); } } if (strpos($pathInfo, '/'.$name) === 0) { - $pathInfo = substr($pathInfo, strlen($name) + 1); + $pathInfo = substr($pathInfo, \strlen($name) + 1); } if (strpos($pathInfo, $name) === 0) { - $pathInfo = substr($pathInfo, strlen($name)); + $pathInfo = substr($pathInfo, \strlen($name)); } if($pathInfo === false || $pathInfo === '/'){ return ''; @@ -770,13 +776,13 @@ class Request implements \ArrayAccess, \Countable, IRequest { * reverse proxies * @return string the script name */ - public function getScriptName() { + public function getScriptName(): string { $name = $this->server['SCRIPT_NAME']; $overwriteWebRoot = $this->config->getSystemValue('overwritewebroot'); if ($overwriteWebRoot !== '' && $this->isOverwriteCondition()) { // FIXME: This code is untestable due to __DIR__, also that hardcoded path is really dangerous - $serverRoot = str_replace('\\', '/', substr(__DIR__, 0, -strlen('lib/private/appframework/http/'))); - $suburi = str_replace('\\', '/', substr(realpath($this->server['SCRIPT_FILENAME']), strlen($serverRoot))); + $serverRoot = str_replace('\\', '/', substr(__DIR__, 0, -\strlen('lib/private/appframework/http/'))); + $suburi = str_replace('\\', '/', substr(realpath($this->server['SCRIPT_FILENAME']), \strlen($serverRoot))); $name = '/' . ltrim($overwriteWebRoot . $suburi, '/'); } return $name; @@ -787,7 +793,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @param array $agent array of agent names * @return bool true if at least one of the given agent matches, false otherwise */ - public function isUserAgent(array $agent) { + public function isUserAgent(array $agent): bool { if (!isset($this->server['HTTP_USER_AGENT'])) { return false; } @@ -804,7 +810,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * whether it is a trusted domain * @return string Server host */ - public function getInsecureServerHost() { + public function getInsecureServerHost(): string { $host = 'localhost'; if (isset($this->server['HTTP_X_FORWARDED_HOST'])) { if (strpos($this->server['HTTP_X_FORWARDED_HOST'], ',') !== false) { @@ -829,7 +835,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * trusted domain if the host isn't in the trusted list * @return string Server host */ - public function getServerHost() { + public function getServerHost(): string { // overwritehost is always trusted $host = $this->getOverwriteHost(); if ($host !== null) { diff --git a/lib/public/IRequest.php b/lib/public/IRequest.php index a14b6b5f45..b313020711 100644 --- a/lib/public/IRequest.php +++ b/lib/public/IRequest.php @@ -1,4 +1,5 @@ Date: Thu, 22 Feb 2018 10:22:11 +0100 Subject: [PATCH 2/4] Fix proper types Signed-off-by: Roeland Jago Douma --- lib/private/AppFramework/Http/Request.php | 12 ++++++++++-- lib/private/Server.php | 2 +- tests/lib/AppFramework/Http/RequestTest.php | 2 +- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index 2294a47104..e6766107cb 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -678,7 +678,11 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @return string HTTP protocol. HTTP/2, HTTP/1.1 or HTTP/1.0. */ public function getHttpProtocol(): string { - $claimedProtocol = strtoupper($this->server['SERVER_PROTOCOL']); + $claimedProtocol = $this->server['SERVER_PROTOCOL']; + + if (\is_string($claimedProtocol)) { + $claimedProtocol = strtoupper($claimedProtocol); + } $validProtocols = [ 'HTTP/1.0', @@ -738,10 +742,14 @@ class Request implements \ArrayAccess, \Countable, IRequest { throw new \Exception("The requested uri($requestUri) cannot be processed by the script '$scriptName')"); } } + if ($name === null) { + $name = ''; + } + if (strpos($pathInfo, '/'.$name) === 0) { $pathInfo = substr($pathInfo, \strlen($name) + 1); } - if (strpos($pathInfo, $name) === 0) { + if ($name !== '' && strpos($pathInfo, $name) === 0) { $pathInfo = substr($pathInfo, \strlen($name)); } if($pathInfo === false || $pathInfo === '/'){ diff --git a/lib/private/Server.php b/lib/private/Server.php index 228f0ab5f9..d586bab15b 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -814,7 +814,7 @@ class Server extends ServerContainer implements IServerContainer { 'cookies' => $_COOKIE, 'method' => (isset($_SERVER) && isset($_SERVER['REQUEST_METHOD'])) ? $_SERVER['REQUEST_METHOD'] - : null, + : '', 'urlParams' => $urlParams, ], $this->getSecureRandom(), diff --git a/tests/lib/AppFramework/Http/RequestTest.php b/tests/lib/AppFramework/Http/RequestTest.php index cc55e33f35..c6b9719b32 100644 --- a/tests/lib/AppFramework/Http/RequestTest.php +++ b/tests/lib/AppFramework/Http/RequestTest.php @@ -307,7 +307,7 @@ class RequestTest extends \Test\TestCase { 'method' => 'PUT', 'server' => [ 'CONTENT_TYPE' => 'image/png', - 'CONTENT_LENGTH' => strlen($data) + 'CONTENT_LENGTH' => (string)strlen($data) ], ); From 6335c6c5ec3e2f66081afa3570fd887b35473352 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 22 Feb 2018 11:29:13 +0100 Subject: [PATCH 3/4] Fix dav server test Signed-off-by: Roeland Jago Douma --- apps/dav/tests/unit/ServerTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/dav/tests/unit/ServerTest.php b/apps/dav/tests/unit/ServerTest.php index d502b24adc..58c77c1b0e 100644 --- a/apps/dav/tests/unit/ServerTest.php +++ b/apps/dav/tests/unit/ServerTest.php @@ -41,6 +41,8 @@ class ServerTest extends \Test\TestCase { public function test() { /** @var IRequest $r */ $r = $this->createMock(IRequest::class); + $r->method('getRequestUri') + ->willReturn('/'); $s = new Server($r, '/'); $this->assertInstanceOf('OCA\DAV\Server', $s); } From 043a824e6aaaf7f6ef85be98ffeea4f458a5d541 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 22 Feb 2018 15:51:09 +0100 Subject: [PATCH 4/4] Fix comments Signed-off-by: Roeland Jago Douma --- lib/private/AppFramework/Http/Request.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index e6766107cb..86ba884141 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -177,7 +177,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * @return int */ public function count(): int { - return \count(array_keys($this->items['parameters'])); + return \count($this->items['parameters']); } /** @@ -313,7 +313,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { */ public function getHeader(string $name): string { - $name = strtoupper(str_replace(['-'], ['_'],$name)); + $name = strtoupper(str_replace('-', '_',$name)); if (isset($this->server['HTTP_' . $name])) { return $this->server['HTTP_' . $name]; }