{J,CS}SResourceLocator: account for symlinks in app path #7061

kyrofa · 2017-11-04T06:50:19Z

Currently, if the app path includes a symlink, the calculated webDir will be incorrect when generating CSS and URLs will be pointing to the wrong place, breaking CSS.

This PR fixes #6028 by using realpath when retrieving app path, which makes these issues go away.

Currently, if the app path includes a symlink, the calculated webDir will be incorrect when generating CSS and URLs will be pointing to the wrong place, breaking CSS. Use realpath when retrieving app path, and these issues go away. Fix nextcloud#6028 Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa · 2017-11-04T06:53:49Z

I'd love to cover this in a test if anyone can point me in the right direction.

codecov · 2017-11-04T07:28:34Z

Codecov Report

Merging #7061 into master will increase coverage by 0.02%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master    #7061      +/-   ##
============================================
+ Coverage     50.73%   50.76%   +0.02%     
  Complexity    24412    24412              
============================================
  Files          1579     1579              
  Lines         93321    93323       +2     
  Branches       1359     1359              
============================================
+ Hits          47348    47375      +27     
+ Misses        45973    45948      -25

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Template/CSSResourceLocator.php	`0% <0%> (ø)`	`26 <0> (ø)`	⬇️
lib/private/Template/JSResourceLocator.php	`50% <100%> (+50%)`	`22 <0> (ø)`	⬇️
apps/files_trashbin/lib/Trashbin.php	`72.28% <0%> (-0.25%)`	`136% <0%> (ø)`
lib/private/Template/ResourceLocator.php	`69.35% <0%> (+1.61%)`	`25% <0%> (ø)`	⬇️

MorrisJobke · 2017-11-07T09:47:12Z

I'd love to cover this in a test if anyone can point me in the right direction.

cc @skjnldsv @juliushaertl

MorrisJobke · 2017-11-07T09:47:33Z

I guess we need the same for the JSCombiner?

skjnldsv · 2017-11-07T10:05:49Z

Is this related to #5289?

juliusknorr · 2017-11-07T11:21:05Z

I'd love to cover this in a test if anyone can point me in the right direction.

I guess you could write a unit test for that with creating a symlink using http://php.net/manual/en/function.symlink.php. It seems that the CSSResourceLocator class is not unit-tested at all: https://github.com/nextcloud/server/tree/master/tests/lib/Template

kyrofa · 2017-11-07T18:00:05Z

Is this related to #5289?

I'm not sure. It's certainly possible.

kyrofa · 2017-11-07T22:53:59Z

@MorrisJobke I'm assuming you're talking about the JSResourceLocator? Assuming so, I've pushed that change here as well.

@juliushaertl I've also pushed tests for both JSResourceLocator as well as CSSResourceLocator that cover the functionality introduced here. I assume they're run automatically in CI, right? Or do I need to alter another file somewhere?

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

MorrisJobke · 2017-11-08T09:41:53Z

@juliushaertl I've also pushed tests for both JSResourceLocator as well as CSSResourceLocator that cover the functionality introduced here. I assume they're run automatically in CI, right? Or do I need to alter another file somewhere?

They run automatically (if they are located in a tests/ folder), but cause a Fatal error:

PHP Fatal error:  Cannot redeclare Test\Template\randomString() (previously declared in /drone/src/github.com/nextcloud/server/tests/lib/Template/CSSResourceLocatorTest.php:47) in /drone/src/github.com/nextcloud/server/tests/lib/Template/JSResourceLocatorTest.php on line 46

This seems to be the only way to have the same helpers used between tests in a manner that works for both standalone phpunit and autotest.sh. Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa · 2017-11-08T21:16:31Z

They run automatically (if they are located in a tests/ folder), but cause a Fatal error:

Thanks @MorrisJobke, all fixed up and ready for review.

juliusknorr · 2017-11-09T10:01:17Z

@kyrofa Do you have some steps to reproduce the issue/test the pr? Symlinking the apps folder doesn't cause the issue on my setup.

kyrofa · 2017-11-09T23:15:58Z

Sure thing @juliushaertl. Note that while the fix is done in the ResourceLocators, the bug doesn't bite until CSS is generated (which is then cached). The problem isn't quite as simple as "app paths with symlinks don't work", the app path in question actually needs to be within a webroot that is a symlink, so let's use the default apps as an example.

