verify the path in the autoloader

[icewind1991]

limit auto loaded files to only the paths we expect code to be in

[icewind1991]

cc @LukasReschke @Xenopathic @DeepDiver1975 @PVince81

[RobinMcCorkell]

👍

[ghost]

🚀 Test PASSed.🚀

[LukasReschke]

PHP Doc please

[PVince81]

Will this potentially break any existing apps ? (especially thinking about the ones where class name doesn't match file name)

[icewind1991]

Will this potentially break any existing apps ? (especially thinking about the ones where class name doesn't match file name)

It only checks the path that the autoloader comes up with, not the classname so it shouldn't break anything using classmaps

[LukasReschke]

What's the state on this?

[scrutinizer-notifier]

A new inspection was created.

[icewind1991]

@LukasReschke should be good to merge

[oparoz]

There is code in /tests/lib/testcase.php that some apps use for tests and now things are broken.

[Exception]
Path not allowed: /tests/lib/testcase.php

[RobinMcCorkell]

@oparoz Make sure you are calling the bootstrap correctly, it initializes the autoloader path

[oparoz]

Thanks for the tip @Xenopathic! I can see that it adds tests to the list of valid roots and that fixed my custom bootstrap, but many apps will break since they either use their own bootstrap (tests/bootstrap.php) or link to lib/base.php directly from phpunit.xml (the official way of doing things):
https://github.com/owncloud/ocdev/blob/master/ocdev/plugins/startapp/templates/8.1/app/phpunit.xml

Couldn't that line be added to lib/base.php instead?

[RobinMcCorkell]

@oparoz The autoloader should be 'locked down' to its minimum set of directories, for maximum security. The fact that no-one has found a security bug in our tests does not mean that there isn't one (we take a lot of liberties in the tests), so it makes sense only to enable the tests subdirectory when we are actually running tests. For that, you have to make sure that the bootstrap is loaded, or you manually add the prefix.

Perhaps it's a documentation issue?

[oparoz]

Ah, got it, thanks :)

I think it would be wise to post something on the mailing list. I'll open an issue on ocdev.

[oparoz]

It has started...
https://travis-ci.org/owncloud/mail/jobs/78364764

[nickvergessen]

I vote for revert, not loading all the stuff by default anymore causes major problems for apps.
Also loading base.php should be enough. to get that working again.

Also the expected minimum for a change like this would be a mail to the dev list with a code sample how to get it working again, don't you think?

[DeepDiver1975]

Revert!

[oparoz]

I agree that the process is broken, but the fix is a one-liner and the patch adds extra security to ownCloud. Maybe fire an email on the dev list asap and keep it in?

[nickvergessen]

Well test folderis removed while creating the packages, so...

[icewind1991]

Don't revert, only impacts test, simple fix

[nickvergessen]

"only"

[fossxplorer]

Some of my apps are symlinks and have always worked, but it's broken:
apps3 -> ../oc/apps3/

An unhandled exception has been thrown:
exception 'Exception' with message 'Path not allowed: /home/user1/oc/apps3/activity/appinfo/application.php' in /home/user1/oc7/lib/autoloader.php:132

Stack trace:
#0 /home/user1/oc7/lib/autoloader.php(152): OC\Autoloader->isValidPath('/home/user1...')
#1 [internal function]: OC\Autoloader->load('OCA\Activity\Ap...')
#2 /home/user1/oc/apps3/activity/appinfo/app.php(26): spl_autoload_call('OCA\Activity\Ap...')
#3 /home/user1/oc7/lib/private/app.php(149): require_once('/home/user1...')
#4 /home/user1/oc7/lib/private/app.php(130): OC_App::requireAppFile('activity')
#5 /home/user1/oc7/lib/private/app.php(109): OC_App::loadApp('activity')
#6 /home/user1/oc7/lib/private/console/application.php(56): OC_App::loadApps()
#7 /home/user1/oc7/console.php(67): OC\Console\Application->loadCommands(Object(Symfony\Component\Console\Output\ConsoleOutput))
#8 /home/user1/oc7/occ(11): require_once('/home/user1...')

[LukasReschke]

Yes. Symlinks seem not to be resolved. For the meantime you can use

core/config/config.sample.php

Lines 621 to 635 in e9e42ff

	/**
	* Use the ``apps_paths`` parameter to set the location of the Apps directory,
	* which should be scanned for available apps, and where user-specific apps
	* should be installed from the Apps store. The ``path`` defines the absolute
	* file system path to the app folder. The key ``url`` defines the HTTP web path
	* to that folder, starting from the ownCloud web root. The key ``writable``
	* indicates if a web server can write files to that folder.
	*/
	'apps_paths' => array(
	array(
	'path'=> '/var/www/owncloud/apps',
	'url' => '/apps',
	'writable' => true,
	),
	),

to work with multiple apps folders.

[BernhardPosselt]

Can we revert this?

Even after adjusting my tests this won't work for cli commandline tests. Why not simply ship without the tests directory? Or very simply don't load test cases when we are not in debug mode ....

[BernhardPosselt]

Apart from that this makes no sense: All tests in the tests folder that are autoloadable should be wrapped in classes that extend a PHPUnit testcase. Now even if someone loads a test through an attack, nothing will happen.

Also this is half assed: what about test folders in an app directory?

TL;DR: PR makes no sense, fix your tests to not execute code when included (if there even are any) 👎

[RobinMcCorkell]

@BernhardPosselt I thought your problem was about loading your app code when running tests? Therefore the problem lies in loading your app, not in registering the tests directory in the autoloader. This was done as part of #18839 to harden ownCloud against outdated installed-but-not-enabled apps.

[BernhardPosselt]

That's fine, but you are also breaking the test suite.

[BernhardPosselt]

Another thing: if you are still going for the appinfo/app.php approach, the autoloader needs to be run before including the app.php, otherwise it depends on what app gets executed first and stuff breaks.

[LukasReschke]

Another thing: if you are still going for the appinfo/app.php approach, the autoloader needs to be run before including the app.php, otherwise it depends on what app gets executed first and stuff breaks.

#19039 should do what you need.