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

Provide multi origin cors values #5740

Merged
merged 7 commits into from Jan 29, 2019

Conversation

Projects
5 participants
@richarddd
Copy link

richarddd commented Jan 23, 2019

What did you implement:

Provide support for using multiple origins in cors configuration. This PR generates a response template which transforms any matched origin and sets the Access-Control-Allow-Origin header to that matched origin

How did you implement it:

Created an extra function in the cors library to generate a valid response template script

How can we verify it:

Create a new template and use the following configuration:

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: hello
          method: get
          cors:
            origins:
              - http://localhost:3000
              - http://localhost:3001
            headers:
              - Content-Type
              - X-Amz-Date
              - Authorization
              - X-Api-Key
              - X-Amz-Security-Token
              - X-Amz-User-Agent
            allowCredentials: false

Try accessing the requested resource from localhost:3000 and localhost:3001

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

davisonr added some commits Jan 23, 2019

@pmuens pmuens added the pr/in-review label Jan 23, 2019

@dschep
Copy link
Member

dschep left a comment

Thanks for the PR @richarddd! Looks good for the most part. Just a few notes & questsion

@@ -100,7 +100,14 @@ describe('#compileCors()', () => {
.Resources.ApiGatewayMethodUsersCreateOptions
.Properties.Integration.IntegrationResponses[0]
.ResponseParameters['method.response.header.Access-Control-Allow-Origin']
).to.equal('\'*,http://example.com\'');
).to.equal('\'http://localhost:3000\'');

This comment has been minimized.

@dschep

dschep Jan 28, 2019

Member

Is there a ResponseParameter for http://example.com as well?

This comment has been minimized.

@richarddd

richarddd Jan 29, 2019

Author

Yes but it is in the transformation template. This tests so that the first value of comma ceparated origins is set as default origin

This comment has been minimized.

@dschep

dschep Jan 29, 2019

Member

Ah ok, so it gets over riden by the transormation. gotcha 👍

},
},
];
},

generateCorsResponseTemplate(origins) {
return (

This comment has been minimized.

@dschep

dschep Jan 28, 2019

Member

Does this only affect preflight requests or all requests? If the later, does it remove the need for users to specify that header manually in their handler?

This comment has been minimized.

@richarddd

richarddd Jan 29, 2019

Author

Only preflight. API Gateway does not support transformation of responses sent through lambda.

Edit: Agree that it would be more ideal to have some "automagical" way of applying cors response headers to all request methods as well 👌

let origins = (config.origins && Array.isArray(config.origins)) ? config.origins : undefined;

if (origin && origin.indexOf(',') !== -1) {
origins = origin.split(',').map(a => a.trim());

This comment has been minimized.

@dschep

dschep Jan 28, 2019

Member

I don't really like the idea of adding extra parsing on top of vanilla(+sls var system) yaml. No real benefit to supporting comma delimited origin when origins can accept an array.

This comment has been minimized.

@richarddd

richarddd Jan 29, 2019

Author

Well, since this changed quite recently so that most modern browsers does only accept one domain in or * as value we need to do this.

“Multiple Values Access-Control-Allow-Origin” by Janosch Maier https://link.medium.com/U8M0LTPHRT

The reason why I suggested to parse both origins and origin is when you want to pass your origins as an env variable or similar (if you have a proxy forward API Gateway method for example) you can't use arrays as env variables

This comment has been minimized.

@dschep

dschep Jan 29, 2019

Member

Fair enough 👍

@eahefnawy eahefnawy added this to In progress in Serverless via automation Jan 29, 2019

Serverless automation moved this from In progress to Reviewer approved Jan 29, 2019

@dschep

dschep approved these changes Jan 29, 2019

Copy link
Member

dschep left a comment

You've addressed all my questions/concerns! :shipit:

@dschep dschep merged commit 43d8b62 into serverless:master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Jan 29, 2019

@dschep dschep added this to the 1.36.4 milestone Feb 5, 2019

alexdilley added a commit to alexdilley/sls-cors that referenced this pull request Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment