New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 27, 2017

Conversation

Projects
None yet
9 participants
@kyrofa
Member

kyrofa commented Nov 23, 2017

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.

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

CSSResourceLocator: handle SCSS in apps outside root
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 #5289

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

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 23, 2017

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%> (ø) ⬇️

codecov bot commented Nov 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Nov 23, 2017

Member

(The failed test does not seem related.)

Member

kyrofa commented Nov 23, 2017

(The failed test does not seem related.)

@kyrofa kyrofa requested review from MorrisJobke and skjnldsv Nov 23, 2017

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Nov 23, 2017

Member

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

Member

MorrisJobke commented Nov 23, 2017

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

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Nov 23, 2017

Member

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

Member

karlitschek commented Nov 23, 2017

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

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Nov 23, 2017

Member

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.

Member

nickvergessen commented Nov 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@skjnldsv

skjnldsv Nov 23, 2017

Member

@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.

Member

skjnldsv commented Nov 23, 2017

@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 skjnldsv referenced this pull request Nov 23, 2017

Closed

Another display issue #442

@shtrom shtrom referenced this pull request Nov 23, 2017

Closed

Display issue #346

@shtrom

This comment has been minimized.

Show comment
Hide comment
@shtrom

shtrom Nov 23, 2017

Contributor

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

Contributor

shtrom commented Nov 23, 2017

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

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Nov 23, 2017

Member

Failing test is unrelated

Member

MorrisJobke commented Nov 23, 2017

Failing test is unrelated

@MorrisJobke MorrisJobke referenced this pull request Nov 23, 2017

Merged

Prepare 12.0.4 RC2 #7261

4 of 5 tasks complete
@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Nov 23, 2017

Member

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?

Member

nickvergessen commented Nov 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@skjnldsv

skjnldsv Nov 23, 2017

Member

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

Member

skjnldsv commented Nov 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Nov 23, 2017

Member

@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?

Member

kyrofa commented Nov 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@skjnldsv

skjnldsv Nov 23, 2017

Member

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

Member

skjnldsv commented Nov 23, 2017

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

@kyrofa

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Nov 23, 2017

Member

@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 😛 .

Member

kyrofa commented Nov 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@skjnldsv

skjnldsv Nov 23, 2017

Member

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! ;)

Member

skjnldsv commented Nov 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Nov 23, 2017

Member

@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 😃 .

Member

kyrofa commented Nov 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Nov 23, 2017

Member

To clarify, does this look okay, then?

Member

kyrofa commented Nov 23, 2017

To clarify, does this look okay, then?

@shtrom

This comment has been minimized.

Show comment
Hide comment
@shtrom

shtrom Nov 23, 2017

Contributor

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.

Contributor

shtrom commented Nov 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Nov 23, 2017

Member

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

Member

kyrofa commented Nov 23, 2017

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

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

1 check failed

continuous-integration/drone/pr the build failed
Details
@tYYGH

This comment has been minimized.

Show comment
Hide comment
@tYYGH

tYYGH Nov 27, 2017

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).

tYYGH commented Nov 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@shtrom

shtrom Nov 28, 2017

Contributor
Contributor

shtrom commented Nov 28, 2017

@mfr-itr

This comment has been minimized.

Show comment
Hide comment
@mfr-itr

mfr-itr Nov 28, 2017

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

mfr-itr commented Nov 28, 2017

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

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Nov 28, 2017

Member

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

Thursday is the planned one.

Member

MorrisJobke commented Nov 28, 2017

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

Thursday is the planned one.

@mfr-itr

This comment has been minimized.

Show comment
Hide comment
@mfr-itr

mfr-itr commented Nov 28, 2017

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment