Skip to content
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

Don't read symlinks in site.include in safe mode #7711

Merged
merged 2 commits into from Jun 23, 2019

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Jun 14, 2019

  • This is a ⚠️ security fix.

Summary

As a result of #7188, any entry in site.include that points to a file within the source_dir is read-in during a build. That created a security issue where an entry referring to a symlink is read-in even if it points to an entity outside the source_dir.

This PR attempts to resolve that by mirroring EntryFilter#symlink? without creating a new EntryFilter instance.

@ashmaroli ashmaroli requested a review from a team June 14, 2019 16:11
@ashmaroli ashmaroli added this to the 4.0 milestone Jun 14, 2019
@mattr-
Copy link
Member

mattr- commented Jun 21, 2019

Why is the filtering here and not in EntryFilter?

@mattr- mattr- closed this Jun 21, 2019
@mattr- mattr- reopened this Jun 21, 2019
@ashmaroli
Copy link
Member Author

Why is the filtering here and not in EntryFilter?

I didn't want to create a new EntryFilter instance to do something possible with just native Ruby.
But I guess its more in line with OOP principles than duplicating logic. So I've made the change.

@DirtyF DirtyF requested a review from a team June 22, 2019 12:16
@mattr-
Copy link
Member

mattr- commented Jun 23, 2019

I'm so sorry. I didn't do a good job of describing what I wanted. My original thought is that there could be a symlink? method in EntryFilter that is a class method. That would keep the symlink logic isolated to EntryFilter and keep from having to allocate an object just for the symlink check.

@ashmaroli
Copy link
Member Author

My original thought is that there could be a symlink? method in EntryFilter that is a class method.

@mattr- Since #symlink? depends on site.source (hence indirectly @site), I feel that a singleton_method :symlink? is just going to be an abstraction just for the sake of it.
The PR in current state looks okay to me.

@mattr-
Copy link
Member

mattr- commented Jun 23, 2019

@ashmaroli Right on. Thanks for the clarification. Do we need to get appveyor passing before merging this? Looks like it's only the Ruby 2.4 build that's failing.

@ashmaroli ashmaroli mentioned this pull request Jun 23, 2019
3 tasks
@ashmaroli
Copy link
Member Author

Do we need to get appveyor passing before merging this?

Its a spurious failure. The builds have all passed now.

@mattr-
Copy link
Member

mattr- commented Jun 23, 2019

Thank you!! ❤️

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 68a31c8 into jekyll:master Jun 23, 2019
jekyllbot added a commit that referenced this pull request Jun 23, 2019
@ashmaroli ashmaroli deleted the handle-site-include-symlink branch June 24, 2019 06:44
@jekyll jekyll locked and limited conversation to collaborators Jun 23, 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.

None yet

4 participants