wbrawner/server
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #8491 from nextcloud/strict_request

Browse source 
Make Request strict
This commit is contained in:
Joas Schilling 2018-02-26 11:02:30 +01:00 committed by GitHub
commit 695e32d0a6
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 95 additions and 78 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
apps/dav/tests/unit/ServerTest.php
View file

@ -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);
}

126
lib/private/AppFramework/Http/Request.php
View file

@ -1,4 +1,5 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2016, ownCloud, Inc.
*
@ -87,8 +88,8 @@ class Request implements \ArrayAccess, \Countable, IRequest {
protected $inputStream;
protected $content;
protected $items = array();
protected $allowedKeys = array(
protected $items = [];
protected $allowedKeys = [
'get',
'post',
'files',
@ -99,7 +100,7 @@ class Request implements \ArrayAccess, \Countable, IRequest {
'parameters',
'method',
'requesttoken',
);
];
/** @var ISecureRandom */
protected $secureRandom;
/** @var IConfig */
@ -131,13 +132,13 @@ class Request implements \ArrayAccess, \Countable, IRequest {
* @param string $stream
* @see http://www.php.net/manual/en/reserved.variables.php
*/
public function __construct(array $vars=array(),
public function __construct(array $vars= [],
ISecureRandom $secureRandom = null,
IConfig $config,
CsrfTokenManager $csrfTokenManager = null,
$stream = 'php://input') {
string $stream = 'php://input') {
$this->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($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,8 +677,12 @@ class Request implements \ArrayAccess, \Countable, IRequest {
*
* @return string HTTP protocol. HTTP/2, HTTP/1.1 or HTTP/1.0.
*/
public function getHttpProtocol() {
$claimedProtocol = strtoupper($this->server['SERVER_PROTOCOL']);
public function getHttpProtocol(): string {
$claimedProtocol = $this->server['SERVER_PROTOCOL'];
if (\is_string($claimedProtocol)) {
$claimedProtocol = strtoupper($claimedProtocol);
}
$validProtocols = [
'HTTP/1.0',
@ -680,7 +690,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 +702,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 +715,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 +737,20 @@ 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);
if ($name === null) {
$name = '';
}
if (strpos($pathInfo, $name) === 0) {
$pathInfo = substr($pathInfo, strlen($name));
if (strpos($pathInfo, '/'.$name) === 0) {
$pathInfo = substr($pathInfo, \strlen($name) + 1);
}
if ($name !== '' && strpos($pathInfo, $name) === 0) {
$pathInfo = substr($pathInfo, \strlen($name));
}
if($pathInfo === false || $pathInfo === '/'){
return '';
@ -770,13 +784,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 +801,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 +818,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 +843,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) {

2
lib/private/Server.php
View file

@ -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(),

41
lib/public/IRequest.php
View file

@ -1,4 +1,5 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2016, ownCloud, Inc.
*
@ -95,7 +96,7 @@ interface IRequest {
* @return string
* @since 6.0.0
*/
public function getHeader($name);
public function getHeader(string $name): string;
/**
* Lets you access post and get parameters by the index
@ -111,7 +112,7 @@ interface IRequest {
* @return mixed the content of the array
* @since 6.0.0
*/
public function getParam($key, $default = null);
public function getParam(string $key, $default = null);
/**
@ -122,7 +123,7 @@ interface IRequest {
* @return array the array with all parameters
* @since 6.0.0
*/
public function getParams();
public function getParams(): array;
/**
* Returns the method of the request
@ -130,7 +131,7 @@ interface IRequest {
* @return string the method of the request (POST, GET, etc)
* @since 6.0.0
*/
public function getMethod();
public function getMethod(): string;
/**
* Shortcut for accessing an uploaded file through the $_FILES array
@ -139,7 +140,7 @@ interface IRequest {
* @return array the file in the $_FILES element
* @since 6.0.0
*/
public function getUploadedFile($key);
public function getUploadedFile(string $key);
/**
@ -149,7 +150,7 @@ interface IRequest {
* @return array the value in the $_ENV element
* @since 6.0.0
*/
public function getEnv($key);
public function getEnv(string $key);
/**
@ -159,7 +160,7 @@ interface IRequest {
* @return string|null the value in the $_COOKIE element
* @since 6.0.0
*/
public function getCookie($key);
public function getCookie(string $key);
/**
@ -168,7 +169,7 @@ interface IRequest {
* @return bool true if CSRF check passed
* @since 6.0.0
*/
public function passesCSRFCheck();
public function passesCSRFCheck(): bool;
/**
* Checks if the strict cookie has been sent with the request if the request
@ -177,7 +178,7 @@ interface IRequest {
* @return bool
* @since 9.0.0
*/
public function passesStrictCookieCheck();
public function passesStrictCookieCheck(): bool;
/**
* Checks if the lax cookie has been sent with the request if the request
@ -186,7 +187,7 @@ interface IRequest {
* @return bool
* @since 9.0.0
*/
public function passesLaxCookieCheck();
public function passesLaxCookieCheck(): bool;
/**
* Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging
@ -195,7 +196,7 @@ interface IRequest {
* @return string
* @since 8.1.0
*/
public function getId();
public function getId(): string;
/**
* Returns the remote address, if the connection came from a trusted proxy
@ -206,7 +207,7 @@ interface IRequest {
* @return string IP address
* @since 8.1.0
*/
public function getRemoteAddress();
public function getRemoteAddress(): string;
/**
* Returns the server protocol. It respects reverse proxy servers and load
@ -215,7 +216,7 @@ interface IRequest {
* @return string Server protocol (http or https)
* @since 8.1.0
*/
public function getServerProtocol();
public function getServerProtocol(): string;
/**
* Returns the used HTTP protocol.
@ -223,7 +224,7 @@ interface IRequest {
* @return string HTTP protocol. HTTP/2, HTTP/1.1 or HTTP/1.0.
* @since 8.2.0
*/
public function getHttpProtocol();
public function getHttpProtocol(): string;
/**
* Returns the request uri, even if the website uses one or more
@ -232,7 +233,7 @@ interface IRequest {
* @return string
* @since 8.1.0
*/
public function getRequestUri();
public function getRequestUri(): string;
/**
* Get raw PathInfo from request (not urldecoded)
@ -241,7 +242,7 @@ interface IRequest {
* @return string Path info
* @since 8.1.0
*/
public function getRawPathInfo();
public function getRawPathInfo(): string;
/**
* Get PathInfo from request
@ -259,7 +260,7 @@ interface IRequest {
* @return string the script name
* @since 8.1.0
*/
public function getScriptName();
public function getScriptName(): string;
/**
* Checks whether the user agent matches a given regex
@ -268,7 +269,7 @@ interface IRequest {
* @return bool true if at least one of the given agent matches, false otherwise
* @since 8.1.0
*/
public function isUserAgent(array $agent);
public function isUserAgent(array $agent): bool;
/**
* Returns the unverified server host from the headers without checking
@ -277,7 +278,7 @@ interface IRequest {
* @return string Server host
* @since 8.1.0
*/
public function getInsecureServerHost();
public function getInsecureServerHost(): string;
/**
* Returns the server host from the headers, or the first configured
@ -286,5 +287,5 @@ interface IRequest {
* @return string Server host
* @since 8.1.0
*/
public function getServerHost();
public function getServerHost(): string;
}

2
tests/lib/AppFramework/Http/RequestTest.php
View file

@ -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)
],
);