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

v1.4.3 is broken on Windows #1948

Closed
XhmikosR opened this Issue Jan 14, 2014 · 20 comments

Comments

Projects
None yet
9 participants
@XhmikosR
Contributor

XhmikosR commented Jan 14, 2014

While with v1.4.2 and the same settings/environment worked fine, with 1.4.3 I get the following

Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\xmr\Desktop\mpc-hc.org>ruby -v
ruby 1.9.3p484 (2013-11-22) [i386-mingw32]

C:\Users\xmr\Desktop\mpc-hc.org>jekyll build --trace
Configuration file: C:/Users/xmr/Desktop/mpc-hc.org/_config.yml
            Source: ./source/
       Destination: ./_site/
      Generating... C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/fileutils.rb:247:
in `mkdir': Invalid argument - C:/Users/xmr/Desktop/mpc-hc.org/_site/C: (Errno::EINVAL)
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/fileutils.rb:247:in `fu_mkdir'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/fileutils.rb:221:in `block (2 levels) in mkdir_p'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/fileutils.rb:219:in `reverse_each'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/fileutils.rb:219:in `block in mkdir_p'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/fileutils.rb:205:in `each'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/fileutils.rb:205:in `mkdir_p'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/convertible.rb:168:in `write'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:259:in `block in write'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:397:in `block (2 levels) in each_site_file'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:396:in `each'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:396:in `block in each_site_file'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:395:in `each'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:395:in `each_site_file'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:259:in `write'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/site.rb:41:in `process'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/command.rb:18:in `process_site'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/commands/build.rb:23:in `build'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/commands/build.rb:7:in `process'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/bin/jekyll:77:in `block (2 levels) in <top (required)>'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/commander-4.1.5/lib/commander/command.rb:180:in `call'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/commander-4.1.5/lib/commander/command.rb:180:in `call'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/commander-4.1.5/lib/commander/command.rb:155:in `run'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/commander-4.1.5/lib/commander/runner.rb:402:in `run_active_command'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/commander-4.1.5/lib/commander/runner.rb:78:in `run!'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/commander-4.1.5/lib/commander/delegates.rb:11:in `run!'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/commander-4.1.5/lib/commander/import.rb:10:in `block in <top (required)>

My _config.yml https://github.com/mpc-hc/mpc-hc.org/blob/jekyll/_config.yml

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 14, 2014

Member

Looks like this line uses / which is a Unix file path convention.

Member

parkr commented Jan 14, 2014

Looks like this line uses / which is a Unix file path convention.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 14, 2014

Member

v1.4.3 is not necessary unless you're running a host, so I'd suggest downgrading to v1.4.2 for now and we'll fix this for v2.0.0 :)

Member

parkr commented Jan 14, 2014

v1.4.3 is not necessary unless you're running a host, so I'd suggest downgrading to v1.4.2 for now and we'll fix this for v2.0.0 :)

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Jan 14, 2014

Contributor

Yeah, I already did, but you know, gem install jekyll will get 1.4.3 :P

Thanks for looking into this, I hope a fix will be out soon. It's already hard to set things up on Windows, let's not make it harder for the poor Windows users :)

Contributor

XhmikosR commented Jan 14, 2014

Yeah, I already did, but you know, gem install jekyll will get 1.4.3 :P

Thanks for looking into this, I hope a fix will be out soon. It's already hard to set things up on Windows, let's not make it harder for the poor Windows users :)

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 14, 2014

Member

We just need to make a small change. '/' -> File::SEPARATOR. I'll see if I can't take care of this later tonight.

Member

mattr- commented Jan 14, 2014

We just need to make a small change. '/' -> File::SEPARATOR. I'll see if I can't take care of this later tonight.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 14, 2014

Member

@mattr- Is that enough, or do we have to set it to either / or C:\\?

Member

parkr commented Jan 14, 2014

@mattr- Is that enough, or do we have to set it to either / or C:\\?

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 14, 2014

Member

Dunno. I'll double check.

Member

mattr- commented Jan 14, 2014

Dunno. I'll double check.

@perlauge

This comment has been minimized.

Show comment
Hide comment
@perlauge

perlauge Jan 15, 2014

As far as I can tell, then the issue is that the destination methods of page.rb and post.rb in the 1.4.3 gem are not the same as those on the master branch on GitHub.

As the implementations of the method seem alike I wonder why it hasn't been pulled up into convertible.rb

perlauge commented Jan 15, 2014

As far as I can tell, then the issue is that the destination methods of page.rb and post.rb in the 1.4.3 gem are not the same as those on the master branch on GitHub.

As the implementations of the method seem alike I wonder why it hasn't been pulled up into convertible.rb

@Nukker

This comment has been minimized.

Show comment
Hide comment
@Nukker

Nukker Jan 16, 2014

Thank you the bug of solution . let me mark this bug

Nukker commented Jan 16, 2014

Thank you the bug of solution . let me mark this bug

@tamouse

This comment has been minimized.

Show comment
Hide comment
@tamouse

tamouse Jan 16, 2014

You will have to adjust the following line as well, since that's checking if the last character is the path separator. Could do self.url[-1] == File::SEPARATOR instead...

tamouse commented Jan 16, 2014

You will have to adjust the following line as well, since that's checking if the last character is the path separator. Could do self.url[-1] == File::SEPARATOR instead...

@perlauge

This comment has been minimized.

Show comment
Hide comment
@perlauge

perlauge Jan 16, 2014

I politely disagree - URLs will use '/' as file separator. And in any case Ruby will convert Windows/DOS file separator '' to '/'
Try out:

C:\>irb
irb(main):001:0> File.expand_path('.')
=> "C:/"
irb(main):002:0>

C:\>

perlauge commented Jan 16, 2014

I politely disagree - URLs will use '/' as file separator. And in any case Ruby will convert Windows/DOS file separator '' to '/'
Try out:

C:\>irb
irb(main):001:0> File.expand_path('.')
=> "C:/"
irb(main):002:0>

C:\>

mikejbrown added a commit to mikejbrown/mikejbrown.github.com that referenced this issue Jan 17, 2014

Update bundle
Note that jekyll 1.4.3 currently breaks on windows,
but github pages builds ok.

See jekyll/jekyll#1948

parkr referenced this issue Jan 27, 2014

sanity check for pages permalink traversal
Signed-off-by: Parker Moore <parkrmoore@gmail.com>
@perlauge

This comment has been minimized.

Show comment
Hide comment
@perlauge

perlauge Feb 6, 2014

As mentioned before, then the issue can be fixed (for Windows) by removing the destination methods of page.rband post.rb then add the following to convertible.rb

    def destination(dest)
      # The url needs to be unescaped in order to preserve the correct filename
      path = File.join(dest, CGI.unescape(self.url))
      path = File.join(path, "index.html") if path[/\.html$/].nil?
      path
    end

This is using the "append index.html" if the path doesn't end with a .html extension as seen in https://github.com/jekyll/jekyll/blob/master/lib/jekyll/post.rb#L261 and not as the page version https://github.com/jekyll/jekyll/blob/master/lib/jekyll/page.rb#L137 where the check is to see if the path ends with a /

I tested this by adding a permalink: ../../../../../../somefile line in the content matter - the result is project path/_site/somefile/index.html as wanted to be contained within the project.

perlauge commented Feb 6, 2014

As mentioned before, then the issue can be fixed (for Windows) by removing the destination methods of page.rband post.rb then add the following to convertible.rb

    def destination(dest)
      # The url needs to be unescaped in order to preserve the correct filename
      path = File.join(dest, CGI.unescape(self.url))
      path = File.join(path, "index.html") if path[/\.html$/].nil?
      path
    end

This is using the "append index.html" if the path doesn't end with a .html extension as seen in https://github.com/jekyll/jekyll/blob/master/lib/jekyll/post.rb#L261 and not as the page version https://github.com/jekyll/jekyll/blob/master/lib/jekyll/page.rb#L137 where the check is to see if the path ends with a /

I tested this by adding a permalink: ../../../../../../somefile line in the content matter - the result is project path/_site/somefile/index.html as wanted to be contained within the project.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 6, 2014

Member

@perlauge Can you please turn that into a PR? :)

Member

parkr commented Feb 6, 2014

@perlauge Can you please turn that into a PR? :)

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Feb 6, 2014

Contributor

👍 for the PR :D

Contributor

XhmikosR commented Feb 6, 2014

👍 for the PR :D

@perlauge

This comment has been minimized.

Show comment
Hide comment
@perlauge

perlauge Feb 7, 2014

As mentioned earlier #1948 (comment), then the version on master is not the same as the one deployed as a gem.

Example:
diff -b GitHub/jekyll/lib/jekyll/page.rb Ruby193/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/page.rb

11a12
>       url
13,14d13
<       dir
<       name
16d14
<       url
138c136
<       path = File.join(dest, self.url)
---
>       path = File.join(dest, File.expand_path(self.url, "/"))

This makes it really difficult to create a nice change, as I would not be changing the true files.

On top of this I'm wondering why this is the case.

perlauge commented Feb 7, 2014

As mentioned earlier #1948 (comment), then the version on master is not the same as the one deployed as a gem.

Example:
diff -b GitHub/jekyll/lib/jekyll/page.rb Ruby193/lib/ruby/gems/1.9.1/gems/jekyll-1.4.3/lib/jekyll/page.rb

11a12
>       url
13,14d13
<       dir
<       name
16d14
<       url
138c136
<       path = File.join(dest, self.url)
---
>       path = File.join(dest, File.expand_path(self.url, "/"))

This makes it really difficult to create a nice change, as I would not be changing the true files.

On top of this I'm wondering why this is the case.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 8, 2014

Member

@perlauge We still need to merge #1946 into master.

Member

parkr commented Feb 8, 2014

@perlauge We still need to merge #1946 into master.

@perlauge

This comment has been minimized.

Show comment
Hide comment
@perlauge

perlauge Feb 8, 2014

@parkr I think we have a miscommunication here. I believe that whatever code was in the 1.4.3 gem should be somewhere on the master branch, but for page.rb the latest change was line 138 modified June 15, 2013 - that means this part should have been in 1.4.2 as well.

The code in the unravelled gem of 1.4.3 does not look like the master - which, to me at least, must mean that someone has modified the checked out code and then built a gem. This sort of worries me.

perlauge commented Feb 8, 2014

@parkr I think we have a miscommunication here. I believe that whatever code was in the 1.4.3 gem should be somewhere on the master branch, but for page.rb the latest change was line 138 modified June 15, 2013 - that means this part should have been in 1.4.2 as well.

The code in the unravelled gem of 1.4.3 does not look like the master - which, to me at least, must mean that someone has modified the checked out code and then built a gem. This sort of worries me.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 8, 2014

Member

We released from the v1-stable branch. It's an official release- nothing to worry about. Master branch doesn't have v1.4.3

Member

parkr commented Feb 8, 2014

We released from the v1-stable branch. It's an official release- nothing to worry about. Master branch doesn't have v1.4.3

parkr added a commit that referenced this issue Feb 18, 2014

parkr added a commit that referenced this issue Feb 18, 2014

parkr added a commit that referenced this issue Feb 20, 2014

parkr added a commit that referenced this issue Feb 24, 2014

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Mar 1, 2014

Contributor

Any news on this?

Contributor

XhmikosR commented Mar 1, 2014

Any news on this?

XhmikosR added a commit to twbs/bootstrap that referenced this issue Mar 10, 2014

Bump Jekyll version used in Travis to v1.4.2.
Note that while v1.4.3 is the latest stable version, it has a nasty Windows bug.
(jekyll/jekyll#1948)

@parkr parkr closed this in #2109 Mar 11, 2014

@thany

This comment has been minimized.

Show comment
Hide comment
@thany

thany Mar 12, 2014

worth noting how to install 1.4.2 instead of latest:

gem install jekyll --version "=1.4.2"

thany commented Mar 12, 2014

worth noting how to install 1.4.2 instead of latest:

gem install jekyll --version "=1.4.2"

@eyecatchup

This comment has been minimized.

Show comment
Hide comment
@eyecatchup

eyecatchup Mar 12, 2014

worth noting how to install 1.4.2 instead of latest:

gem install jekyll --version "=1.4.2"

True. Came accross the same issue today and only found an answer on SO. For completeness, if you've already installed Jekyll 1.4.3, run gem uninstall jekyll first, followed by gem install jekyll --version "=1.4.2".

eyecatchup commented Mar 12, 2014

worth noting how to install 1.4.2 instead of latest:

gem install jekyll --version "=1.4.2"

True. Came accross the same issue today and only found an answer on SO. For completeness, if you've already installed Jekyll 1.4.3, run gem uninstall jekyll first, followed by gem install jekyll --version "=1.4.2".

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