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

Bugfix: CORS Security #828

Closed
wants to merge 6 commits into from
Closed

Bugfix: CORS Security #828

wants to merge 6 commits into from

Conversation

jbutz
Copy link

@jbutz jbutz commented Feb 28, 2019

Issue #: #717

Description of changes:

I have updated the logic that creates the Swagger OPTIONS path operations objects for CORS and added a security requirements object with an object indicating no security. This resolves #717 and is similar to how a CloudFormation Macro was used to fix the issue caused by having both a default authorizer object and CORS object configured.

Description of how you validated changes:

I have used the CorsFixer, described here, to resolve this issue separately and things look to be matching what I got from it fairly closely. Even with the tests failing locally with mocking errors the output is matching the expected out. I have not been able to run the transform from the CLI, I get an error when I run bin/sam-translate.py locally. The unit tests pass for Python 3 on Travis but not for Python 2.7. The errors for 2.7 also occur on my local machine and I have no idea what the issue it and could use help in resolving it. I'm not a python developer and I welcome any advice or help.

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation

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

@brettstack
Copy link
Contributor

Thanks for the PR @jbutz. I don't think we should be adding this in every scenario. Please add an additional property to the Auth property called AddDefaultAuthorizerToCorsPreflight. This property should have a default value of True so that we don't break anyone relying on this functionality.

@jbutz
Copy link
Author

jbutz commented Mar 1, 2019

Going back to redo this, I think have the code to do it mostly there right now but the unit tests are giving me troubles.

Closing this PR

@jbutz jbutz closed this Mar 1, 2019
@keetonian
Copy link
Contributor

@jbutz reach out if you need help with the unit tests. It's usually the hash on the end of the logical id for APIGW deployment resources. If you run them locally, run it in Python 2.7 if possible as that's what the tests check for those hashes.

@xrendan
Copy link
Contributor

xrendan commented Jun 6, 2019

@brettstack @keetonian I'm looking to pick this up and I've worked passed it now, but with the current implementation of the test suite, when the test encounters an exception, it throws an UnboundLocalError instead of giving a stacktrace with reference to this bug: wolever/parameterized#66 in parameterized.

Right now I'm using the workaround specified in the thread but it could be worth committing it so that contributing is easier for other people in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants