fix(core) stop percent decoding regex path (#9024)

It's not a very reliable and sound way to support percent-encoding in regex. We choose to tell users that we have a normalized (standard) form to match with so there's no ambiguity. #8140 (comment) fix CT-344
Kong · Jul 7, 2022 · 64ebac8 · 64ebac8
1 parent a41135b
commit 64ebac8
Show file tree

Hide file tree

Showing 5 changed files with 202 additions and 80 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -125,6 +125,12 @@
   [#8988](https://github.com/Kong/kong/pull/8988)
 - `ngx.ctx.balancer_address` does not exist anymore, please use `ngx.ctx.balancer_data` instead.
   [#9043](https://github.com/Kong/kong/pull/9043)
+- Stop normalizing regex `route.path`. Regex path pattern matches with normalized URI,
+  and we used to replace percent-encoding in regex path pattern to ensure different forms of URI matches.
+  That is no longer supported. Except for reserved characters defined in
+  [rfc3986](https://datatracker.ietf.org/doc/html/rfc3986#section-2.2),
+  we should write all other characters without percent-encoding.
+  [#9024](https://github.com/Kong/kong/pull/9024)
 
 
 

diff --git a/kong/db/migrations/core/016_280_to_300.lua b/kong/db/migrations/core/016_280_to_300.lua
@@ -1,10 +1,14 @@
 local log = require "kong.cmd.utils.log"
-
+local arrays = require "pgmoon.arrays"
 
 local fmt = string.format
 local assert = assert
 local ipairs = ipairs
 local cassandra = require "cassandra"
+local find = string.find
+local upper = string.upper
+local re_find = ngx.re.find
+local encode_array  = arrays.encode_array
 
 
 -- remove repeated targets, the older ones are not useful anymore. targets with
@@ -211,6 +215,178 @@ local function c_drop_vaults_beta(coordinator)
   return true
 end
 
+-- We do not percent decode route.path after 3.0, so here we do 1 last time for them
+local normalize_regex
+do
+  local RESERVED_CHARACTERS = {
+    [0x21] = true, -- !
+    [0x23] = true, -- #
+    [0x24] = true, -- $
+    [0x25] = true, -- %
+    [0x26] = true, -- &
+    [0x27] = true, -- '
+    [0x28] = true, -- (
+    [0x29] = true, -- )
+    [0x2A] = true, -- *
+    [0x2B] = true, -- +
+    [0x2C] = true, -- ,
+    [0x2F] = true, -- /
+    [0x3A] = true, -- :
+    [0x3B] = true, -- ;
+    [0x3D] = true, -- =
+    [0x3F] = true, -- ?
+    [0x40] = true, -- @
+    [0x5B] = true, -- [
+    [0x5D] = true, -- ]
+  }
+  local REGEX_META_CHARACTERS = {
+    [0x2E] = true, -- .
+    [0x5E] = true, -- ^
+    -- $ in RESERVED_CHARACTERS
+    -- * in RESERVED_CHARACTERS
+    -- + in RESERVED_CHARACTERS
+    [0x2D] = true, -- -
+    -- ? in RESERVED_CHARACTERS
+    -- ( in RESERVED_CHARACTERS
+    -- ) in RESERVED_CHARACTERS
+    -- [ in RESERVED_CHARACTERS
+    -- ] in RESERVED_CHARACTERS
+    [0x7B] = true, -- {
+    [0x7D] = true, -- }
+    [0x5C] = true, -- \
+    [0x7C] = true, -- |
+  }
+  local ngx_re_gsub = ngx.re.gsub
+  local string_char = string.char
+
+  local function percent_decode(m)
+    local hex = m[1]
+    local num = tonumber(hex, 16)
+    if RESERVED_CHARACTERS[num] then
+      return upper(m[0])
+    end
+
+    local chr = string_char(num)
+    if REGEX_META_CHARACTERS[num] then
+      return "\\" .. chr
+    end
+
+    return chr
+  end
+
+  function normalize_regex(regex)
+    if find(regex, "%", 1, true) then
+      -- Decoding percent-encoded triplets of unreserved characters
+      return ngx_re_gsub(regex, "%([\\dA-F]{2})", percent_decode, "joi")
+    end
+    return regex
+  end
+end
+
+local function is_not_regex(path)
+  return (re_find(path, [[[a-zA-Z0-9\.\-_~/%]*$]], "ajo"))
+end
+
+local function migrate_regex(reg)
+  -- also add regex indicator
+  return normalize_regex(reg)
+end
+
+local function c_normalize_regex_path(coordinator)
+  for rows, err in coordinator:iterate("SELECT id, paths FROM routes") do
+    if err then
+      return nil, err
+    end
+
+    for i = 1, #rows do
+      local route = rows[i]
+
+
+      local changed = false
+      for i, path in ipairs(route.paths) do
+        if is_not_regex(path) then
+          goto continue
+        end
+
+        local normalized_path = migrate_regex(path)
+        if normalized_path ~= path then
+          changed = true
+          route.paths[i] = normalized_path
+        end
+        ::continue::
+      end
+
+      if changed then
+        local _, err = coordinator:execute(
+          "UPDATE routes SET paths = ? WHERE partition = 'routes' AND id = ?",
+          { cassandra.list(route.paths), cassandra.uuid(route.id) }
+        )
+        if err then
+          return nil, err
+        end
+      end
+    end
+  end
+  return true
+end
+
+local function render(template, keys)
+  return (template:gsub("$%(([A-Z_]+)%)", keys))
+end
+
+local function p_migrate_regex_path(connector)
+  for route, err in connector:iterate("SELECT id, paths FROM routes") do
+    if err then
+      return nil, err
+    end
+
+    local changed = false
+    for i, path in ipairs(route.paths) do
+      if is_not_regex(path) then
+        goto continue
+      end
+
+      local normalized_path = migrate_regex(path)
+      if normalized_path ~= path then
+        changed = true
+        route.paths[i] = normalized_path
+      end
+      ::continue::
+    end
+
+    if changed then
+      local sql = render(
+        "UPDATE routes SET paths = $(NORMALIZED_PATH) WHERE id = '$(ID)'", {
+        NORMALIZED_PATH = encode_array(route.paths),
+        ID = route.id,
+      })
+
+      local _, err = connector:query(sql)
+      if err then
+        return nil, err
+      end
+    end
+  end
+
+  return true
+end
+
+local function p_update_cache_key(connector)
+  local _, err = connector:query([[
+    DELETE FROM targets t1
+          USING targets t2
+          WHERE t1.created_at < t2.created_at
+            AND t1.upstream_id = t2.upstream_id
+            AND t1.target = t2.target;
+    UPDATE targets SET cache_key = CONCAT('targets:', upstream_id, ':', target, '::::', ws_id);
+    ]])
+
+  if err then
+    return nil, err
+  end
+
+  return true
+end
 
 return {
   postgres = {
@@ -291,15 +467,12 @@ return {
       $$;
     ]],
     teardown = function(connector)
-      local _, err = connector:query([[
-        DELETE FROM targets t1
-              USING targets t2
-              WHERE t1.created_at < t2.created_at
-                AND t1.upstream_id = t2.upstream_id
-                AND t1.target = t2.target;
-        UPDATE targets SET cache_key = CONCAT('targets:', upstream_id, ':', target, '::::', ws_id);
-        ]])
+      local _, err = p_update_cache_key(connector)
+      if err then
+        return nil, err
+      end
 
+      _, err = p_migrate_regex_path(connector)
       if err then
         return nil, err
       end
@@ -375,6 +548,11 @@ return {
         return nil, err
       end
 
+      _, err = c_normalize_regex_path(coordinator)
+      if err then
+        return nil, err
+      end
+
       return true
     end
   },

diff --git a/kong/router.lua b/kong/router.lua
@@ -56,74 +56,6 @@ local function append(destination, value)
 end
 
 
-local normalize_regex
-do
-  local RESERVED_CHARACTERS = {
-    [0x21] = true, -- !
-    [0x23] = true, -- #
-    [0x24] = true, -- $
-    [0x25] = true, -- %
-    [0x26] = true, -- &
-    [0x27] = true, -- '
-    [0x28] = true, -- (
-    [0x29] = true, -- )
-    [0x2A] = true, -- *
-    [0x2B] = true, -- +
-    [0x2C] = true, -- ,
-    [0x2F] = true, -- /
-    [0x3A] = true, -- :
-    [0x3B] = true, -- ;
-    [0x3D] = true, -- =
-    [0x3F] = true, -- ?
-    [0x40] = true, -- @
-    [0x5B] = true, -- [
-    [0x5D] = true, -- ]
-  }
-  local REGEX_META_CHARACTERS = {
-    [0x2E] = true, -- .
-    [0x5E] = true, -- ^
-    -- $ in RESERVED_CHARACTERS
-    -- * in RESERVED_CHARACTERS
-    -- + in RESERVED_CHARACTERS
-    [0x2D] = true, -- -
-    -- ? in RESERVED_CHARACTERS
-    -- ( in RESERVED_CHARACTERS
-    -- ) in RESERVED_CHARACTERS
-    -- [ in RESERVED_CHARACTERS
-    -- ] in RESERVED_CHARACTERS
-    [0x7B] = true, -- {
-    [0x7D] = true, -- }
-    [0x5C] = true, -- \
-    [0x7C] = true, -- |
-  }
-  local ngx_re_gsub = ngx.re.gsub
-  local string_char = string.char
-
-  local function percent_decode(m)
-    local hex = m[1]
-    local num = tonumber(hex, 16)
-    if RESERVED_CHARACTERS[num] then
-      return upper(m[0])
-    end
-
-    local chr = string_char(num)
-    if REGEX_META_CHARACTERS[num] then
-      return "\\" .. chr
-    end
-
-    return chr
-  end
-
-  function normalize_regex(regex)
-    if find(regex, "%", 1, true) then
-      -- Decoding percent-encoded triplets of unreserved characters
-      return ngx_re_gsub(regex, "%([\\dA-F]{2})", percent_decode, "joi")
-    end
-    return regex
-  end
-end
-
-
 local log
 do
   log = function(lvl, ...)
@@ -507,7 +439,6 @@ local function marshall_route(r)
           max_uri_length = max(max_uri_length, #path)
 
         else
-          local path = normalize_regex(path)
 
           -- regex URI
           local strip_regex  = REGEX_PREFIX .. path .. [[(?<uri_postfix>.*)]]

diff --git a/spec/01-unit/08-router_spec.lua b/spec/01-unit/08-router_spec.lua
@@ -1476,7 +1476,7 @@ describe("Router", function()
             },
           },
         },
-        -- regex
+        -- regex. It is no longer normalized since 3.0
         {
           service = service,
           route   = {
@@ -1489,7 +1489,7 @@ describe("Router", function()
           service = service,
           route   = {
             paths = {
-              "/regex-meta/%5Cd\\+%2E", -- /regex/\d+.
+              "/regex-meta/%5Cd\\+%2E", -- /regex-meta/\d\+.
             },
           },
         },
@@ -1515,6 +1515,9 @@ describe("Router", function()
 
       it("matches against regex paths", function()
         local match_t = router.select("GET", "/regex/123", "example.com")
+        assert.falsy(match_t)
+
+        match_t = router.select("GET", "/reg%65x/123", "example.com")
         assert.truthy(match_t)
         assert.same(use_case[2].route, match_t.route)
 
@@ -1527,6 +1530,9 @@ describe("Router", function()
         assert.falsy(match_t)
 
         match_t = router.select("GET", "/regex-meta/\\d+.", "example.com")
+        assert.falsy(match_t)
+
+        match_t = router.select("GET", "/regex-meta/%5Cd+%2E", "example.com")
         assert.truthy(match_t)
         assert.same(use_case[3].route, match_t.route)
       end)

diff --git a/spec/02-integration/05-proxy/02-router_spec.lua b/spec/02-integration/05-proxy/02-router_spec.lua
@@ -3,6 +3,7 @@ local helpers = require "spec.helpers"
 local cjson   = require "cjson"
 local path_handling_tests = require "spec.fixtures.router_path_handling_tests"
 
+local tonumber = tonumber
 
 local enable_buffering
 local enable_buffering_plugin