Skip to content

Commit 9edabfa

Browse files
susnuxsusnux
authored
Merge pull request #50800 from nextcloud/fix/fix-psalm-taint-errors
fix: Fix psalm taint errors
2 parents 737f832 + c1c59f9 commit 9edabfa

File tree

12 files changed

+87
-118
lines changed

12 files changed

+87
-118
lines changed

‎apps/admin_audit/lib/Actions/Action.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,8 @@ public function log(string $text,
3737
);
3838
} else {
3939
$this->logger->critical(
40-
sprintf(
41-
'$params["' . $element . '"] was missing. Transferred value: %s',
42-
print_r($params, true)
43-
),
44-
['app' => 'admin_audit']
40+
'$params["' . $element . '"] was missing. Transferred value: {params}',
41+
['app' => 'admin_audit', 'params' => $params]
4542
);
4643
}
4744
return;

‎build/psalm-baseline-security.xml‎

Lines changed: 0 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,5 @@
11
<?xml version="1.0" encoding="UTF-8"?>
22
<files psalm-version="5.26.1@d747f6500b38ac4f7dfc5edbcae6e4b637d7add0">
3-
<file src="apps/admin_audit/lib/Actions/Action.php">
4-
<TaintedHtml>
5-
<code><![CDATA[$params]]></code>
6-
</TaintedHtml>
7-
</file>
83
<file src="apps/files_external/lib/Config/ConfigAdapter.php">
94
<TaintedCallable>
105
<code><![CDATA[$objectClass]]></code>
@@ -16,40 +11,11 @@
1611
<code><![CDATA[$imageFile]]></code>
1712
</TaintedFile>
1813
</file>
19-
<file src="lib/base.php">
20-
<TaintedHeader>
21-
<code><![CDATA['Location: ' . $url]]></code>
22-
<code><![CDATA['Location: ' . \OC::$WEBROOT . '/']]></code>
23-
</TaintedHeader>
24-
</file>
2514
<file src="lib/private/Config.php">
2615
<TaintedHtml>
2716
<code><![CDATA[$this->cache]]></code>
2817
</TaintedHtml>
2918
</file>
30-
<file src="lib/private/EventSource.php">
31-
<TaintedHeader>
32-
<code><![CDATA['Location: ' . \OC::$WEBROOT]]></code>
33-
</TaintedHeader>
34-
</file>
35-
<file src="lib/private/Http/CookieHelper.php">
36-
<TaintedHeader>
37-
<code><![CDATA[$header]]></code>
38-
</TaintedHeader>
39-
</file>
40-
<file src="lib/private/Installer.php">
41-
<TaintedFile>
42-
<code><![CDATA[$baseDir]]></code>
43-
</TaintedFile>
44-
</file>
45-
<file src="lib/private/OCS/ApiHelper.php">
46-
<TaintedHtml>
47-
<code><![CDATA[$body]]></code>
48-
</TaintedHtml>
49-
<TaintedTextWithQuotes>
50-
<code><![CDATA[$body]]></code>
51-
</TaintedTextWithQuotes>
52-
</file>
5319
<file src="lib/private/Route/Router.php">
5420
<TaintedCallable>
5521
<code><![CDATA[$appNameSpace . '\\Controller\\' . basename($file->getPathname(), '.php')]]></code>
@@ -70,46 +36,9 @@
7036
<code><![CDATA[$sqliteFile]]></code>
7137
</TaintedFile>
7238
</file>
73-
<file src="lib/private/legacy/OC_Helper.php">
74-
<TaintedFile>
75-
<code><![CDATA[$dest]]></code>
76-
<code><![CDATA[$dest]]></code>
77-
<code><![CDATA[$dir]]></code>
78-
<code><![CDATA[$dir]]></code>
79-
</TaintedFile>
80-
</file>
81-
<file src="lib/private/legacy/OC_JSON.php">
82-
<TaintedHeader>
83-
<code><![CDATA['Location: ' . \OC::$WEBROOT]]></code>
84-
</TaintedHeader>
85-
</file>
86-
<file src="lib/private/legacy/OC_Template.php">
87-
<TaintedHtml>
88-
<code><![CDATA[$exception->getTraceAsString()]]></code>
89-
</TaintedHtml>
90-
<TaintedTextWithQuotes>
91-
<code><![CDATA[$exception->getTraceAsString()]]></code>
92-
</TaintedTextWithQuotes>
93-
</file>
9439
<file src="lib/public/DB/QueryBuilder/IQueryBuilder.php">
9540
<TaintedSql>
9641
<code><![CDATA[$column]]></code>
9742
</TaintedSql>
9843
</file>
99-
<file src="lib/public/IDBConnection.php">
100-
<TaintedSql>
101-
<code><![CDATA[$sql]]></code>
102-
<code><![CDATA[$sql]]></code>
103-
<code><![CDATA[$sql]]></code>
104-
<code><![CDATA[$sql]]></code>
105-
</TaintedSql>
106-
</file>
107-
<file src="ocs-provider/index.php">
108-
<TaintedHtml>
109-
<code><![CDATA[$controller->buildProviderList()->render()]]></code>
110-
</TaintedHtml>
111-
<TaintedTextWithQuotes>
112-
<code><![CDATA[$controller->buildProviderList()->render()]]></code>
113-
</TaintedTextWithQuotes>
114-
</file>
11544
</files>

