Don't unescape URIs twice

Resolves: #149.

app/main.lua

@@ -39,6 +39,7 @@ if config.throttler ~= nil and config.throttler.whitelist[ip_address] == nil the
     end
 end
 
+-- Be careful: keys and values are unescaped according to URI escaping rules.
 local args, args_err = ngx.req.get_uri_args()
 if args.api == '3' then
     -- Internal redirection to the old api, if requested

spec/client_spec.lua

@@ -257,33 +257,35 @@ describe("client", function()
         end)
 
         it("max redirects error", function()
-            local redirect = 'https://ory.weserv.nl/image2.jpg?foo=bar'
+            -- Make sure that we unescape the redirect URI.
+            -- See: https://github.com/weserv/images/issues/142
+            local redirect = 'https://ory.weserv.nl/%252A/image2.jpg?foo=bar'
 
             response.status = 302
             response.headers['Location'] = redirect
 
-            local res, err = client:request("ory.weserv.nl/image.jpg?foo=bar")
+            local res, err = client:request("ory.weserv.nl/%2A/image.jpg?foo=bar")
 
             assert.equal(404, err.status)
             assert.equal(string.format("Will not follow more than %d redirects", default_config.max_redirects),
                 err.message)
             assert.falsy(res)
 
             assert.spy(stubbed_http.request).was.called(10)
-            assert.spy(stubbed_http.request).was.called_with(match._, match.same({
+            assert.spy(stubbed_http.request).was.called_with(match._, match.contains({
                 headers = {
                     ['User-Agent'] = default_config.user_agent
                 },
-                path = '/image.jpg',
+                path = '/%2A/image.jpg',
                 query = 'foo=bar',
             }))
-            assert.spy(stubbed_http.request).was.called_with(match._, match.same({
+            assert.spy(stubbed_http.request).was.called_with(match._, match.contains({
                 headers = {
                     ['User-Agent'] = default_config.user_agent,
                     -- Referer needs to be added
-                    ['Referer'] = 'http://ory.weserv.nl/image.jpg?foo=bar'
+                    ['Referer'] = 'http://ory.weserv.nl/%2A/image.jpg?foo=bar'
                 },
-                path = '/image2.jpg',
+                path = '/%2A/image2.jpg',
                 query = 'foo=bar',
             }))
             assert.spy(stubbed_http.close).was.called()

spec/helpers/utils_spec.lua

@@ -90,33 +90,40 @@ describe("utils", function()
             { utils.parse_uri('http:\\ory.weserv.nl') })
         assert.are.same({ nil, "Invalid domain label" },
             { utils.parse_uri('--example--.org') })
+        -- Doesn't escape special / reserved characters
+        -- in accordance with RFC 3986
+        -- See: https://github.com/weserv/images/issues/145 and
+        -- https://github.com/weserv/images/issues/144
         assert.are.same({
             'https', 'ory.weserv.nl', 443,
-            "/-._~!$'()*%20,;=:#@",
+            "/-._~!$'()*+,;=:#@%",
             "bar=foo"
         }, {
-            unpack(utils.parse_uri("//ory.weserv.nl/-._~!$'()*+,;=:#@?bar=foo"))
+            unpack(utils.parse_uri("//ory.weserv.nl/-._~!$'()*+,;=:#@%?bar=foo"))
         })
         assert.are.same({
             'https', 'ory.weserv.nl', 443,
-            "/-._~!$'()*+,;=:/?#@",
-            "bar=foo"
+            "/",
+            "bar=-._~!$'()*+,;=:/@%"
         }, {
-            unpack(utils.parse_uri("//ory.weserv.nl/%2D%2E%5F%7E%21%24%27%28%29%2A%2B%2C%3B%3D%3A%2F%3F%23%40?bar=foo"))
+            unpack(utils.parse_uri("//ory.weserv.nl/?bar=-._~!$'()*+,;=:/@%"))
         })
+
+        -- Doesn't unescape twice
+        -- See: https://github.com/weserv/images/issues/149
         assert.are.same({
             'https', 'ory.weserv.nl', 443,
             "/",
-            "bar=-._~!$'()*%20,;=:/@"
+            "bar=%2D%2E%5F%7E%21%24%27%28%29%2A%2B%2C%3B%3D%3A%2F%40"
         }, {
-            unpack(utils.parse_uri("//ory.weserv.nl/?bar=-._~!$'()*+,;=:/@"))
+            unpack(utils.parse_uri("//ory.weserv.nl/?bar=%2D%2E%5F%7E%21%24%27%28%29%2A%2B%2C%3B%3D%3A%2F%40"))
         })
         assert.are.same({
             'https', 'ory.weserv.nl', 443,
-            "/",
-            "bar=-._~!$'()*+,;=:/@"
+            "/%2D%2E%5F%7E%21%24%27%28%29%2A%2B%2C%3B%3D%3A%2F%3F%23%40",
+            "bar=foo"
         }, {
-            unpack(utils.parse_uri("//ory.weserv.nl/?bar=%2D%2E%5F%7E%21%24%27%28%29%2A%2B%2C%3B%3D%3A%2F%40"))
+            unpack(utils.parse_uri("//ory.weserv.nl/%2D%2E%5F%7E%21%24%27%28%29%2A%2B%2C%3B%3D%3A%2F%3F%23%40?bar=foo"))
         })
     end)
 

src/weserv/client.lua

@@ -183,7 +183,8 @@ function client:request(uri, addl_headers, redirect_nr)
         }
 
         -- recursive call
-        return self:request(res.headers['Location'], referer, redirect_nr + 1)
+        -- Note: Make sure that we unescape the redirect URI.
+        return self:request(ngx.unescape_uri(res.headers['Location']), referer, redirect_nr + 1)
     end
 
     local valid, invalid_err = self:is_valid_response(res)

src/weserv/helpers/utils.lua

@@ -27,10 +27,14 @@ local VIPS_META_ORIENTATION = 'orientation'
 -- vips_icc_transform() operations.
 local VIPS_META_ICC_NAME = 'icc-profile-data'
 
--- Only alphanumerics, the special characters "-._~!$&'()*+,;=", and reserved
--- characters used for delimiters purposes may be used unencoded within a URI.
+-- Only alphanumerics, the percent character (prevents us from encoding
+-- already percent-encoded URIs), the special characters "-._~!$&'()*+,;=",
+-- and reserved characters used for delimiters purposes may be used
+-- unencoded within a URI. The regex (in accordance with the naming
+-- convention of RFC 3986) is as follows:
+-- *( unreserved / pct-encoded / gen-delims / sub-delims )
 -- See: https://tools.ietf.org/html/rfc3986#section-2.2
-local REGEX_DISALLOWED_CHARS = "[^%w%-%._~!%$&'%(%)%*%+,;=:/%?#@]"
+local REGEX_DISALLOWED_CHARS = "[^%w%-%._~%%!%$&'%(%)%*%+,;=:/%?#@]"
 
 --- Are pixel values in this image 16-bit integer?
 -- @param interpretation The VipsInterpretation
@@ -127,12 +131,13 @@ function utils.percent_encode(char)
 end
 
 --- Determines a "canonical" equivalent of a URI path.
+-- Assumes that the URI path is already unescaped.
 -- @param path The URI path to canonicalise.
 -- @return The canonicalised path.
 function utils.canonicalise_path(path)
     local segments = {}
     for segment in path:gmatch("/([^/]*)") do
-        segments[#segments + 1] = ngx.unescape_uri(segment):gsub(REGEX_DISALLOWED_CHARS, utils.percent_encode)
+        segments[#segments + 1] = segment:gsub(REGEX_DISALLOWED_CHARS, utils.percent_encode)
     end
     local len = #segments
     if len == 0 then
@@ -144,13 +149,14 @@ function utils.canonicalise_path(path)
 end
 
 --- Determines a "canonical" equivalent of a query string.
+-- Assumes that the query string is already unescaped.
 -- @param path The query string to canonicalise.
 -- @return The canonicalised query string.
 function utils.canonicalise_query_string(query)
     local q = {}
     for key, val in query:gmatch("([^&=]+)=?([^&]*)") do
-        key = ngx.unescape_uri(key):gsub(REGEX_DISALLOWED_CHARS, utils.percent_encode)
-        val = ngx.unescape_uri(val):gsub(REGEX_DISALLOWED_CHARS, utils.percent_encode)
+        key = key:gsub(REGEX_DISALLOWED_CHARS, utils.percent_encode)
+        val = val:gsub(REGEX_DISALLOWED_CHARS, utils.percent_encode)
         q[#q + 1] = key .. "=" .. val
     end
     return table.concat(q, "&")