-
Notifications
You must be signed in to change notification settings - Fork 134
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
Allow load_paths in safe mode with sanitization #50
Conversation
I should have looked at open PRs. It looks like #45 does something similar, but allowing multiple values for The globbing in this PR is a nice feature, but not absolutely necessary. I could get around not having it by specifying each nested /cc @benbalter @parkr @spudowiar |
# Expand file globs (e.g. `node_modules/*/node_modules` ) | ||
paths = paths.map { |path| Dir.glob(path) }.flatten.uniq | ||
|
||
paths.select { |path| File.directory?(path) } |
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.
Should this line and line 82 be combined into one string rather than re-assigning paths
? Thus paths.map { }.flatten.uniq.select { }
.
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.
Any idea what's going on with the tests? They're all passing for me locally, but getting some weird errors on Travis:
|
This looks great! @jekyll/security, would you mind taking a quick 👀 to ensure this is production-ready? Seems OK, but easier to ask now than later. 😉
@bkeepers We should remove the tests for Jekyll 2.5 for future versions of this gem. Ruby 2.3 changed the way singleton classes work and that seems to have caused a serious issue with Jekyll 2.5. It's not supported any longer so let's just nix it. |
* origin/master: (24 commits) Don't need history sections. Update history to reflect merge of jekyll#52 [ci skip] Don't test Jekyll 2.5 against Ruby 2.3. Whoops, they have to be h3's Update history to reflect merge of jekyll#46 [ci skip] travis: Jekyll 3.1.2 travis: match rvm versions with jekyll/jekyll Release 💎 v1.4.0 Update history to reflect merge of jekyll#42 add_charset false by default, but still strip BOM Update history to reflect merge of jekyll#39 Update history to reflect merge of jekyll#41 Test Jekyll 2 & 3 explicitly fix converter spec for jekyll 3 travis: update to match jekyll-watch Travis: use container infra Update Gemfile Add Jekyll 2 & 3 to test matrix Update history to reflect merge of jekyll#40 Update the version of Sass to be 3.4.x ...
@@ -230,4 +230,60 @@ def converter(overrides = {}) | |||
|
|||
end | |||
|
|||
context "importing from internal libraries" do | |||
let(:internal_library) { source_dir("bower_components/jquery") } | |||
let(:converter) { site.getConverterImpl(Jekyll::Converters::Scss) } |
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.
This will have to be site.find_converter_instance
if Jekyll::VERSION >= "3.0"
@bkeepers 2 more comments above, then LGTM. ☝️ |
Thanks for the review, @parkr! |
This LGTM. Thank you! @benbalter, would you mind being my second pair of 👀 on this? |
# Expand file globs (e.g. `node_modules/*/node_modules` ) | ||
Dir.chdir(@config["source"]) do | ||
paths = paths.map { |path| Dir.glob(path) }.flatten.uniq. | ||
map { |path| File.expand_path(path) } |
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.
I'd rather we did the Jekyll.sanitized_path
after the expanding/globbing. Dir.glob
and File.expand_path
should be safe after the Jekyll.sanitized_path
call, but they might do something weird that I don't know about.
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.
👍 on that, thanks for the review, @mastahyeti. @bkeepers, would that work for you?
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.
There's probably some DOS vectors if we don't sanitize before (.e.g. /**/*
would scan the entire file system). Should we do both?
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.
I'm 👍 on that. Sanitizing shouldn't be a very expensive operation.
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.
@bkeepers Renewed interest in this. Would you mind taking a sec to update this path sanitization?
@parkr ok, I made updates based on feedback. |
4db6883
to
5683bc6
Compare
5683bc6
to
b693eb1
Compare
(user_sass_load_paths + [sass_dir_relative_to_site_source]).uniq | ||
end.select { |load_path| File.directory?(load_path) } | ||
# Sanitize paths to prevent any attack vectors (.e.g. `/**/*`) | ||
paths.map! { |path| Jekyll.sanitized_path(@config["source"], path) } |
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.
What would happen if a malicious user set source: ../../../../../password
? Aren't we using untrusted user input here to sanitize user input? (Or is that checked further down the stack @parkr?)
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.
On GitHub Pages, we override source
so no users could do this. Does that mitigate your concerns?
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.
Additionally, @config["source"]
comes from site.config
. A quick solution is to do what Site does:
@source = File.expand_path(@config["source"]).freeze
@parkr @benbalter anything I can do to get this merged? |
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.
My two concerns around source
are resolved since source is going to be overridden on any safe
implementation. 👍 from me but defer to @parkr in case there's anything I missed.
@jekyllbot: merge +minor |
load_paths
is currently only supported in safe mode. This enables it all the time, but sanitizes the paths withJekyll.sanitized_path
in safe mode to ensure all paths are relative to the source directory. This also adds support for globbing (e.g.**/*/node_modules
to add node modules and all transient dependencies to the load path).Having multiple paths in the sass load path is really helpful when using package managers like bower or NPM. For example:
The motivation for this change is so I can use the latest version of primer on a jekyll site.
/cc @jonrohan