Skip to content

Commit e1f9ac7

Browse files
committed
fix(tools.uri) normalization function excessive percent decoding
### Summary This is alternative to PR #8139 where we actually fix the normalization function to not do excessive percent-decoding on normalization.
1 parent 8af9f0e commit e1f9ac7

File tree

4 files changed

+70
-16
lines changed

4 files changed

+70
-16
lines changed

kong/runloop/handler.lua

+1-2
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ local subsystem = ngx.config.subsystem
3939
local clear_header = ngx.req.clear_header
4040
local http_version = ngx.req.http_version
4141
local unpack = unpack
42-
local escape = require("kong.tools.uri").escape
4342

4443

4544
local is_http_module = subsystem == "http"
@@ -1337,7 +1336,7 @@ return {
13371336
-- router, which might have truncated it (`strip_uri`).
13381337
-- `host` is the original header to be preserved if set.
13391338
var.upstream_scheme = match_t.upstream_scheme -- COMPAT: pdk
1340-
var.upstream_uri = escape(match_t.upstream_uri)
1339+
var.upstream_uri = match_t.upstream_uri
13411340
if match_t.upstream_host then
13421341
var.upstream_host = match_t.upstream_host
13431342
end

kong/tools/uri.lua

+19-12
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,3 @@
1-
local _M = {}
2-
3-
41
local string_char = string.char
52
local string_upper = string.upper
63
local string_find = string.find
@@ -35,6 +32,7 @@ local RESERVED_CHARACTERS = {
3532
[0x5D] = true, -- ]
3633
}
3734

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

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

5452

55-
local function escape(m)
53+
local function percent_escape(m)
5654
return string_format("%%%02X", string_byte(m[0]))
5755
end
5856

5957

60-
function _M.normalize(uri, merge_slashes)
58+
local function escape(uri)
59+
if ngx_re_find(uri, ESCAPE_PATTERN, "joi") then
60+
return ngx_re_gsub(uri, ESCAPE_PATTERN, percent_escape, "joi")
61+
end
62+
63+
return uri
64+
end
65+
66+
67+
local function unescape(uri, merge_slashes)
6168
-- check for simple cases and early exit
6269
if uri == "" or uri == "/" then
6370
return uri
@@ -148,13 +155,13 @@ function _M.normalize(uri, merge_slashes)
148155
end
149156

150157

151-
function _M.escape(uri)
152-
if ngx_re_find(uri, ESCAPE_PATTERN, "joi") then
153-
return ngx_re_gsub(uri, ESCAPE_PATTERN, escape, "joi")
154-
end
155-
156-
return uri
158+
local function normalize(uri, merge_slashes)
159+
return escape(unescape(uri, merge_slashes))
157160
end
158161

159162

160-
return _M
163+
return {
164+
escape = escape,
165+
unescape = unescape,
166+
normalize = normalize,
167+
}

spec/01-unit/08-router_spec.lua

+38
Original file line numberDiff line numberDiff line change
@@ -2494,6 +2494,44 @@ describe("Router", function()
24942494
assert.equal(2, #match_t.matches.uri_captures)
24952495
end)
24962496

2497+
it("returns uri_captures normalized, fix #7913", function()
2498+
local use_case = {
2499+
{
2500+
service = service,
2501+
route = {
2502+
paths = { [[/users/(?P<fullname>[a-zA-Z\s\d%]+)/profile/?(?P<scope>[a-z]*)]] },
2503+
},
2504+
},
2505+
}
2506+
2507+
local router = assert(Router.new(use_case))
2508+
local _ngx = mock_ngx("GET", "/users/%6aohn%20doe/profile", { host = "domain.org" })
2509+
2510+
router._set_ngx(_ngx)
2511+
local match_t = router.exec()
2512+
assert.equal("john%20doe", match_t.matches.uri_captures[1])
2513+
assert.equal("john%20doe", match_t.matches.uri_captures.fullname)
2514+
assert.equal("", match_t.matches.uri_captures[2])
2515+
assert.equal("", match_t.matches.uri_captures.scope)
2516+
-- returns the full match as well
2517+
assert.equal("/users/john%20doe/profile", match_t.matches.uri_captures[0])
2518+
-- no stripped_uri capture
2519+
assert.is_nil(match_t.matches.uri_captures.stripped_uri)
2520+
assert.equal(2, #match_t.matches.uri_captures)
2521+
2522+
-- again, this time from the LRU cache
2523+
local match_t = router.exec()
2524+
assert.equal("john%20doe", match_t.matches.uri_captures[1])
2525+
assert.equal("john%20doe", match_t.matches.uri_captures.fullname)
2526+
assert.equal("", match_t.matches.uri_captures[2])
2527+
assert.equal("", match_t.matches.uri_captures.scope)
2528+
-- returns the full match as well
2529+
assert.equal("/users/john%20doe/profile", match_t.matches.uri_captures[0])
2530+
-- no stripped_uri capture
2531+
assert.is_nil(match_t.matches.uri_captures.stripped_uri)
2532+
assert.equal(2, #match_t.matches.uri_captures)
2533+
end)
2534+
24972535
it("returns no uri_captures from a [uri prefix] match", function()
24982536
local use_case = {
24992537
{

spec/01-unit/18-tools_uri_spec.lua

+12-2
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,12 @@ describe("kong.tools.uri", function()
4747
assert.equal("/a/b/", uri.normalize("/a//b//", true))
4848
end)
4949

50-
it("decodes non-ASCII characters that are unreserved, issue #2366", function()
51-
assert.equal("/endeløst", uri.normalize("/endel%C3%B8st"))
50+
it("does not decode non-ASCII characters that are unreserved, issue #2366", function()
51+
assert.equal("/endel%C3%B8st", uri.normalize("/endel%C3%B8st"))
52+
end)
53+
54+
it("does normalize complex uri that has characters outside of normal uri charset", function()
55+
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))
5256
end)
5357
end)
5458

@@ -66,4 +70,10 @@ describe("kong.tools.uri", function()
6670
assert.equal("/endel%C3%B8st", uri.escape("/endeløst"))
6771
end)
6872
end)
73+
74+
describe("unescape()", function()
75+
it("decodes non-ASCII characters that are unreserved, issue #2366", function()
76+
assert.equal("/endeløst", uri.unescape("/endel%C3%B8st"))
77+
end)
78+
end)
6979
end)

0 commit comments

Comments
 (0)