feat: Expose an option for (real_)request_uri to be used in proxy-rewrite plugin

[ilteriseroglu-ty]

Description

Hello!

During our tests we have noticed that APISIX always uses the normalized/decoded URI coming from nginx (as per its design, which isn't an issue), and this creates an edge case where a backend that requires URL Encoded URIs to not work properly (i.e. GitLab API calls that reference paths inside a project use %2F instead of /).

Looking at the codebase, we've also seen the native request_uri variable being moved to the real_request_uri variable. And because of how proxy-rewrites rewrite schema work; utilizing real_request_uri to bypass the encoding is not possible.

We know that this is a really unsafe behavior to utilize real_request_uri but the cost of the edge cases this causes is critical. We've copied over proxy-rewrite to our downstream custom plugins repo as proxy-rewrite-unsafe with our addition of the real_request_uri override:

--- proxy-rewrite.lua	2022-07-05 09:49:53.000000000 +0300
+++ proxy-rewrite-unsafe.lua	2022-07-05 10:02:39.000000000 +0300
@@ -15,7 +15,7 @@
 -- limitations under the License.
 --
 local core        = require("apisix.core")
-local plugin_name = "proxy-rewrite"
+local plugin_name = "proxy-rewrite-unsafe"
 local pairs       = pairs
 local ipairs      = ipairs
 local ngx         = ngx
@@ -40,6 +40,11 @@
 local schema = {
     type = "object",
     properties = {
+        use_real_request_uri_unsafe = {
+            description = "use real_request_uri (request_uri in nginx), THIS IS UNSAFE.)",
+            type        = "boolean",
+            default     = false,
+        },
         uri = {
             description = "new uri for upstream",
             type        = "string",
@@ -161,7 +166,9 @@
     end
 
     local upstream_uri = ctx.var.uri
-    if conf.uri ~= nil then
+    if conf.use_real_request_uri_unsafe then
+        upstream_uri = core.utils.resolve_var("$real_request_uri", ctx.var)
+    elseif conf.uri ~= nil then
         upstream_uri = core.utils.resolve_var(conf.uri, ctx.var)
     elseif conf.regex_uri ~= nil then
         local uri, _, err = re_sub(ctx.var.uri, conf.regex_uri[1],
@@ -178,18 +185,22 @@
     end
 
     local index = str_find(upstream_uri, "?")
-    if index then
-        upstream_uri = core.utils.uri_safe_encode(sub_str(upstream_uri, 1, index-1)) ..
-                       sub_str(upstream_uri, index)
-    else
-        upstream_uri = core.utils.uri_safe_encode(upstream_uri)
+    if not conf.use_real_request_uri_unsafe then
+        if index then
+            upstream_uri = core.utils.uri_safe_encode(sub_str(upstream_uri, 1, index-1)) ..
+                           sub_str(upstream_uri, index)
+        else
+            upstream_uri = core.utils.uri_safe_encode(upstream_uri)
+        end
     end
 
     if ctx.var.is_args == "?" then
-        if index then
-            ctx.var.upstream_uri = upstream_uri .. "&" .. (ctx.var.args or "")
-        else
-            ctx.var.upstream_uri = upstream_uri .. "?" .. (ctx.var.args or "")
+        if not conf.use_real_request_uri_unsafe then
+            if index then
+                ctx.var.upstream_uri = upstream_uri .. "&" .. (ctx.var.args or "")
+            else
+                ctx.var.upstream_uri = upstream_uri .. "?" .. (ctx.var.args or "")
+            end
         end
     else
         ctx.var.upstream_uri = upstream_uri

Is it possible to expose an option on upstream like in the diff above?

[tzssangglass]

This seems to be the implementation of: #5654 (comment)

I can accept this suggestion, but it seems that the implementation could be even better.

We can use use_real_request_uri_unsafe to control whether to use core.utils.uri_safe_encode?

[spacewander]

The suggestion LGTM.

[ilteriseroglu-ty]

This seems to be the implementation of: #5654 (comment)

I can accept this suggestion, but it seems that the implementation could be even better.

We can use use_real_request_uri_unsafe to control whether to use core.utils.uri_safe_encode?

The best I can think of is to merge both the if nots for use_real_request_uri_unsafe:

--- proxy-rewrite.lua	2022-07-05 09:49:53.000000000 +0300
+++ proxy-rewrite-unsafe.lua	2022-07-06 10:17:58.000000000 +0300
@@ -15,7 +15,7 @@
 -- limitations under the License.
 --
 local core        = require("apisix.core")
-local plugin_name = "proxy-rewrite"
+local plugin_name = "proxy-rewrite-unsafe"
 local pairs       = pairs
 local ipairs      = ipairs
 local ngx         = ngx
@@ -40,6 +40,11 @@
 local schema = {
     type = "object",
     properties = {
+        use_real_request_uri_unsafe = {
+            description = "use real_request_uri (request_uri in nginx), THIS IS VERY UNSAFE.",
+            type        = "boolean",
+            default     = false,
+        },
         uri = {
             description = "new uri for upstream",
             type        = "string",
@@ -161,7 +166,9 @@
     end
 
     local upstream_uri = ctx.var.uri
-    if conf.uri ~= nil then
+    if conf.use_real_request_uri_unsafe then
+        upstream_uri = core.utils.resolve_var("$real_request_uri", ctx.var)
+    elseif conf.uri ~= nil then
         upstream_uri = core.utils.resolve_var(conf.uri, ctx.var)
     elseif conf.regex_uri ~= nil then
         local uri, _, err = re_sub(ctx.var.uri, conf.regex_uri[1],
@@ -177,22 +184,24 @@
         end
     end
 
-    local index = str_find(upstream_uri, "?")
-    if index then
-        upstream_uri = core.utils.uri_safe_encode(sub_str(upstream_uri, 1, index-1)) ..
-                       sub_str(upstream_uri, index)
-    else
-        upstream_uri = core.utils.uri_safe_encode(upstream_uri)
-    end
-
-    if ctx.var.is_args == "?" then
+    if not conf.use_real_request_uri_unsafe then
+        local index = str_find(upstream_uri, "?")
         if index then
-            ctx.var.upstream_uri = upstream_uri .. "&" .. (ctx.var.args or "")
+            upstream_uri = core.utils.uri_safe_encode(sub_str(upstream_uri, 1, index-1)) ..
+                           sub_str(upstream_uri, index)
+        else
+            upstream_uri = core.utils.uri_safe_encode(upstream_uri)
+        end
+
+        if ctx.var.is_args == "?" then
+            if index then
+                ctx.var.upstream_uri = upstream_uri .. "&" .. (ctx.var.args or "")
+            else
+                ctx.var.upstream_uri = upstream_uri .. "?" .. (ctx.var.args or "")
+            end
         else
-            ctx.var.upstream_uri = upstream_uri .. "?" .. (ctx.var.args or "")
+            ctx.var.upstream_uri = upstream_uri
         end
-    else
-        ctx.var.upstream_uri = upstream_uri
     end
 
     if conf.headers then

To give you an example of this issue, this is an URL from GitLab's API to fetch metadata from a file in a repository: https://gitlab.com/api/v4/projects/28679334/repository/files/testfiles%2Fgentestfiles.sh?ref=master&hotel=trivago

[tzssangglass]

The best I can think of is to merge both the if nots for use_real_request_uri_unsafe:

PR is welcome!