Webpacker: url() Path Resolution in SASS/SCSS/CSS Stylesheets Exhaustively Searches Entire Project Root

[mvastola]

So this is potentially actually a security concern and I actually first brought it up to the Rails Security Team, but it was decided to move this over to a GitHub issue as the impact is fairly low and it's unclear how best to address this issue.

The Issue

In short, if you were to create a new rails app with rails new . --webpack and create app/javascript/application.scss with:

body {
  background: url('images/pretty-picture.jpg');
}

..and replaced app/javascript/packs/application.js so it contained only:

import '../application.scss'

..and placed an image named pretty-picture.jpg at say private/images/pretty-picture.jpg (relative to the project root), and then ran rake webpacker:compile, webpack(er) would grab the image from what should ostensibly be a non-public directory and place it in public/packs.

The 'Why'

This has obvious implications in that normally inaccessible content (or content ordinarily behind by an access control mechanism within the app -- such as attachments to models) could be accidentally made public if there should be a file naming conflict.

Additionally, this is also not expected behavior as:

• only the directories specified by webpacker.yml's source_path and resolved_paths directives should be searched for files
• the css url() directive (in the absence of an absolute uri) is supposed to be taken as relative to either the scss/sass file itself, or the target css file.

The Cause

This concerning path resolution is actually due to the merging of PR #387 (resolving issue #384), which added the package resolve-url-loader, for which this exhaustive path searching is actually intended behavior. (Which is what makes the solution here less than straightforward.)

Per resolve-url-loader's README.md:

Usually the asset is found relative to the original source file O(1). However in some cases there is no immediate match (cough bootstrap cough) and we so we start searching both deeper and shallower from the starting directory O(n).

The Solution

There is no option to turn off this 'feature' within resolve-url-loader.

The README.md for sass-loader provides additional background on why resolve-url-loader is needed.

Currently, my thinking is that the best way to address this is to offer a PR to resolve-url-loader to turn off it's exhaustive search feature (which is theoretically a minor change), since per the sass-loader README.md the bootstrap issue is fixed by using bootstrap-sass for the time being. This is a somewhat breaking change though, so it definitely warrants discussion.

Thoughts, suggestions, hypotheses, etc, welcome.

Apologies for the long post and all the context, but I thought would be useful.

[mvastola]

Oh, and a huge thanks to @jeremy for helping to investigate this issue!

[jeremy]

/cc @guilleiguaran @gauravtiwari

[gauravtiwari]

Lets configure root option so it always sticks to source_path:
https://github.com/bholloway/resolve-url-loader#options

root An optional directory within which search may be performed. Relative paths are permitted. Where omitted process.cwd() is used and should be sufficient for most use cases.

Could you please add this code to your environment and see if it fixes the problem?

const { environment } = require('@rails/webpacker')
const config = require('@rails/webpacker/package/config');
const merge = require('webpack-merge')

const resolveLoaderOptions = {
  root: config.source_path
}

const styleLoader = environment.loaders.get('style')
const resolveLoader = styleLoader.use.find(el => el.loader === 'resolve-url-loader')
resolveLoader.options = resolveLoaderOptions

module.exports = environment

[gauravtiwari]

BTW, I get this when I try to link something outside of the app/javascript:

[mvastola]

Will try. I thought of that fix, but theoretically any resolved_paths need to be searched as well to keep things consistent and there's no option for multiple roots. :-/

And when you repro don't use a ./. It should be impled.

[gauravtiwari]

Is private/images under resolved_paths in your setup?

[mvastola]

No.

[mvastola]

If you are having issues reproducing, see https://github.com/mvastola/webpacker-issue-932.

[gauravtiwari]

Great, thanks 👍 will take a look

[gauravtiwari]

@mvastola Thanks for the example, I can reproduce it. Yes, the simple fix would be to make a PR on resolve-url-loader to turn off exhaustive search and perhaps make root option to use resolve/modules (resolved_paths as we call it) option from webpack itself so it only searches in that context and not outside of it.

What do you think?

[mvastola]

Agreed on the first part.

For the latter part, I'm not sure I exactly understand. If the exhaustive search option is off, does root even apply?

If you're suggesting adding a check on relative paths to make sure they don't extend outside source_path and/or resolved_paths, I think we need to be careful about that. Though I agree there might be security merit in preventing a malicious stylesheet from, say, using url() to traverse up the directory structure outside of its resolved_path (or potentially even outside of the project directory), as a rule we generally don't limit the abilities of external libraries out of theoretical security concerns, leaving it up to coders to exercise discretion before running unknown code.

Just as importantly, I imagine quite a few people have this sort of setup:
In app/javascript/application.scss

body {
  background: url('../assets/images/pretty-picture.jpg');
}

.. which (I believe) currently works even without app/assets being in resolved_paths.

[mvastola]

To clarify, by all that I mean to say I think it's safe if exhaustive searching is just turned off and URLs only resolve relative to either an SCSS/SASS stylesheet or the target CSS file (if applicable).

For instance, if I have a CSS file at /foo.css:

@import url('foo/bar.scss');

For instance, and an SCSS file at /foo/bar.scss:

body {
  background: url('images/pretty-picture.jpg');
}

Then /foo/bar.scss should only be looked for in that location (foo/bar.scss relative to /foo.css) and images/pretty-picture.jpg should be looked for in both /images/pretty-picture.jpg and /foo/images/pretty-picture.jpg.

