Bug: CORS Support in start-api

[sleepwithcoffee]

Which issue(s) does this change fix?

#4161

Why is this change necessary?

To address a bug raised in ticket of local API gateway that does not properly return CORS headers. Currently, a workaround of using external plugin is required.

How does it address the issue?

Inject CORS response headers when returning to client. There are generated based on request Origin header.

What side effects does this change have?

I would expect none since this is a bug fix and an improvement. With this, user would not need to use external plugin to enable CORS for the local stack.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests
• Write/update functional tests if needed
• make pr passes
• make update-reproducible-reqs if dependencies were changed
• Write documentation (as below)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Current behavior:

• Request header "Access-Control-Allow-Origin" is passed on as response header "Access-Control-Allow-Origin" without processing
  New behavior:
• Get request header "Access-Control-Allow-Origin" and use it (either single domain or multiple domains or *) to match with Origin request header
  • If a match was found, only return that single matched domain as response "Access-Control-Allow-Origin"
  • If no match was found, reject the request by not returning according response header "Access-Control-Allow-Origin"
• If there is no "Origin" header, straightout reject the request

Some consideration:

• I didn't put the new logic under Cors class since this logic is supposed to be contextual of incoming requests
• I named the new function _response_cors_headers since it is a private function, made it static for unit testing
• The code prefers being restrictive over permissive due to the nature of this handling

[jfuss]

Is it possible to have integration tests that could help us validate the behavior?

[jfuss]

Why unset this? It looks like it should be removed in the case that response_allowed_origin is not set. If so, I would move this to be closer to line 437. I think it just make it a little easier to read since the mutation of the cors_headers is all in one spot.

[sleepwithcoffee]

Hi @jfuss , thank you for taking your time to review this

we unset and set again if a match is found (restrictive manner)

if multiple domains are allowed, we only send back 1 allowed domain in our response header
if all domains are allowed, we also only send back 1 allowed domain in our response header

we do not return this header (implying a deny) if no matches is found

[sleepwithcoffee]

hi @jfuss please also refer to my below comment:
#4991 (comment)

[lucashuy]

Thanks for creating this PR! Did we need to restrict this behaviour to only run for HTTP APIs? From the docs and the linked issue, this seems to be something that the Lambda function deals with on for REST, but we would need to manage for HTTP.

[sleepwithcoffee]

hi @lucashuy

During the implementation, I referred to MDN web docs for reference:

I also referred to the implementation of middy cors middleware

below piece of code will basically parse and return a single origin if matched:

it is explained if looking deeper into the getOrigin() function:

Hope these help explain the reasoning behind my code.

Thanks,

[jfuss]

Thanks for the patch!

[sleepwithcoffee]

merged and resolved conflicts

[hawflau]

LGTM!

[moelasmar]

@sleepwithcoffee .. We had to revert this PR as it cause some regression, and some of the integration test cases are failing. Please check the revert PR to get more details. Could you please update this PR to fix the failing test cases, and add new integration test cases that cover the cases you are fixing here.