fix(appmanager): Fix tainted file path when loading appinfos

[nickvergessen]

Comments

• It's arguable whether killing the bool $path parameter is a breaking change and should be done. But I'd say the risk is minimal. The only usage I found is the \OC\Installer code.
• The entry is actually a "false positive", as the claimed call is with $path = false and therefore

  $file - lib/private/App/AppManager.php:747:4
    		$file = $appId;
  

  is never applicable (it's the if ($path) { block). But to work around that would mean we need to add many psalm comments in all the places calling it, until upstream fixes it (and I'm not sure they can with reasonable effort).
• So I thought splitting the method in a "tainted" and "path" way is the better solution.

After

• 1 less baseline entry

Before

TaintedFile - lib/private/App/InfoParser.php:38:50 - Detected tainted file handling (see https://psalm.dev/255)
  oca\updatenotification\controller\changelogcontroller::showchangelog#1 - apps/updatenotification/lib/Controller/ChangelogController.php:41:32
	public function showChangelog(string $app, ?string $version = null): TemplateResponse {

  $app - apps/updatenotification/lib/Controller/ChangelogController.php:41:39
	public function showChangelog(string $app, ?string $version = null): TemplateResponse {

  call to OCP\App\IAppManager::getAppInfo - apps/updatenotification/lib/Controller/ChangelogController.php:43:44
		$appInfo = $this->appManager->getAppInfo($app) ?? [];

  OCP\App\IAppManager::getAppInfo#1 - lib/public/App/IAppManager.php:34:36
	public function getAppInfo(string $appId, bool $path = false, $lang = null);

  OC\App\AppManager::getAppInfo#1 - apps/files_versions/lib/AppInfo/Application.php:118:39
			$appInfo = $appManager->getAppInfo($app);

  $appId - lib/private/App/AppManager.php:745:36
	public function getAppInfo(string $appId, bool $path = false, $lang = null) {

  $file - lib/private/App/AppManager.php:747:4
			$file = $appId;

  call to OC\App\InfoParser::parse - lib/private/App/AppManager.php:761:26
		$data = $parser->parse($file);

  OC\App\InfoParser::parse#1 - lib/private/App/InfoParser.php:25:24
	public function parse($file) {

  $file - lib/private/App/InfoParser.php:25:24
	public function parse($file) {

  call to file_get_contents - lib/private/App/InfoParser.php:38:50
		$xml = simplexml_load_string(file_get_contents($file));

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[come-nc]

With this change I can now write an application with id spreed in the info.xml and overwrite spreed infos, no?
I do not see a check for $data['id'] to match original appId.

[nickvergessen]

well we don't always have an appId at hand to compare with.
I also thought about the overwriting.

Best idea I have would be moving the caching into the part that is based on appId and basically in here remove the cache based on id, so it can be readded outside or when it was loaded with a full path, the next call would cache it

[come-nc]

I think it’s fine to move the caching to the other method and not use it for the path version.