Repository navigation
fix(router): forward the matched path when nginx normalizes the request uri - #14035
shreemaan-abhishek wants to merge 3 commits into
Conversation
| path = str_sub(path, 1, args_pos - 1) | ||
| end | ||
| if path ~= uri and has_dot_segment(path) then | ||
| api_ctx.var.upstream_uri = core.utils.uri_safe_encode(uri) |
There was a problem hiding this comment.
Setting $upstream_uri here, before any plugin has run, freezes both the path and the raw query for every dot-segment request. A non-empty $upstream_uri wins in proxy_pass, so anything that later edits the request through ngx.req.set_uri / set_uri_args instead of writing $upstream_uri gets silently thrown away. The worst case is hide_credentials: key-auth / jwt-auth strip a query credential with set_uri_args, so the credential now reaches the upstream as soon as the client adds a ./:
# consumer key SECRET; route /ka/* with key-auth {hide_credentials = true}
--- request
GET /ka/./x?apikey=SECRET&a=1
--- response_body
request-uri=/ka/x?a=1
That passes on master. With this PR the upstream gets /ka/x?apikey=SECRET&a=1. Same thing with a serverless function: ngx.req.set_uri("/svl/changed") on GET /svl/./x reaches the upstream as /svl/x, and ngx.req.set_uri_args("added=1") on GET /sva/./x?b=2 still sends ?b=2. Reading the code, degraphql's GET path (set_uri_args) and grpc-web (ngx.req.set_uri) have the same problem. proxy-mirror mirrors the frozen value, credential included.
ext-plugin is affected too, because it treats a non-empty $upstream_uri as "rewritten" (ext-plugin/init.lua ~494). On dot-segment requests the runner gets the encoded path instead of var.uri, and the frozen query is written back into $args with set_uri_args, so ext-plugin-post-req puts back the credential key-auth just removed. A runner that rewrites only args loses the change (~708), because upstream_uri is only rebuilt when the runner also returns a path. The comment in the websocket path ("upstream_uri is only ever set by plugins like proxy-rewrite", ~1227), which ext-plugin, ext-plugin-post-resp and proxy-mirror also depend on, is no longer true either.
The raw query also ends up in $upstream_uri, and the default access_log_format logs that variable, so data-mask's query masking no longer covers these requests. With data-mask replacing password, GET /mask/./x?password=abc logs "http://localhost/mask/x?password=abc" next to the masked $request_line. On master nothing unmasked is logged.
I'd leave $upstream_uri alone and invalidate the unparsed URI instead. Nginx then rebuilds the request line from the live r->uri / r->args at proxy time, which is already what happens on master whenever key-auth strips a query credential:
if uri_normalized_by_nginx(api_ctx.var.request_uri, uri) then
-- binary: $uri may hold decoded control chars, proxy_pass escapes them
ngx.req.set_uri(uri, false, true)
endI tried this locally (with the $uri-oracle helper from my other comment). Every case above goes back to master's behaviour, the bypass cases stay fixed, the PR's test file passes, and t/router, key-auth, jwt-auth, proxy-rewrite, proxy-mirror, data-mask, serverless and core/ctx show no new failures compared with this PR head. You need the binary flag: without it, GET /api/./a%0Ab becomes a 500 (unsafe byte "0x0a").
I also tried the other direction: keep the flag here, but set $upstream_uri late (right before handle_upstream, only if it is still empty), from ngx.var.uri plus ngx.var.args. That's the same split Kong uses between access.before and access.after. It fixes hide_credentials, the set_uri/set_uri_args cases and ext-plugin, because all of those run before the assignment. But the raw query still ends up in the access log, so I prefer set_uri.
|
|
||
| -- whether the decoded path has a "." or ".." segment | ||
| local function has_dot_segment(path) | ||
| if not core.string.find(path, ".") and not core.string.find(path, "%") then |
There was a problem hiding this comment.
has_dot_segment re-implements part of nginx's normalization and misses the forms it doesn't model. One of them is a literal #: nginx cuts the path there before it resolves dot segments, so GET /admin/..#x gets $uri = /. The decoded raw path /admin/..#x/ contains no /../, so the raw line is forwarded unchanged even though the request was matched as /:
# route uri = "/" without auth, upstream echoes $request_uri
--- raw_request eval
"GET /admin/..#x HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n"
--- response_body
request-uri=/
(It currently returns request-uri=/admin/..#x.) This is narrower in practice than ..%2F, but it's the same class. Rather than modelling nginx, I'd use $uri itself as the oracle, the way build_match_uri_keep_encoded_slash already does: the raw path is safe to pass through only if its percent-decoding, with slashes merged, equals $uri. That covers dot segments, fragments and anything else nginx does, and it still leaves %2F-only and //-only requests alone (TEST 6 and TEST 7 here still pass with it).
It's also cheaper. Right now every request with a query pays for a str_sub copy, and every percent-encoded path pays for a re_gsub decode with a Lua callback. With a has_prefix check first, an unchanged request costs a single comparison and no allocation:
local function uri_normalized_by_nginx(request_uri, uri)
if request_uri == uri then
return false
end
if core.string.has_prefix(request_uri, uri)
and str_byte(request_uri, #uri + 1) == str_byte("?") then
return false
end
local path = request_uri
local args_pos = str_find(path, "?", 1, true)
if args_pos then
path = str_sub(path, 1, args_pos - 1)
end
local decoded = percent_decode(path, false)
if decoded == uri then
return false
end
return re_gsub(decoded, [[//+]], "/", "jo") ~= uri
end| -- Nginx resolved dot segments in $uri, so forward the path the route was | ||
| -- matched on. | ||
| if not (local_conf.apisix and local_conf.apisix.normalize_uri_like_servlet) then | ||
| local path = api_ctx.var.request_uri |
There was a problem hiding this comment.
nit: this query strip duplicates the one in the match_uri_encoded_slash block a few lines below. If the helper takes request_uri directly (sketch in the comment on has_dot_segment), the raw path is derived in one place.
|
|
||
| -- Nginx resolved dot segments in $uri, so forward the path the route was | ||
| -- matched on. | ||
| if not (local_conf.apisix and local_conf.apisix.normalize_uri_like_servlet) then |
There was a problem hiding this comment.
Minor: this changes what upstreams receive by default for every dot-segment request, and there's no switch to get the old behaviour back. I think that's fine for a security fix. Kong did the same in 2.3.2 (Kong/kong#6821): it changed the default in a patch release without a switch. It's still worth a CHANGELOG / upgrade note, because an upstream that relied on seeing the raw form will notice the difference.
| merges consecutive slashes. When the decoded request path has no `.` or `..` | ||
| segment, APISIX forwards the original request line to the upstream, consecutive | ||
| slashes included. When it has one, APISIX forwards the matched path re-encoded, | ||
| followed by the original query string. For example, a request to |
There was a problem hiding this comment.
Two things here don't match what actually happens.
With normalize_uri_like_servlet this block is skipped, and the servlet branch sets $upstream_uri without args, so the query is dropped. That's already the case on master, and for every request, not just dot-segment ones:
# yaml: apisix.normalize_uri_like_servlet: true, route /api/*
--- request
GET /api/x?q=1
--- response_body
request-uri=/api/x?q=1
The upstream gets /api/x, both on master and on this PR. That probably needs its own fix, but this sentence shouldn't read as if it also applied to servlet mode.
It's also worth documenting a trade-off: the forwarded path is $uri re-encoded, so encoded delimiters near a dot segment arrive decoded. GET /api/./a%2Fb%3Bc reaches the upstream as /api/a/b;c, while GET /api/a%2Fb is forwarded raw. For the ..%2F form this can't be avoided, since that %2F is exactly the slash nginx treated as a separator when matching. For /./a%2Fb-style requests it could be avoided by removing dot segments on the raw path and using the result only when its decoding equals $uri. I don't think that's worth the extra code, though, so a sentence here is enough.
|
|
||
|
|
||
|
|
||
| === TEST 8: proxy-rewrite uri still takes precedence |
There was a problem hiding this comment.
The cases that would have caught the problems above are missing:
- a plugin that changes the args or the path after matching (key-auth with
hide_credentialsand the key in the query, a serverlessngx.req.set_uri/set_uri_args) - a single
.segment (/api/./x; every case here uses..) - a
#after.. - the interaction with
delete_uri_tail_slash(/api/./x/should forward/api/x/like/api/x/does, which works today because the pre-stripuriis used) and with servlet mode
Snippets for most of these are in the init.lua comments.
3 participants
Description
Routes are matched on the normalized
$uri: Nginx percent-decodes the path, resolves dot segments (including encoded ones such as..%2Fand%2e%2e), cuts the path at#and merges consecutive slashes. With$upstream_urileft empty,proxy_passthen forwards the original request line. When normalization changed the path, the upstream receives a different path from the one the route was matched on. For example,/admin/..%2Fapi/xmatches a route for/api/*, but the upstream receives/admin/..%2Fapi/x.This PR forwards the path the route was matched on whenever Nginx normalized the request path:
http_access_phase,uri_normalized_by_nginxcompares the raw request path with$uri. If its percent-decoding, with consecutive slashes merged, differs from$uri, Nginx did more than decode the path, andngx.req.set_uri($uri, false, true)is called.proxy_passthen rebuilds the request line from the current path and query string.$upstream_uristays empty, so path and query changes made by plugins after matching (key-auth and jwt-authhide_credentials,ngx.req.set_uri/set_uri_argsin serverless functions) still reach the upstream, and the query string is not copied into$upstream_uri.binaryflag lets a decoded control char in$urithrough;proxy_passescapes it.$urionly by percent-encoding or consecutive slashes still forward the original request line, so%2Finside path parameters and URLs embedded in the path are preserved. An unchanged request costs a single comparison.$upstream_uri(such asproxy-rewrite) still take precedence.normalize_uri_like_servletis left as is, since it sets the upstream path itself.Behavior change
Upstreams that received a request path Nginx normalized (
/a/../b,/a/%2e%2e/b,/a/..%2Fb,/a/..#x) now receive the normalized path (/b,/), escaped by Nginx. Encoded delimiters in such a path arrive decoded (/a/./b%2Fcreaches the upstream as/a/b/c). Other requests are unaffected. There is no switch to restore the old behavior, so this is worth an upgrade note in the CHANGELOG.Which issue(s) this PR fixes:
Fixes #
Checklist