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

patch URLFilters to prevent `//` #6058

Merged
merged 2 commits into from Jun 14, 2017

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Apr 29, 2017

Fix #6057

/cc @jekyll/ecosystem
requesting additional review from @parkr as well.

Show outdated Hide outdated lib/jekyll/filters/url_filters.rb
def sanitized_baseurl
baseurl = site.config["baseurl"]
return baseurl unless baseurl.nil? || (baseurl == "/")

This comment has been minimized.

@0xdevalias

0xdevalias Apr 29, 2017

Will this fail and not return if baseurl is nil? (Unsure of ruby specifics for unless)

What happens if baseurl is set to /foo/bar/ or even something really silly like "///////" or /foo/bar//" etc?

Can we compress trailing slashes to a single one, or removed entirely, as required by the concatenation?

@0xdevalias

0xdevalias Apr 29, 2017

Will this fail and not return if baseurl is nil? (Unsure of ruby specifics for unless)

What happens if baseurl is set to /foo/bar/ or even something really silly like "///////" or /foo/bar//" etc?

Can we compress trailing slashes to a single one, or removed entirely, as required by the concatenation?

This comment has been minimized.

@0xdevalias

0xdevalias Apr 29, 2017

And I guess I haven't really checked what currently happens, but what if similarly there is some silliness going on in the site URL part with slashes. Can we robustly handle this in both potential locations?

@0xdevalias

0xdevalias Apr 29, 2017

And I guess I haven't really checked what currently happens, but what if similarly there is some silliness going on in the site URL part with slashes. Can we robustly handle this in both potential locations?

This comment has been minimized.

@pathawks

pathawks Apr 29, 2017

Member

What happens if baseurl is set to /foo/bar/ or even something really silly like "///////" or /foo/bar//" etc?

Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?

@pathawks

pathawks Apr 29, 2017

Member

What happens if baseurl is set to /foo/bar/ or even something really silly like "///////" or /foo/bar//" etc?

Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?

This comment has been minimized.

@ashmaroli

ashmaroli Apr 30, 2017

Member

Will this fail and not return if baseurl is nil?

Ruby will return nil if baseurl equals nil or /

Can we compress trailing slashes to a single one, or removed entirely, as required by the concatenation?

IMO substituting a regex match with something is not necessary as it is known to affect performance. But again, there is no issue in employing that method if there's a strong use case. Setting baseurl with multiple slashes is wrong.
The least appropriate measure would be to output a warning message that the baseurl has been configured incorrectly.

@ashmaroli

ashmaroli Apr 30, 2017

Member

Will this fail and not return if baseurl is nil?

Ruby will return nil if baseurl equals nil or /

Can we compress trailing slashes to a single one, or removed entirely, as required by the concatenation?

IMO substituting a regex match with something is not necessary as it is known to affect performance. But again, there is no issue in employing that method if there's a strong use case. Setting baseurl with multiple slashes is wrong.
The least appropriate measure would be to output a warning message that the baseurl has been configured incorrectly.

Show outdated Hide outdated test/test_filters.rb
@@ -448,6 +457,15 @@ def select; end
})
assert_equal "/base", filter.relative_url(page_url)
end
should "not prepend a forward slash if both input and baseurl are simply '/'" do

This comment has been minimized.

@0xdevalias

0xdevalias Apr 29, 2017

Should this be "append"?

@0xdevalias

0xdevalias Apr 29, 2017

Should this be "append"?

@0xdevalias

This comment has been minimized.

Show comment
Hide comment
@0xdevalias

0xdevalias Apr 30, 2017

0xdevalias commented Apr 30, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Apr 30, 2017

Member

But multiple slashes are perfectly legal in a URL. Do we really want to stop supporting that just so setting baseurl: "////////" has no effect?

Let's try to figure out what the root problem is and try to fix that with as few unintended consequences as possible, rather than trying to decide how hypothetical situations should be magically changed without the users permission.

Member

pathawks commented Apr 30, 2017

But multiple slashes are perfectly legal in a URL. Do we really want to stop supporting that just so setting baseurl: "////////" has no effect?

Let's try to figure out what the root problem is and try to fix that with as few unintended consequences as possible, rather than trying to decide how hypothetical situations should be magically changed without the users permission.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 30, 2017

Member

updated to return valid url if baseurl == /base/

Member

ashmaroli commented Apr 30, 2017

updated to return valid url if baseurl == /base/

@parkr

parkr approved these changes Jun 14, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 14, 2017

Member

@jekyllbot: merge +bug

Member

parkr commented Jun 14, 2017

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 2a4d33e into jekyll:master Jun 14, 2017

2 checks passed

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

@jekyllbot jekyllbot added bug fix labels Jun 14, 2017

jekyllbot added a commit that referenced this pull request Jun 14, 2017

@DirtyF DirtyF added this to the 3.5 milestone Jun 18, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Jun 20, 2017

Member

Would it make sense to partially revert this (remove the private method :site introduced here) instead of fixing plugins downstream?

/cc @parkr @benbalter @pathawks

Member

ashmaroli commented Jun 20, 2017

Would it make sense to partially revert this (remove the private method :site introduced here) instead of fixing plugins downstream?

/cc @parkr @benbalter @pathawks

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 20, 2017

Member

Would it make sense to partially revert this (remove the private method :site introduced here) instead of fixing plugins downstream?

Making this more backwards-compatible sounds like a great idea!

Member

parkr commented Jun 20, 2017

Would it make sense to partially revert this (remove the private method :site introduced here) instead of fixing plugins downstream?

Making this more backwards-compatible sounds like a great idea!

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