Remove duplicate regexp phrase: ^\A #3089

Merged
merged 1 commit into from Nov 13, 2014

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Nov 10, 2014

Addresses @mastahyeti's comment in #3077: #3077 (comment)

No clue how the tests passed with that invalid regexp. It's frustrating not being able to CI this on Windows.

@parkr parkr added the fix label Nov 10, 2014

@parkr parkr added this to the 2.5.2 milestone Nov 10, 2014

@parkr parkr self-assigned this Nov 10, 2014

@parkr parkr referenced this pull request Nov 10, 2014

Merged

Fix Windows Path Sanitation #3077

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 10, 2014

Member

@jekyll/windows: would you please test this sometime today?

Member

parkr commented Nov 10, 2014

@jekyll/windows: would you please test this sometime today?

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 10, 2014

Contributor

AppVeyor :P

Seems to work fine here, like 2.5.1 did too.

Contributor

XhmikosR commented Nov 10, 2014

AppVeyor :P

Seems to work fine here, like 2.5.1 did too.

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

This comment has been minimized.

@mastahyeti

mastahyeti Nov 10, 2014

Contributor

So, if you're running this in safe mode with Windows, an attacker could write to the correct path, but on any drive, right?

@mastahyeti

mastahyeti Nov 10, 2014

Contributor

So, if you're running this in safe mode with Windows, an attacker could write to the correct path, but on any drive, right?

This comment has been minimized.

@parkr

parkr Nov 10, 2014

Member

Because we strip the drive from the clean_path, yes, we have to be drive-agnostic when comparing. Maybe we compare the full clean_path and base_directory with drive names, then, if they don't overlap the way we prescribe, then we strip the drive name from clean_path and stuff it onto base_directory?

def sanitized_path(base_directory, questionable_path)
  clean_path = File.expand_path(questionable_path, '/')
  unless clean_path.start_with?(base_directory)
    File.join(base_directory, clean_path.sub(/\A\w:\//, '/'))
  else
    clean_path
  end
end
@parkr

parkr Nov 10, 2014

Member

Because we strip the drive from the clean_path, yes, we have to be drive-agnostic when comparing. Maybe we compare the full clean_path and base_directory with drive names, then, if they don't overlap the way we prescribe, then we strip the drive name from clean_path and stuff it onto base_directory?

def sanitized_path(base_directory, questionable_path)
  clean_path = File.expand_path(questionable_path, '/')
  unless clean_path.start_with?(base_directory)
    File.join(base_directory, clean_path.sub(/\A\w:\//, '/'))
  else
    clean_path
  end
end

This comment has been minimized.

@parkr

parkr Nov 11, 2014

Member

@mastahyeti Will ship with your approval. Perhaps I need to stub out File.expand_path such that returns the Windows paths. The real problem is that the method as it stands right now can't be fluidly tested on a unix machine.

@parkr

parkr Nov 11, 2014

Member

@mastahyeti Will ship with your approval. Perhaps I need to stub out File.expand_path such that returns the Windows paths. The real problem is that the method as it stands right now can't be fluidly tested on a unix machine.

This comment has been minimized.

@mastahyeti

mastahyeti Nov 11, 2014

Contributor

Let's not worry about this for now. I was just thinking out loud.

@mastahyeti

mastahyeti Nov 11, 2014

Contributor

Let's not worry about this for now. I was just thinking out loud.

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Nov 11, 2014

Contributor

👍

Contributor

mastahyeti commented Nov 11, 2014

👍

@parkr parkr merged commit 314fb87 into master Nov 13, 2014

1 check passed

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

@parkr parkr deleted the remove-duplication-in-regexp branch Nov 13, 2014

parkr added a commit that referenced this pull request Nov 13, 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.