‎lib/private/AppFramework/OCS/BaseResponse.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ protected function renderResult(array $meta): string {
9999
];
100100

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

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

114+
/**
115+
* @psalm-taint-escape has_quotes
116+
* @psalm-taint-escape html
117+
*/
118+
protected function toJson(array $array): string {
119+
return \json_encode($array, \JSON_HEX_TAG);
120+
}
121+
114122
protected function toXML(array $array, \XMLWriter $writer): void {
115123
foreach ($array as $k => $v) {
116124
if ($k === '@attributes' && is_array($v)) {

‎lib/private/AppFramework/Utility/SimpleContainer.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ public function registerAlias($alias, $target) {
176176
}, false);
177177
}
178178

179-
/*
179+
/**
180180
* @param string $name
181181
* @return string
182182
*/

‎lib/private/Config.php‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,16 +65,36 @@ public function getKeys() {
6565
*/
6666
public function getValue($key, $default = null) {
6767
if (isset($this->envCache[$key])) {
68-
return $this->envCache[$key];
68+
return self::trustSystemConfig($this->envCache[$key]);
6969
}
7070

7171
if (isset($this->cache[$key])) {
72-
return $this->cache[$key];
72+
return self::trustSystemConfig($this->cache[$key]);
7373
}
7474

7575
return $default;
7676
}
7777

78+
/**
79+
* Since system config is admin controlled, we can tell psalm to ignore any taint
80+
*
81+
* @psalm-taint-escape callable
82+
* @psalm-taint-escape cookie
83+
* @psalm-taint-escape file
84+
* @psalm-taint-escape has_quotes
85+
* @psalm-taint-escape header
86+
* @psalm-taint-escape html
87+
* @psalm-taint-escape include
88+
* @psalm-taint-escape ldap
89+
* @psalm-taint-escape shell
90+
* @psalm-taint-escape sql
91+
* @psalm-taint-escape unserialize
92+
* @psalm-pure
93+
*/
94+
public static function trustSystemConfig(mixed $value): mixed {
95+
return $value;
96+
}
97+
7898
/**
7999
* Sets and deletes values and writes the config.php
80100
*

‎lib/private/L10N/Factory.php‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,7 @@ public function get($app, $lang = null, $locale = null) {
108108
$locale = $forceLocale;
109109
}
110110

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

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

131+
/**
132+
* Check that $lang is an existing language and not null, otherwise return the language to use instead
133+
*
134+
* @psalm-taint-escape callable
135+
* @psalm-taint-escape cookie
136+
* @psalm-taint-escape file
137+
* @psalm-taint-escape has_quotes
138+
* @psalm-taint-escape header
139+
* @psalm-taint-escape html
140+
* @psalm-taint-escape include
141+
* @psalm-taint-escape ldap
142+
* @psalm-taint-escape shell
143+
* @psalm-taint-escape sql
144+
* @psalm-taint-escape unserialize
145+
*/
146+
private function validateLanguage(string $app, ?string $lang): string {
147+
if ($lang === null || !$this->languageExists($app, $lang)) {
148+
return $this->findLanguage($app);
149+
} else {
150+
return $lang;
151+
}
152+
}
153+
133154
/**
134155
* Find the best language
135156
*

‎lib/private/Setup/MySQL.php‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public function setupDatabase($username) {
5959
/**
6060
* @param \OC\DB\Connection $connection
6161
*/
62-
private function createDatabase($connection) {
62+
private function createDatabase($connection): void {
6363
try {
6464
$name = $this->dbName;
6565
$user = $this->dbUser;
@@ -91,23 +91,23 @@ private function createDatabase($connection) {
9191
* @param IDBConnection $connection
9292
* @throws \OC\DatabaseSetupException
9393
*/
94-
private function createDBUser($connection) {
94+
private function createDBUser($connection): void {
9595
try {
9696
$name = $this->dbUser;
9797
$password = $this->dbPassword;
9898
// we need to create 2 accounts, one for global use and one for local user. if we don't specify the local one,
9999
// the anonymous user would take precedence when there is one.
100100

101101
if ($connection->getDatabasePlatform() instanceof Mysql80Platform) {
102-
$query = "CREATE USER '$name'@'localhost' IDENTIFIED WITH mysql_native_password BY '$password'";
103-
$connection->executeUpdate($query);
104-
$query = "CREATE USER '$name'@'%' IDENTIFIED WITH mysql_native_password BY '$password'";
105-
$connection->executeUpdate($query);
102+
$query = "CREATE USER ?@'localhost' IDENTIFIED WITH mysql_native_password BY ?";
103+
$connection->executeUpdate($query, [$name,$password]);
104+
$query = "CREATE USER ?@'%' IDENTIFIED WITH mysql_native_password BY ?";
105+
$connection->executeUpdate($query, [$name,$password]);
106106
} else {
107-
$query = "CREATE USER '$name'@'localhost' IDENTIFIED BY '$password'";
108-
$connection->executeUpdate($query);
109-
$query = "CREATE USER '$name'@'%' IDENTIFIED BY '$password'";
110-
$connection->executeUpdate($query);
107+
$query = "CREATE USER ?@'localhost' IDENTIFIED BY ?";
108+
$connection->executeUpdate($query, [$name,$password]);
109+
$query = "CREATE USER ?@'%' IDENTIFIED BY ?";
110+
$connection->executeUpdate($query, [$name,$password]);
111111
}
112112
} catch (\Exception $ex) {
113113
$this->logger->error('Database user creation failed.', [
@@ -119,7 +119,7 @@ private function createDBUser($connection) {
119119
}
120120

121121
/**
122-
* @param $username
122+
* @param string $username
123123
* @param IDBConnection $connection
124124
*/
125125
private function createSpecificUser($username, $connection): void {

‎lib/private/SystemConfig.php‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -116,24 +116,6 @@ public function __construct(
116116
) {
117117
}
118118

119-
/**
120-
* Since system config is admin controlled, we can tell psalm to ignore any taint
121-
*
122-
* @psalm-taint-escape sql
123-
* @psalm-taint-escape html
124-
* @psalm-taint-escape ldap
125-
* @psalm-taint-escape callable
126-
* @psalm-taint-escape file
127-
* @psalm-taint-escape ssrf
128-
* @psalm-taint-escape cookie
129-
* @psalm-taint-escape header
130-
* @psalm-taint-escape has_quotes
131-
* @psalm-pure
132-
*/
133-
public static function trustSystemConfig(mixed $value): mixed {
134-
return $value;
135-
}
136-
137119
/**
138120
* Lists all available config keys
139121
* @return array an array of key names
@@ -170,7 +152,7 @@ public function setValues(array $configs) {
170152
* @return mixed the value or $default
171153
*/
172154
public function getValue($key, $default = '') {
173-
return $this->trustSystemConfig($this->config->getValue($key, $default));
155+
return $this->config->getValue($key, $default);
174156
}
175157

176158
/**

‎lib/private/TaskProcessing/Manager.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -999,7 +999,7 @@ public function setTaskResult(int $id, ?string $error, ?array $result, bool $isU
999999
$task->setEndedAt(time());
10001000
$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';
10011001
$task->setErrorMessage($error);
1002-
$this->logger->error($error . ' Output was: ' . var_export($result, true), ['exception' => $e]);
1002+
$this->logger->error($error, ['exception' => $e, 'output' => $result]);
10031003
} catch (NotPermittedException $e) {
10041004
$task->setProgress(1);
10051005
$task->setStatus(Task::STATUS_FAILED);

‎lib/private/legacy/OC_JSON.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ public static function checkAdminUser() {
7474
* Send json error msg
7575
* @deprecated 12.0.0 Use a AppFramework JSONResponse instead
7676
* @suppress PhanDeprecatedFunction
77-
* @psalm-taint-escape html
7877
*/
7978
public static function error($data = []) {
8079
$data['status'] = 'error';
@@ -86,7 +85,6 @@ public static function error($data = []) {
8685
* Send json success msg
8786
* @deprecated 12.0.0 Use a AppFramework JSONResponse instead
8887
* @suppress PhanDeprecatedFunction
89-
* @psalm-taint-escape html
9088
*/
9189
public static function success($data = []) {
9290
$data['status'] = 'success';
@@ -97,6 +95,9 @@ public static function success($data = []) {
9795
/**
9896
* Encode JSON
9997
* @deprecated 12.0.0 Use a AppFramework JSONResponse instead
98+
*
99+
* @psalm-taint-escape has_quotes
100+
* @psalm-taint-escape html
100101
*/
101102
private static function encode($data) {
102103
return json_encode($data, JSON_HEX_TAG);

0 commit comments

Comments
 (0)