feat(cors) match configured origins as a regular expression #2482

p0pr0ck5 · 2017-05-02T05:46:25Z

Summary

Use the ngx.re API to match configured origins as regular expressions against the client Origin header. In cases where a single origin is configured for the plugin, set the ACAO header based on the plugin configuration if the configuration contains only non-PCRE metacharacters; otherwise, treat the single configured origin as a though multiple origins were configured, by iterating through the array and setting the ACAO header based on the client Origin.

This changeset will be accompanied by a sister PR that includes a migration of existing configured origins to escape . characters. Because such a migration will target Kong 0.11, and this PR targets Kong 0.10.3, we will commit these as separate PRs.

Full changelog

Examine the client Origin header against configured origins as a full expression
Test setting ACAO as the plugin-configured origin when it doesn't look like a regex
Test setting ACAO based on the client Origin when the plugin-configured origin looks like a regex
Validate the value of configured origins as a valid regular expression.

thibaultcha

Nice and user-friendly approach 👍 Just a few things to tweak imo

thibaultcha · 2017-05-02T20:26:51Z

kong/plugins/cors/handler.lua

@@ -25,21 +25,26 @@ local function configure_origin(ngx, conf)
  if #conf.origins == 1 then
    if conf.origins[1] == "*" then
      ngx.ctx.cors_allow_all = true
+      ngx.header["Access-Control-Allow-Origin"] = "*"
+      return

    else


because of the new return in the previous branch, this else becomes obsolete, better remove it

thibaultcha · 2017-05-02T20:28:40Z

kong/plugins/cors/handler.lua

+    -- if this doesnt look like a regex, set the ACAO header directly
+    -- otherwise, we'll fall through to an iterative search and
+    -- set the ACAO header based on the client Origin
+    if re_find(conf.origins[1], "^[A-Za-z0-9.:/-]+$", "jo") then


I think we should log possible errors here, just as a standardized practice among our codebase when using the ngx.re.* API (I believe other places do it, or should).

thibaultcha · 2017-05-02T20:30:22Z

kong/plugins/cors/schema.lua

+    for _, origin in ipairs(value) do
+      local _, err = re_match("just a string to test", origin)
+      if err then
+        return false, "origin " .. origin .. " is not a valid regex"


Mind adding single quotes around the user-provided origin that failed this check? Error messages in our codebase do that, or at least, should :)

BTW: cool trick!

thibaultcha · 2017-05-02T20:31:13Z

spec/03-plugins/14-cors/01-access_spec.lua

+        ["www.example.com"] = true,
+        ["example-foo.com"] = true,
+        ["www.example-foo.com"] = true,
+        ["www.example-fo0.com"] = false


style: missing a trailing comma, I can see this list being revised in the future

thibaultcha · 2017-05-02T20:32:15Z

spec/03-plugins/14-cors/01-access_spec.lua

+        })
+        assert.res_status(200, res)
+        assert.equal(domains[domain] and domain or nil,
+          res.headers["Access-Control-Allow-Origin"])


style: arguments on subsequent lines should align below the first argument (domains[domain]) of the caller's line

thibaultcha · 2017-05-02T20:34:46Z

spec/03-plugins/14-cors/02-schema_spec.lua

+      local ok, err = validate_entity({ origins = mock_origins }, cors_schema)
+
+      assert.False(ok)
+      assert.same(err.origins, "origin " .. mock_origins[2] .. " is not a valid regex")


The order of arguments is mixed up here: arg #1 should be the expected value, and arg #2 is the result (this would bring confusing error messages in case of failure). Also, side note: prefer assert.equal for string comparison (explicitness and convention followed - to its best - in the codebase).

ah yep, i had this as assert.same as i was comparing the whole table, then just switched to the field without changing the assertion, good catch

thibaultcha · 2017-05-02T22:27:37Z

kong/plugins/cors/handler.lua

+    -- if this doesnt look like a regex, set the ACAO header directly
+    -- otherwise, we'll fall through to an iterative search and
+    -- set the ACAO header based on the client Origin
+    local plain, err = re_find(conf.origins[1], "^[A-Za-z0-9.:/-]+$", "jo")


local from, to, err =, as per our recent change to the router :)

thibaultcha · 2017-05-02T22:28:26Z

kong/plugins/cors/handler.lua

+    -- set the ACAO header based on the client Origin
+    local plain, err = re_find(conf.origins[1], "^[A-Za-z0-9.:/-]+$", "jo")
+    if err then
+      ngx.log(ngx.ERR, "[cors] could not inspect origin for type")


Shouldn't we log the error itself?

"[cors] could not determine if configured origin is a regex: ", err)

thibaultcha · 2017-05-02T22:29:40Z

spec/03-plugins/14-cors/02-schema_spec.lua

+      local ok, err = validate_entity({ origins = mock_origins }, cors_schema)
+
+      assert.False(ok)
+      assert.same("origin '" .. mock_origins[2] .. "' is not a valid regex", err.origins)


(this is still assert.same)

thibaultcha · 2017-05-02T22:53:15Z

kong/plugins/cors/handler.lua

+    -- if this doesnt look like a regex, set the ACAO header directly
+    -- otherwise, we'll fall through to an iterative search and
+    -- set the ACAO header based on the client Origin
+    local from, to, err = re_find(conf.origins[1], "^[A-Za-z0-9.:/-]+$", "jo")


I think this will fail linting because to is unused :(

seems like it didn't, maybe we're ignoring those now? That's ok I think!

Yes, I ran the linter before submitting as I think this came up once before? I'll make a note to look over linter behavior shortly.

Yes, it happened on a spec/ file which, while not elegant, was acceptable. It is worrying that it is happening in a source file though...

Shall we adjust to local from, _, err then?

Depends on the linter's current state of rules (maight be desired, might be a bug), I will check... Feel free to if you have the chance!

Use the ngx.re API to match configured origins as regular expressions against the client Origin header. In cases where a single origin is configured for the plugin, set the ACAO header if the configuration contains only non-PCRE metacharacters; otherwise, treat the single configured origin as a though multiple origins were configured, by iterating through the array and setting the ACAO header based on the client Origin.

Introduced in Kong/kong#2482

configured in case of non-matched. The CORS plugin currently always sets the ACAO header based on the plugin configuration if the configuration only has a single entry and contains only non-PCRE metacharacters. This behavior is first introduced in #2482 and doesn't seem to have any real impact on the functionality I think. But this seems is not following the [mozilla guidelines](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin) > Specifies an origin. Only a single origin can be specified. If the server supports clients from multiple origins, it must return the origin for the specific client making the request. This fixes behavior by no longer sending an ACAO header in this case.

p0pr0ck5 added the pr/status/please review label May 2, 2017

p0pr0ck5 force-pushed the feat/cors-regex branch from c3cd1fe to cd3c42e Compare May 2, 2017 16:14

thibaultcha requested changes May 2, 2017

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 2, 2017

p0pr0ck5 force-pushed the feat/cors-regex branch from cd3c42e to e12bbcd Compare May 2, 2017 21:26

p0pr0ck5 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels May 2, 2017

thibaultcha reviewed May 2, 2017

View reviewed changes

p0pr0ck5 force-pushed the feat/cors-regex branch from e12bbcd to b57cd5e Compare May 2, 2017 22:38

thibaultcha reviewed May 2, 2017

View reviewed changes

thibaultcha approved these changes May 3, 2017

View reviewed changes

thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/please review labels May 3, 2017

p0pr0ck5 force-pushed the feat/cors-regex branch from b57cd5e to 5e52e92 Compare May 15, 2017 20:00

p0pr0ck5 force-pushed the feat/cors-regex branch from 5e52e92 to eeebcd0 Compare May 15, 2017 20:16

p0pr0ck5 merged commit 900ec4e into master May 15, 2017

p0pr0ck5 deleted the feat/cors-regex branch May 15, 2017 21:51

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request May 25, 2017

docs(cors) mention supported regexps in config.origins

8d5a669

Introduced in Kong/kong#2482

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request May 25, 2017

docs(cors) mention supported regexps in config.origins

2bc26e9

Introduced in Kong/kong#2482

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request May 25, 2017

docs(cors) mention supported regexps in config.origins

efe7464

Introduced in Kong/kong#2482

ms2008 mentioned this pull request May 19, 2023

fix(cors): don't send Access-Control-Allow-Origin header where a single origin(not regex) is configured #10898

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cors) match configured origins as a regular expression #2482

feat(cors) match configured origins as a regular expression #2482

p0pr0ck5 commented May 2, 2017

thibaultcha left a comment

thibaultcha May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

p0pr0ck5 May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

thibaultcha May 2, 2017

p0pr0ck5 May 2, 2017

thibaultcha May 2, 2017

p0pr0ck5 May 2, 2017 •

edited

Loading

thibaultcha May 2, 2017

feat(cors) match configured origins as a regular expression #2482

feat(cors) match configured origins as a regular expression #2482

Conversation

p0pr0ck5 commented May 2, 2017

Summary

Full changelog

thibaultcha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0pr0ck5 May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0pr0ck5 May 2, 2017 •

edited

Loading