Skip to content

fix(router): forward the matched path when nginx normalizes the request uri - #14035

Open
shreemaan-abhishek wants to merge 1 commit into
apache:masterfrom
shreemaan-abhishek:fix/forward-matched-uri
Open

shreemaan-abhishek wants to merge 1 commit into
apache:masterfrom
shreemaan-abhishek:fix/forward-matched-uri

Conversation

@shreemaan-abhishek

Copy link
Copy Markdown
Contributor

Description

Routes are matched on the normalized $uri: Nginx percent-decodes the path, resolves dot segments (including encoded ones such as ..%2F and %2e%2e) and merges consecutive slashes. With $upstream_uri left empty, proxy_pass then 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/x matches a route for /api/*, but the upstream receives /admin/..%2Fapi/x.

This PR forwards the path the route was matched on whenever Nginx did more than percent-decode it:

  • In http_access_phase, if a plain percent-decode of the raw request path differs from $uri, $upstream_uri is set to the re-encoded $uri plus the original query string. This reuses the same equivalence check as match_uri_encoded_slash.
  • Requests that only need percent-decoding still forward the original request line unchanged, so %2F inside path parameters and other encodings are preserved.
  • Plugins that set $upstream_uri later (such as proxy-rewrite) still take precedence.
  • normalize_uri_like_servlet is left as is, since it already forwards the normalized path.

Behavior change

Upstreams that received raw dot segments or consecutive slashes in the request path (/a/../b, /a/%2e%2e/b, /a//b) now receive the normalized path (/b, /a/b). Requests without these are unaffected.

Which issue(s) this PR fixes:

Fixes #

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant