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

CORS support #525

Merged
merged 11 commits into from
Aug 20, 2019
Merged

CORS support #525

merged 11 commits into from
Aug 20, 2019

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Aug 16, 2019

This PR installs the CORS middleware to the Prism instance when the --cors flag is enabled (which is enabled by default).

Docs and changeling added as well

Closes #412

@XVincentX XVincentX marked this pull request as ready for review August 16, 2019 19:44
@philsturgeon
Copy link
Contributor

Reviewing on mobile so I might just have missed it but I couldn’t see a test that shows CORS actually working? A test harness to show tyre and false do things, and maybe some unit or integration with more specifics.

@XVincentX
Copy link
Contributor Author

@philsturgeon See #412 — I added some comments there. We need to take a "product" decision before bringing this over the finish line.

@XVincentX XVincentX force-pushed the feat/cors branch 4 times, most recently from 2c90382 to e05be76 Compare August 19, 2019 15:33
@XVincentX XVincentX force-pushed the feat/cors branch 2 times, most recently from a12bf3f to f88f34a Compare August 19, 2019 16:38
@XVincentX
Copy link
Contributor Author

@philsturgeon I've added two tests checking out the response, the headers and what happens if the CORS is disabled. Let me know if you want to test some particular scenarios.

Copy link
Contributor

@lag-of-death lag-of-death left a comment

Choose a reason for hiding this comment

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

I think we need one more e2e test where both OPTIONS is defined in a spec and CORS is enabled (so that we can see that OPTIONS from spec is ignored).

docs/guides/cors.md Outdated Show resolved Hide resolved
test-harness/specs/cors/cors-headers-disabled.txt Outdated Show resolved Hide resolved
packages/cli/src/util/createServer.ts Show resolved Hide resolved
@XVincentX
Copy link
Contributor Author

I think we need one more e2e test where both OPTIONS is defined in a spec and CORS is enabled (so that we can see that OPTIONS from spec is ignored).

Very good point, I'll add one!

lag-of-death
lag-of-death previously approved these changes Aug 20, 2019
docs/guides/cors.md Outdated Show resolved Hide resolved
Co-Authored-By: Phil Sturgeon <[email protected]>
opts.config.cors
? server.route({
url: '*',
method: ['GET', 'DELETE', 'HEAD', 'PATCH', 'POST', 'PUT'],
Copy link
Contributor

Choose a reason for hiding this comment

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

if we are going to whitelist methods we should at least have all the ones OpenAPI supports.

Should we switch to method: '*' instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we cannot use * because otherwise the CORS plugin would complain (you need to leave the OPTIONS verb free); so I put all the other verbs that Fastify supports. Any other will make the framework scream at me.

@XVincentX XVincentX merged commit ab17450 into master Aug 20, 2019
@XVincentX XVincentX deleted the feat/cors branch August 20, 2019 13:36
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.

Add CORS Support
3 participants