WIP: fix(cors) allow pre-flight requests to options / trace / connect endpoints #4899

eshepelyuk · 2019-08-13T10:31:17Z

Summary

Fix cors plugin configuration and behavior for processing pre-flight requests to OPTIONS / TRACE / CONNECT endpoints

Full changelog

allow pre-flight requests to options / trace / connect endpoints

Issues resolved

CLAassistant · 2019-08-13T10:31:25Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

intpp · 2019-08-13T10:40:13Z

👍

thibaultcha

Thanks for the patch! Would you also contribute test cases for this fix please?

kong/plugins/cors/handler.lua

kong/plugins/cors/schema.lua

eshepelyuk · 2019-08-13T17:08:27Z

Thanks for the patch! Would you also contribute test cases for this fix please?

sure, just waiting for the design approval (some sort of message or anything), that means the team understands and welcomes those changes and possible implications ( i believe, existing cients can be affected)

thibaultcha · 2019-08-13T21:42:19Z

@eshepelyuk Yes, the chosen approach is fine.

eshepelyuk · 2019-08-16T08:25:20Z

@thibaultcha should I rebase on master or on particular release branch ?

thibaultcha · 2019-08-16T17:48:53Z

@eshepelyuk We are currently in feature freeze with the upcoming 1.3.0 release (next Wednesday), but given this is a fix (and rather minimal), we could still ship it by then if it is ready (no rush, if not on Wednesday, it will ship next time around) cc @Kong/team-core.

We always submit fixes against master (as per our contribution guidelines), so better rebase the PR against it. The patch would apply cleanly in either branch anyway.

Thanks for taking care of it! Truly appreciated, this is a nice fix.

…oints

hishamhm · 2019-08-19T13:36:16Z

kong/plugins/cors/api.lua

@@ -0,0 +1,3 @@
+return {


@eshepelyuk A minor concern about this PR: the api.lua module name has a special meaning in plugins (it is used to specify customization to the generated Admin API endpoints). Please rename this module to something else to avoid namespace clashes.

In fact, better not introduce such a new module at all. I understand that the intent is to not duplicate the list between the plugin's code and tests, but we actually would rather duplicate it. This avoids accidental changes to the list going unnoticed (since they need to be updated in the tests and in the code). This is a pattern that this codebase has been following for a long time now.

hishamhm · 2019-08-19T13:36:40Z

@eshepelyuk Left a minor change request, otherwise i think this PR is good to go!

thibaultcha · 2019-08-19T19:21:18Z

kong/plugins/cors/handler.lua

@@ -181,7 +181,9 @@ end


 function CorsHandler:access(conf)
-  if kong.request.get_method() ~= "OPTIONS" then
+  if not (kong.request.get_method() == "OPTIONS"


style: please remove the extra set of parenthesis around this expression. They are not necessary.

thibaultcha · 2019-08-19T19:24:03Z

kong/plugins/cors/api.lua

@@ -0,0 +1,3 @@
+return {


In fact, better not introduce such a new module at all. I understand that the intent is to not duplicate the list between the plugin's code and tests, but we actually would rather duplicate it. This avoids accidental changes to the list going unnoticed (since they need to be updated in the tests and in the code). This is a pattern that this codebase has been following for a long time now.

thibaultcha · 2019-08-19T19:26:29Z

kong/plugins/cors/handler.lua

@@ -212,7 +214,6 @@ function CorsHandler:access(conf)
  end

  local methods = conf.methods and concat(conf.methods, ",")
-                  or "GET,HEAD,PUT,PATCH,POST,DELETE"


Please do not remove the fallback here (it doesn't seem like config.methods is properly labelled as being required). And if it is, then we'd need to update the above assignment conditions anyway. But better stay safe here, we are shipping this patch as a fix without an RC cycle, let's not forget.

thibaultcha · 2019-08-19T19:27:30Z

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

-        assert.is_nil(res.headers["Access-Control-Max-Age"])
-        assert.is_nil(res.headers["Vary"])
-      end)
+-- TODO rewrite test


This is going to be a blocker. Can you rewrite the test, if it needs to? And if not, can you elaborate on what the issue is? Nonetheless, we cannot just disable a regression test just before shipping a major release.

thibaultcha · 2019-08-19T19:34:24Z

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

            ["Access-Control-Request-Headers"] = "origin,accepts",
+            ["Access-Control-Request-Method"] = "GET",


Would you mind pointing us to the test that asserts the behavior fixed by this PR (i.e. that non-preflight OPTIONS requests get properly proxied after a preflight request)?

thibaultcha · 2019-08-19T19:35:12Z

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

    end)
  end)
 end)
