Restrict autoloaded paths to loaded apps (and other enhancements) #18839

Merged
merged 3 commits into from Sep 6, 2015

Projects

None yet

8 participants

@Xenopathic
Member

Unloaded apps (aka not enabled apps) will not get any files autoloaded anymore, which can prevent an unloaded app becoming an attack vector if it is not regularly updated.

In addition, the autoloader will properly resolve symlinked app directories, to allow for a semi-common usecase where apps are stored elsewhere but are symlinked into the ownCloud directory. Fixes issues as noted in #18396 (comment)

@fossxplorer want to test this out with your symlinked apps?

cc @icewind1991 @MorrisJobke @PVince81 @LukasReschke

@Xenopathic Xenopathic added this to the 8.2-current milestone Sep 4, 2015
Xenopathic added some commits Sep 4, 2015
@Xenopathic Xenopathic Only add valid root for enabled apps b3acf09
@Xenopathic Xenopathic Resolve autoloader valid roots before checking
Allows symlinked app directories to work properly
895e633
@LukasReschke
Member

That's cool 👍

@Xenopathic
Member

Oh, I've inadvertently fixed some other bugs too 🚀

Fixes #18224, fixes #18317, fixes #18836, fixes #18305, fixes #18296, fixes #18322, fixes #18329, fixes #18333, ........ (yes, I included the issues that were marked as duplicates of the original one, simply to show the impact of the bug).

The line that fixes all those bugs is the move of self::$loadedApps[] = $app; from loadApps() to loadApp(). Give me a second to prepare backport PRs, assuming that's wanted @karlitschek ?

@karlitschek
Member

nice 👍 please backport

@icewind1991
Member

Looks goods 👍

@Xenopathic
Member

CI hasn't run the full autotest suite, so I'm not merging just yet...

@Xenopathic Xenopathic Unique exception for invalid autoload paths, better handling
Background jobs are tolerant of stale entries left by disabled apps,
which will cause an autoload exception.
0fac2e3
@Xenopathic
Member

The latest commit makes the autoloader throw a unique exception class, which the background job runner catches to prevent stale jobs breaking things.

@MorrisJobke
Member

The line that fixes all those bugs is the move of self::$loadedApps[] = $app; from loadApps() to loadApp(). Give me a second to prepare backport PRs, assuming that's wanted @karlitschek ?

Correct. Because loadApp is also called during app upgrade 🙈

@MorrisJobke
Member

Code looks good, fixes my issue and CI runs fine locally 👍

@MorrisJobke MorrisJobke merged commit c57595b into master Sep 6, 2015

3 checks passed

server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=vm-slave-02 Build #738 succeeded in 1 min 25 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=vm-slave-02 Build #738 succeeded in 3 min 22 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #822 succeeded in 1 min 40 sec
Details
@MorrisJobke MorrisJobke deleted the autoloader-supersecure branch Sep 6, 2015
@oparoz
Contributor
oparoz commented Sep 6, 2015

Completely breaks apps which don't rely solely on unit tests...

What is the workaround for tests this time?!?

@oparoz
Contributor
oparoz commented Sep 6, 2015

Traits are not loaded:

[OCP\AutoloadNotAllowedException]
Autoload path not allowed: /apps/gallery/controller/pathmanipulation.php

@Xenopathic
Member

@oparoz is your app being loaded in your tests? That is the requirement - that any app is properly loaded through OC_App::loadApp() to enable the autoload path.

Alternatively, add \OC::$loader->addValidRoot('your/app/path/here'); to your bootstrap 😄

@oparoz
Contributor
oparoz commented Sep 6, 2015

@Xenopathic - Last time I tried, loading the app did not work. At least it didn't bring routes up, but I'll try again to see if it solves this one.

@Xenopathic
Member

@nickvergessen when you see this on Monday, before you get too excited and revert immediately, I will be sending out a mailing list email to clarify the updated requirements, as soon as we find an appropriate solution with @oparoz

@oparoz
Contributor
oparoz commented Sep 6, 2015

I can confirm that loading the app in the boostrap still does nothing. I suspect a bug in the loader, unless someone can show me an app which loads (1. load the app 2. check the navigation menu).

