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

[ms2008]

Summary

The CORS plugin currently always sets the Access-Control-Allow-Origin 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

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.

These changes seem reasonable to me, but happy to hear feedback.

Checklist

• The Pull Request has tests
• There's an entry in the CHANGELOG
• There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

• don't send ACAO header where a single origin(not regex) is configured in case of non-matched.

Issue reference

Fix FTI-4745

[sumimakito]

LGTM

[bungle]

Since this has been so long in a products, do we consider this as a fix. It seems like a potentially breaking change?

@hbagdi, @kikito?

[hbagdi]

I recall putting in a comment but I cannot see it here now.
I agree with Aapo that this can cause breakage, a large one because we put this in to be smart about this.

[ms2008]

Please move this forward @hbagdi @kikito

[kikito]

I don’t like what we are currently doing, and I don’t like that we are not following the CORS standard. I would have zero concerns about doing this on a major release (4.0).

However, CORS is our most used plugin according to our stats. Making changes such as this on a minor release can be very problematic.

@ms2008 is it possible to fix the customer issue (missing port) without removing the "smartness" from the plugin? If there is, that would be my preferred way to do this.

While we decide this, can we give the customer a workaround? E.g. would it be possible to sidestep this problem by adding a "dummy" domain in the config?

To summarize, my proposed solution would be:

• Give the customer a workaround if there's any
• Close this PR and open a new one.
• Add the port to the current "smart" code
• Add a deprecation notice that puts a log line saying that this behavior is deprecated and will change in 4.0. Put a comment linking to this PR
• Document this behavior (and the fact that it will change in 4.0) on the plugin docs page.
• When we start implementing 4.0, we should be able to grep for deprecation deprecation notices, and be pointed to this PR.

[ms2008]

@kikito I read the RFC and W3C specifications carefully, and I think the behavior of stripping the port part(if it is equal to the default value) is reasonable even though they are explicitly specified in the Origin request header.

I have tried to verify this behavior(stripping the default port) on Chrome and have determined if Chrome recognizes this behavior. Unfortunately, Chrome does not allow custom CORS requests for origin at all.

I still think this is an issue with the customer's own specific software and should be left up to them to fix(e.g. customize their own CORS plugin), not to change our behavior.

[VicYP]

@kikito @hbagdi We do need to consider fixing this one in the future major release. Could we "freeze" this one and let it be ready for the coming major release.

[hbagdi]

Could we "freeze" this one and

What does 'freeze' mean in the context of this PR?

[VicYP]

Can this one be a feature candidate in 3.5? @hbagdi @ms2008

[ms2008]

I think the concerns raised by Apao and Enrique are valid. Considering that it seems like a potentially breaking change and doesn't have any substantial impact on functionality, I lean towards closing this PR.