feat(pdk) normalize kong.request.get_path

CHANGELOG.md

@@ -102,6 +102,10 @@
   [#8815](https://github.com/Kong/kong/pull/8815)
 - The dataplane config cache was removed. The config persistence is now done automatically with LMDB.
   [#8704](https://github.com/Kong/kong/pull/8704)
+- The `kong.request.get_path()` PDK function now performs path normalization
+  on the string that is returned to the caller. The raw, non-normalized version
+  of the request path can be fetched via `kong.request.get_raw_path()`.
+  [8823](https://github.com/Kong/kong/pull/8823)
 
 #### Admin API
 

kong/pdk/request.lua

@@ -9,6 +9,7 @@
 local cjson = require "cjson.safe".new()
 local multipart = require "multipart"
 local phase_checker = require "kong.pdk.private.phases"
+local normalize = require("kong.tools.uri").normalize
 
 
 local ngx = ngx
@@ -370,18 +371,48 @@ local function new(self)
   end
 
 
+  ---
+  -- Returns the normalized path component of the request's URL. The return
+  -- value is the same as `kong.request.get_raw_path()` but normalized according
+  -- to RFC 3986 section 6:
+  --
+  -- * Percent-encoded values of unreserved characters are decoded (`%20`
+  --   becomes ` `).
+  -- * Percent-encoded values of reserved characters have their hexidecimal
+  --   value uppercased (`%2f` becomes `%2F`).
+  -- * Relative path elements (`/.` and `/..`) are dereferenced.
+  -- * Duplicate slashes are consolidated (`//` becomes `/`).
+  --
+  -- @function kong.request.get_path
+  -- @phases rewrite, access, header_filter, response, body_filter, log, admin_api
+  -- @treturn string the path
+  -- @usage
+  -- -- Given a request to https://example.com/t/Abc%20123%C3%B8%2f/parent/..//test/./
+  --
+  -- kong.request.get_path() -- "/t/Abc 123ø%2F/test/"
+  function _REQUEST.get_path()
+    return normalize(_REQUEST.get_raw_path(), true)
+  end
+
+
   ---
   -- Returns the path component of the request's URL. It is not normalized in
   -- any way and does not include the query string.
   --
-  -- @function kong.request.get_path
+  -- **NOTE:** Using the raw path to perform string comparision during request
+  -- handling (such as in routing, ACL/authorization checks, setting rate-limit
+  -- keys, etc) is widely regarded as insecure, as it can leave plugin code
+  -- vulnerable to path traversal attacks. Prefer `kong.request.get_path()` for
+  -- such use cases.
+  --
+  -- @function kong.request.get_raw_path
   -- @phases rewrite, access, header_filter, response, body_filter, log, admin_api
   -- @treturn string The path.
   -- @usage
-  -- -- Given a request to https://example.com:1234/v1/movies?movie=foo
+  -- -- Given a request to https://example.com/t/Abc%20123%C3%B8%2f/parent/..//test/./?movie=foo
   --
-  -- kong.request.get_path() -- "/v1/movies"
-  function _REQUEST.get_path()
+  -- kong.request.get_raw_path() -- "/t/Abc%20123%C3%B8%2f/parent/..//test/./"
+  function _REQUEST.get_raw_path()
     check_phase(PHASES.request)
 
     local uri = ngx.var.request_uri or ""

t/01-pdk/04-request/00-phase_checks.t

@@ -162,6 +162,18 @@ qq{
                 body_filter   = true,
                 log           = true,
                 admin_api     = true,
+            }, {
+                method        = "get_raw_path",
+                args          = {},
+                init_worker   = false,
+                certificate   = "pending",
+                rewrite       = true,
+                access        = true,
+                header_filter = true,
+                response      = true,
+                body_filter   = true,
+                log           = true,
+                admin_api     = true,
             }, {
                 method        = "get_path_with_query",
                 args          = {},

t/01-pdk/04-request/09-get_path.t

@@ -17,13 +17,13 @@ __DATA__
             local PDK = require "kong.pdk"
             local pdk = PDK.new()
 
-            ngx.say("path: ", pdk.request.get_path())
+            ngx.say("normalized path: ", pdk.request.get_path())
         }
     }
 --- request
 GET /t
 --- response_body
-path: /t
+normalized path: /t
 --- no_error_log
 [error]
 
@@ -37,33 +37,33 @@ path: /t
             local PDK = require "kong.pdk"
             local pdk = PDK.new()
 
-            ngx.say("path: ", pdk.request.get_path())
+            ngx.say("normalized path: ", pdk.request.get_path())
         }
     }
 --- request
 GET http://kong
 --- response_body
-path: /
+normalized path: /
 --- no_error_log
 [error]
 
 
 
-=== TEST 3: request.get_path() is not normalized
+=== TEST 3: request.get_path() is normalized
 --- http_config eval: $t::Util::HttpConfig
 --- config
     location /t/ {
         access_by_lua_block {
             local PDK = require "kong.pdk"
             local pdk = PDK.new()
 
-            ngx.say("path: ", pdk.request.get_path())
+            ngx.say("normalized path: ", pdk.request.get_path())
         }
     }
 --- request
-GET /t/Abc%20123%C3%B8/../test/.
+GET /t/Abc%20123%C3%B8/parent/../test/.
 --- response_body
-path: /t/Abc%20123%C3%B8/../test/.
+normalized path: /t/Abc 123ø/test/
 --- no_error_log
 [error]
 
@@ -77,12 +77,12 @@ path: /t/Abc%20123%C3%B8/../test/.
             local PDK = require "kong.pdk"
             local pdk = PDK.new()
 
-            ngx.say("path: ", pdk.request.get_path())
+            ngx.say("normalized path: ", pdk.request.get_path())
         }
     }
 --- request
 GET /t/demo?param=value
 --- response_body
-path: /t/demo
+normalized path: /t/demo
 --- no_error_log
 [error]

t/01-pdk/04-request/20-get_raw_path.t

@@ -0,0 +1,88 @@
+use strict;
+use warnings FATAL => 'all';
+use Test::Nginx::Socket::Lua;
+do "./t/Util.pm";
+
+plan tests => repeat_each() * (blocks() * 3);
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: request.get_raw_path() returns path component of uri
+--- http_config eval: $t::Util::HttpConfig
+--- config
+    location = /t {
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            ngx.say("path: ", pdk.request.get_raw_path())
+        }
+    }
+--- request
+GET /t
+--- response_body
+path: /t
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: request.get_raw_path() returns at least slash
+--- http_config eval: $t::Util::HttpConfig
+--- config
+    location = / {
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            ngx.say("path: ", pdk.request.get_raw_path())
+        }
+    }
+--- request
+GET http://kong
+--- response_body
+path: /
+--- no_error_log
+[error]
+
+
+
+=== TEST 3: request.get_raw_path() is not normalized
+--- http_config eval: $t::Util::HttpConfig
+--- config
+    location /t/ {
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            ngx.say("path: ", pdk.request.get_raw_path())
+        }
+    }
+--- request
+GET /t/Abc%20123%C3%B8/../test/.
+--- response_body
+path: /t/Abc%20123%C3%B8/../test/.
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: request.get_raw_path() strips query string
+--- http_config eval: $t::Util::HttpConfig
+--- config
+    location /t/ {
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            ngx.say("path: ", pdk.request.get_raw_path())
+        }
+    }
+--- request
+GET /t/demo?param=value
+--- response_body
+path: /t/demo
+--- no_error_log
+[error]