-
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
Added Access-Control-Allow-Credentials for CORS settings fixes #2182 #2736
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -401,6 +401,7 @@ describe('#validate()', () => { | |
headers: ['Content-Type', 'X-Amz-Date', 'Authorization', 'X-Api-Key', 'X-Amz-Security-Token'], | ||
methods: ['OPTIONS', 'POST'], | ||
origins: ['*'], | ||
allowCredentials: false, | ||
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. Could we do the same like proposed above (for the other test) and remove this configuration so that we test for the default case (which should set 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. This is correct validate should default to false. |
||
}); | ||
}); | ||
|
||
|
@@ -550,10 +551,11 @@ describe('#validate()', () => { | |
headers: ['X-Foo-Bar'], | ||
methods: ['POST', 'OPTIONS'], | ||
origins: ['acme.com'], | ||
allowCredentials: false, | ||
}); | ||
}); | ||
|
||
it('should merge all preflight origins, method, and headers for a path', () => { | ||
it('should merge all preflight origins, method, headers and allowCredentials for a path', () => { | ||
awsCompileApigEvents.serverless.service.functions = { | ||
first: { | ||
events: [ | ||
|
@@ -565,6 +567,7 @@ describe('#validate()', () => { | |
origins: [ | ||
'http://example.com', | ||
], | ||
allowCredentials: true, | ||
}, | ||
}, | ||
}, { | ||
|
@@ -609,6 +612,10 @@ describe('#validate()', () => { | |
.to.deep.equal(['http://example2.com', 'http://example.com']); | ||
expect(validated.corsPreflight['users/{id}'].headers) | ||
.to.deep.equal(['TestHeader2', 'TestHeader']); | ||
expect(validated.corsPreflight.users.allowCredentials) | ||
.to.equal(true); | ||
expect(validated.corsPreflight['users/{id}'].allowCredentials) | ||
.to.equal(false); | ||
}); | ||
|
||
it('should add default statusCode to custom statusCodes', () => { | ||
|
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.
Could we remove this setup so that we test that it defaults to
false
?Then we have one test w/o any
allowCredentials
setup, one which tests if it's set totrue
and one which tests if it's set tofalse
.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.
The code in validate.js applies the default so there is no default logic to test here.
compileCors
operates on the validated input (see cors.js). I am just following the convention established in the existing code.