+
+describe("methods in cors schema", function()
+  for _,method in ipairs({"HEAD", "GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS", "TRACE", "CONNECT"}) do


style: missing a space after , and in the table definition ({ "HEAD", ... }).

thibaultcha · 2019-08-20T20:55:03Z

Manually merged with the appropriate tweaks. Thank you!

eshepelyuk · 2019-08-20T20:59:08Z

Guys ! What are you doing ? The PR is not completed and i am working on tests :(((

hishamhm · 2019-08-21T15:30:56Z

@eshepelyuk Don't worry! :) We really wanted your fix to go into 1.3.0 which is due to release today, so we took it to ourselves to make the final tweaks and get it merged. So this is marked as "closed" but it was really accepted — it's already live in the master branch and in the release/1.3.0 branch as well. If you have any changes to propose beyond what's in master, feel free to open another PR!

Thank you for contributing!

(Also, could you sign the CLA at https://cla-assistant.io/Kong/kong?pullRequest=4899 ? Thank you!)

hishamhm · 2019-08-21T15:36:02Z

@eshepelyuk Also, you need to sign the CLA using the account that owns the email that's in the commit: eshepelyuknewagesol — thanks!

eshepelyuk · 2019-08-21T17:09:41Z

@eshepelyuk Also, you need to sign the CLA using the account that owns the email that's in the commit: eshepelyuknewagesol — thanks!

@hishamhm I tried it dozen of times and although I was told that SLA was signed - it's still pending :(
Am I missing smth ?

hishamhm · 2019-08-26T13:29:24Z

@eshepelyuk I think the problem might be that you used two different Github accounts? Perhaps try logging in a private browser window, log into Github with the other account and then access https://cla-assistant.io/Kong/kong?pullRequest=4899 — let's hope this works!

eshepelyuk changed the title ~~fix(cors) allow pre-flight requests to options endpoints~~ WIP: fix(cors) allow pre-flight requests to options endpoints Aug 13, 2019

thibaultcha requested changes Aug 13, 2019

View reviewed changes

kong/plugins/cors/handler.lua Outdated Show resolved Hide resolved

kong/plugins/cors/schema.lua Outdated Show resolved Hide resolved

nijikokun mentioned this pull request Aug 15, 2019

Support for pre-flight requests to OPTIONS endpoints in cors plugin #4898

Closed

eshepelyuk changed the title ~~WIP: fix(cors) allow pre-flight requests to options endpoints~~ WIP: fix(cors) allow pre-flight requests to options / trace / connect endpoints Aug 17, 2019

fix(cors) allow pre-flight requests to options / trace / connect endp…

0f220e8

…oints

hishamhm requested changes Aug 19, 2019

View reviewed changes

thibaultcha requested changes Aug 19, 2019

View reviewed changes

thibaultcha added this to the 1.3.0 milestone Aug 20, 2019

thibaultcha self-assigned this Aug 20, 2019

thibaultcha closed this Aug 20, 2019

kikito mentioned this pull request Sep 23, 2020

Preflight OPTIONS request not handled in Kong gateway #4859

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: fix(cors) allow pre-flight requests to options / trace / connect endpoints #4899

WIP: fix(cors) allow pre-flight requests to options / trace / connect endpoints #4899

eshepelyuk commented Aug 13, 2019 •

edited

Loading

CLAassistant commented Aug 13, 2019

intpp commented Aug 13, 2019

thibaultcha left a comment

eshepelyuk commented Aug 13, 2019

thibaultcha commented Aug 13, 2019

eshepelyuk commented Aug 16, 2019

thibaultcha commented Aug 16, 2019

hishamhm Aug 19, 2019

thibaultcha Aug 19, 2019

hishamhm commented Aug 19, 2019

thibaultcha Aug 19, 2019

thibaultcha Aug 19, 2019

thibaultcha Aug 19, 2019

thibaultcha Aug 19, 2019

thibaultcha Aug 19, 2019

thibaultcha Aug 19, 2019

thibaultcha commented Aug 20, 2019

eshepelyuk commented Aug 20, 2019

hishamhm commented Aug 21, 2019

hishamhm commented Aug 21, 2019 •

edited

Loading

eshepelyuk commented Aug 21, 2019

hishamhm commented Aug 26, 2019

		["Access-Control-Request-Headers"] = "origin,accepts",
		["Access-Control-Request-Method"] = "GET",

WIP: fix(cors) allow pre-flight requests to options / trace / connect endpoints #4899

WIP: fix(cors) allow pre-flight requests to options / trace / connect endpoints #4899

Conversation

eshepelyuk commented Aug 13, 2019 • edited Loading

Summary

Full changelog

Issues resolved

CLAassistant commented Aug 13, 2019

intpp commented Aug 13, 2019

thibaultcha left a comment

Choose a reason for hiding this comment

eshepelyuk commented Aug 13, 2019

thibaultcha commented Aug 13, 2019

eshepelyuk commented Aug 16, 2019

thibaultcha commented Aug 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamhm commented Aug 19, 2019

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

thibaultcha commented Aug 20, 2019

eshepelyuk commented Aug 20, 2019

hishamhm commented Aug 21, 2019

hishamhm commented Aug 21, 2019 • edited Loading

eshepelyuk commented Aug 21, 2019

hishamhm commented Aug 26, 2019

eshepelyuk commented Aug 13, 2019 •

edited

Loading

hishamhm commented Aug 21, 2019 •

edited

Loading