ProxyServer::getTime() - Code Metrics - Inspection of "Added housekeeping for imageproxy fixes #4449" - SimpleMachines/SMF2.1 - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — release-2.1 (#4535)

by Colin

created 2018-03-08 07:56 UTC

ProxyServer::getTime() A

↳ Parent: ProxyServer

Complexity

Conditions	2
Paths	2

Size

Total Lines	12
Code Lines	7

Duplication

Lines	0
Ratio	0 %

Importance

Changes

Metric	Value
cc	2
eloc	7
nc	2
nop	0
dl	0
loc	12
rs	9.4285
c	0
b	0
f	0

<?php

/**
 * This is a lightweight proxy for serving images, generally meant to be used alongside SSL
 *
 * Simple Machines Forum (SMF)
 *
 * @package SMF
 * @author Simple Machines http://www.simplemachines.org
 * @copyright 2018 Simple Machines and individual contributors
 * @license http://www.simplemachines.org/about/smf/license.php BSD
 *
 * @version 2.1 Beta 4
 */

define('SMF', 'proxy');

global $proxyhousekeeping;
function myFunction($a, $b) {
    // Do something
}

/**
 * Class ProxyServer
 */
class ProxyServer
{
	/** @var bool $enabled Whether or not this is enabled */
	protected $enabled;

	/** @var int $maxSize The maximum size for files to cache */
	protected $maxSize;

	/** @var string $secret A secret code used for hashing */
	protected $secret;

	/** @var string The cache directory */
	protected $cache;
	
	/** @var int $maxDays until enties get deleted */
	protected $maxDays;

	/** @var int time() value */
	protected $time;

	/**
	 * Constructor, loads up the Settings for the proxy
	 *
	 * @access public
	 */
	public function __construct()
	{
		global $image_proxy_enabled, $image_proxy_maxsize, $image_proxy_secret, $cachedir, $sourcedir;
function myFunction($a, $b) {
    // Do something
}

		require_once(dirname(__FILE__) . '/Settings.php');
		require_once($sourcedir . '/Class-CurlFetchWeb.php');

		// Turn off all error reporting; any extra junk makes for an invalid image.
		error_reporting(0);

		$this->enabled = (bool) $image_proxy_enabled;
		$this->maxSize = (int) $image_proxy_maxsize;
		$this->secret = (string) $image_proxy_secret;
		$this->cache = $cachedir . '/images';
		$this->maxDays = 5;
	}

	/**
	 * Checks whether the request is valid or not
	 *
	 * @access public
	 * @return bool Whether the request is valid

	 */
	public function checkRequest()
	{
		if (!$this->enabled)
			return false;

		// Try to create the image cache directory if it doesn't exist
		if (!file_exists($this->cache))
			if (!mkdir($this->cache) || !copy(dirname($this->cache) . '/index.php', $this->cache . '/index.php'))
				return false;

		if (empty($_GET['hash']) || empty($_GET['request']) || ($_GET['request'] === "http:") || ($_GET['request'] === "https:"))
<?php

$singleQuoted = 'Value';
$doubleQuoted = "\tSingle is $singleQuoted";

print $doubleQuoted;
			return false;

		$hash = $_GET['hash'];
		$request = $_GET['request'];

		if (md5($request . $this->secret) != $hash)
			return false;

		// Attempt to cache the request if it doesn't exist
		if (!$this->isCached($request))
			return $this->cacheImage($request);

		return true;
	}

	/**
	 * Serves the request
	 *
	 * @access public
	 * @return void
	 */
	public function serve()
	{
		$request = $_GET['request'];
		$cached_file = $this->getCachedPath($request);
		$cached = json_decode(file_get_contents($cached_file), true);

		// Did we get an error when trying to fetch the image
		$response = $this->checkRequest();
		if ($response === -1)
		{
			// Throw a 404
			header('HTTP/1.0 404 Not Found');
			exit;
		}
		// Right, image not cached? Simply redirect, then.
		if ($response === 0)
		{
			$this::redirectexit($request);
		}

		$time = $this->getTime();

		// Is the cache expired?
		if (!$cached || time() - $cached['time'] > ($this->maxDays * 86400))
		{
			@unlink($cached_file);
// For example instead of
@mkdir($dir);

// Better use
if (@mkdir($dir) === false) {
    throw new \RuntimeException('The directory '.$dir.' could not be created.');
}
			if ($this->checkRequest())
				$this->serve();
			$this::redirectexit($request);
		}

		$eTag = '"' . substr(sha1($request) . $cached['time'], 0, 64) . '"';
		if (!empty($_SERVER['HTTP_IF_NONE_MATCH']) && strpos($_SERVER['HTTP_IF_NONE_MATCH'], $eTag) !== false)
		{
			header('HTTP/1.1 304 Not Modified');
			exit;
		}

		// Make sure we're serving an image
		$contentParts = explode('/', !empty($cached['content_type']) ? $cached['content_type'] : '');
		if ($contentParts[0] != 'image')
			exit;

		$max_age = $time - $cached['time'] + (5 * 86400);
		header('content-type: ' . $cached['content_type']);
		header('content-length: ' . $cached['size']);
		header('cache-control: public, max-age=' . $max_age );
		header('last-modified: ' . gmdate('D, d M Y H:i:s', $cached['time']) . ' GMT');
		header('etag: ' . $eTag);
		echo base64_decode($cached['body']);
	}

	/**
	 * Returns the request's hashed filepath
	 *
	 * @access public
	 * @param string $request The request to get the path for
	 * @return string The hashed filepath for the specified request
	 */
	protected function getCachedPath($request)
	{
		return $this->cache . '/' . sha1($request . $this->secret);
	}

	/**
	 * Check whether the image exists in local cache or not
	 *
	 * @access protected
	 * @param string $request The image to check for in the cache
	 * @return bool Whether or not the requested image is cached
	 */
	protected function isCached($request)
	{
		return file_exists($this->getCachedPath($request));
	}

	/**
	 * Attempts to cache the image while validating it
	 *
	 * @access protected
	 * @param string $request The image to cache/validate
	 * @return int -1 error, 0 too big, 1 valid image
	 */
	protected function cacheImage($request)
	{
		$dest = $this->getCachedPath($request);
		$curl = new curl_fetch_web_data(array(CURLOPT_BINARYTRANSFER => 1));
		$curl_request = $curl->get_url_data($request);
		$responseCode = $curl_request->result('code');
		$response = $curl_request->result();

		if (empty($response) || $responseCode != 200)
		{
			return -1;
		}

		$headers = $response['headers'];

		// Make sure the url is returning an image
		$contentParts = explode('/', !empty($headers['content-type']) ? $headers['content-type'] : '');
		if ($contentParts[0] != 'image')
			return -1;

		// Validate the filesize
		if ($response['size'] > ($this->maxSize * 1024))
			return 0;

		$time = $this->getTime();

		return file_put_contents($dest, json_encode(array(
			'content_type' => $headers['content-type'],
			'size' => $response['size'],
			'time' => $time,
			'body' => base64_encode($response['body']),
		))) === false ? -1 : 1; 
	}

	/**
	 * Static helper function to redirect a request
	 * 
	 * @access public
	 * @param type $request
	 * @return void
	 */
	static public function redirectexit($request)

	{
		header('Location: ' . $request, false, 301);
if ( ! in_array($value, array('this-is-allowed', 'and-this-too'), true)) {
    throw new \InvalidArgumentException('This input is not allowed.');
}
		exit;
	}

	/**
	 * Helper function to call time() once with the right logic
	 * 
	 * @return int
	 */
	protected function getTime()
	{
		if (empty($this->time))
		{
			$old_timezone = date_default_timezone_get();
			date_default_timezone_set('GMT');
			$this->time = time();
			date_default_timezone_set($old_timezone);
		}

		return $this->time;
	}

	/**
	 * Delete all old entries
	 *
	 * @access public
	 * @return void
	 */
	public function housekeeping()
	{
		$path = $this->cache . '/';
		if ($handle = opendir($path)) {

			while (false !== ($file = readdir($handle)))
			{ 
				$filelastmodified = filemtime($path . $file);

				if ((time() - $filelastmodified) > ($this->maxDays * 86400))
				{
				   unlink($path . $file);
				}

			}

			closedir($handle); 
		}
	}
}

if (empty($proxyhousekeeping))
{
	$proxy = new ProxyServer();
	$proxy->serve();
}

?>

Pull Request — release-2.1 (#4535)

ProxyServer::getTime() A

Complexity

Size

Duplication

Importance

1. Pass all data via parameters

2. Create a class that maintains your state

1. Pass all data via parameters

2. Create a class that maintains your state

1 path for user data to reach this point

Response Splitting Attacks

General Strategies to prevent injection

1		<?php
2
3		/**
4		* This is a lightweight proxy for serving images, generally meant to be used alongside SSL
5		*
6		* Simple Machines Forum (SMF)
7		*
8		* @package SMF
9		* @author Simple Machines http://www.simplemachines.org
10		* @copyright 2018 Simple Machines and individual contributors
11		* @license http://www.simplemachines.org/about/smf/license.php BSD
12		*
13		* @version 2.1 Beta 4
14		*/
15
16		define('SMF', 'proxy');
17
18		global $proxyhousekeeping;
		0 ignored issues – show Compatibility Best Practice introduced 2018-02-09 18:24 UTC by Report Bug Copy Issue Report Use of `global` functionality is not recommended; it makes your code harder to test, and less reusable. Instead of relying on `global` state, we recommend one of these alternatives: 1. Pass all data via parameters function myFunction($a, $b) { // Do something } 2. Create a class that maintains your state class MyClass { private $a; private $b; public function __construct($a, $b) { $this->a = $a; $this->b = $b; } public function myFunction() { // Do something } } Loading history...
19
20		/**
21		* Class ProxyServer
22		*/
23		class ProxyServer
24		{
25		/** @var bool $enabled Whether or not this is enabled */
26		protected $enabled;
27
28		/** @var int $maxSize The maximum size for files to cache */
29		protected $maxSize;
30
31		/** @var string $secret A secret code used for hashing */
32		protected $secret;
33
34		/** @var string The cache directory */
35		protected $cache;
36
37		/** @var int $maxDays until enties get deleted */
38		protected $maxDays;
39
40		/** @var int time() value */
41		protected $time;
42
43		/**
44		* Constructor, loads up the Settings for the proxy
45		*
46		* @access public
47		*/
48		public function __construct()
49		{
50		global $image_proxy_enabled, $image_proxy_maxsize, $image_proxy_secret, $cachedir, $sourcedir;
		0 ignored issues – show Compatibility Best Practice introduced 2017-08-25 21:51 UTC by Report Bug Copy Issue Report Use of `global` functionality is not recommended; it makes your code harder to test, and less reusable. Instead of relying on `global` state, we recommend one of these alternatives: 1. Pass all data via parameters function myFunction($a, $b) { // Do something } 2. Create a class that maintains your state class MyClass { private $a; private $b; public function __construct($a, $b) { $this->a = $a; $this->b = $b; } public function myFunction() { // Do something } } Loading history...
51
52		require_once(dirname(__FILE__) . '/Settings.php');
53		require_once($sourcedir . '/Class-CurlFetchWeb.php');
54
55		// Turn off all error reporting; any extra junk makes for an invalid image.
56		error_reporting(0);
57
58		$this->enabled = (bool) $image_proxy_enabled;
59		$this->maxSize = (int) $image_proxy_maxsize;
60		$this->secret = (string) $image_proxy_secret;
61		$this->cache = $cachedir . '/images';
62		$this->maxDays = 5;
63		}
64
65		/**
66		* Checks whether the request is valid or not
67		*
68		* @access public
69		* @return bool Whether the request is valid
		0 ignored issues – show Documentation introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report Should the return type not be `integer\|boolean`? This check compares the return type specified in the `@return` annotation of a function or method doc comment with the types returned by the function and raises an issue if they mismatch. Loading history...
70		*/
71		public function checkRequest()
72		{
73		if (!$this->enabled)
74		return false;
75
76		// Try to create the image cache directory if it doesn't exist
77		if (!file_exists($this->cache))
78		if (!mkdir($this->cache) \|\| !copy(dirname($this->cache) . '/index.php', $this->cache . '/index.php'))
79		return false;
80
81		if (empty($_GET['hash']) \|\| empty($_GET['request']) \|\| ($_GET['request'] === "http:") \|\| ($_GET['request'] === "https:"))
		0 ignored issues – show Coding Style Comprehensibility introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report The string literal `http:` does not require double quotes, as per coding-style, please use single quotes. PHP provides two ways to mark string literals. Either with single quotes `'literal'` or with double quotes `"literal"`. The difference between these is that string literals in double quotes may contain variables with are evaluated at run-time as well as escape sequences. String literals in single quotes on the other hand are evaluated very literally and the only two characters that needs escaping in the literal are the single quote itself (`\'`) and the backslash (`\\`). Every other character is displayed as is. Double quoted string literals may contain other variables or more complex escape sequences. <?php $singleQuoted = 'Value'; $doubleQuoted = "\tSingle is $singleQuoted"; print $doubleQuoted; will print an indented: `Single is Value` If your string literal does not contain variables or escape sequences, it should be defined using single quotes to make that fact clear. For more information on PHP string literals and available escape sequences see the PHP core documentation. Loading history... Coding Style Comprehensibility introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report The string literal `https:` does not require double quotes, as per coding-style, please use single quotes. PHP provides two ways to mark string literals. Either with single quotes `'literal'` or with double quotes `"literal"`. The difference between these is that string literals in double quotes may contain variables with are evaluated at run-time as well as escape sequences. String literals in single quotes on the other hand are evaluated very literally and the only two characters that needs escaping in the literal are the single quote itself (`\'`) and the backslash (`\\`). Every other character is displayed as is. Double quoted string literals may contain other variables or more complex escape sequences. <?php $singleQuoted = 'Value'; $doubleQuoted = "\tSingle is $singleQuoted"; print $doubleQuoted; will print an indented: `Single is Value` If your string literal does not contain variables or escape sequences, it should be defined using single quotes to make that fact clear. For more information on PHP string literals and available escape sequences see the PHP core documentation. Loading history...
82		return false;
83
84		$hash = $_GET['hash'];
85		$request = $_GET['request'];
86
87		if (md5($request . $this->secret) != $hash)
88		return false;
89
90		// Attempt to cache the request if it doesn't exist
91		if (!$this->isCached($request))
92		return $this->cacheImage($request);
93
94		return true;
95		}
96
97		/**
98		* Serves the request
99		*
100		* @access public
101		* @return void
102		*/
103		public function serve()
104		{
105		$request = $_GET['request'];
106		$cached_file = $this->getCachedPath($request);
107		$cached = json_decode(file_get_contents($cached_file), true);
108
109		// Did we get an error when trying to fetch the image
110		$response = $this->checkRequest();
111		if ($response === -1)
112		{
113		// Throw a 404
114		header('HTTP/1.0 404 Not Found');
115		exit;
116		}
117		// Right, image not cached? Simply redirect, then.
118		if ($response === 0)
119		{
120		$this::redirectexit($request);
121		}
122
123		$time = $this->getTime();
124
125		// Is the cache expired?
126		if (!$cached \|\| time() - $cached['time'] > ($this->maxDays * 86400))
127		{
128		@unlink($cached_file);
		0 ignored issues – show Security Best Practice introduced 2016-12-27 02:48 UTC by Report Bug Copy Issue Report It seems like you do not handle an error condition here. This can introduce security issues, and is generally not recommended. If you suppress an error, we recommend checking for the error condition explicitly: // For example instead of @mkdir($dir); // Better use if (@mkdir($dir) === false) { throw new \RuntimeException('The directory '.$dir.' could not be created.'); } Loading history...
129		if ($this->checkRequest())
130		$this->serve();
131		$this::redirectexit($request);
132		}
133
134		$eTag = '"' . substr(sha1($request) . $cached['time'], 0, 64) . '"';
135	View Code Duplication	if (!empty($_SERVER['HTTP_IF_NONE_MATCH']) && strpos($_SERVER['HTTP_IF_NONE_MATCH'], $eTag) !== false)
136		{
137		header('HTTP/1.1 304 Not Modified');
138		exit;
139		}
140
141		// Make sure we're serving an image
142		$contentParts = explode('/', !empty($cached['content_type']) ? $cached['content_type'] : '');
143		if ($contentParts[0] != 'image')
144		exit;
145
146		$max_age = $time - $cached['time'] + (5 * 86400);
147		header('content-type: ' . $cached['content_type']);
148		header('content-length: ' . $cached['size']);
149		header('cache-control: public, max-age=' . $max_age );
150		header('last-modified: ' . gmdate('D, d M Y H:i:s', $cached['time']) . ' GMT');
151		header('etag: ' . $eTag);
152		echo base64_decode($cached['body']);
153		}
154
155		/**
156		* Returns the request's hashed filepath
157		*
158		* @access public
159		* @param string $request The request to get the path for
160		* @return string The hashed filepath for the specified request
161		*/
162		protected function getCachedPath($request)
163		{
164		return $this->cache . '/' . sha1($request . $this->secret);
165		}
166
167		/**
168		* Check whether the image exists in local cache or not
169		*
170		* @access protected
171		* @param string $request The image to check for in the cache
172		* @return bool Whether or not the requested image is cached
173		*/
174		protected function isCached($request)
175		{
176		return file_exists($this->getCachedPath($request));
177		}
178
179		/**
180		* Attempts to cache the image while validating it
181		*
182		* @access protected
183		* @param string $request The image to cache/validate
184		* @return int -1 error, 0 too big, 1 valid image
185		*/
186		protected function cacheImage($request)
187		{
188		$dest = $this->getCachedPath($request);
189		$curl = new curl_fetch_web_data(array(CURLOPT_BINARYTRANSFER => 1));
190		$curl_request = $curl->get_url_data($request);
191		$responseCode = $curl_request->result('code');
192		$response = $curl_request->result();
193
194		if (empty($response) \|\| $responseCode != 200)
195		{
196		return -1;
197		}
198
199		$headers = $response['headers'];
200
201		// Make sure the url is returning an image
202		$contentParts = explode('/', !empty($headers['content-type']) ? $headers['content-type'] : '');
203		if ($contentParts[0] != 'image')
204		return -1;
205
206		// Validate the filesize
207		if ($response['size'] > ($this->maxSize * 1024))
208		return 0;
209
210		$time = $this->getTime();
211
212		return file_put_contents($dest, json_encode(array(
213		'content_type' => $headers['content-type'],
214		'size' => $response['size'],
215		'time' => $time,
216		'body' => base64_encode($response['body']),
217		))) === false ? -1 : 1;
218		}
219
220		/**
221		* Static helper function to redirect a request
222		*
223		* @access public
224		* @param type $request
225		* @return void
226		*/
227		static public function redirectexit($request)
		0 ignored issues – show Coding Style introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report As per PSR2, the `static` declaration should come after the visibility declaration. Loading history...
228		{
229		header('Location: ' . $request, false, 301);
		0 ignored issues – show Security Response Splitting introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report `'Location: ' . $request` can contain request data and is used in response header context(s) leading to a potential security vulnerability. 1 path for user data to reach this point Read from `$_GET,` and `$request` is assigned in proxy.php on line 105 `$request` is passed to ProxyServer::redirectexit() in proxy.php on line 120 Response Splitting Attacks Allowing an attacker to set a response header, opens your application to response splitting attacks; effectively allowing an attacker to send any response, he would like. General Strategies to prevent injection In general, it is advisable to prevent any user-data to reach this point. This can be done by white-listing certain values: if ( ! in_array($value, array('this-is-allowed', 'and-this-too'), true)) { throw new \InvalidArgumentException('This input is not allowed.'); } For numeric data, we recommend to explicitly cast the data: $sanitized = (integer) $tainted; Loading history...
230		exit;
231		}
232
233		/**
234		* Helper function to call time() once with the right logic
235		*
236		* @return int
237		*/
238		protected function getTime()
239		{
240		if (empty($this->time))
241		{
242		$old_timezone = date_default_timezone_get();
243		date_default_timezone_set('GMT');
244		$this->time = time();
245		date_default_timezone_set($old_timezone);
246		}
247
248		return $this->time;
249		}
250
251		/**
252		* Delete all old entries
253		*
254		* @access public
255		* @return void
256		*/
257		public function housekeeping()
258		{
259		$path = $this->cache . '/';
260		if ($handle = opendir($path)) {
261
262		while (false !== ($file = readdir($handle)))
263		{
264		$filelastmodified = filemtime($path . $file);
265
266		if ((time() - $filelastmodified) > ($this->maxDays * 86400))
267		{
268		unlink($path . $file);
269		}
270
271		}
272
273		closedir($handle);
274		}
275		}
276		}
277
278		if (empty($proxyhousekeeping))
279		{
280		$proxy = new ProxyServer();
281		$proxy->serve();
282		}
283
284		?>

SimpleMachines / SMF2.1

Pull Request — release-2.1 (#4535)

ProxyServer::getTime() A

Complexity

Size

Duplication

Importance

1. Pass all data via parameters

2. Create a class that maintains your state

1. Pass all data via parameters

2. Create a class that maintains your state

1 path for user data to reach this point

Response Splitting Attacks

General Strategies to prevent injection

Duplication Side-by-Side

Filter issues like