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

Fix symlinked static files not being correctly built in unsafe mode #909

Merged
merged 3 commits into from Apr 3, 2013

Conversation

Projects
None yet
4 participants
@x3ro
Contributor

x3ro commented Mar 31, 2013

PR #824 changed Site#filter_entries so that symlinks are allowed in unsafe mode. However, there was an additional check if the file is a symlink in Site#read_directories, which caused the peculiar behaviour that symlinked directories were correctly copied/rendered in unsafe mode, but symlinked files were ignored. This PR fixes that odd behaviour and adds a regression test.

@mattr-

This comment has been minimized.

mattr- commented on test/test_site.rb in 0bebe0f Apr 1, 2013

this test code seems oddly misplaced, especially since these tests are testing the filter_entries method. I think this test (and the one you added below it) deserve their own explicit test cases. Could you split those out and update your branch?

This comment has been minimized.

Owner

x3ro replied Apr 1, 2013

I went by the description of the test cases, one of which is "not filter symlink entries when safe mode disabled". Since that fits perfectly for what I'm testing, I recycled those cases 😄 Very well then, I shall think of a name and create separete cases :)

@mattr-

This comment has been minimized.

mattr- commented on 2c73252 Apr 1, 2013

I think I would prefer it if this condition were more explicit, such as

elsif normal_file?(f_abs)

where normal_file? is defined as

def normal_file?(f_abs)
    !File.socket?(f_abs) && !File.pipe?(f_abs)
end

Technically, we accept sockets and pipes now, so we solve two things in one go: skipping files we should accept and not allowing things we shouldn't.

This comment has been minimized.

Owner

x3ro replied Apr 1, 2013

But shouldn't the filtering of sockets and pipes also happen in filter_entries? Splitting the filtering logic so that it happens in two places seems to be what caused this bug in the first place.

This comment has been minimized.

mattr- replied Apr 1, 2013

This comment has been minimized.

Owner

x3ro replied Apr 2, 2013

The line in question seems to exist since 0cb1ebcd, when filter_entries didn't include the check for symlinks yet. Said check was introducted in 13cc44fb, and I'm guessing that @tmm1 didn't notice that there is an additional check for symlinks further below in read_directories. But as I said, I'm only guessing 😄

@parkr

This comment has been minimized.

Member

parkr commented Apr 3, 2013

@mattr-, look good to you?

@mattr-

This comment has been minimized.

Member

mattr- commented Apr 3, 2013

👍 :shipit:

parkr added a commit that referenced this pull request Apr 3, 2013

Merge pull request #909 from x3ro/symlinked-static-files
Fix symlinked static files not being correctly built in unsafe mode

@parkr parkr merged commit cf461ea into jekyll:master Apr 3, 2013

1 check passed

default The Travis build passed
Details

parkr added a commit that referenced this pull request Apr 3, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.