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

Pass allowCredentials directive to method and integration response (#7575) #7576

Merged
merged 1 commit into from Apr 30, 2020
Merged

Pass allowCredentials directive to method and integration response (#7575) #7576

merged 1 commit into from Apr 30, 2020

Conversation

ThomasAribart
Copy link
Contributor

@ThomasAribart ThomasAribart commented Apr 13, 2020

What did you implement

Closes #7575

Pass down allowCredentials directive to method and integration responses: Enable browsers to read the request subsequent to the OPTIONS request without having to additionally specify the Access-Control-Allow-Credentials header.

How can we verify it

functions:
  test:
    handler: functions/test.main
    events:
      - http:
          method: get
          path: api/test
          integration: lambda
          cors:
            origin: http://localhost:3000/
            allowCredentials: true

The Access-Control-Allow-Credentials header should be configured in the method and integration responses.

api gateway

Inside a browser, the same header should be present in the responses, not only to the preflight request but also to the subsequent request.

browser response

Todos

Useful Scripts
  • npm run test:ci --> Run all validation checks on proposed changes
  • npm run lint:updated --> Lint all the updated files
  • npm run lint:fix --> Automatically fix lint problems (if possible)
  • npm run prettier-check:updated --> Check if updated files adhere to Prettier config
  • npm run prettify:updated --> Prettify all the updated files
  • Write and run all tests
  • Write documentation
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

@BenEllerby
Copy link
Contributor

@ThomasAribart what cookies does the browser need access to and why?

@ThomasAribart
Copy link
Contributor Author

@BenEllerby I'm using cross-domain authenticated HTTP requests from a client (front-end) to a private API, with an authentication bearer token provided in the Authorization header (no cookie).

It is not the cookie but the request response that is blocked from the browser:

"When a request's credentials mode is include, browsers will only expose the response to frontend JavaScript code if the Access-Control-Allow-Credentials value is true.

For a CORS request with credentials, in order for browsers to expose the response to frontend JavaScript code, both the server (using the Access-Control-Allow-Credentials header) and the client (by setting the credentials mode for the XHR, Fetch, or Ajax request) must indicate that they’re opting in to including credentials."

Source: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@ThomasAribart thanks for this fix! Please minor suggestions I proposed, and we'll be happy to take it

Comment on lines 145 to 147
_.merge(integrationResponseHeaders, {
'Access-Control-Allow-Credentials': `'${http.cors.allowCredentials}'`,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use plain JavaScript, and literally assign "true", as it's a boolean value

integrationResponseHeaders['Access-Control-Allow-Credentials'] = "true"

@medikoo
Copy link
Contributor

medikoo commented Apr 23, 2020

Additionally it'll be good to have some regression test, or even integration test.

e.g. we have one that confirms on CORS in HTTP API integration:

it('should support CORS when indicated', async () => {
const testEndpoint = `${endpoint}/bar/whatever`;
const response = await fetch(testEndpoint, {
method: 'GET',
headers: { Origin: 'https://serverless.com' },
});
expect(response.headers.get('access-control-allow-origin')).to.equal('*');
expect(response.headers.get('access-control-expose-headers')).to.equal('x-foo');
});

We may add similar one when testing API Gateway: https://github.com/serverless/serverless/blob/2e56dea5652540cf5d82c9d35a999c8c921fa020/tests/integration-all/api-gateway/tests.js

@ThomasAribart
Copy link
Contributor Author

Thanks @medikoo I took your feedbacks ! Let me know if it's okay for you 👍

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@ThomasAribart great thanks for update! We're nearly there, please see my few style improvement suggestions

Comment on lines 145 to 147
_.merge(integrationResponseHeaders, {
'Access-Control-Allow-Credentials': "'true'",
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use plain JS as:

integrationResponseHeaders['Access-Control-Allow-Credentials'] = 'true'

Comment on lines 27 to 29
_.merge(methodResponseHeaders, {
'Access-Control-Allow-Credentials': "'true'",
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

// Only set Access-Control-Allow-Credentials when explicitly allowed (omit if false)
if (http.cors.allowCredentials) {
_.merge(integrationResponseHeaders, {
'Access-Control-Allow-Credentials': "'true'",
Copy link
Contributor

Choose a reason for hiding this comment

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

While by spec, any value makes it truthy, let's setup header without quotes so 'true' and not "'true'"

@ThomasAribart
Copy link
Contributor Author

@medikoo Done 👌

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @ThomasAribart !

@medikoo medikoo merged commit bd9fbfb into serverless:master Apr 30, 2020
@ThomasAribart ThomasAribart deleted the use-cors-allowCredentials-in-integration-responses branch May 2, 2020 10:50
fredericbarthelet pushed a commit to fredericbarthelet/serverless that referenced this pull request May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass CORS allowCredentials directive to method and integration responses
3 participants