Get a completely clean, not-installed Nextcloud
The webroot needs to be a symlink, so instead of /var/www/nextcloud, put Nextcloud in /var/www/actual-nextcloud.
Now create /var/www/nextcloud as a symlink pointing to /var/www/actual-nextcloud (webserver should be pointing to the symlink).
Before installing, add an apps_paths item pointing to the default apps, but use the symlink path for it, not the real one, e.g.:

<?php
$CONFIG = array (
  'instanceid' => 'oc4ugkbj34v5',
  'apps_paths' => array(
        array(
                # Note the use of nextcloud instead of actual-nextcloud
                'path'=> '/var/www/nextcloud/apps',
                'url' => '/apps',
                'writable' => true,
        ),
  ),
);

Now you've set things up such that:

The CSS has not yet been generated since Nextcloud is not installed
The webroot, which is a symlink, will be resolved into /var/www/actual-nextcloud
The app path uses the symlink, but in master, doesn't get resolved (that's what this PR fixes)
When the CSS is generated, that webroot will be used to rewrite URLs for the apps, but the app path will still be /var/www/nextcloud/apps/foo and the rewrite will go sideways, creating invalid URLs to assets, thus bad CSS.

Go ahead and install Nextcloud now, and you'll see things like this:

juliusknorr · 2017-11-12T14:47:22Z

lib/private/Template/JSResourceLocator.php

+		// Account for the possibility of having symlinks in app path. Doing
+		// this in a separate variable, because an empty argument to realpath
+		// gets turned into cwd, which makes it hard to see if app_path got set.
+		$real_app_path = realpath($app_path);


Shouldn't we also do this after the check if the app_path is empty like it is done in in the CSSResourceLocator? Otherwise $real_app_path still might be the working directory for empty app paths.

Yeah the diff is hiding that from you, expand is a little below this. It's done here. I believe it's done after looking for l10n because those should be able to not exist without raising errors.

Although this will cause weird behavior with using the cwd for l10n, huh. I refactored it just a tad to account for that, and pushed it up. It means we need to check twice, but I don't see a straight-forward way around that.

Can't you just move the check got app_path and app_url in front of the realpath call? That way we don't append any js sources at cwd.

Sure, but not without changing the l10n behavior. I don't claim to be an expert in the code, of course, but right now it seems that if a l10n file doesn't exist, the error is ignored. If we move the check above that logic, we start raising an error instead. Are we wanting that change?

Ah, ok, makes sense to keep it like this i guess. 👍

juliusknorr

Thanks @kyrofa I could reproduce it and your fix works great for that.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

MorrisJobke · 2017-11-13T06:39:26Z

The failed unit test is unrelated.

kyrofa · 2017-11-14T15:38:30Z

Can we get this back into stable12 as well? It's the last remaining issue for the snap.

MorrisJobke · 2017-11-14T16:43:32Z

Can we get this back into stable12 as well? It's the last remaining issue for the snap.

Could you open a backport to the stable12 branch as PR?

kyrofa · 2017-11-14T17:36:55Z

Could you open a backport to the stable12 branch as PR?

Sure thing! #7170

This was referenced Nov 4, 2017

SCSS cacher: use realpath on path #7060

Closed

Update to 12.0 nextcloud-snap/nextcloud-snap#280

Closed

kyrofa force-pushed the bugfix/6028/app_path_realpath branch from 8a2a077 to b0d2966 Compare November 4, 2017 06:53

MorrisJobke added the 3. to review Waiting for reviews label Nov 7, 2017

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 7, 2017

JSResourceLocator: account for symlinks in app path

06ba1a8

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa force-pushed the bugfix/6028/app_path_realpath branch from 8e3e053 to 06ba1a8 Compare November 7, 2017 22:54

kyrofa force-pushed the bugfix/6028/app_path_realpath branch 3 times, most recently from ab14ffd to 5a69f9b Compare November 8, 2017 19:45

skjnldsv mentioned this pull request Nov 8, 2017

NC12: apps outside of nextcloud's root directory result in "ResourceLocator can not find a web root" #5289

Closed

tests: change helper scope

9c24b7b

This seems to be the only way to have the same helpers used between tests in a manner that works for both standalone phpunit and autotest.sh. Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa force-pushed the bugfix/6028/app_path_realpath branch from 5a69f9b to 9c24b7b Compare November 8, 2017 20:45

kyrofa added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 8, 2017

MorrisJobke requested review from rullzer, nickvergessen and LukasReschke November 8, 2017 22:38

MorrisJobke requested review from juliusknorr and skjnldsv November 8, 2017 22:38

MorrisJobke added this to the Nextcloud 13 milestone Nov 8, 2017

