Fix for symlinked themes #5156

Merged
merged 2 commits into from Aug 2, 2016

Conversation

Projects
None yet
3 participants
@benbalter
Contributor

benbalter commented Jul 28, 2016

If a user uses Rbenv, their theme is going to be symlinked. We run File.realpath on the path before we grab things like the includes folder, but not on the theme root, meaning when the two are passed to Jekyll.sanitized_path, Jekyll will think the unresolved root is the real root, and that the path (with resolved root already prepended) is an unsafe path outside the directory, thus prepending the unresolved theme path.

This PR resolves that by resolving the theme root path before passing things to sanitized path, ensuring the string comparison happens on both resolved paths, not one symlink and one resolved path.

Fixes #5145.

@parkr parkr modified the milestone: 3.2.1 Jul 28, 2016

@parkr parkr referenced this pull request Jul 28, 2016

Closed

Release v3.2.1 #5159

5 of 5 tasks complete

@parkr parkr added the fix label Aug 1, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 1, 2016

Member

LGTM. Is there a test we could add for this? I'm fine either way.

Member

parkr commented Aug 1, 2016

LGTM. Is there a test we could add for this? I'm fine either way.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Aug 1, 2016

Contributor

I think #5154 should be the test for this... at least I can't get jekyll new to build using the default theme on Travis without this PR.

Contributor

benbalter commented Aug 1, 2016

I think #5154 should be the test for this... at least I can't get jekyll new to build using the default theme on Travis without this PR.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 2, 2016

Member

🆒 Thanks!

@jekyllbot: merge +bug

Member

parkr commented Aug 2, 2016

🆒 Thanks!

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 95e9774 into master Aug 2, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Approved by @parkr. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Aug 2, 2016

@jekyllbot jekyllbot deleted the theme-source-fix branch Aug 2, 2016

jekyllbot added a commit that referenced this pull request Aug 2, 2016

parkr added a commit that referenced this pull request Aug 2, 2016

Merge branch 'master' into build-the-site
* master:
  Update history to reflect merge of #5156 [ci skip]
  Update history to reflect merge of #5177 [ci skip]
  Update history to reflect merge of #5173 [ci skip]
  Minor updates and corrections
  Future True on GitHub Pages note
  resolve theme root before sanitizing
  dont double sanitize theme folder paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment