[stable12] CSSResourceLocator: handle SCSS in apps outside root #7257

kyrofa · 2017-11-23T05:37:51Z

Currently static CSS files work fine in apps outside of the root. However, as soon as an app uses SCSS, Nextcloud starts being unable to find the web root.

This PR fixes #5289 by backporting select snippets from master specifically targeting this issue (note that master does not have this issue), and adding a test to ensure it doesn't regress.

Currently static CSS files work fine in apps outside of the root. However, as soon as an app uses SCSS, Nextcloud starts being unable to find the web root. Fix this problem by backporting select snippets from master specifically targeting this issue, and add a test to ensure it doesn't regress. Fix nextcloud#5289 Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

codecov · 2017-11-23T06:27:24Z

Codecov Report

Merging #7257 into stable12 will increase coverage by 0.06%.
The diff coverage is 71.05%.

@@              Coverage Diff               @@
##             stable12    #7257      +/-   ##
==============================================
+ Coverage       53.79%   53.86%   +0.06%     
- Complexity      22590    22594       +4     
==============================================
  Files            1384     1384              
  Lines           86666    86667       +1     
  Branches         1329     1329              
==============================================
+ Hits            46620    46681      +61     
+ Misses          40046    39986      -60

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Template/ResourceLocator.php	`77.41% <64%> (+10.75%)`	`25 <7> (+5)`	⬆️
lib/private/Template/CSSResourceLocator.php	`72.13% <84.61%> (+72.13%)`	`26 <0> (-1)`	⬇️
core/js/js.js	`61.83% <0%> (+0.55%)`	`0% <0%> (ø)`	⬇️

kyrofa · 2017-11-23T06:48:00Z

(The failed test does not seem related.)

MorrisJobke · 2017-11-23T09:13:18Z

@rullzer @nickvergessen @LukasReschke @karlitschek Opinions on getting this into 12.0.4 and building an RC2?

karlitschek · 2017-11-23T09:18:26Z

I can't judge the importance and the risk. Your call @MorrisJobke :-)

nickvergessen · 2017-11-23T09:27:34Z

Well if we don't do this, apps should not be allowed to use SCSS at all, because the app dev can not know where their app is installed to.

And I think it looks easy enough, so if it works why not.

skjnldsv · 2017-11-23T11:00:23Z

@kyrofa We definitely should put an advertisement if the scss file is unreachable though. Because without alias for apache or root change for nginx, the webserver can't serve a file outside of its allowed declared roots. It could lead to confusion for the user.

shtrom · 2017-11-23T11:48:00Z

This PR fixed the issues in two apps with 12.0.0. Cheers! 👍

MorrisJobke · 2017-11-23T11:55:12Z

Failing test is unrelated

nickvergessen · 2017-11-23T13:49:12Z

Doesn't seem to work here:
I have my nextcloud in: nextcloud12r.local/server
Contacts app in: nextcloud12r.local/noapps/contacts

And the path to the css is generated as nextcloud12r.local/index.php/css/contacts/....

The problem is that findWebRoot returns null.

Or which paths were we trying to fix here?

skjnldsv · 2017-11-23T13:52:32Z

@nickvergessen path outside your default root :)
The css path you provided seems good to me since the scss are located within the data folder.

kyrofa · 2017-11-23T15:16:36Z

@kyrofa We definitely should put an advertisement if the scss file is unreachable though. Because without alias for apache or root change for nginx, the webserver can't serve a file outside of its allowed declared roots. It could lead to confusion for the user.

@skjnldsv I'm not quite clear on what you're saying here. The SCSS file isn't served up by Apache or Nginx, it's compiled and then cached into the data dir as CSS, then served by Nextcloud itself (which is why paths look like domain/index.php/css/...). Do you mean you'd like to see a warning here? That line is currently hit for CSS files (i.e. we check for SCSS first), so we'd need to rewrite this a bit more. Can you think of a way to do what you want without changing the logic here?

skjnldsv · 2017-11-23T15:26:57Z

@kyrofa, well, without alias this wont' work. Nginx have to serve the correct files. This is not only related to scss. :)

kyrofa · 2017-11-23T15:54:08Z

@skjnldsv unless I misunderstand something (always possible!), SCSS files should work fine without an alias, it's CSS files (and other static assets in the app) that won't. Here are a few URLs in the head using the Calendar app (which uses static CSS):

<link rel="icon" href="/extra-apps/calendar/img/favicon.ico">
<link rel="apple-touch-icon-precomposed" href="/extra-apps/calendar/img/favicon-touch.png">
<!-- ... -->
<link rel="stylesheet" href="/extra-apps/calendar/css/public/vendor.min.css?v=1135315055fd35753215151fd0fb3652-0">

All of those require an alias. However, take a look at a CSS URL for SCSS (using the Contacts app):

<link rel="stylesheet" href="/index.php/css/contacts/c5a4464a656e8aa73f8111e50f6b8d5b-style.css?v=1135315055fd35753215151fd0fb3652-0">

Note how compiled SCSS, being in the data dir, is not served as a static asset using the webserver, but as a dynamic one using PHP. This particular case, as far as I can see, does not involve the alias. That's not to say it's not important, of course, or all the other stuff would break 😛 .

skjnldsv · 2017-11-23T17:32:23Z

Okay, I mixed up two different issues. This one is only about the scss fetch from the scss lib. :)

Note how compiled SCSS, being in the data dir, is not served as a static asset using the webserver, but as a dynamic one using PHP.

@kyrofa I built the scss integration in nextcloud! ;)

kyrofa · 2017-11-23T18:02:08Z

@kyrofa I built the scss integration in nextcloud! ;)

No offense intended my friend! I simply wasn't sure what you were talking about, so I tried to be as explicit as possible 😃 .

kyrofa · 2017-11-23T18:23:48Z

To clarify, does this look okay, then?

shtrom · 2017-11-23T23:02:45Z

Though it works for me, I'm wondering whether it wouldn't make more sense to proxy any asset through the standard /app/BLAH URI, maybe doing something like nginx's try_files, looking for an asset file to serve and defaulting to routing to controllers.

This would avoid having to rewrite URLs or fiddle with the server's configuration.

kyrofa · 2017-11-23T23:06:35Z

That's well beyond what master does today. I suspect that's probably not suitable for a backport.

rullzer

Seems to do the trick for me

tYYGH · 2017-11-27T10:52:52Z

Thank you! This will improve my self-hosted Nextcloud a lot!

@shtrom Do not forget the case when Nginx does not have access to the files: try_files is useless in such situations.
In my case, Nginx is in the DMZ and has no access to any personal data. uwsgi is actually hosting Nextcloud, on the backend server (obviously with access to the data).

shtrom · 2017-11-28T03:21:27Z

On Mon 27 Nov 2017 at 10:53:01 +0000, tYYGH wrote: @shtrom Do not forget the case when Nginx does not have access to the files: `try_files` is useless in such situations.

Oh, yeah, I was just taking it as an example of behaviour, but it would need to be reimplemented in a server-agnostic way.

…

-- Olivier Mehani <shtrom@ssji.net> PGP fingerprint: 4435 CF6A 7C8D DD9B E2DE F5F9 F012 A6E2 98C6 6655 Confidentiality cannot be guaranteed on emails sent or received unencrypted.

mfr-itr · 2017-11-28T08:39:01Z

Do you have an ETA for the release or should I apply the patch manually?

MorrisJobke · 2017-11-28T09:42:23Z

Do you have an ETA for the release or should I apply the patch manually?

Thursday is the planned one.

mfr-itr · 2017-11-28T09:44:10Z

Nice!

kyrofa added this to the Nextcloud 12.0.4 milestone Nov 23, 2017

kyrofa force-pushed the bugfix/5289/apps_outside_webroot branch from d8e37ea to 7a143c0 Compare November 23, 2017 05:39

kyrofa force-pushed the bugfix/5289/apps_outside_webroot branch from 7a143c0 to 7f8f3dc Compare November 23, 2017 05:41

This was referenced Nov 23, 2017

Polls app takes for granted that it lives under /apps nextcloud/polls#249

Closed

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

Closed

kyrofa requested review from MorrisJobke and skjnldsv November 23, 2017 06:48

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

MorrisJobke requested review from rullzer and nickvergessen November 23, 2017 09:29

splitt3r mentioned this pull request Nov 23, 2017

Switchin from scss to css. nextcloud/polls#250

Closed

skjnldsv mentioned this pull request Nov 23, 2017

Another display issue nextcloud/contacts#442

Closed

shtrom mentioned this pull request Nov 23, 2017

Display issue nextcloud/deck#346

Closed

MorrisJobke approved these changes Nov 23, 2017

View reviewed changes

MorrisJobke mentioned this pull request Nov 23, 2017

Prepare 12.0.4 RC2 #7261

Merged

5 tasks

rullzer approved these changes Nov 27, 2017

View reviewed changes

rullzer merged commit 7e1ca61 into nextcloud:stable12 Nov 27, 2017

MorrisJobke mentioned this pull request Nov 28, 2017

Update to 12.0.4 RC3 fails on web #7331

Closed

tcitworld mentioned this pull request Feb 9, 2018

App broken on Nextcloud 12.0.x nextcloud/tasks#145

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

[stable12] CSSResourceLocator: handle SCSS in apps outside root #7257

[stable12] CSSResourceLocator: handle SCSS in apps outside root #7257

kyrofa commented Nov 23, 2017

codecov bot commented Nov 23, 2017 •

edited

Loading

kyrofa commented Nov 23, 2017

MorrisJobke commented Nov 23, 2017

karlitschek commented Nov 23, 2017

nickvergessen commented Nov 23, 2017

skjnldsv commented Nov 23, 2017

shtrom commented Nov 23, 2017

MorrisJobke commented Nov 23, 2017

nickvergessen commented Nov 23, 2017

skjnldsv commented Nov 23, 2017

kyrofa commented Nov 23, 2017 •

edited

Loading

skjnldsv commented Nov 23, 2017

kyrofa commented Nov 23, 2017 •

edited

Loading

skjnldsv commented Nov 23, 2017

kyrofa commented Nov 23, 2017

kyrofa commented Nov 23, 2017

shtrom commented Nov 23, 2017

kyrofa commented Nov 23, 2017

rullzer left a comment

tYYGH commented Nov 27, 2017

shtrom commented Nov 28, 2017 via email

mfr-itr commented Nov 28, 2017

MorrisJobke commented Nov 28, 2017

mfr-itr commented Nov 28, 2017

[stable12] CSSResourceLocator: handle SCSS in apps outside root #7257

[stable12] CSSResourceLocator: handle SCSS in apps outside root #7257

Conversation

kyrofa commented Nov 23, 2017

codecov bot commented Nov 23, 2017 • edited Loading

Codecov Report

kyrofa commented Nov 23, 2017

MorrisJobke commented Nov 23, 2017

karlitschek commented Nov 23, 2017

nickvergessen commented Nov 23, 2017

skjnldsv commented Nov 23, 2017

shtrom commented Nov 23, 2017

MorrisJobke commented Nov 23, 2017

nickvergessen commented Nov 23, 2017

skjnldsv commented Nov 23, 2017

kyrofa commented Nov 23, 2017 • edited Loading

skjnldsv commented Nov 23, 2017

kyrofa commented Nov 23, 2017 • edited Loading

skjnldsv commented Nov 23, 2017

kyrofa commented Nov 23, 2017

kyrofa commented Nov 23, 2017

shtrom commented Nov 23, 2017

kyrofa commented Nov 23, 2017

rullzer left a comment

Choose a reason for hiding this comment

tYYGH commented Nov 27, 2017

shtrom commented Nov 28, 2017 via email

mfr-itr commented Nov 28, 2017

MorrisJobke commented Nov 28, 2017

mfr-itr commented Nov 28, 2017

codecov bot commented Nov 23, 2017 •

edited

Loading

kyrofa commented Nov 23, 2017 •

edited

Loading

kyrofa commented Nov 23, 2017 •

edited

Loading