Secure serving of user files from extensions (#7495)

* Secure serving of user files from extensions
fix https://github.com/FreshRSS/FreshRSS/issues/4930

* More fixes

* Typo

app/Controllers/extensionController.php

@@ -298,4 +298,40 @@ class FreshRSS_extension_Controller extends FreshRSS_ActionController {
 
 		Minz_Request::forward($url_redirect, true);
 	}
+
+	// Supported types with their associated content type
+	public const MIME_TYPES = [
+		'css' => 'text/css; charset=UTF-8',
+		'gif' => 'image/gif',
+		'jpeg' => 'image/jpeg',
+		'jpg' => 'image/jpeg',
+		'js' => 'application/javascript; charset=UTF-8',
+		'png' => 'image/png',
+		'svg' => 'image/svg+xml',
+	];
+
+	public function serveAction(): void {
+		$extensionName = Minz_Request::paramString('x');
+		$filename = Minz_Request::paramString('f');
+		$mimeType = pathinfo($filename, PATHINFO_EXTENSION);
+		if ($extensionName === '' || $filename === '' || $mimeType === '' || empty(self::MIME_TYPES[$mimeType])) {
+			header('HTTP/1.1 400 Bad Request');
+			die('Bad Request!');
+		}
+		$extension = Minz_ExtensionManager::findExtension($extensionName);
+		if ($extension === null || !$extension->isEnabled() || ($mtime = $extension->mtimeFile($filename)) === null) {
+			header('HTTP/1.1 404 Not Found');
+			die('Not Found!');
+		}
+
+		$this->view->_layout(null);
+
+		$content_type = self::MIME_TYPES[$mimeType];
+		header("Content-Type: {$content_type}");
+		header("Content-Disposition: inline; filename='{$filename}'");
+		header('Referrer-Policy: same-origin');
+		if (!httpConditional($mtime, cacheSeconds: 604800, cachePrivacy: 2)) {
+			echo $extension->getFile($filename);
+		}
+	}
 }

docs/en/developers/03_Backend/05_Extensions.md

@@ -116,7 +116,9 @@ The `Minz_Extension` abstract class defines a set of methods that can be overrid
 
 The `Minz_Extension` abstract class defines another set of methods that should not be overridden:
 * the `getName`, `getEntrypoint`, `getPath`, `getAuthor`, `getDescription`, `getVersion`, and `getType` methods return the extension internal properties. Those properties are extracted from the `metadata.json` file.
-* the `getFileUrl` returns the URL of the selected file. The file must exist in the `static` folder of the extension.
+* `getFileUrl(string $filename, bool $isStatic = true): string` will return the URL to a file in the `static` directory.
+	The first parameter is the name of the file (without `static/`).
+	Set `$isStatic` to true for user-independent files, and to `false` for files saved in a user’s own directory.
 * the `registerController` method register an extension controller in FreshRSS. The selected controller must be defined in the extension *Controllers* folder, its file name must be `\<name\>Controller.php`, and its class name must be `FreshExtension_\<name\>_Controller`.
 * the `registerViews` method registers the extension views in FreshRSS.
 * the `registerTranslates` method registers the extension translation files in FreshRSS.

docs/fr/developers/03_Backend/05_Extensions.md

@@ -171,9 +171,9 @@ Your class will benefit from four methods to redefine:
 	`getName()`, `getEntrypoint()`, `getPath()` (allows you to retrieve the
 	path to your extension), `getAuthor()`, `getDescription()`,
 	`getVersion()`, `getType()`.
-* `getFileUrl($filename, $type)` will return the URL to a file in the
-	`static` directory. The first parameter is the name of the file (without
-	`static /`), the second is the type of file to be used (`css` or `js`).
+* `getFileUrl(string $filename, bool $isStatic = true): string` will return the URL to a file in the `static` directory.
+	The first parameter is the name of the file (without `static/`).
+	Set `$isStatic` to true for user-independent files, and to `false` for files saved in a user’s own directory.
 * `registerController($base_name)` will tell Minz to take into account the
 	given controller in the routing system. The controller must be located in
 	your `Controllers` directory, the name of the file must be `<base_name>Controller.php` and the name of the

lib/Minz/Extension.php

@@ -178,12 +178,26 @@ abstract class Minz_Extension {
 	}
 
 	/** Return whether a user-specific, extension-specific, file exists */