kyrofa changed the title ~~CSSResourceLocator: account for symlinks in app path~~ {J,CS}SResourceLocator: account for symlinks in app path Nov 8, 2017

juliusknorr requested changes Nov 12, 2017

View reviewed changes

JSResourceLocator: re-use $app_path

d2a2793

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

juliusknorr approved these changes Nov 14, 2017

View reviewed changes

MorrisJobke approved these changes Nov 14, 2017

View reviewed changes

MorrisJobke merged commit ab0a3fb into nextcloud:master Nov 14, 2017

MorrisJobke added the backport-request label Nov 14, 2017

kyrofa mentioned this pull request Nov 14, 2017

[stable12] {J,CS}SResourceLocator: account for symlinks in app path #7170

Merged

kyrofa deleted the bugfix/6028/app_path_realpath branch November 14, 2017 17:36

MorrisJobke removed the backport-request label Nov 14, 2017

MorrisJobke mentioned this pull request Nov 20, 2017

Upgrading to 12.0.3RC2 breaks header bar #6542

Closed

kyrofa mentioned this pull request Mar 14, 2019

ResourceLocator can not find a web root on several extensions nextcloud-snap/nextcloud-snap#902

Closed

the-sane mentioned this pull request Aug 5, 2019

[regression] symlinked apps and CSS - ResourceLocator can not find a web root #13556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

{J,CS}SResourceLocator: account for symlinks in app path #7061

{J,CS}SResourceLocator: account for symlinks in app path #7061

kyrofa commented Nov 4, 2017 •

edited

Loading

kyrofa commented Nov 4, 2017

codecov bot commented Nov 4, 2017 •

edited

Loading

MorrisJobke commented Nov 7, 2017

MorrisJobke commented Nov 7, 2017

skjnldsv commented Nov 7, 2017

juliusknorr commented Nov 7, 2017

kyrofa commented Nov 7, 2017

kyrofa commented Nov 7, 2017 •

edited

Loading

MorrisJobke commented Nov 8, 2017

kyrofa commented Nov 8, 2017

juliusknorr commented Nov 9, 2017

kyrofa commented Nov 9, 2017 •

edited

Loading

juliusknorr Nov 12, 2017

kyrofa Nov 12, 2017 •

edited

Loading

kyrofa Nov 12, 2017 •

edited

Loading

juliusknorr Nov 13, 2017

kyrofa Nov 13, 2017 •

edited

Loading

juliusknorr Nov 14, 2017

juliusknorr left a comment

MorrisJobke commented Nov 13, 2017

kyrofa commented Nov 14, 2017

MorrisJobke commented Nov 14, 2017

kyrofa commented Nov 14, 2017

{J,CS}SResourceLocator: account for symlinks in app path #7061

{J,CS}SResourceLocator: account for symlinks in app path #7061

Conversation

kyrofa commented Nov 4, 2017 • edited Loading

kyrofa commented Nov 4, 2017

codecov bot commented Nov 4, 2017 • edited Loading

Codecov Report

MorrisJobke commented Nov 7, 2017

MorrisJobke commented Nov 7, 2017

skjnldsv commented Nov 7, 2017

juliusknorr commented Nov 7, 2017

kyrofa commented Nov 7, 2017

kyrofa commented Nov 7, 2017 • edited Loading

MorrisJobke commented Nov 8, 2017

kyrofa commented Nov 8, 2017

juliusknorr commented Nov 9, 2017

kyrofa commented Nov 9, 2017 • edited Loading

juliusknorr Nov 12, 2017

Choose a reason for hiding this comment

kyrofa Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

kyrofa Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

juliusknorr Nov 13, 2017

Choose a reason for hiding this comment

kyrofa Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

juliusknorr Nov 14, 2017

Choose a reason for hiding this comment

juliusknorr left a comment

Choose a reason for hiding this comment

MorrisJobke commented Nov 13, 2017

kyrofa commented Nov 14, 2017

MorrisJobke commented Nov 14, 2017

kyrofa commented Nov 14, 2017

kyrofa commented Nov 4, 2017 •

edited

Loading

codecov bot commented Nov 4, 2017 •

edited

Loading

kyrofa commented Nov 7, 2017 •

edited

Loading

kyrofa commented Nov 9, 2017 •

edited

Loading

kyrofa Nov 12, 2017 •

edited

Loading

kyrofa Nov 12, 2017 •

edited

Loading

kyrofa Nov 13, 2017 •

edited

Loading