Skip to content

Commit

Permalink
fix(tools.uri) normalization function excessive percent decoding
Browse files Browse the repository at this point in the history
### Summary

This is alternative to PR #8139 where we actually fix the normalization
function to not do excessive percent-decoding on normalization.
  • Loading branch information
bungle committed Jan 3, 2022
1 parent 8af9f0e commit e1f9ac7
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 16 deletions.
3 changes: 1 addition & 2 deletions kong/runloop/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ local subsystem = ngx.config.subsystem
local clear_header = ngx.req.clear_header
local http_version = ngx.req.http_version
local unpack = unpack
local escape = require("kong.tools.uri").escape


local is_http_module = subsystem == "http"
Expand Down Expand Up @@ -1337,7 +1336,7 @@ return {
-- router, which might have truncated it (`strip_uri`).
-- `host` is the original header to be preserved if set.
var.upstream_scheme = match_t.upstream_scheme -- COMPAT: pdk
var.upstream_uri = escape(match_t.upstream_uri)
var.upstream_uri = match_t.upstream_uri
if match_t.upstream_host then
var.upstream_host = match_t.upstream_host
end
Expand Down
31 changes: 19 additions & 12 deletions kong/tools/uri.lua
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
local _M = {}


local string_char = string.char
local string_upper = string.upper
local string_find = string.find
Expand Down Expand Up @@ -35,6 +32,7 @@ local RESERVED_CHARACTERS = {
[0x5D] = true, -- ]
}


local ESCAPE_PATTERN = "[^!#$&'()*+,/:;=?@[\\]A-Z\\d-_.~%]"

local TMP_OUTPUT = require("table.new")(16, 0)
Expand All @@ -52,12 +50,21 @@ local function percent_decode(m)
end


local function escape(m)
local function percent_escape(m)
return string_format("%%%02X", string_byte(m[0]))
end


function _M.normalize(uri, merge_slashes)
local function escape(uri)
if ngx_re_find(uri, ESCAPE_PATTERN, "joi") then
return ngx_re_gsub(uri, ESCAPE_PATTERN, percent_escape, "joi")
end

return uri
end


local function unescape(uri, merge_slashes)
-- check for simple cases and early exit
if uri == "" or uri == "/" then
return uri
Expand Down Expand Up @@ -148,13 +155,13 @@ function _M.normalize(uri, merge_slashes)
end


function _M.escape(uri)
if ngx_re_find(uri, ESCAPE_PATTERN, "joi") then
return ngx_re_gsub(uri, ESCAPE_PATTERN, escape, "joi")
end

return uri
local function normalize(uri, merge_slashes)
return escape(unescape(uri, merge_slashes))
end


return _M
return {
escape = escape,
unescape = unescape,
normalize = normalize,
}
38 changes: 38 additions & 0 deletions spec/01-unit/08-router_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2494,6 +2494,44 @@ describe("Router", function()
assert.equal(2, #match_t.matches.uri_captures)
end)

it("returns uri_captures normalized, fix #7913", function()
local use_case = {
{
service = service,
route = {
paths = { [[/users/(?P<fullname>[a-zA-Z\s\d%]+)/profile/?(?P<scope>[a-z]*)]] },
},
},
}

local router = assert(Router.new(use_case))
local _ngx = mock_ngx("GET", "/users/%6aohn%20doe/profile", { host = "domain.org" })

router._set_ngx(_ngx)
local match_t = router.exec()
assert.equal("john%20doe", match_t.matches.uri_captures[1])
assert.equal("john%20doe", match_t.matches.uri_captures.fullname)
assert.equal("", match_t.matches.uri_captures[2])
assert.equal("", match_t.matches.uri_captures.scope)
-- returns the full match as well
assert.equal("/users/john%20doe/profile", match_t.matches.uri_captures[0])
-- no stripped_uri capture
assert.is_nil(match_t.matches.uri_captures.stripped_uri)
assert.equal(2, #match_t.matches.uri_captures)

-- again, this time from the LRU cache
local match_t = router.exec()
assert.equal("john%20doe", match_t.matches.uri_captures[1])
assert.equal("john%20doe", match_t.matches.uri_captures.fullname)
assert.equal("", match_t.matches.uri_captures[2])
assert.equal("", match_t.matches.uri_captures.scope)
-- returns the full match as well
assert.equal("/users/john%20doe/profile", match_t.matches.uri_captures[0])
-- no stripped_uri capture
assert.is_nil(match_t.matches.uri_captures.stripped_uri)
assert.equal(2, #match_t.matches.uri_captures)
end)

it("returns no uri_captures from a [uri prefix] match", function()
local use_case = {
{
Expand Down
14 changes: 12 additions & 2 deletions spec/01-unit/18-tools_uri_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,12 @@ describe("kong.tools.uri", function()
assert.equal("/a/b/", uri.normalize("/a//b//", true))
end)

it("decodes non-ASCII characters that are unreserved, issue #2366", function()
assert.equal("/endeløst", uri.normalize("/endel%C3%B8st"))
it("does not decode non-ASCII characters that are unreserved, issue #2366", function()
assert.equal("/endel%C3%B8st", uri.normalize("/endel%C3%B8st"))
end)

it("does normalize complex uri that has characters outside of normal uri charset", function()
assert.equal("/%C3%A4/a./a/_%99%AF%2F%2F" , uri.normalize("/ä/a/%2e./a%2E//a/%2e/./a/../a/%2e%2E/%5f%99%af%2f%2F", true))
end)
end)

Expand All @@ -66,4 +70,10 @@ describe("kong.tools.uri", function()
assert.equal("/endel%C3%B8st", uri.escape("/endeløst"))
end)
end)

describe("unescape()", function()
it("decodes non-ASCII characters that are unreserved, issue #2366", function()
assert.equal("/endeløst", uri.unescape("/endel%C3%B8st"))
end)
end)
end)

0 comments on commit e1f9ac7

Please sign in to comment.