Fix sassc relative import from input file

[ntkme]

After another close look at the import behavior reported in #135, I believe that sass-embedded behavior is correct and sassc integration is buggy.

The root cause is that sassc integration currently sets the filename argument incorrectly (basename instead of relative path), so that the relative import started from the input file is handled incorrectly. E.g. Let's assume we have the following two files in a project:

// /project-root/test.scss
a { b: c; }

---
---
// /project-root/assets/style.scss
@import "test"; // this works for sassc, but it should not, which is a side-effect of setting `filename` incorrectly
@import "../test"; // this does not work for sassc, but does work for sass-embedded

Because sassc integration does not set filename correctly, what ended up happening is that the import in the example above is resolved against /project-root/style.scss (does not exist) instead of /project-root/assets/style.scss. Thus causing the buggy behavior. In order words, since it is always set to the basename of the input file and the dirname of the basename is just the root of the project, imports can always be incorrectly resolved relative to the root of the project, which is equivlent to adding . to sass load_path.

---

• Closes #135

[ashmaroli]

At first pass-through, I'm not sure if this is related to #135.. The linked issue exposed a change in behavior that css/main.scss cannot import _sass/main.scss with sass_embedded implementation. I don't see how local directory traversal using paths with leading ../ comes into the picture. At the outset, it looks like a security issue if one can import files outside Jekyll source directory (not sure, just speculating..).

[ntkme]

@ashmaroli It’s linked because that sass-embedded cannot do so is correct and consistent. Even for normal sassc command line is it not possible do so as it is a circular import. Only reason the import worked for sassc is that we set the filename incorrectly that the first attempt to load file relatively resolved it to non exist location (thus no longer circular), then load_path is searched, which is a side effect. Does it make sense?

e.g. if we have main.scss trying to load _scss/main.scss, even with the current sassc integration it won’t work, because in this case basename of the file is the same as the path, thus relative import would resolve back to itself and lead to a circular import:

  Conversion error: Jekyll::Converters::Scss encountered an error while converting 'main.scss':
                    Error: An @import loop has been found: main.scss imports main.scss on line 1:1 of main.scss >> --- ^ 
                    ------------------------------------------------
      Jekyll 4.2.2   Please append `--trace` to the `build` command 
                     for any additional information or backtrace. 
                    ------------------------------------------------
/usr/local/bundle/gems/jekyll-sass-converter-2.2.0/lib/jekyll/converters/scss.rb:200:in `rescue in sass_convert': Error: An @import loop has been found: (Jekyll::Converters::Scss::SyntaxError)
           main.scss imports main.scss
        on line 1:1 of main.scss
>> ---

   ^

[ntkme]

And if you're concerned about loading files outside of the project... You can do it from the very beginning by importing an absolute path. IMO this is a concern only if someone is building non-trusted sites in non-sandbox environments.

[ashmaroli]

@ntkme I would like investigate more over the weekend. In the meantime, shouldn't the following be a memoized function?

def sass_page_dirname
  @sass_page_dirname = File.dirname(sass_page_absolute_path)
end

[ashmaroli]

Requesting feedback from @parkr on this PR and the various arguments put forth.

[ntkme]

@parkr Would you mind take a look when you get a chance?

[parkr]

And if you're concerned about loading files outside of the project... You can do it from the very beginning by importing an absolute path. IMO this is a concern only if someone is building non-trusted sites in non-sandbox environments.

GitHub Pages is generally quite concerned with the security parameters of file imports. If you @import "../../../../../../etc/passwd", will it throw an error with the contents of /etc/passwd, or will it validate that the file ends with the extension .sass or .scss? If it validates for the extension, then I think it's fine.

Only reason the import worked for sassc is that we set the filename incorrectly that the first attempt to load file relatively resolved it to non exist location (thus no longer circular), then load_path is searched, which is a side effect.

The user expects that assets/main.scss can import _sass/main.scss when using jekyll-sass-converter. However, this behavior is considered broken per the Sass spec, meaning jekyll-sass-converter has diverged (for years) from the spec, is that correct?

My proposal would be to produce a new MAJOR version of jekyll-sass-converter that:

1. Defaults to the sass-embedded implementation
2. Changes this import behavior
3. Removes the sassc compatibility entirely
4. Documents the upgrade path

I believe breaking the user expectations necessitates a MAJOR version bump.

Is there a common language-agnostic spec we can run against so that we know we're compliant with other implementations like node-sass?

[ntkme]

GitHub Pages is generally quite concerned with the security parameters of file imports. If you @import "../../../../../../etc/passwd", will it throw an error with the contents of /etc/passwd, or will it validate that the file ends with the extension .sass or .scss? If it validates for the extension, then I think it's fine.

require 'sassc'
puts SassC::Engine.new('@import "/etc/passwd"').render

/etc/passwd:1: Error: Invalid CSS after "r": expected 1 selector or at-rule, was "root:x:0:0:root:/ro" (SassC::SyntaxError)
        on line 1:1 of ../../../etc/passwd
        from line 1:1 of stdin

require 'sass-embedded'
puts Sass.compile_string('@import "/etc/passwd"')

/usr/local/bundle/gems/sass-embedded-1.53.0/lib/sass/embedded/protofier.rb:16:in `from_proto_compile_response': Can't find stylesheet to import. (Sass::CompileError)

The user expects that assets/main.scss can import _sass/main.scss when using jekyll-sass-converter. However, this behavior is considered broken per the Sass spec, meaning jekyll-sass-converter has diverged (for years) from the spec, is that correct?

Yes, this is the correct interpretation.

My proposal would be to produce a new MAJOR version of jekyll-sass-converter that:

1. Defaults to the sass-embedded implementation
2. Changes this import behavior
3. Removes the sassc compatibility entirely
4. Documents the upgrade path

I believe breaking the user expectations necessitates a MAJOR version bump.

Yes, this PR in its current form is indeed a breaking change for end users. A 3.0 release without sassc without fixing this sounds like a solution.

Is there a common language-agnostic spec we can run against so that we know we're compliant with other implementations like node-sass?

We have sass-spec but it only tests the importer behavior given correct input is given.

[ntkme]

@parkr sassc does not require a file extension (see my previous comment), and sass-embedded / dart-sass does enforce .sass, .scss, or .css file extension. So from a security perspective having a 3.0 that drops sassc would be good idea.