Skip to content

3.7.x: EntryFilter#filter symlink fix#7224

Merged
parkr merged 7 commits into3.7-stablefrom
3.7-entryfilter-symlink-fix
Sep 7, 2018
Merged

3.7.x: EntryFilter#filter symlink fix#7224
parkr merged 7 commits into3.7-stablefrom
3.7-entryfilter-symlink-fix

Conversation

@parkr
Copy link
Copy Markdown
Member

@parkr parkr commented Sep 6, 2018

In EntryFilter#filter, we check for include? before we check for symlink?, 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.

@parkr parkr added the security label Sep 6, 2018
@parkr parkr requested a review from a team September 6, 2018 17:10
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.
special?(e) || backup?(e) || excluded?(e) || symlink?(e)
end
next true if symlink?(e)
next false if included?(e)
Copy link
Copy Markdown
Member

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 =)

Copy link
Copy Markdown
Member

@DirtyF DirtyF Sep 6, 2018

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment!

# 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"]))
Copy link
Copy Markdown
Member

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"]
)

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"
Copy link
Copy Markdown
Member

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..?

Copy link
Copy Markdown
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @parkr ❤️

@DirtyF
Copy link
Copy Markdown
Member

DirtyF commented Sep 6, 2018

@pathawks is Utterson preventing Travis from running tests here?

@pathawks
Copy link
Copy Markdown
Member

pathawks commented Sep 6, 2018

@DirtyF I can't imagine why that would be the case 🤷‍♂️

@parkr
Copy link
Copy Markdown
Member Author

parkr commented Sep 7, 2018

I don't see why Travis / AppVeyor aren't working... I ran script/cibuild locally and the tests pass.

@parkr parkr self-assigned this Sep 7, 2018
@DirtyF
Copy link
Copy Markdown
Member

DirtyF commented Sep 7, 2018

Looks like Checks API is waiting for Utterson to complete before launching Travis CI tests. 🤔

Tests are also passing locally on my end.

@parkr
Copy link
Copy Markdown
Member Author

parkr commented Sep 7, 2018

Looks like Checks API is waiting for Utterson to complete before launching Travis CI tests. 🤔

Where do you see this?

@DirtyF
Copy link
Copy Markdown
Member

DirtyF commented Sep 7, 2018

@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.

@ashmaroli
Copy link
Copy Markdown
Member

@parkr Thank you for addressing my concerns..
I have 1 final request..: Can you please change the base branch to jekyll:master and backport the commit to 3.7-stable after its merged..?
Thanks once again..

@parkr
Copy link
Copy Markdown
Member Author

parkr commented Sep 7, 2018

@ashmaroli I'll make a new PR to Jekyll:master after this one is approved! The 3.7.x series is what GitHub runs and was thus my first priority. After this, I plan to issue a PR to master then backport it to 3.8-stable, and 3.6-stable.

@parkr parkr merged commit 4108ddb into 3.7-stable Sep 7, 2018
@parkr parkr deleted the 3.7-entryfilter-symlink-fix branch September 7, 2018 17:28
sjockers added a commit to datenguide/datenguide that referenced this pull request Oct 9, 2018
@jekyll jekyll locked and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants