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
3.7.x: EntryFilter#filter symlink fix #7224
Conversation
Previously, you could include the name of a symlinked file and Jekyll would not filter it. This is considered a bypass of the symlink checking, and thus a security bug.
lib/jekyll/entry_filter.rb
Outdated
special?(e) || backup?(e) || excluded?(e) || symlink?(e) | ||
end | ||
next true if symlink?(e) | ||
next false if included?(e) |
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.
@parkr For knowledge purposes, can you pleas explain what these two lines mean..?
Thanks =)
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.
We first check for symlinks and directly go to next entry if true
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.
Added a comment!
test/test_entry_filter.rb
Outdated
# no support for symlinks on Windows | ||
skip_if_windows "Jekyll does not currently support symlinks on Windows." | ||
|
||
site = Site.new(site_configuration("safe" => true, "include" => ["symlinked-file-outside-source"])) |
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.
We have a helper for this.. fixture_site
site = fixture_site(
:safe => true,
:include => ["filename"]
)
test/test_layout_reader.rb
Outdated
should "only read the layouts which are in the site" do | ||
layouts = LayoutReader.new(@site).read | ||
|
||
refute layouts.keys.include?("symlink"), "Should not read the symlinked layout" |
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.
Prefer using Hash#include?
or
Hash#key?` instead..?
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.
LGTM, thanks @parkr ❤️
@pathawks is Utterson preventing Travis from running tests here? |
@DirtyF I can't imagine why that would be the case 🤷♂️ |
I don't see why Travis / AppVeyor aren't working... I ran |
Looks like Checks API is waiting for Utterson to complete before launching Travis CI tests. 🤔 Tests are also passing locally on my end. |
Where do you see this? |
@parkr It's just a guess based on what is displayed on https://github.com/jekyll/jekyll/pull/7224/checks if Utterson is queued (and is blocked because of EC2), I conclude that Travis tests will not start until first job is finished. |
@parkr Thank you for addressing my concerns.. |
@ashmaroli I'll make a new PR to |
This reverts commit f5cd15c.
In
EntryFilter#filter
, we check forinclude?
before we check forsymlink?
, which allows symlinks to be read in a build when they shouldn't be by just including them.I am committing a failing test in one commit, and a fix in another commit so we can demonstrate what fails.