-	final protected function hasFile(string $filename): bool {
+	final public function hasFile(string $filename): bool {
+		if ($filename === '' || str_contains($filename, '..')) {
+			return false;
+		}
 		return file_exists($this->getExtensionUserPath() . '/' . $filename);
 	}
 
-	/** Return the user-specific, extension-specific, file content, or null if it does not exist */
-	final protected function getFile(string $filename): ?string {
+	/** Return the motification time of the user-specific, extension-specific, file or null if it does not exist */
+	final public function mtimeFile(string $filename): ?int {
+		if (!$this->hasFile($filename)) {
+			return null;
+		}
+		return @filemtime($this->getExtensionUserPath() . '/' . $filename) ?: null;
+	}
+
+	/** Return the user-specific, extension-specific, file content or null if it does not exist */
+	final public function getFile(string $filename): ?string {
+		if (!$this->hasFile($filename)) {
+			return null;
+		}
 		$content = @file_get_contents($this->getExtensionUserPath() . '/' . $filename);
 		return is_string($content) ? $content : null;
 	}
@@ -192,26 +206,27 @@ abstract class Minz_Extension {
 	 * Return the url for a given file.
 	 *
 	 * @param string $filename name of the file to serve.
-	 * @param 'css'|'js'|'svg' $type the type (js or css or svg) of the file to serve.
+	 * @param '' $type MIME type of the file to serve. Deprecated: always use the file extension.
 	 * @param bool $isStatic indicates if the file is a static file or a user file. Default is static.
 	 * @return string url corresponding to the file.
 	 */
-	final public function getFileUrl(string $filename, string $type, bool $isStatic = true): string {
+	final public function getFileUrl(string $filename, string $type = '', bool $isStatic = true): string {
 		if ($isStatic) {
 			$dir = basename($this->path);
 			$file_name_url = urlencode("{$dir}/static/{$filename}");
 			$mtime = @filemtime("{$this->path}/static/{$filename}");
+			return Minz_Url::display("/ext.php?f={$file_name_url}&{$mtime}", 'php');
 		} else {
 			$username = Minz_User::name();
 			if ($username == null) {
 				return '';
 			}
-			$path = $this->getExtensionUserPath() . "/{$filename}";
-			$file_name_url = urlencode("{$username}/extensions/{$this->getEntrypoint()}/{$filename}");
-			$mtime = @filemtime($path);
+			return Minz_Url::display(['c' => 'extension', 'a' => 'serve', 'params' => [
+				'x' => $this->getName(),
+				'f' => $filename,
+				'm' => $this->mtimeFile($filename),	// cache-busting
+			]]);
 		}
-
-		return Minz_Url::display("/ext.php?f={$file_name_url}&t={$type}&{$mtime}", 'php');
 	}
 
 	/**

lib/core-extensions/UserCSS/extension.php

@@ -11,7 +11,7 @@ final class UserCSSExtension extends Minz_Extension {
 
 		$this->registerTranslates();
 		if ($this->hasFile(self::FILENAME)) {
-			Minz_View::appendStyle($this->getFileUrl(self::FILENAME, 'css', false));
+			Minz_View::appendStyle($this->getFileUrl(self::FILENAME, isStatic: false));
 		}
 	}
 

lib/core-extensions/UserCSS/metadata.json

@@ -2,7 +2,7 @@
 	"name": "User CSS",
 	"author": "hkcomori, Marien Fressinaud",
 	"description": "Give possibility to overwrite the CSS with a user-specific rules.",
-	"version": "1.0.0",
+	"version": "1.1.0",
 	"entrypoint": "UserCSS",
 	"type": "user"
 }

lib/core-extensions/UserJS/extension.php

@@ -11,7 +11,7 @@ final class UserJSExtension extends Minz_Extension {
 
 		$this->registerTranslates();
 		if ($this->hasFile(self::FILENAME)) {
-			Minz_View::appendScript($this->getFileUrl(self::FILENAME, 'js', false));
+			Minz_View::appendScript($this->getFileUrl(self::FILENAME, isStatic: false));
 		}
 	}
 

lib/core-extensions/UserJS/metadata.json

@@ -2,7 +2,7 @@
 	"name": "User JS",
 	"author": "hkcomori, Frans de Jonge",
 	"description": "Apply user JS.",
-	"version": "1.0.0",
+	"version": "1.1.0",
 	"entrypoint": "UserJS",
 	"type": "user"
 }

p/ext.php

@@ -2,42 +2,21 @@
 declare(strict_types=1);
 require(__DIR__ . '/../constants.php');
 
-// Supported types with their associated content type
-const SUPPORTED_TYPES = [
-	'css' => 'text/css; charset=UTF-8',
-	'js' => 'application/javascript; charset=UTF-8',
-	'png' => 'image/png',
-	'jpeg' => 'image/jpeg',
-	'jpg' => 'image/jpeg',
-	'gif' => 'image/gif',
-	'svg' => 'image/svg+xml',
-];
-
 function get_absolute_filename(string $file_name): string {
 	$core_extension = realpath(CORE_EXTENSIONS_PATH . '/' . $file_name);
 	if (false !== $core_extension) {
 		return $core_extension;
 	}
 
-	$extension = realpath(EXTENSIONS_PATH . '/' . $file_name);
-	if (false !== $extension) {
-		return $extension;
-	}
-
 	$third_party_extension = realpath(THIRDPARTY_EXTENSIONS_PATH . '/' . $file_name);
 	if (false !== $third_party_extension) {
 		return $third_party_extension;
 	}
 
-	$user = realpath(USERS_PATH . '/' . $file_name);
-	if (false !== $user) {
-		return $user;
-	}
-
 	return '';
 }
 
-function is_valid_path_extension(string $path, string $extensionPath, bool $isStatic = true): bool {
+function is_valid_path_extension(string $path, string $extensionPath): bool {
 	// It must be under the extension path.
 	$real_ext_path = realpath($extensionPath);
 	if ($real_ext_path == false) {
@@ -53,11 +32,6 @@ function is_valid_path_extension(string $path, string $extensionPath, bool $isSt
 		return false;
 	}
 
-	// User files do not need further validations
-	if (!$isStatic) {
-		return true;
-	}
-
 	// Static files to serve must be under a `ext_dir/static/` directory.
 	$path_relative_to_ext = substr($path, strlen($real_ext_path) + 1);
 	[, $static, $file] = sscanf($path_relative_to_ext, '%[^/]/%[^/]/%s') ?? [null, null, null];
@@ -78,28 +52,26 @@ function is_valid_path_extension(string $path, string $extensionPath, bool $isSt
  * @return bool true if it can be served, false otherwise.
  */
 function is_valid_path(string $path): bool {
-	return is_valid_path_extension($path, CORE_EXTENSIONS_PATH) || is_valid_path_extension($path, THIRDPARTY_EXTENSIONS_PATH)
-		|| is_valid_path_extension($path, USERS_PATH, false);
+	return is_valid_path_extension($path, CORE_EXTENSIONS_PATH) || is_valid_path_extension($path, THIRDPARTY_EXTENSIONS_PATH);
 }
 
 function sendBadRequestResponse(?string $message = null): never {
 	header('HTTP/1.1 400 Bad Request');
-	die($message);
+	die($message ?? 'Bad Request!');
 }
 
 function sendNotFoundResponse(): never {
 	header('HTTP/1.1 404 Not Found');
-	die();
+	die('Not Found!');
 }
 
-if (!isset($_GET['f'], $_GET['t']) || !is_string($_GET['f']) || !is_string($_GET['t'])) {
+if (!is_string($_GET['f'] ?? null)) {
 	sendBadRequestResponse('Query string is incomplete.');
 }
 
 $file_name = urldecode($_GET['f']);
-$file_type = $_GET['t'];
-if (empty(SUPPORTED_TYPES[$file_type]) ||
-	empty(SUPPORTED_TYPES[pathinfo($file_name, PATHINFO_EXTENSION)])) {
+$file_type = pathinfo($file_name, PATHINFO_EXTENSION);
+if (empty(FreshRSS_extension_Controller::MIME_TYPES[$file_type])) {
 	sendBadRequestResponse('File type is not supported.');
 }
 
@@ -113,7 +85,7 @@ if (!is_valid_path($absolute_filename)) {
 	sendBadRequestResponse('File is not supported.');
 }
 
-$content_type = SUPPORTED_TYPES[$file_type];
+$content_type = FreshRSS_extension_Controller::MIME_TYPES[$file_type];
 header("Content-Type: {$content_type}");
 header("Content-Disposition: inline; filename='{$file_name}'");
 header('Referrer-Policy: same-origin');