fix(api schema) better check for request_host

The old check would allow a value of `/hello` as a `request_host`.

This change uses a Lua pattern to validate a `request_host` and does so
in the `check_request_host` function, removing the need for the `regex`
field in schema (not used anywhere else). However, the `regex` schema field
capability is not removed by this change.

This also slightly improves the checks made on `request_host` and
`request_path` and adds many test cases for `rquest_host` validation.

Fix Kong#856

kong/dao/schemas/apis.lua

@@ -13,22 +13,46 @@ local function validate_upstream_url_protocol(value)
   return true
 end
 
-local function check_request_host_and_path(value, api_t)
+local function check_request_host_and_path(api_t)
   local request_host = type(api_t.request_host) == "string" and stringy.strip(api_t.request_host) or ""
   local request_path = type(api_t.request_path) == "string" and stringy.strip(api_t.request_path) or ""
 
   if request_path == "" and request_host == "" then
     return false, "At least a 'request_host' or a 'request_path' must be specified"
   end
 
-  -- Validate wildcard request_host
-  if request_host then
+  return true
+end
+
+local host_allowed_chars = "[%d%a%-%.%_]"
+local ext_allowed_chars = "[%d%a]"
+local dns_pattern = "^"..host_allowed_chars.."+%."..ext_allowed_chars..ext_allowed_chars.."+$"
+
+local function check_request_host(request_host, api_t)
+  local valid, err = check_request_host_and_path(api_t)
+  if valid == false then
+    return false, err
+  end
+
+  if request_host ~= nil and request_host ~= "" then
     local _, count = request_host:gsub("%*", "")
-    if count > 1 then
-      return false, "Only one wildcard is allowed: "..request_host
-    elseif count > 0 then
-      local pos = request_host:find("%*")
+    if count == 0 then
+      -- Validate regular request_host
+      local match = request_host:match(dns_pattern)
+      if match == nil then
+        return false, "Invalid value: "..request_host
+      end
+
+      -- Reject prefix/trailing dashes and dots in each segment
+      for _, segment in ipairs(stringy.split(request_host, ".")) do
+        if segment == "" or segment:match("^-") or segment:match("-$") or segment:match("^%.") or segment:match("%.$") then
+          return false, "Invalid value: "..request_host
+        end
+      end
+    elseif count == 1 then
+      -- Validate wildcard request_host
       local valid
+      local pos = request_host:find("%*")
       if pos == 1 then
         valid = request_host:match("^%*%.") ~= nil
       elseif pos == string.len(request_host) then
@@ -38,17 +62,21 @@ local function check_request_host_and_path(value, api_t)
       if not valid then
         return false, "Invalid wildcard placement: "..request_host
       end
+    else
+      return false, "Only one wildcard is allowed: "..request_host
     end
   end
+
+  return true
 end
 
 local function check_request_path(request_path, api_t)
-  local valid, err = check_request_host_and_path(request_path, api_t)
+  local valid, err = check_request_host_and_path(api_t)
   if valid == false then
     return false, err
   end
 
-  if request_path then
+  if request_path ~= nil and request_path ~= "" then
     request_path = string.gsub(request_path, "^/*", "")
     request_path = string.gsub(request_path, "/*$", "")
 
@@ -79,7 +107,7 @@ local function default_name(api_t)
   local default_name, err, _
 
   default_name = api_t.request_host
-  if not default_name and api_t.request_path then
+  if default_name == nil and api_t.request_path ~= nil then
     default_name = api_t.request_path:sub(2):gsub("/", "-")
   end
 
