Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix psalm taint errors #50800

Merged
merged 9 commits into from
Feb 17, 2025
7 changes: 2 additions & 5 deletions apps/admin_audit/lib/Actions/Action.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,8 @@ public function log(string $text,
);
} else {
$this->logger->critical(
sprintf(
'$params["' . $element . '"] was missing. Transferred value: %s',
print_r($params, true)
),
['app' => 'admin_audit']
'$params["' . $element . '"] was missing. Transferred value: {params}',
['app' => 'admin_audit', 'params' => $params]
);
}
return;
Expand Down
71 changes: 0 additions & 71 deletions build/psalm-baseline-security.xml
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<files psalm-version="5.26.1@d747f6500b38ac4f7dfc5edbcae6e4b637d7add0">
<file src="apps/admin_audit/lib/Actions/Action.php">
<TaintedHtml>
<code><![CDATA[$params]]></code>
</TaintedHtml>
</file>
<file src="apps/files_external/lib/Config/ConfigAdapter.php">
<TaintedCallable>
<code><![CDATA[$objectClass]]></code>
Expand All @@ -16,40 +11,11 @@
<code><![CDATA[$imageFile]]></code>
</TaintedFile>
</file>
<file src="lib/base.php">
<TaintedHeader>
<code><![CDATA['Location: ' . $url]]></code>
<code><![CDATA['Location: ' . \OC::$WEBROOT . '/']]></code>
</TaintedHeader>
</file>
<file src="lib/private/Config.php">
<TaintedHtml>
<code><![CDATA[$this->cache]]></code>
</TaintedHtml>
</file>
<file src="lib/private/EventSource.php">
<TaintedHeader>
<code><![CDATA['Location: ' . \OC::$WEBROOT]]></code>
</TaintedHeader>
</file>
<file src="lib/private/Http/CookieHelper.php">
<TaintedHeader>
<code><![CDATA[$header]]></code>
</TaintedHeader>
</file>
<file src="lib/private/Installer.php">
<TaintedFile>
<code><![CDATA[$baseDir]]></code>
</TaintedFile>
</file>
<file src="lib/private/OCS/ApiHelper.php">
<TaintedHtml>
<code><![CDATA[$body]]></code>
</TaintedHtml>
<TaintedTextWithQuotes>
<code><![CDATA[$body]]></code>
</TaintedTextWithQuotes>
</file>
<file src="lib/private/Route/Router.php">
<TaintedCallable>
<code><![CDATA[$appNameSpace . '\\Controller\\' . basename($file->getPathname(), '.php')]]></code>
Expand All @@ -70,46 +36,9 @@
<code><![CDATA[$sqliteFile]]></code>
</TaintedFile>
</file>
<file src="lib/private/legacy/OC_Helper.php">
<TaintedFile>
<code><![CDATA[$dest]]></code>
<code><![CDATA[$dest]]></code>
<code><![CDATA[$dir]]></code>
<code><![CDATA[$dir]]></code>
</TaintedFile>
</file>
<file src="lib/private/legacy/OC_JSON.php">
<TaintedHeader>
<code><![CDATA['Location: ' . \OC::$WEBROOT]]></code>
</TaintedHeader>
</file>
<file src="lib/private/legacy/OC_Template.php">
<TaintedHtml>
<code><![CDATA[$exception->getTraceAsString()]]></code>
</TaintedHtml>
<TaintedTextWithQuotes>
<code><![CDATA[$exception->getTraceAsString()]]></code>
</TaintedTextWithQuotes>
</file>
<file src="lib/public/DB/QueryBuilder/IQueryBuilder.php">
<TaintedSql>
<code><![CDATA[$column]]></code>
</TaintedSql>
</file>
<file src="lib/public/IDBConnection.php">
<TaintedSql>
<code><![CDATA[$sql]]></code>
<code><![CDATA[$sql]]></code>
<code><![CDATA[$sql]]></code>
<code><![CDATA[$sql]]></code>
</TaintedSql>
</file>
<file src="ocs-provider/index.php">
<TaintedHtml>
<code><![CDATA[$controller->buildProviderList()->render()]]></code>
</TaintedHtml>
<TaintedTextWithQuotes>
<code><![CDATA[$controller->buildProviderList()->render()]]></code>
</TaintedTextWithQuotes>
</file>
</files>
10 changes: 9 additions & 1 deletion lib/private/AppFramework/OCS/BaseResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ protected function renderResult(array $meta): string {
];

if ($this->format === 'json') {
return json_encode($response, JSON_HEX_TAG);
return $this->toJson($response);
}

$writer = new \XMLWriter();
Expand All @@ -111,6 +111,14 @@ protected function renderResult(array $meta): string {
return $writer->outputMemory(true);
}

