Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: honour the AllowCredentials CORS option in SAM templates #2536

Merged
merged 10 commits into from
Jan 19, 2021

Conversation

discorev
Copy link
Contributor

Which issue(s) does this change fix?

#1645

Why is this change necessary?

This change is needed to complete an incomplete feature in the local API CORS implementation. Currently the AllowCredentials CORS option is ignored in the local API mock and blocks any local development that has a CORS requirement and needs the Access-Control-Allow-Credentials header to be set.

How does it address the issue?

Whilst parsing CORS options for a REST API, the AllowCredentials option is parsed allowing for a boolean or quoted string value. If and only if this value is a representation of true, the header is set to the string true

Whilst parsing CORS options for a Http API the AllowCredentials options is parsed only allowing for a boolean value (following the documentation). If this is set to true, the header is set to the string true

The header is always output as a lower-cased string to be compliant with section 5.6 of RFC7480

There are no new dependancies introduced and this does not impact any other areas of functionality.

What side effects does this change have?

There are no side effects outside of having the Access-Control-Allow-Credentials header set when the template demands it.

Checklist

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

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

@discorev discorev changed the title Feature/cors allow credentials Fixes: honour the AllowCredentials CORS option in SAM templates Jan 17, 2021
@hoffa hoffa changed the title Fixes: honour the AllowCredentials CORS option in SAM templates fix: honour the AllowCredentials CORS option in SAM templates Jan 17, 2021
@hoffa
Copy link
Contributor

hoffa commented Jan 17, 2021

Thanks for the PR!

Related PRs for reference: #1648 (reverted in #1664) and #2058.

@hoffa hoffa added area/local/start-api sam local start-api command stage/pr Has a PR ready for review labels Jan 17, 2021
Copy link
Contributor

@hoffa hoffa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments; overall looks great!

samcli/lib/providers/cfn_base_api_provider.py Outdated Show resolved Hide resolved
samcli/lib/providers/cfn_base_api_provider.py Show resolved Hide resolved
@hoffa hoffa merged commit 14276cd into aws:develop Jan 19, 2021
@hoffa
Copy link
Contributor

hoffa commented Jan 19, 2021

Thanks @discorev; merged!

@mgrandis mgrandis added the stage/waiting-for-release Fix has been merged to develop and is waiting for a release label Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cors area/local/start-api sam local start-api command stage/pr Has a PR ready for review stage/waiting-for-release Fix has been merged to develop and is waiting for a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants