Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core) explict flag for regex route.path #9027

Merged
merged 23 commits into from
Jul 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@
[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)

- Use `"~"` as prefix to indicate a `route.path` is a regex pattern. We no longer guess
whether a path pattern is a regex, and all path without the `"~"` prefix is considered plain text.
[#9027](https://github.com/Kong/kong/pull/9027)


#### Admin API
Expand Down
4 changes: 2 additions & 2 deletions kong/db/migrations/core/016_280_to_300.lua
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,8 @@ local function is_not_regex(path)
end

local function migrate_regex(reg)
-- also add regex indicator
return normalize_regex(reg)
local normalized = normalize_regex(reg)
return "~" .. normalized
end

local function c_normalize_regex_path(coordinator)
Expand Down
14 changes: 7 additions & 7 deletions kong/db/schema/typedefs.lua
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,10 @@ typedefs.port = Schema.define {

typedefs.path = Schema.define {
type = "string",
starts_with = "/",
match_any = {
patterns = {"^/", "^~/"},
err = "should start with: / (fixed path) or ~/ (regex path)",
},
match_none = {
{ pattern = "//",
err = "must not have empty segments"
Expand Down Expand Up @@ -446,15 +449,12 @@ local function validate_path_with_regexes(path)
return ok, err, err_code
end

-- We can't take an ok from validate_path as a success just yet,
-- because the router is currently more strict than RFC 3986 for
-- non-regex paths:
if ngx.re.find(path, [[^[a-zA-Z0-9\.\-_~/%]*$]]) then
if path:sub(1, 1) ~= "~" then
StarlightIbuki marked this conversation as resolved.
Show resolved Hide resolved
return true
end

-- URI contains characters outside of the list recognized by the
-- router as valid non-regex paths.
path = path:sub(2)

-- the value will be interpreted as a regex by the router; but is it a
-- valid one? Let's dry-run it with the same options as our router.
local _, _, err = ngx.re.find("", path, "aj")
Expand Down
4 changes: 3 additions & 1 deletion kong/router.lua
Original file line number Diff line number Diff line change
Expand Up @@ -425,8 +425,9 @@ local function marshall_route(r)
match_weight = match_weight + 1
for i = 1, count do
local path = paths[i]
local is_regex = sub(path, 1, 1) == "~"
StarlightIbuki marked this conversation as resolved.
Show resolved Hide resolved

if re_find(path, [[[a-zA-Z0-9\.\-_~/%]*$]], "ajo") then
if not is_regex then
-- plain URI or URI prefix

local uri_t = {
Expand All @@ -440,6 +441,7 @@ local function marshall_route(r)

else

path = sub(path, 2)
-- regex URI
local strip_regex = REGEX_PREFIX .. path .. [[(?<uri_postfix>.*)]]

Expand Down
2 changes: 1 addition & 1 deletion spec/01-unit/01-db/01-schema/05-services_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ describe("services", function()

local ok, err = Services:validate(service)
assert.falsy(ok)
assert.equal("should start with: /", err.path)
assert.equal("should start with: / (fixed path) or ~/ (regex path)", err.path)
end)

it("must not have empty segments (/foo//bar)", function()
Expand Down
4 changes: 2 additions & 2 deletions spec/01-unit/01-db/01-schema/06-routes_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ describe("routes schema", function()

local ok, err = Routes:validate(route)
assert.falsy(ok)
assert.equal("should start with: /", err.paths[1])
assert.equal("should start with: / (fixed path) or ~/ (regex path)", err.paths[1])
end)

it("must not have empty segments (/foo//bar)", function()
Expand All @@ -251,7 +251,7 @@ describe("routes schema", function()
local u = require("spec.helpers").unindent

local invalid_paths = {
[[/users/(foo/profile]],
[[~/users/(foo/profile]],
}

for i = 1, #invalid_paths do
Expand Down
2 changes: 1 addition & 1 deletion spec/01-unit/01-db/01-schema/09-upstreams_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ describe("load upstreams", function()
{{ active = { concurrency = 0 }}, pos_integer },
{{ active = { concurrency = -10 }}, pos_integer },
{{ active = { http_path = "" }}, len_min_default },
{{ active = { http_path = "ovo" }}, "should start with: /" },
{{ active = { http_path = "ovo" }}, "should start with: / (fixed path) or ~/ (regex path)" },
{{ active = { https_sni = "127.0.0.1", }}, invalid_ip },
{{ active = { https_sni = "127.0.0.1:8080", }}, invalid_ip },
{{ active = { https_sni = "/example", }}, invalid_host },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ describe("declarative config: validate", function()
["routes"] = {
{
["paths"] = {
"should start with: /"
"should start with: / (fixed path) or ~/ (regex path)"
}
}
}
Expand Down
38 changes: 19 additions & 19 deletions spec/01-unit/08-router_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -832,7 +832,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/users/\d+/profile]] },
paths = { [[~/users/\d+/profile]] },
},
},
}
Expand All @@ -853,19 +853,19 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/route/persons/\d{3}]] },
paths = { [[~/route/persons/\d{3}]] },
},
},
{
service = service,
route = {
paths = { [[/route/persons/\d{3}/following]] },
paths = { [[~/route/persons/\d{3}/following]] },
},
},
{
service = service,
route = {
paths = { [[/route/persons/\d{3}/[a-z]+]] },
paths = { [[~/route/persons/\d{3}/[a-z]+]] },
},
},
}
Expand All @@ -888,7 +888,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/route/persons/\d+/profile]] },
paths = { [[~/route/persons/\d+/profile]] },
},
},
}
Expand Down Expand Up @@ -916,7 +916,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { "/route/(fixture)" },
paths = { "~/route/(fixture)" },
},
},
}
Expand Down Expand Up @@ -952,7 +952,7 @@ describe("Router", function()
service = service,
route = {
hosts = { "route.com" },
paths = { "/(path)" },
paths = { "~/(path)" },
},
},
}
Expand Down Expand Up @@ -1325,7 +1325,7 @@ describe("Router", function()
service = service,
route = {
hosts = { "*.example.com" },
paths = { [[/users/\d+/profile]] },
paths = { [[~/users/\d+/profile]] },
},
},
{
Expand Down Expand Up @@ -1481,23 +1481,23 @@ describe("Router", function()
service = service,
route = {
paths = {
"/reg%65x/\\d+", -- /regex/\d+
"~/reg%65x/\\d+", -- /regex/\d+
},
},
},
{
service = service,
route = {
paths = {
"/regex-meta/%5Cd\\+%2E", -- /regex-meta/\d\+.
"~/regex-meta/%5Cd\\+%2E", -- /regex-meta/\d\+.
},
},
},
{
service = service,
route = {
paths = {
"/regex-reserved%2Fabc", -- /regex-reserved/abc
"~/regex-reserved%2Fabc", -- /regex-reserved/abc
},
},
},
Expand Down Expand Up @@ -1712,14 +1712,14 @@ describe("Router", function()
service = service,
route = {
methods = { "GET" },
paths = { [[/users/\d+/profile]] },
paths = { [[~/users/\d+/profile]] },
},
},
{
service = service,
route = {
methods = { "POST" },
paths = { [[/users/\d*/profile]] },
paths = { [[~/users/\d*/profile]] },
},
},
}
Expand Down Expand Up @@ -2440,7 +2440,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/users/\d+/profile]] },
paths = { [[~/users/\d+/profile]] },
},
},
}
Expand Down Expand Up @@ -2500,7 +2500,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/users/(?P<user_id>\d+)/profile/?(?P<scope>[a-z]*)]] },
paths = { [[~/users/(?P<user_id>\d+)/profile/?(?P<scope>[a-z]*)]] },
},
},
}
Expand Down Expand Up @@ -2572,7 +2572,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/users/\d+/profile]] },
paths = { [[~/users/\d+/profile]] },
},
},
}
Expand Down Expand Up @@ -2823,7 +2823,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/users/\d+/profile]] },
paths = { [[~/users/\d+/profile]] },
strip_path = true,
},
},
Expand All @@ -2843,7 +2843,7 @@ describe("Router", function()
{
service = service,
route = {
paths = { [[/users/(\d+)/profile]] },
paths = { [[~/users/(\d+)/profile]] },
strip_path = true,
},
},
Expand Down Expand Up @@ -3195,7 +3195,7 @@ describe("Router", function()
if line.route_path then -- skip test cases which match on host
for j, test in ipairs(line:expand()) do
local strip = test.strip_path and "on" or "off"
local regex = "/[0]?" .. test.route_path:sub(2, -1)
local regex = "~/[0]?" .. test.route_path:sub(2, -1)
local description = string.format("(%d-%d) regex, %s with %s, strip = %s, %s. req: %s",
i, j, test.service_path, regex, strip, test.path_handling, test.request_path)

Expand Down
4 changes: 2 additions & 2 deletions spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ describe("Admin API: #" .. strategy, function()
})
body = assert.res_status(400, res)
local json = cjson.decode(body)
assert.same({ hash_on_cookie_path = "should start with: /" }, json.fields)
assert.same({ hash_on_cookie_path = "should start with: / (fixed path) or ~/ (regex path)" }, json.fields)

-- Invalid cookie in hash fallback
res = assert(client:send {
Expand Down Expand Up @@ -455,7 +455,7 @@ describe("Admin API: #" .. strategy, function()
})
body = assert.res_status(400, res)
local json = cjson.decode(body)
assert.same({ hash_on_cookie_path = "should start with: /" }, json.fields)
assert.same({ hash_on_cookie_path = "should start with: / (fixed path) or ~/ (regex path)" }, json.fields)

end
end)
Expand Down
4 changes: 2 additions & 2 deletions spec/02-integration/04-admin_api/10-services_routes_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -843,11 +843,11 @@ for _, strategy in helpers.each_strategy() do
message = unindent([[
2 schema violations
(host: required field missing;
path: should start with: /)
path: should start with: / (fixed path) or ~/ (regex path))
]], true, true),
fields = {
host = "required field missing",
path = "should start with: /",
path = "should start with: / (fixed path) or ~/ (regex path)",
},
}, json
)
Expand Down
Loading