-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Changes from all commits
01b529a
d9575d4
c68b433
3dff171
e2563d7
a820191
867d112
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Original file line | Diff line number | Diff line change |
---|---|---|---|
|
@@ -11,10 +11,20 @@ module.exports = { | ||
const corsMethodLogicalId = this.provider.naming | const corsMethodLogicalId = this.provider.naming | ||
.getMethodLogicalId(resourceName, 'options'); | .getMethodLogicalId(resourceName, 'options'); | ||
|
|
||
// TODO remove once "origins" config is deprecated | |||
let origin = config.origin; | let origin = config.origin; | ||
if (config.origins && config.origins.length) { | let origins = (config.origins && Array.isArray(config.origins)) ? config.origins : undefined; | ||
origin = config.origins.join(','); |
|
||
if (origin && origin.indexOf(',') !== -1) { | |||
origins = origin.split(',').map(a => a.trim()); | |||
} | |||
|
|||
if (origins) { | |||
origin = origins[0]; | |||
} | |||
|
|||
if (!origin && !origins) { | |||
const errorMessage = 'must specify either origin or origins'; | |||
throw new this.serverless.classes.Error(errorMessage); | |||
} | } | ||
|
|
||
const preflightHeaders = { | const preflightHeaders = { | ||
|
@@ -61,7 +71,8 @@ module.exports = { | ||
'application/json': '{statusCode:200}', | 'application/json': '{statusCode:200}', | ||
}, | }, | ||
ContentHandling: 'CONVERT_TO_TEXT', | ContentHandling: 'CONVERT_TO_TEXT', | ||
IntegrationResponses: this.generateCorsIntegrationResponses(preflightHeaders), | IntegrationResponses: | ||
this.generateCorsIntegrationResponses(preflightHeaders, origins), | |||
}, | }, | ||
ResourceId: resourceRef, | ResourceId: resourceRef, | ||
RestApiId: this.provider.getApiGatewayRestApiId(), | RestApiId: this.provider.getApiGatewayRestApiId(), | ||
|
@@ -89,7 +100,7 @@ module.exports = { | ||
]; | ]; | ||
}, | }, | ||
|
|
||
generateCorsIntegrationResponses(preflightHeaders) { | generateCorsIntegrationResponses(preflightHeaders, origins) { | ||
const responseParameters = _.mapKeys(preflightHeaders, | const responseParameters = _.mapKeys(preflightHeaders, | ||
(value, header) => `method.response.header.${header}`); | (value, header) => `method.response.header.${header}`); | ||
|
|
||
|
@@ -98,9 +109,21 @@ module.exports = { | ||
StatusCode: '200', | StatusCode: '200', | ||
ResponseParameters: responseParameters, | ResponseParameters: responseParameters, | ||
ResponseTemplates: { | ResponseTemplates: { | ||
'application/json': '', | 'application/json': | ||
Array.isArray(origins) ? this.generateCorsResponseTemplate(origins) : '', | |||
}, | }, | ||
}, | }, | ||
]; | ]; | ||
}, | }, | ||
|
|||
generateCorsResponseTemplate(origins) { | |||
return ( | |||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 👌 |
|||
'#set($origin = $input.params("Origin"))\n' + | |||
'#if($origin == "") #set($origin = $input.params("origin")) #end\n' + | |||
`#if(${origins | |||
.map((o, i, a) => `$origin == "${o}"${i < a.length - 1 ? ' || ' : ''}`) | |||
.join('')}` + | |||
') #set($context.responseOverride.header.Access-Control-Allow-Origin = $origin) #end' | |||
); | |||
}, | |||
}; | }; |
Original file line number | Original file line | Diff line number | Diff line change |
---|---|---|---|
|
@@ -71,7 +71,7 @@ describe('#compileCors()', () => { | ||
cacheControl: 'max-age=600, s-maxage=600', | cacheControl: 'max-age=600, s-maxage=600', | ||
}, | }, | ||
'users/create': { | 'users/create': { | ||
origins: ['*', 'http://example.com'], | origins: ['http://localhost:3000', 'http://example.com'], | ||
headers: ['*'], | headers: ['*'], | ||
methods: ['OPTIONS', 'POST'], | methods: ['OPTIONS', 'POST'], | ||
allowCredentials: true, | allowCredentials: true, | ||
|
@@ -87,7 +87,7 @@ describe('#compileCors()', () => { | ||
cacheControl: 'max-age=600, s-maxage=600', | cacheControl: 'max-age=600, s-maxage=600', | ||
}, | }, | ||
'users/any': { | 'users/any': { | ||
origins: ['http://example.com'], | origin: 'http://localhost:3000,http://example.com', | ||
headers: ['*'], | headers: ['*'], | ||
methods: ['OPTIONS', 'ANY'], | methods: ['OPTIONS', 'ANY'], | ||
allowCredentials: false, | allowCredentials: false, | ||
|
@@ -100,7 +100,14 @@ describe('#compileCors()', () => { | ||
.Resources.ApiGatewayMethodUsersCreateOptions | .Resources.ApiGatewayMethodUsersCreateOptions | ||
.Properties.Integration.IntegrationResponses[0] | .Properties.Integration.IntegrationResponses[0] | ||
.ResponseParameters['method.response.header.Access-Control-Allow-Origin'] | .ResponseParameters['method.response.header.Access-Control-Allow-Origin'] | ||
).to.equal('\'*,http://example.com\''); | ).to.equal('\'http://localhost:3000\''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a ResponseParameter for http://example.com as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes but it is in the transformation template. This tests so that the first value of comma ceparated origins is set as default origin There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, so it gets over riden by the transormation. gotcha 👍 |
|||
|
|||
expect( | |||
awsCompileApigEvents.serverless.service.provider.compiledCloudFormationTemplate | |||
.Resources.ApiGatewayMethodUsersCreateOptions | |||
.Properties.Integration.IntegrationResponses[0] | |||
.ResponseTemplates['application/json'] | |||
).to.equal('#set($origin = $input.params("Origin"))\n#if($origin == "") #set($origin = $input.params("origin")) #end\n#if($origin == "http://localhost:3000" || $origin == "http://example.com") #set($context.responseOverride.header.Access-Control-Allow-Origin = $origin) #end'); | |||
|
|
||
expect( | expect( | ||
awsCompileApigEvents.serverless.service.provider.compiledCloudFormationTemplate | awsCompileApigEvents.serverless.service.provider.compiledCloudFormationTemplate | ||
|
@@ -221,8 +228,8 @@ describe('#compileCors()', () => { | ||
awsCompileApigEvents.serverless.service.provider.compiledCloudFormationTemplate | awsCompileApigEvents.serverless.service.provider.compiledCloudFormationTemplate | ||
.Resources.ApiGatewayMethodUsersAnyOptions | .Resources.ApiGatewayMethodUsersAnyOptions | ||
.Properties.Integration.IntegrationResponses[0] | .Properties.Integration.IntegrationResponses[0] | ||
.ResponseParameters['method.response.header.Access-Control-Allow-Origin'] | .ResponseTemplates['application/json'] | ||
).to.equal('\'http://example.com\''); | ).to.equal('#set($origin = $input.params("Origin"))\n#if($origin == "") #set($origin = $input.params("origin")) #end\n#if($origin == "http://localhost:3000" || $origin == "http://example.com") #set($context.responseOverride.header.Access-Control-Allow-Origin = $origin) #end'); | ||
|
|
||
expect( | expect( | ||
awsCompileApigEvents.serverless.service.provider.compiledCloudFormationTemplate | awsCompileApigEvents.serverless.service.provider.compiledCloudFormationTemplate | ||
|
@@ -247,6 +254,19 @@ describe('#compileCors()', () => { | ||
}); | }); | ||
}); | }); | ||
|
|
||
it('should throw error if no origin or origins is provided', () => { | |||
awsCompileApigEvents.validated.corsPreflight = { | |||
'users/update': { | |||
headers: ['*'], | |||
methods: ['OPTIONS', 'PUT'], | |||
allowCredentials: false, | |||
}, | |||
}; | |||
|
|||
expect(() => awsCompileApigEvents.compileCors()) | |||
.to.throw(Error, 'must specify either origin or origins'); | |||
}); | |||
|
|||
it('should throw error if maxAge is not an integer greater than 0', () => { | it('should throw error if maxAge is not an integer greater than 0', () => { | ||
awsCompileApigEvents.validated.corsPreflight = { | awsCompileApigEvents.validated.corsPreflight = { | ||
'users/update': { | 'users/update': { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
whenorigins
can accept an array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough 👍