[gauravtiwari]

For latter one, I meant resolve-url-loader can depend on webpack resolve.modules option instead of creating it's own root option: https://webpack.js.org/configuration/resolve/#resolve-modules

https://github.com/rails/webpacker/blob/master/package/environment.js#L87

which would mean that root no longer would be a single scope but instead the same scope as supplied to webpack and it will cover resolved_paths as well - since that's where we add it to.

[gauravtiwari]

It will be more like whitelisted paths to lookup, which is what webpack does by default so, it would make sense to rely on that option instead of having it's own root option

[mvastola]

If exhaustive search is off, what purpose does root serve? Or are you saying the exhaustive search option should be a boolean exposed in webpacker.yml and if it's on then root should be set to this variable?

[gauravtiwari]

Sorry, I wasn't very clear. You see, webpack has this option called resolve.modules, which basically is like places it looks for modules to compile: https://webpack.js.org/configuration/resolve/#resolve-modules.

We have configured a few paths here as you can see (source, node_modules, resolved_paths): https://github.com/rails/webpacker/blob/master/package/environment.js#L87

I was saying, to remove root option altogether for searching part and instead rely on webpack internal option resolve.modules to lookup files. No more other option needed anywhere and it's safe too.

[mvastola]

Oh, so you would keep the exhaustive search, but just inside those 'safe' directories? You confused me because you said

Yes, the simple fix would be to make a PR on resolve-url-loader to turn off exhaustive search and perhaps make root option to use...

This bothers me a bit because if I have url(spacer.gif) (insert any common file name), there are any number of places to look for it (where it actually might be found, to unexpected results). And if I typo the name, what really should be happening is I should get a compile error. It shouldn't just find the file in a random module and compile it. These paths are supposed to be taken as relative to the stylesheet.

Obviously I will abide by whatever you guys choose, but this exhaustive searching (even of safe directories) in a way that deviates from how the CSS url() directive works strikes me as a really hacky solution that we shouldn't encourage/continue.

[gauravtiwari]

Ahh I see, in that sense would be ideal to turn off this sorcery and instead behave as intended - find modules relative to it.

So 👍 for turning off exhaustive search

[mvastola]

Okay, so if that's settled, I'm happy to play around with a PR for resolve-url-loader, but I think it would be prudent if someone more proficient in nodejs than I were up to the task. (I'd probably take a bit longer and I'd be worried of doing something wrong.)

(If you want, I know enough of what I'm doing to amend https://github.com/btc/webpacker/blob/master/lib/install/config/loaders/core/sass.js#L11 once all is said and done. :-p)

[gauravtiwari]

@mvastola Would probably make sense to draw up an issue on resolve-url-loader repo first and bring up this discussion there.

[mvastola]

Good point. On it.

[mvastola]

See bholloway/resolve-url-loader#69

[bholloway]

update

So (correct me if I am wrong @mvastola) there seems to be a good result on resolve-url-loader for using a new option attempts=1 to limit search.

This new option is currently on top of some unrelated refactoring which might itself require a major release cycle.

@mvastola would like me to back-port the new attempts option to master. Before spending time on a back-port I would like to gauge the feeling here...

Noting that my policy on resolve-url-loader is to be maximally compatible. The back-port would be a low risk, opt-in enhancement. I would assign it a minor version change.

question

It seems that webpacker trust me in that respect and is pinned to the major release ^2.x.x. So all I need to do is this back-port and @mvastola will be happy. Does this sound right?

consideration

Presuming all that, if webpacker additionally wants to default attempts=1 then that is your prerogative. It is probably a good idea.

However file search is in the original feature set so would be a breaking change for some people.

That said, I would probably set default attempts=1 in the next major update of resolve-url-loader, so you could just wait until I force the issue.

[mvastola]

@bholloway, w.r.t. everything that was directed at me, that's a go.

@gauravtiwari and/or @jeremy just to fill you in: @bholloway has slated for inclusion in the next major release of his module an attempts option (which will default to a value of 1) which will signify the max number of different paths resolve-url-loader will check before giving up.

If it finds the asset it is looking for in one of the paths it checks, that path will be used. If the path is not found, the fallback of evaluating the path relative to the entry file will be performed. When attempts is set to 1 the first/only path to be checked by resolve-url-loader is evaluating the path in the url directive relative to the file containing that directive.

I am asking @bholloway, because he seems amenable, to -- in addition to defaulting to atttempts=1 in the next major version -- make the attempts option available on an opt-in basis for the next minor version so that it will match the version constraints webpacker has set.

In order "end this sourcery" as it was so aptly put above, webpacker would then need in set attempts to 1 when configuring the loader (this change, sans the debug option), since the default would (for now) be 0 (meaning no limit).

@gauravtiwari and/or @jeremy does this sound kosher?

[mvastola]

Two more thoughts:

1. In addition to that small change to the loader options, obviously a yarn update resolve-url-loader will be needed.
2. Also to be decided: do we expose the option to revert back to the old setting in webpacker? If so, where?
3. Before we push this change (but after it's backported and isolated) it'd pay to throw a bunch of well known libraries against it and make sure they're not broken. The deep searching workaround was originally done for bootstrap, but it's not even clear if bootstrap still needs that and/or if this approach resolves the issue. (If there's a way to resolve the issue, say, by setting a sass variable, it would be worth it to have instructions prepared.)