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

Fix Windows Path Sanitation #3077

Merged
merged 2 commits into from
Nov 9, 2014
Merged

Fix Windows Path Sanitation #3077

merged 2 commits into from
Nov 9, 2014

Conversation

parkr
Copy link
Member

@parkr parkr commented Nov 8, 2014

Fixes #3068, alternative to #3071.

Benchmarks

Before

Calculating -------------------------------------
with no questionable path
                         23598 i/100ms
with a single-part questionable path
                         23137 i/100ms
with a multi-part questionable path
                         21023 i/100ms
with a single-part traversal path
                         22509 i/100ms
with a multi-part traversal path
                         22209 i/100ms
with the exact same paths
                         27870 i/100ms
with a single-part absolute path including the base_directory
                         20155 i/100ms
with a multi-part absolute path including the base_directory
                         19565 i/100ms
with a single-part traversal path including the base_directory
                         19598 i/100ms
with a multi-part traversal path including the base_directory
                         19123 i/100ms
-------------------------------------------------
with no questionable path
                       338105.9 (±5.6%) i/s -    1699056 in   5.041212s
with a single-part questionable path
                       318878.6 (±6.5%) i/s -    1596453 in   5.027918s
with a multi-part questionable path
                       275802.9 (±6.2%) i/s -    1387518 in   5.050206s
with a single-part traversal path
                       314833.7 (±6.6%) i/s -    1575630 in   5.027440s
with a multi-part traversal path
                       296817.0 (±6.0%) i/s -    1488003 in   5.031807s
with the exact same paths
                       399532.6 (±5.8%) i/s -    2006640 in   5.039580s
with a single-part absolute path including the base_directory
                       264620.6 (±6.4%) i/s -    1330230 in   5.047639s
with a multi-part absolute path including the base_directory
                       256029.4 (±5.9%) i/s -    1291290 in   5.061697s
with a single-part traversal path including the base_directory
                       259016.4 (±6.5%) i/s -    1293468 in   5.016217s
with a multi-part traversal path including the base_directory
                       257328.8 (±5.5%) i/s -    1300364 in   5.068768s

After

Calculating -------------------------------------
with no questionable path
                         27521 i/100ms
with a single-part questionable path
                         26225 i/100ms
with a multi-part questionable path
                         23491 i/100ms
with a single-part traversal path
                         24573 i/100ms
with a multi-part traversal path
                         24986 i/100ms
with the exact same paths
                        147425 i/100ms
with a single-part absolute path including the base_directory
                         23544 i/100ms
with a multi-part absolute path including the base_directory
                         23084 i/100ms
with a single-part traversal path including the base_directory
                         22661 i/100ms
with a multi-part traversal path including the base_directory
                         22161 i/100ms
-------------------------------------------------
with no questionable path
                       341029.3 (±2.5%) i/s -    1706302 in   5.006646s
with a single-part questionable path
                       323923.5 (±3.0%) i/s -    1625950 in   5.024114s
with a multi-part questionable path
                       284311.7 (±5.7%) i/s -    1432951 in   5.058581s
with a single-part traversal path
                       325828.7 (±2.8%) i/s -    1646391 in   5.057129s
with a multi-part traversal path
                       304258.6 (±3.1%) i/s -    1524146 in   5.014410s
with the exact same paths
                      7386492.5 (±4.1%) i/s -   37003675 in   5.019432s
with a single-part absolute path including the base_directory
                       283879.2 (±3.2%) i/s -    1436184 in   5.064790s
with a multi-part absolute path including the base_directory
                       277743.3 (±3.3%) i/s -    1408124 in   5.075801s
with a single-part traversal path including the base_directory
                       275436.9 (±2.3%) i/s -    1382321 in   5.021342s
with a multi-part traversal path including the base_directory
                       264333.5 (±2.8%) i/s -    1329660 in   5.034274s

@jekyll/windows, would you please test?

@XhmikosR
Copy link
Contributor

XhmikosR commented Nov 8, 2014

I can confirm it works fine here!

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

C:\Users\xmr\Desktop\jekyll>bundle exec rake site:preview
Running Jekyll...
Configuration file: C:/Users/xmr/Desktop/jekyll/site/_config.yml
            Source: C:/Users/xmr/Desktop/jekyll/site
       Destination: C:/Users/xmr/Desktop/jekyll/site/_site
      Generating...
C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/posix-spawn-0.3.9/lib/posix/spawn.rb:164: warning: cannot close fd before spawn
'which' is not recognized as an internal or external command, operable program or batch file.
Opening in browser...
                    done.
  Please add the following to your Gemfile to avoid polling for changes:
    require 'rbconfig'
    if RbConfig::CONFIG['target_os'] =~ /mswin|mingw|cygwin/i
      gem 'wdm', '>= 0.1.0'
    end

 Auto-regeneration: enabled for 'C:/Users/xmr/Desktop/jekyll/site'
Configuration file: C:/Users/xmr/Desktop/jekyll/site/_config.yml
    Server address: http://127.0.0.1:4000/
  Server running... press ctrl-c to stop.

BTW, maybe wdm should be part of the dependencies on Windows?

@parkr
Copy link
Member Author

parkr commented Nov 8, 2014

Great!! So glad it works. :)

I was thinking about how we could bundle wdm, but the gemspec is turned into yaml so there are no conditional gems. So either everyone has to install wdm or no one gets it for free. A shortcoming of rubygems

@XhmikosR
Copy link
Contributor

XhmikosR commented Nov 8, 2014

No big deal I guess, I mean it works even though the bundle doesn't have wdm here.

unless clean_path.start_with?(base_directory)
clean_path = clean_path.sub(/^\A\w\:\//, '/')

unless clean_path.start_with?(base_directory.sub(/^\A\w\:\//, '/'))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mastahyeti @gregose Does this look OK to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change the regex? I'm not even sure what combining ^ and \A would result in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I did this... need more 👀 on my PR's. Want to join Jekyll core? 😉 I'll get a fix for this out tonight as 2.5.2. D'oh! Thanks for catching it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking care of it here: #3089

@parkr parkr added the fix label Nov 9, 2014
@parkr parkr self-assigned this Nov 9, 2014
@parkr parkr added this to the 2.5.1 milestone Nov 9, 2014
@parkr parkr merged commit 95b62e5 into master Nov 9, 2014
@parkr parkr deleted the fix-windows-path-sanitation branch November 9, 2014 06:07
parkr added a commit that referenced this pull request Nov 9, 2014
parkr added a commit that referenced this pull request Nov 10, 2014
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jekyll 2.5.0 broken on Windows
4 participants