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(path): correctly encode delimiters and relative paths #108

Merged
merged 17 commits into from
Apr 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"version": "2.0.0",
"tasks": [
{
"label": "rake: test",
"type": "shell",
"group":{
"kind": "test",
"isDefault": true
},
"problemMatcher": {
"owner": "ruby",
"fileLocation": ["relative", "${workspaceFolder}"],
"pattern": [
{
"regexp": "^([^:]+: .+)",
"message": 1
},
{
"regexp": "^ ([^:]+):(\\d+)",
"file": 1,
"line": 2
}
]
},
"command": "bundle exec rake test"
}
]
}
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,7 @@ gemspec
gem 'rake'
gem 'json'
gem 'minitest'
gem 'minitest-reporters'
gem 'webmock'
gem 'benchmark-ips'

48 changes: 40 additions & 8 deletions lib/imgix/path.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,14 @@ def initialize(prefix, secure_url_token, path = "/")
@secure_url_token = secure_url_token
@path = path
@options = {}

@path = CGI.escape(@path) if /^https?/ =~ @path
@path = "/#{@path}" if @path[0] != "/"
end

def to_url(opts = {})
sanitized_path = sanitize_path(@path)
prev_options = @options.dup
@options.merge!(opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but it would be great if this state mutation could be removed by passing state in method calls, like you're doing with sanitized_path.


current_path_and_params = path_and_params
current_path_and_params = path_and_params(sanitized_path)
url = @prefix + current_path_and_params

if @secure_url_token
Expand Down Expand Up @@ -130,12 +128,46 @@ def method_missing(method, *args, &block)

private

def signature(current_path_and_params)
Digest::MD5.hexdigest(@secure_url_token + current_path_and_params)
# Escape and encode any characters in path that are reserved and not utf8 encoded.
# This includes " +?:#" characters. If a path is being used as a proxy, utf8
# encode everything. If it is not being used as proxy, leave certain chars, like
# "/", alone. Method assumes path is not already encoded.
def sanitize_path(path)
# remove the leading "/", we'll add it back after encoding
path = path.slice(1, path.length) if Regexp.new('^/') =~ path
# if path is being used as a proxy, encode the entire thing
if /^https?/ =~ path
return encode_URI_Component(path)
else
# otherwise, encode only specific characters
return encode_URI(path)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Variant PR, which shifts a lot of this escaping/encoding work to application boot, this would ideally be done in the Variant initializer: https://github.com/imgix/imgix-rb/pull/104/files#diff-f7f33d40dff4d8a40100ac8a68f7fab2e082a98a683b678c8ee04c1bc930102aR8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't had a chance to dig into #104 too much, but loved what I saw so far. Will definitely keep this in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @stevehodgkiss. I could not find any encoding issues when to_url is called multiple times on the same Path, but I’m sure there will be edge cases that I haven’t thought of. Also, I think the ivar for sanitized_path creates a more predictable experience for the user.

I’ve implemented a solution based on your idea:

def to_url(opts = {})
sanitized_path ||= sanitize_path(@path)
prev_options = @options.dup
@options.merge!(opts)
current_path_and_params = path_and_params(sanitized_path)
url = @prefix + current_path_and_params
if @secure_url_token
url += (has_query? ? "&" : "?") + "s=#{signature(current_path_and_params)}"
end

Before these changes, @path was being modified in a way that was not explicit. There may be a case in the future where we want an @sanitized_path var or a santize_path! func, but for the purposes of this PR that is out of scope.

There are a lot of improvements needed in this file and the library throughout, but I felt these changes have to least potential for producing breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! I take back my previous comment about "this would ideally be done in the Variant initializer". This can't be done there since it's about the Path itself, not the Variant.


# URL encode the entire path
def encode_URI_Component(path)
return "/" + CGI.escape(path)
end

# URL encode every character in the path, including
# " +?:#" characters.
def encode_URI(path)
# For each component in the path, URL encode it and add it
# to the array path component.
path_components = []
path.split("/").each do |str|
path_components << ERB::Util.url_encode(str)
end
# Prefix and join the encoded path components.
"/#{path_components.join('/')}"
end

def signature(rest)
Digest::MD5.hexdigest(@secure_url_token + rest)
end

def path_and_params
has_query? ? "#{@path}?#{query}" : @path
def path_and_params(path)
has_query? ? "#{path}?#{query}" : path
end

def query
Expand Down
3 changes: 3 additions & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
require "minitest/autorun"
require "imgix"
require "webmock/minitest"
require "minitest/reporters"

Minitest::Reporters.use! Minitest::Reporters::SpecReporter.new

class Imgix::Test < MiniTest::Test
end
48 changes: 48 additions & 0 deletions test/units/path_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,34 @@ def test_path_with_multiple_params
assert_equal url, path.to_url
end

def test_relative_path_with_params
url = "https://demo.imgix.net/images/demo.png?h=200&w=200&s=d570a1ecd765470f7b34a69b56718a7a"
path = client.path("images/demo.png").h(200).w(200)
assert_equal url, path.to_url
end

def test_file_path_with_reserved_delimiters
urls = [
"https://demo.imgix.net/%20%3C%3E%5B%5D%7B%7D%7C%5C%5E%25.jpg?h=200&w=200&s=1731846fd046c84270d052b1152b0cfa",
"https://demo.imgix.net/%26%24%2B%2C%3A%3B%3D%3F%40%23.jpg?h=200&w=200&s=08730633f350ceb3cc6bce4caa4be55a",
]
paths = [
"/ <>[]{}|\\^%.jpg",
"&$+,:;=?@#.jpg",
]
result = true
message = "URLS Encoded Incorrectly: "

urls.each_with_index do |url, idx|
current_url = client.path(paths[idx]).h(200).w(200).to_url
if (url != current_url)
message += "\n #{current_url} != \n #{url}"
result = false
end
end
assert(result, message)
end

def test_path_with_multi_value_param_safely_encoded
url = "https://demo.imgix.net/images/demo.png?markalign=middle%2Ccenter&s=f0d0e28a739f022638f4ba6dddf9b694"
path = client.path("/images/demo.png").markalign("middle,center")
Expand All @@ -73,6 +101,26 @@ def test_param_values_are_escaped
assert_equal "https://demo.imgix.net/demo.png?hello_world=%2Ffoo%22%3E%20%3Cscript%3Ealert%28%22hacked%22%29%3C%2Fscript%3E%3C", ix_url
end

def test_unicode_path_variants_are_utf8_encoded
luqven marked this conversation as resolved.
Show resolved Hide resolved
urls = [
"https://demo.imgix.net/I%20cann%C3%B8t%20bel%C3%AE%C3%A9v%E2%88%91%20it%20wor%EF%A3%BFs%21%20%F0%9F%98%B1",
"https://demo.imgix.net/%D8%B3%D8%A7%D9%86%D8%AF%D9%88%DB%8C%DA%86.jpg"
]
paths = [ "I cannøt belîév∑ it wors! 😱", "ساندویچ.jpg"]
result = true
message = "URLs Encoded Incorrectly: "

urls.each_with_index do |url, idx|
current_url = unsigned_client.path(paths[idx]).to_url
if (url != current_url)
result = false
message += "\n #{current_url} != \n #{url}"
end
end

assert(result, message)
end

def test_base64_param_variants_are_base64_encoded
ix_url = unsigned_client.path("~text").to_url({txt64: "I cannøt belîév∑ it wors! 😱"})

Expand Down
16 changes: 16 additions & 0 deletions test/units/url_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,22 @@ def test_signing_with_multiple_params
assert_equal expected, path.to_url(h: 200, w: 200)
end

def test_calling_to_url_many_times
path = client.path(DEMO_IMAGE_PATH)
expected = ["https://demo.imgix.net/images/demo.png?h=200&w=200&s=d570a1ecd765470f7b34a69b56718a7a"]
result = []

10.times do
expected << expected[0]
end

expected.length.times do
result << path.to_url(h: 200, w: 200)
end

assert_equal expected, result
end

private

def client
Expand Down