/**
* @psalm-taint-escape has_quotes
* @psalm-taint-escape html
*/
protected function toJson(array $array): string {
return \json_encode($array, \JSON_HEX_TAG);
}

protected function toXML(array $array, \XMLWriter $writer): void {
foreach ($array as $k => $v) {
if ($k === '@attributes' && is_array($v)) {
Expand Down
2 changes: 1 addition & 1 deletion lib/private/AppFramework/Utility/SimpleContainer.php
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ public function registerAlias($alias, $target) {
}, false);
}

/*
/**
* @param string $name
* @return string
*/
Expand Down
24 changes: 22 additions & 2 deletions lib/private/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,36 @@ public function getKeys() {
*/
public function getValue($key, $default = null) {
if (isset($this->envCache[$key])) {
return $this->envCache[$key];
return self::trustSystemConfig($this->envCache[$key]);
}

if (isset($this->cache[$key])) {
return $this->cache[$key];
return self::trustSystemConfig($this->cache[$key]);
}

return $default;
}

/**
* Since system config is admin controlled, we can tell psalm to ignore any taint
*
* @psalm-taint-escape callable
* @psalm-taint-escape cookie
* @psalm-taint-escape file
* @psalm-taint-escape has_quotes
* @psalm-taint-escape header
* @psalm-taint-escape html
* @psalm-taint-escape include
* @psalm-taint-escape ldap
* @psalm-taint-escape shell
* @psalm-taint-escape sql
* @psalm-taint-escape unserialize
* @psalm-pure
*/
public static function trustSystemConfig(mixed $value): mixed {
return $value;
}

/**
* Sets and deletes values and writes the config.php
*
Expand Down
27 changes: 24 additions & 3 deletions lib/private/L10N/Factory.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ public function get($app, $lang = null, $locale = null) {
$locale = $forceLocale;
}

if ($lang === null || !$this->languageExists($app, $lang)) {
$lang = $this->findLanguage($app);
}
$lang = $this->validateLanguage($app, $lang);

if ($locale === null || !$this->localeExists($locale)) {
$locale = $this->findLocale($lang);
Expand All @@ -130,6 +128,29 @@ public function get($app, $lang = null, $locale = null) {
});
}

/**
* Check that $lang is an existing language and not null, otherwise return the language to use instead
*
* @psalm-taint-escape callable
* @psalm-taint-escape cookie
* @psalm-taint-escape file
* @psalm-taint-escape has_quotes
* @psalm-taint-escape header
* @psalm-taint-escape html
* @psalm-taint-escape include
* @psalm-taint-escape ldap
* @psalm-taint-escape shell
* @psalm-taint-escape sql
* @psalm-taint-escape unserialize
*/
private function validateLanguage(string $app, ?string $lang): string {
if ($lang === null || !$this->languageExists($app, $lang)) {
return $this->findLanguage($app);
} else {
return $lang;
}
}

/**
* Find the best language
*
Expand Down
22 changes: 11 additions & 11 deletions lib/private/Setup/MySQL.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public function setupDatabase($username) {
/**
* @param \OC\DB\Connection $connection
*/
private function createDatabase($connection) {
private function createDatabase($connection): void {
try {
$name = $this->dbName;
$user = $this->dbUser;
Expand Down Expand Up @@ -91,23 +91,23 @@ private function createDatabase($connection) {
* @param IDBConnection $connection
* @throws \OC\DatabaseSetupException
*/
private function createDBUser($connection) {
private function createDBUser($connection): void {
try {
$name = $this->dbUser;
$password = $this->dbPassword;
// we need to create 2 accounts, one for global use and one for local user. if we don't specify the local one,
// the anonymous user would take precedence when there is one.

if ($connection->getDatabasePlatform() instanceof Mysql80Platform) {
$query = "CREATE USER '$name'@'localhost' IDENTIFIED WITH mysql_native_password BY '$password'";
$connection->executeUpdate($query);
$query = "CREATE USER '$name'@'%' IDENTIFIED WITH mysql_native_password BY '$password'";
$connection->executeUpdate($query);
$query = "CREATE USER ?@'localhost' IDENTIFIED WITH mysql_native_password BY ?";
$connection->executeUpdate($query, [$name,$password]);
$query = "CREATE USER ?@'%' IDENTIFIED WITH mysql_native_password BY ?";
$connection->executeUpdate($query, [$name,$password]);
} else {
$query = "CREATE USER '$name'@'localhost' IDENTIFIED BY '$password'";
$connection->executeUpdate($query);
$query = "CREATE USER '$name'@'%' IDENTIFIED BY '$password'";
$connection->executeUpdate($query);
$query = "CREATE USER ?@'localhost' IDENTIFIED BY ?";
$connection->executeUpdate($query, [$name,$password]);
$query = "CREATE USER ?@'%' IDENTIFIED BY ?";
$connection->executeUpdate($query, [$name,$password]);
}
} catch (\Exception $ex) {
$this->logger->error('Database user creation failed.', [
Expand All @@ -119,7 +119,7 @@ private function createDBUser($connection) {
}

/**
* @param $username
* @param string $username
* @param IDBConnection $connection
*/
private function createSpecificUser($username, $connection): void {
Expand Down
20 changes: 1 addition & 19 deletions lib/private/SystemConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,24 +116,6 @@ public function __construct(
) {
}

/**
* Since system config is admin controlled, we can tell psalm to ignore any taint
*
* @psalm-taint-escape sql
* @psalm-taint-escape html
* @psalm-taint-escape ldap
* @psalm-taint-escape callable
* @psalm-taint-escape file
* @psalm-taint-escape ssrf
* @psalm-taint-escape cookie
* @psalm-taint-escape header
* @psalm-taint-escape has_quotes
* @psalm-pure
*/
public static function trustSystemConfig(mixed $value): mixed {
return $value;
}

/**
* Lists all available config keys
* @return array an array of key names
Expand Down Expand Up @@ -170,7 +152,7 @@ public function setValues(array $configs) {
* @return mixed the value or $default
*/
public function getValue($key, $default = '') {
return $this->trustSystemConfig($this->config->getValue($key, $default));
return $this->config->getValue($key, $default);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/private/TaskProcessing/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ public function setTaskResult(int $id, ?string $error, ?array $result, bool $isU
$task->setEndedAt(time());
$error = 'The task was processed successfully but the provider\'s output doesn\'t pass validation against the task type\'s outputShape spec and/or the provider\'s own optionalOutputShape spec';
$task->setErrorMessage($error);
$this->logger->error($error . ' Output was: ' . var_export($result, true), ['exception' => $e]);
$this->logger->error($error, ['exception' => $e, 'output' => $result]);
} catch (NotPermittedException $e) {
$task->setProgress(1);
$task->setStatus(Task::STATUS_FAILED);
Expand Down
5 changes: 3 additions & 2 deletions lib/private/legacy/OC_JSON.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public static function checkAdminUser() {
* Send json error msg
* @deprecated 12.0.0 Use a AppFramework JSONResponse instead
* @suppress PhanDeprecatedFunction
* @psalm-taint-escape html
*/
public static function error($data = []) {
$data['status'] = 'error';
Expand All @@ -86,7 +85,6 @@ public static function error($data = []) {
* Send json success msg
* @deprecated 12.0.0 Use a AppFramework JSONResponse instead
* @suppress PhanDeprecatedFunction
* @psalm-taint-escape html
*/
public static function success($data = []) {
$data['status'] = 'success';
Expand All @@ -97,6 +95,9 @@ public static function success($data = []) {
/**
* Encode JSON
* @deprecated 12.0.0 Use a AppFramework JSONResponse instead
*
* @psalm-taint-escape has_quotes
* @psalm-taint-escape html
*/
private static function encode($data) {
return json_encode($data, JSON_HEX_TAG);
Expand Down
12 changes: 10 additions & 2 deletions lib/private/legacy/OC_Template.php
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,15 @@ public static function printExceptionErrorPage($exception, $statusCode = 503) {
die();
}

private static function printPlainErrorPage(\Throwable $exception, bool $debug = false) {
/**
* @psalm-taint-escape has_quotes
* @psalm-taint-escape html
*/
private static function fakeEscapeForPlainText(string $str): string {
return $str;
}

private static function printPlainErrorPage(\Throwable $exception, bool $debug = false): void {
header('Content-Type: text/plain; charset=utf-8');
print("Internal Server Error\n\n");
print("The server encountered an internal error and was unable to complete your request.\n");
Expand All @@ -323,7 +331,7 @@ private static function printPlainErrorPage(\Throwable $exception, bool $debug =
if ($debug) {
print("\n");
print($exception->getMessage() . ' ' . $exception->getFile() . ' at ' . $exception->getLine() . "\n");
print($exception->getTraceAsString());
print(self::fakeEscapeForPlainText($exception->getTraceAsString()));
}
}
}
3 changes: 3 additions & 0 deletions lib/public/AppFramework/Http/JSONResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ public function __construct(
* @return string the rendered json
* @since 6.0.0
* @throws \Exception If data could not get encoded
*
* @psalm-taint-escape has_quotes
* @psalm-taint-escape html
*/
public function render() {
return json_encode($this->data, JSON_HEX_TAG | JSON_THROW_ON_ERROR | $this->encodeFlags, 2048);
Expand Down
Loading