@@ -120,8 +148,7 @@ return {
     id = {type = "id", dao_insert_value = true},
     created_at = {type = "timestamp", immutable = true, dao_insert_value = true},
     name = {type = "string", unique = true, queryable = true, default = default_name, func = check_name},
-    request_host = {type = "string", unique = true, queryable = true, func = check_request_host_and_path,
-                  regex = "([a-zA-Z0-9-]+(\\.[a-zA-Z0-9-]+)*)"},
+    request_host = {type = "string", unique = true, queryable = true, func = check_request_host},
     request_path = {type = "string", unique = true, func = check_request_path},
     strip_request_path = {type = "boolean"},
     upstream_url = {type = "url", required = true, func = validate_upstream_url_protocol},

spec/integration/admin_api/apis_routes_spec.lua

@@ -56,11 +56,15 @@ describe("Admin API", function()
           assert.equal(400, status)
           assert.equal('{"message":"Cannot parse JSON body"}\n', response)
         end)
-        it_content_types("should return proper validation errors", function(content_type)
+        it_content_types("return proper validation errors", function(content_type)
           return function()
             local response, status = http_client.post(BASE_URL, {}, {["content-type"] = content_type})
             assert.equal(400, status)
             assert.equal([[{"upstream_url":"upstream_url is required","request_path":"At least a 'request_host' or a 'request_path' must be specified","request_host":"At least a 'request_host' or a 'request_path' must be specified"}]], stringy.strip(response))
+
+            response, status = http_client.post(BASE_URL, {request_host = "/httpbin", upstream_url = "http://mockbin.com"}, {["content-type"] = content_type})
+            assert.equal(400, status)
+            assert.equal([[{"request_host":"Invalid value: \/httpbin"}]], stringy.strip(response))
           end
         end)
         it_content_types("should return HTTP 409 if already exists", function(content_type)
@@ -221,7 +225,7 @@ describe("Admin API", function()
                 request_host = " "
               }, {["content-type"] = content_type})
               assert.equal(400, status)
-              assert.equal([[{"request_host":"request_host has an invalid value"}]], stringy.strip(response))
+              assert.equal([[{"request_host":"At least a 'request_host' or a 'request_path' must be specified"}]], stringy.strip(response))
             end
           end)
         end)
@@ -241,7 +245,7 @@ describe("Admin API", function()
           assert.equal(204, status)
           assert.falsy(response)
         end)
-        it("delete an API by name #only", function()
+        it("delete an API by name", function()
           local response, status = http_client.delete(BASE_URL.."to-delete")
           assert.equal(204, status)
           assert.falsy(response)

spec/unit/dao/entities_schemas_spec.lua

@@ -67,7 +67,7 @@ describe("Entities Schemas", function()
       assert.True(valid)
     end)
 
-    it("should complain if missing `request_host` and `request_path`", function()
+    it("should complain if missing request_host and request_path", function()
       local valid, errors = validate_entity({
         name = "mockbin"
       }, api_schema)
@@ -84,15 +84,86 @@ describe("Entities Schemas", function()
       assert.equal("At least a 'request_host' or a 'request_path' must be specified", errors.request_host)
     end)
 
-    it("should complain if `request_host` is an empty string", function()
+    it("should complain if request_host is an empty string", function()
       local t = {request_host = "", upstream_url = "http://mockbin.com"}
 
       local valid, errors = validate_entity(t, api_schema)
       assert.False(valid)
-      assert.equal("request_host has an invalid value", errors.request_host)
+      assert.equal("At least a 'request_host' or a 'request_path' must be specified", errors.request_host)
       assert.equal("At least a 'request_host' or a 'request_path' must be specified", errors.request_path)
     end)
 
+    it("should complain if request_path is an empty string", function()
+      local t = {request_path = "", upstream_url = "http://mockbin.com"}
+
+      local valid, errors = validate_entity(t, api_schema)
+      assert.False(valid)
+      assert.equal("At least a 'request_host' or a 'request_path' must be specified", errors.request_host)
+      assert.equal("At least a 'request_host' or a 'request_path' must be specified", errors.request_path)
+    end)
+
+    it("should not accept an invalid request_host", function()
+      local invalids = {"/mockbin", ".mockbin", "mockbin.", "mockbin.f", "mock;bin",
+                        "mockbin.com-org", "mockbin.com/org", "mockbin.com_org",
+                        "-mockbin.org", "mockbin-.org", "mockbin.or-g", "mockbin.org-",
+                        "mockbin.-org", "hello.-mockbin.com", "hello..mockbin.com", "hello-.mockbin.com"}
+
+      for _, v in ipairs(invalids) do
+        local t = {request_host = v, upstream_url = "http://mockbin.com"}
+        local valid, errors = validate_entity(t, api_schema)
+        assert.equal("Invalid value: "..v, (errors and errors.request_host or ""))
+        assert.falsy(errors.request_path)
+        assert.False(valid)
+      end
+    end)
+
+    it("should accept valid request_host", function()
+      local valids = {"hello.com", "hello.fr", "test.hello.com", "1991.io", "hello.COM",
+                      "HELLO.com", "123helloWORLD.com", "mockbin.123", "mockbin-api.com",
+                      "hello.abcd", "mockbin_api.com"}
+
+      for _, v in ipairs(valids) do
+        local t = {request_host = v, upstream_url = "http://mockbin.com"}
+        local valid, errors = validate_entity(t, api_schema)
+        assert.falsy(errors)
+        assert.True(valid)
+      end
+    end)
+
+    it("should accept valid wildcard request_host", function()
+      local valids = {"mockbin.*", "*.mockbin.org"}
+
+      for _, v in ipairs(valids) do
+        local t = {request_host = v, upstream_url = "http://mockbin.com"}
+        local valid, errors = validate_entity(t, api_schema)
+        assert.falsy(errors)
+        assert.True(valid)
+      end
+    end)
+
+    it("should refuse request_host with more than one wildcard", function()
+      local api_t = {
+        name = "mockbin",
+        request_host = "*.mockbin.*",
+        upstream_url = "http://mockbin.com"
+      }
+
+      local valid, errors = validate_entity(api_t, api_schema)
+      assert.False(valid)
+      assert.equal("Only one wildcard is allowed: *.mockbin.*", errors.request_host)
+    end)
+
+    it("should refuse invalid wildcard request_host", function()
+      local invalids = {"*mockbin.com", "www.mockbin*", "mock*bin.com"}
+
+      for _, v in ipairs(invalids) do
+        local t = {request_host = v, upstream_url = "http://mockbin.com"}
+        local valid, errors = validate_entity(t, api_schema)
+        assert.equal("Invalid wildcard placement: "..v, (errors and errors.request_host or ""))
+        assert.False(valid)
+      end
+    end)
+
     it("should set the name from request_host if not set", function()
       local t = {request_host = "mockbin.com", upstream_url = "http://mockbin.com"}
 
@@ -123,12 +194,12 @@ describe("Entities Schemas", function()
     end)
 
     it("should normalize a name for URI if coming from request_host or request_path", function()
-      local t = {upstream_url = "http://mockbin.com", request_host = "mockbin#com"}
+      local t = {upstream_url = "http://mockbin.com", request_host = "mockbin.com"}
 
       local valid, errors = validate_entity(t, api_schema)
       assert.True(valid)
       assert.falsy(errors)
-      assert.equal("mockbin-com", t.name)
+      assert.equal("mockbin.com", t.name)
 
       t = {upstream_url = "http://mockbin.com", request_path = "/mockbin/status"}
 
@@ -138,47 +209,7 @@ describe("Entities Schemas", function()
       assert.equal("mockbin-status", t.name)
     end)
 
-    it("should accept valid wildcard request_host", function()
-      local valid, errors = validate_entity({
-        name = "mockbin",
-        request_host = "*.mockbin.org",
-        upstream_url = "http://mockbin.com"
-      }, api_schema)
-      assert.True(valid)
-      assert.falsy(errors)
-
-      valid, errors = validate_entity({
-        name = "mockbin",
-        request_host = "mockbin.*",
-        upstream_url = "http://mockbin.com"
-      }, api_schema)
-      assert.True(valid)
-      assert.falsy(errors)
-    end)
-
-    it("should refuse invalid wildcard request_host", function()
-      local api_t = {
-        name = "mockbin",
-        request_host = "*.mockbin.*",
-        upstream_url = "http://mockbin.com"
-      }
-
-      local valid, errors = validate_entity(api_t, api_schema)
-      assert.False(valid)
-      assert.equal("Only one wildcard is allowed: *.mockbin.*", errors.request_host)
-
-      api_t.request_host = "*mockbin.com"
-      valid, errors = validate_entity(api_t, api_schema)
-      assert.False(valid)
-      assert.equal("Invalid wildcard placement: *mockbin.com", errors.request_host)
-
-      api_t.request_host = "www.mockbin*"
-      valid, errors = validate_entity(api_t, api_schema)
-      assert.False(valid)
-      assert.equal("Invalid wildcard placement: www.mockbin*", errors.request_host)
-    end)
-
-    it("should only accept alphanumeric `request_path`", function()
+    it("should only accept alphanumeric request_path", function()
       local valid, errors = validate_entity({
         name = "mockbin",
         request_path = "/[a-zA-Z]{3}",
@@ -202,7 +233,7 @@ describe("Entities Schemas", function()
       assert.True(valid)
     end)
 
-    it("should prefix a `request_path` with a slash and remove trailing slash", function()
+    it("should prefix a request_path with a slash and remove trailing slash", function()
       local api_t = { name = "mockbin", request_path = "status", upstream_url = "http://mockbin.com" }
       validate_entity(api_t, api_schema)
       assert.equal("/status", api_t.request_path)
@@ -352,4 +383,18 @@ describe("Entities Schemas", function()
       end)
     end)
   end)
+
+  describe("update", function()
+    it("should only validate updated fields", function()
+      local t = {request_host = "", upstream_url = "http://mockbin.com"}
+
+      local valid, errors = validate_entity(t, api_schema, {
+        update = true
+      })
+      assert.False(valid)
+      assert.same({
+        request_host = "At least a 'request_host' or a 'request_path' must be specified"
+      }, errors)
+    end)
+  end)
 end)