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

Enable OPTIONS preflight and a method to enable cros origins #359

Merged
merged 5 commits into from
Jun 20, 2024

Conversation

giurgiur99
Copy link
Contributor

Reasoning

  • At the moment, no support for OPTIONS request is implemented. This is the preflight request for CORS checks, thus API calls to a serverless function from a web browser is failing.

Changes

  • Enable preflight response [OPTIONS] with generic cors allow.
  • Export a function "cors" where you can specify the allowed origins.

@giurgiur99 giurgiur99 mentioned this pull request Jun 5, 2024
@lholmquist
Copy link
Member

tests are failing

index.js Outdated
@@ -136,7 +139,7 @@ function initializeServer(config) {
// Add a parser for application/x-www-form-urlencoded
server.addContentTypeParser(
'application/x-www-form-urlencoded',
function(_, payload, done) {
function (_, payload, done) {
Copy link
Member

Choose a reason for hiding this comment

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

unrelated formatting

index.js Outdated
@@ -156,7 +159,7 @@ function initializeServer(config) {
server.addContentTypeParser(
'*',
{ parseAs: 'buffer' },
function(req, body, done) {
function (req, body, done) {
Copy link
Member

Choose a reason for hiding this comment

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

unrelated formatting

lib/invoker.js Outdated
'access-control-allow-methods':
'OPTIONS, GET, DELETE, POST, PUT, HEAD, PATCH',
'access-control-allow-origin': '*'
'access-control-allow-methods': 'OPTIONS, GET, POST',
Copy link
Member

Choose a reason for hiding this comment

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

why are you removing the other methods(DELETE, PUT, HEAD, PATCH)? This should stay the same since it is the default, otherwise it would be a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

invocation-handler.js

module.exports = function use(fastify, opts, done) {
  fastify.get('/', doGet);
  fastify.post('/', doPost);
  fastify.options('/', doOptions);
  const invokeFunction = invoker(opts);

As you can see here, only the post and get were supported and now i added options. Why should the rest of the methods be allowed if they cannot be used? Anyway, I added the rest of allowed methods in cors.

@giurgiur99
Copy link
Contributor Author

@lholmquist I fixed the code, according to the tests and removed formatting differences. Can you take another look? Thanks

@giurgiur99 giurgiur99 requested a review from lholmquist June 6, 2024 14:17
@giurgiur99
Copy link
Contributor Author

Hello @lholmquist , can you take a look at this PR?

@lholmquist
Copy link
Member

@giurgiur99 can you please add a test/tests for this new functionality?

@giurgiur99
Copy link
Contributor Author

@lholmquist I added tests for preflight and allowed origins

Copy link
Member

@lholmquist lholmquist left a comment

Choose a reason for hiding this comment

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

looks good. the ci is failing, but not because of this PR

@lholmquist lholmquist merged commit b3fa2b9 into nodeshift:main Jun 20, 2024
1 of 7 checks passed
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.

2 participants