Implement custom importers
pistolero opened this issue · 38 comments
I would like to use libsass with user-generated SCSS templates, but exposing libsass to user creates security threat. By crafting @import directives users can read some, possibly sensitive files, for server filesystem.
I would be nice if libsass would have support for custom importers, and have control over access to filesystem.
I also agree with the issue. It would be great to have some interface that implement custom importers.
I have implemented this functionality in my fork: https://github.com/Igorbek/libsass/tree/SourceProvider
Wow, that is fantastic
@pistolero yep, I'll do it today after take some code cleaning.
Is this feature still planned? I could need it too 😄
I believe this is already solved. Please let me know if it's still an issue.
Hi @hcatlin, I've had a look through the source code and while I've found code to allow adding a vector of custom include paths, I haven't spotted the ability for tools that bind to libsass (e.g. node-sass) to provide a custom @import resolver. Could you point me to where I could find that?
The use-case I'm thinking of is that binding libraries could provide hooks into that ecosystem's package manager. For instance node-sass could provide an @import resolver that transverses npm's node_modules folder. This is would be similar to Rework providing an npm loader. In the same vein, the ruby bindings could provide a resolver to transverse the rubygems folder structure.
Thanks.
Hmm... yeah, we haven't written anything like that. I bet we could do it
with a callback that you can register.
In your mind, would you expect the @import callback to act like a "last
ditch" or "method missing" resolver? Or, have it be the first thing
called, and then if it's unaware of a package name, go to the standard Sass
import resolver?
On Mon, Oct 6, 2014 at 10:36 AM, Johannes Ewald notifications@github.com
wrote:
@BPScott https://github.com/BPScott that's exactly the use-case [image:
👍]Reply to this email directly or view it on GitHub
#21 (comment).
I think it would need to be the first thing to try. This functionality feels like an override rather than a fallback coping mechanism.
I believe @import 'foo/bar; would need to look within the foo package first, then if that fails it should use the default resolver to try the relative file path ./foo/bar.scss (though it could be up to the binding library to skip trying the default resolver). Otherwise you could end up with oddness where files relative to the current file take precedence over the files from the package manager which would be undesirable.
Hmm… I think I’d assume it did local resolution first. But, I can see your point.
On Mon, Oct 6, 2014 at 11:38 AM, Ben Scott notifications@github.com
wrote:
I think it would need to be the first thing to try. This functionality feels like an override rather than a fallback coping mechanism.
I believe
@import 'foo/bar;would need to look within the foo package first, then if that fails it should use the default resolver to try the relative file path./foo/bar.scss(though it could be up to the binding library to skip trying the default resolver). Otherwise you could end up with oddness where files relative to the current file take precedence over the files from the package manager which would be undesirable.Reply to this email directly or view it on GitHub:
#21 (comment)
A quick nose around the rework importer and the npm docs suggests that npm dodges this problem by saying that imports are only considered to be relative if they start with /, ../ or ./.
So with npm-style importing rules @import 'foo/bar'; would be considered invalid even if the relative file ./foo/bar.scss exists. So if node-sass wanted to allow npm-style importing then it would have to kick in before (and disable) the default resolver.
Other package managers may treat these paths differently so I think this needs to solved by each binding library depending on their platform's package manager.
Following the Node module resolver heuristic, @import 'foo/bar'; would mean import the file bar.scss from module foo. Which means @import './node_modules/foo/bar.scss';.
In other terms Sass modules could be shipped and consumed as npm dependencies; thus making a very explicit graph of relationships between BEM blocks (for example).
@hcatlin I'd assume that the callback provides the rewritten path. Basically I want to configure somewhere (e.g. using node-sass):
...
resolveImport: function (path) {
// do some magic rewrite or throw an error if the path is illegal
return path;
}
...@hcatlin, @jhnns – I want to be able to provide a callback to actually returns the SCSS code to be imported, not just to return a resolved path. So by providing this callback, I could ensure Sass never touches the disk at all.
...
importer: function (name, cb) {
// look up `name`, load it, and maybe do some custom preprocessing...
// then call back with the SCSS
cb(null, scssString);
}This would allow me to properly use libsass in a transform pipeline. Current applications of this are hacky and limited, due to the fact that libsass wants to access the disk directly when it needs to import something.
Imagine I want an extra preprocessing step before rendering my SCSS. I can have main.scss.foo, and I can set up a pipeline that does my 'foo' rendering (maybe injecting some config from environment variables) and then the resulting SCSS code is fed into node-sass. This is already possible because libsass lets me feed in the SCSS code at the start. libsass doesn't need to know that main.scss doesn't actually exist on disk. This is good.
But what if my SCSS contains @import 'partial';? Suddenly libsass wants to access the disk directly. Maybe there is no file called _partial.scss, but instead I've got _partial.scss.foo, and I want to put it through the same pipeline. There is currently no way to do this.
This is why I want to be able to provide a callback that takes care of all file reading. Because it's not just about controlling disk access or resolving paths, but also about importing from a non-disk source, such as an in-memory pipeline.
Agreed 👍
A source should always have a path/filename associated, since this is what we use to create source-map information. In regard to source-maps the whole thing becomes even more complex, as a preprocessor that alters the original data will make the mappings invalid.
So IMO a proper implementation should be able to return:
- ['filename'] - libsass will load the file itself
- ['filename', 'filesource'] - libsass will use the source
- ['filename', 'filesource', 'srcmapdata'] - futureproof
Beside that case, an implementer may also want to add multiple files from one import!
So IMO it should also be possible to return a array of the above entries:
[
['filename'],
['filename', 'filesource'],
['filename', 'filesource', 'srcmapdata']
]
IMO this would cover all possible use cases! Or did I miss anything?
The srcmapdata could be used to incorporate that information back into the the final source-map. This is not a very easy task, but I've done it already in perl (POC). I also started to add some pieces of it into a c++ library to handle source-maps.
Good point, I didn't think of sourcemaps.
But I don't get why an importer would want to add multiple files from one import?
I guess someone might want to implement a globbing importer.
Or a preprocessor might load additional files (header/footer).
So just in case, I guess this way it feels more "complete".
IMO this is not implemented yet, so re-opening.
Already got it to the point where I can inject custom importers.
Now I "only" need to parse the return result and implement it as described above!
I have a working implementation in my local branch!
It should implement what I propesed earlier in this thread.
But more testing is needed to verify that is works correctly!
Awesome! I'm glad to see progress on this issue. 👍
I'm just wondering how node.js would be able to load the file asynchronously in the custom importer...
I'm not a c-programmer and don't know much about libuv. Any suggestions?
Same here. I'd like to help test, but I don't know C. @mgreter can you explain how I would test your branch? Presumably I would make a custom sassc build using your libsass branch, but then I don't know how I would run that executable in such a way that I can respond to its requests to load imports... or even if that makes any sense
To implement it someone definitely has to know C, there is no way around.
But I can give you a hint from my initial test how such a function would look like:
Note that source will be owned and freed by sass, so make a copy if necessary!
struct Sass_Import** sass_importer(const char* url, void* cookie)
{
struct Sass_Import** includes = sass_make_import_list(1);
includes[0] = sass_make_import_entry("path", strdup("source"), 0);
return includes;
}This C function is then registered with the sass context:
sass_option_set_importer(sass_options, sass_make_importer(sass_importer, *importer_sv));Note that importer_sv can be a pointer to anything (you get it back on the call).
I use it to store the perl function reference that should be invoked by sass_importer.
By the way, the most recent version you find in #635
Since commit mgreter@df5b7c3 this should be possible!
There is a wiki page with some API Documentation.
@mgreter: that is fantastic. Thank you for your work and the time you put into this <3
\o/
Awesome, thx! 👍
Thank you! 👍
The Sass Importer abstraction is a well defined API. It doesn't require the sass file to be on disk. It allows you to load Sass files from a database or over HTTP. It allows you to construct a sass file on the fly (Eg. compass sprites)
The resolution order is always relative to the current Sass file and then falling back to searching the load path from the beginning.
Is this how this works? I'm having a hard time understanding this from the code and wiki.
https://github.com/sass/sass/blob/stable/lib/sass/importers/base.rb is the Ruby API.
Unfortunately most information is scattered around in various issues. I'll try to explain the basics.
You get called for every import statement libsass sees (and get passed the url). If you return NULL, libsass should treat it as if the custom importer would not have existed. Alternatively you return a import_list back to libsass, which can have zero or more import_entry items. Each of this entry can have a file_name, a source_content and a source_map_content.
- If only the file_name is given, libsass will try to load it as any normal import statement (so I guess it will also use the same lookup methods as libsass normally does, like resolving partials etc.).
- If the content is also given, we will skip the loading from disk and use that data instead.
- The source_map is already there, as I'm planning to implement to map the resulting source map back (I already do this in another project and it works pretty well, but code needs to be ported from perl to C++, which I plan to do in a sperate project for now).
struct Sass_Import** sass_importer(const char* url, void* cookie)
{
struct Sass_Import** incs = sass_make_import_list(2);
incs[0] = sass_make_import_entry("virtual.scss", strdup(source), 0);
incs[1] = sass_make_import_entry("include.scss", 0, 0);
return incs;
}I hope this clears it up a little? But full ACK that the documentation could still need a lot of more examples and more detailed information, but I hope it's still better than what we had before ;)
I don't see how this scheme handles relative import resolution.
I don't think I understand the concerns correctly? If you pass back a relative path to libsass, it should take care of that, as it should still see that import within the context of the current file (or how is it different from normal relative import resolution libsass already does?). But maybe you mean that the implementor would have to know what the current file beeing processed is, so it can actually make the imports relative from there? Not sure if this is already possible ...
So if a file imports "foo/bar/baz" and then that file imports "asdf" then the file that should be imported is "foo/bar/asdf" if such a file exists otherwise a file "asdf" on the import path should be looked for. With importers the concept of a file is abstract, so there's a relative resolution that is passed the import statement as well as the current filename. filenames and import statements are not necessarily filesystem paths, they can be any string that the importer understands.