But adding this to the boostrap fixes the issue: OC::$loader->addValidRoot('../');

It just doesn't make sense to have to whitelist the app being tested, but it's better than hundreds of failing tests ;).

@oparoz
Contributor
oparoz commented Sep 6, 2015

Apart from the problems, I do applaud the move to protect the system from dormant apps 👏

@Xenopathic
Member

@oparoz Well, for unit tests that line will work just fine, but you realise it doesn't work as expected? All paths are relative to the OC::$SERVERROOT, so you basically just disabled the autoloader verification for everything 😆

@oparoz
Contributor
oparoz commented Sep 6, 2015

Arf, yes... Missing __DIR__

@oparoz
Contributor
oparoz commented Sep 7, 2015

LoadApp() works, but there is a problem with loadApps() #18863

@nickvergessen
Contributor

@nickvergessen when you see this on Monday, before you get too excited and revert immediately, I will be sending out a mailing list email to clarify the updated requirements, as soon as we find an appropriate solution with @oparoz

Sorry @Xenopathic but this is 💩
Leaving developers with a known broken system. Please notify people in advance. This really makes developing an app a pain. Third breakage in 5 days on the same part. Please take more time on your fixes and test them, before merging them. All apps are publicly available on github. Check them out and run their unit tests locally, so you see what's the problem and stop messing it up.

So the now required code for the bootstrap.php is:

\OC::$loader->addValidRoot(OC::$SERVERROOT . '/tests');
\OC_App::loadApp('<app name here>');
@nickvergessen nickvergessen referenced this pull request in owncloud/activity Sep 7, 2015
Merged

Fix unit tests partly #386

@Xenopathic
Member

@nickvergessen Actually, I was wondering why we only load a minimal set of apps in bootstrap.php: https://github.com/owncloud/core/blob/master/tests/bootstrap.php#L14-L15. We should be loading all enabled apps, which would prevent these issues?

@Xenopathic
Member

OK, so the consensus is that we should explicitly require that the app (and any dependencies of that app) are loaded with OC_App::loadApp() before unit tests are run?

@oparoz
Contributor
oparoz commented Sep 7, 2015

@Xenopathic - The boostrap still works when I remove \OC::$loader->addValidRoot(OC::$SERVERROOT . '/tests');. Is that intended?

@georgehrke georgehrke referenced this pull request in owncloud/calendar Sep 7, 2015
Merged

finish object cache and webcal implementation #24

@MorrisJobke
Member

@Xenopathic - The boostrap still works when I remove \OC::$loader->addValidRoot(OC::$SERVERROOT . '/tests');. Is that intended?

Yep. The root is added on appLoad()

@oparoz
Contributor
oparoz commented Sep 7, 2015

@MorrisJobke - OK, there may be a bug then:
owncloudarchive/mail#1058 (comment)

@MorrisJobke MorrisJobke added the 8.1.2 label Sep 9, 2015
@oparoz
Contributor
oparoz commented Sep 9, 2015

@MorrisJobke - In Gallery, addValidRoot is also still required, even with loadApps();
Keep that in mind for the dev documentation.

@MorrisJobke MorrisJobke added a commit that referenced this pull request Apr 11, 2016
@MorrisJobke MorrisJobke Catch the AutoloadNotAllowedException also for legacy jobs
* same as #18839 for legacy jobs
* avoids spamming the log with useless entries
420fe0b
@DeepDiver1975 DeepDiver1975 added a commit that referenced this pull request Apr 11, 2016
@MorrisJobke @DeepDiver1975 MorrisJobke + DeepDiver1975 Catch the AutoloadNotAllowedException also for legacy jobs (#23901)
* same as #18839 for legacy jobs
* avoids spamming the log with useless entries
ddbb9b7
@MorrisJobke MorrisJobke added a commit that referenced this pull request Apr 12, 2016
@MorrisJobke MorrisJobke Catch the AutoloadNotAllowedException also for legacy jobs
* same as #18839 for legacy jobs
* avoids spamming the log with useless entries
aecfcf6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment