-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix SCSS usage in apps #3619
Fix SCSS usage in apps #3619
Conversation
MorrisJobke
commented
Feb 25, 2017
- fix the web root detection of the ResourceLocator for apps
@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @bartv2 and @butonic to be potential reviewers. |
Codecov Report
@@ Coverage Diff @@
## master #3619 +/- ##
===========================================
- Coverage 54.31% 54.3% -0.01%
- Complexity 20926 20930 +4
===========================================
Files 1297 1297
Lines 79828 79844 +16
Branches 1253 1253
===========================================
+ Hits 43358 43362 +4
- Misses 36470 36482 +12
Continue to review full report at Codecov.
|
Seems to fix the loading of the compiled css file, but the log shows this after loading the Tasks app:
However, the call to e.g. Also the images referenced with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for the errors in the log seems to be that line
throw new ResourceNotFoundException($file, $webRoot); |
The images not loaded seems to be a separate issue: #3606 (comment)
* fix the web root detection of the ResourceLocator for apps Signed-off-by: Morris Jobke <[email protected]>
resource list Signed-off-by: Roeland Jago Douma <[email protected]>
Signed-off-by: Roeland Jago Douma <[email protected]>
cf3e2eb
to
f86b5c2
Compare
Ok I think (tm) this works now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works on my testing instance now. The css file is generated, the file is included and loads correctly and also the paths to icon files are calculated correctly now (although url('../../../apps/tasks/css/../img/sprites.svg')
could be shortened to url('../../../apps/tasks/img/sprites.svg')
.
@raimund-schluessler yes, but it's easier to just add a piece of string than splitting it, popping an entry and remerging it again :) |
@skjnldsv |
@skjnldsv Would you have time to review if the PR is working for you?
I think we should merge the PR if it fixes the issue. Regarding the security implications I can't say anything. However, I would still merge this PR since the paths are already created like this in server. In case the security impact is real I would prefer fixing this in a separate PR. Also @LukasReschke. |
I don't see any security issue here, since those path traversals are only used in CSS url statements and will be resolved by your webserver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't test right now, but seems good to me :)
I tested it and it works 👍 |
@rullzer @MorrisJobke : I did several tests. I got an exception thrown under the following condition: Edit: Found the problem, maybe it is of interest: I´ve had the folder "/var/www/" symlinked to an external sd, that is encrypted and rather slow. The problem was that no folder icons were drawn and the exception (see below) occured. Now i have moved the "/var/www/nextcloud" back to the internal parititon and everything is fine. Log was: |