Allow symlinks if they point to stuff inside site.source #4710

Merged
merged 4 commits into from Apr 22, 2016

Conversation

Projects
None yet
5 participants
@fenollp
Contributor

fenollp commented Mar 25, 2016

Tests are failing, working on updating them.

There are a couple places where the previous implementation of EntryFilter#symlink? was "inlined". I'm wondering if this code should be updated too?

lib/jekyll/collection.rb:97:            File.directory?(path) || (File.symlink?(f) && site.safe)
lib/jekyll/collection.rb:138:      File.directory?(directory) && !(File.symlink?(directory) && site.safe)
lib/jekyll/readers/data_reader.rb:29:      return unless File.directory?(dir) && (!site.safe || !File.symlink?(dir))
lib/jekyll/readers/data_reader.rb:37:        next if File.symlink?(path) && site.safe
lib/jekyll/entry_filter.rb
+ end
+
+ def bad_symlink?(entry)
+ site_source_realpath = File.realpath(base_directory)

This comment has been minimized.

@fenollp

fenollp Mar 25, 2016

Contributor

Hm base_directory is not what I understood it was

@fenollp

fenollp Mar 25, 2016

Contributor

Hm base_directory is not what I understood it was

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 25, 2016

Member

Can you please add a new test which matches your condition instead of changing both the other pre-existing tests? If you need to point the symlink in the previous examples to something outside the site source, that's just fine. I think it's important to keep those tests for the purpose of removing symlinks that aren't in the source. 😄

Member

parkr commented Mar 25, 2016

Can you please add a new test which matches your condition instead of changing both the other pre-existing tests? If you need to point the symlink in the previous examples to something outside the site source, that's just fine. I think it's important to keep those tests for the purpose of removing symlinks that aren't in the source. 😄

lib/jekyll/entry_filter.rb
+ end
+
+ def bad_symlink?(entry)
+ ! File.realpath(entry).start_with?(File.realpath(@site.source))

This comment has been minimized.

@parkr

parkr Mar 25, 2016

Member

What do you think about:

def symlink_outside_site_source?(entry)
  !File.realpath(entry).start_with?(Filerealpath(site.source))
end

I think that reads a bit more directly.

@parkr

parkr Mar 25, 2016

Member

What do you think about:

def symlink_outside_site_source?(entry)
  !File.realpath(entry).start_with?(Filerealpath(site.source))
end

I think that reads a bit more directly.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 25, 2016

Member

There are a couple places where the previous implementation of EntryFilter#symlink? was "inlined". I'm wondering if this code should be updated too?

Yes, that's a splendid idea! We should be using the EntryFilter there anyway.

Member

parkr commented Mar 25, 2016

There are a couple places where the previous implementation of EntryFilter#symlink? was "inlined". I'm wondering if this code should be updated too?

Yes, that's a splendid idea! We should be using the EntryFilter there anyway.

@parkr parkr added the enhancement label Mar 25, 2016

@parkr parkr added this to the 3.2 milestone Mar 25, 2016

@fenollp

This comment has been minimized.

Show comment
Hide comment
@fenollp

fenollp Mar 26, 2016

Contributor

@parkr Updated according to review. Though I think there is code to explicitly disallow symlinks in _includes and am not sure what to do about this case.

Contributor

fenollp commented Mar 26, 2016

@parkr Updated according to review. Though I think there is code to explicitly disallow symlinks in _includes and am not sure what to do about this case.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 14, 2016

Member

Though I think there is code to explicitly disallow symlinks in _includes

It only disallows symlinks which point outside your repo in safe mode, right?

Member

parkr commented Apr 14, 2016

Though I think there is code to explicitly disallow symlinks in _includes

It only disallows symlinks which point outside your repo in safe mode, right?

@fenollp

This comment has been minimized.

Show comment
Hide comment
@fenollp

fenollp Apr 14, 2016

Contributor

yes

Contributor

fenollp commented Apr 14, 2016

yes

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 21, 2016

Member

It only disallows symlinks which point outside your repo in safe mode, right?

yep

Ok, yeah that's what I'm looking for. Due to security restrictions, it's very unlikely you'll ever be able to include a file which is symlinked to something outside your repo in safe mode. In unsafe mode, you can do pretty much anything.

Member

parkr commented Apr 21, 2016

It only disallows symlinks which point outside your repo in safe mode, right?

yep

Ok, yeah that's what I'm looking for. Due to security restrictions, it's very unlikely you'll ever be able to include a file which is symlinked to something outside your repo in safe mode. In unsafe mode, you can do pretty much anything.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 21, 2016

Member

:shipit:

@envygeeks, how does this look to you?

@benbalter This adds support for symlinks in _includes in safe mode only if they point to a file inside the repo. That cool?

Member

parkr commented Apr 21, 2016

:shipit:

@envygeeks, how does this look to you?

@benbalter This adds support for symlinks in _includes in safe mode only if they point to a file inside the repo. That cool?

@@ -52,7 +52,11 @@ def excluded?(entry)
end
def symlink?(entry)

This comment has been minimized.

@envygeeks

envygeeks Apr 21, 2016

Contributor

This should be a singleton method. If you are initializing a class for a single method, it deserves to be a singleton.

@envygeeks

envygeeks Apr 21, 2016

Contributor

This should be a singleton method. If you are initializing a class for a single method, it deserves to be a singleton.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 21, 2016

Contributor

There is only on problem I see here, I see no recursion. If my symlink has symlinks that symlink to other sources, I could break this, the question isn't whether it's a problem, it's whether or not we should retarget what it claims to do?

Contributor

envygeeks commented Apr 21, 2016

There is only on problem I see here, I see no recursion. If my symlink has symlinks that symlink to other sources, I could break this, the question isn't whether it's a problem, it's whether or not we should retarget what it claims to do?

@fenollp

This comment has been minimized.

Show comment
Hide comment
@fenollp

fenollp Apr 21, 2016

Contributor

@envygeeks File.realpath takes care of this.

mkdir u
ln -s u a
ln -s a b
ls -lh
drwxr-xr-x    6 pete staff    1 Apr 21 10:55 u
lrwxr-xr-x    1 pete staff    1 Apr 21 10:55 a -> u
lrwxr-xr-x    1 pete staff    1 Apr 21 10:55 b -> a
0 ~  λ ruby -e 'puts File.realpath "u"'
/Users/pete/u
0 ~  λ ruby -e 'puts File.realpath "a"'
/Users/pete/u
0 ~  λ ruby -e 'puts File.realpath "b"'
/Users/pete/u
Contributor

fenollp commented Apr 21, 2016

@envygeeks File.realpath takes care of this.

mkdir u
ln -s u a
ln -s a b
ls -lh
drwxr-xr-x    6 pete staff    1 Apr 21 10:55 u
lrwxr-xr-x    1 pete staff    1 Apr 21 10:55 a -> u
lrwxr-xr-x    1 pete staff    1 Apr 21 10:55 b -> a
0 ~  λ ruby -e 'puts File.realpath "u"'
/Users/pete/u
0 ~  λ ruby -e 'puts File.realpath "a"'
/Users/pete/u
0 ~  λ ruby -e 'puts File.realpath "b"'
/Users/pete/u
@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Apr 21, 2016

Contributor

This adds support for symlinks in _includes in safe mode only if they point to a file inside the repo. That cool?

👍

Contributor

benbalter commented Apr 21, 2016

This adds support for symlinks in _includes in safe mode only if they point to a file inside the repo. That cool?

👍

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 21, 2016

Contributor

File.realpath takes care of this.

@fenollp you're right, for some reason last night I was under the impression of directories for some reason, of which File#realpath wouldn't resolve sub-links. However I don't believe include allows directories anyways and re-reviewing this morning says exactly that in the source! Carry on, I apologize.

Contributor

envygeeks commented Apr 21, 2016

File.realpath takes care of this.

@fenollp you're right, for some reason last night I was under the impression of directories for some reason, of which File#realpath wouldn't resolve sub-links. However I don't believe include allows directories anyways and re-reviewing this morning says exactly that in the source! Carry on, I apologize.

@benbalter benbalter changed the title from Allow symlinks iff they point to stuff inside site.source to Allow symlinks if they point to stuff inside site.source Apr 21, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 22, 2016

Member

Lookin' goooood.

@jekyllbot: merge +minor

Member

parkr commented Apr 22, 2016

Lookin' goooood.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 1d78820 into jekyll:master Apr 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Apr 22, 2016

@fenollp fenollp deleted the fenollp:safe-symlinks branch Apr 22, 2016

laughinghan added a commit to laughinghan/mvsf that referenced this pull request Jun 9, 2016

Hide Jekyll stuff in _jekyll
Based on https://jekyllrb.com/docs/configuration/#default-configuration
except for Sass, which isn't documented there, but is documented at
https://jekyllrb.com/docs/assets/#sassscss

Except for _posts/, which isn't configurable, but it turns out we don't
need it at all!

Note that at first I tried setting `source: _jekyll`, but then Jekyll
couldn't find index.html, so I tried to symlink _jekyll/index.html to
../index.html, but it turns out that for security reasons Jekyll only
supports symlinks within site.source:
jekyll/jekyll#4710
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment