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

fix: allow regex support for Access-Control-Allow-Origin (#4470) #5077

Closed
wants to merge 1 commit into from

Conversation

prescottprue
Copy link

@prescottprue prescottprue commented Apr 1, 2021

Description

Adds support for Regex and Regex within an array for cors.origin setting in apollo-server-lambda and apollo-server-cloud-functions to match configuration option offered by Express

Fixes

Notes

  • Not the cleanest looking code, but the goal was to stick within existing if condition to minimize changes
  • Noticed that apollo-server-lambda has a variable holding the request origin, so I mirrored this within apollo-server-cloud-functions
  • Didn't find any tests in place for testing cors.origin in either package - going to look into adding, but wanted to confirm this in an intended change first

Open Questions

  • Should this logic be moved into a function which can be reused? I personally would, but not sure exactly where we would want that to live
  • Are these docs in the README automatically updated with the types or should that be included in this change?
  • Still have to look to see which other packages are impacted by this - is anyone aware of any?

…ow-Origin (apollographql#4470)

fix(apollo-server-cloud-functions): allow regex support for Access-Control-Allow-Origin (apollographql#4470)
@apollo-cla
Copy link

@prescottprue: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@prescottprue
Copy link
Author

@apollo-cla - signed

@glasser
Copy link
Member

glasser commented Apr 1, 2021

Hi @prescottprue.

I'm getting more and more convinced that building out a miniature implementation of every express plugin inside apollo-server-lambda isn't the best approach. I'm about to switch into full time focus on Apollo Server 3 and I am planning to seriously consider replacing this whole package with the combination of apollo-server-express and existing packages that help you put express apps inside Lambda. See #5078

Also considering making CORS in general something that you configure outside of AS just like for any other web app instead of built-in. See #4859 . Of course this only works for Lambda if we also do #5078.

So I'm not sure that expanding this API is the best idea right now.

@prescottprue
Copy link
Author

@glasser Thanks for the quick response. Totally makes sense about not expanding the API given the goal of v3 - saw your comment and am looking forward to it

@marcoacierno
Copy link

marcoacierno commented Apr 26, 2021

Thanks for the PR @prescottprue! I am trying to use RegEx to configure a wildcard RegEx domain like https://*-python-italia.vercel.app/ as I cannot do this in API Gateway cors

@glasser I think your comment above makes sense, but I wonder if it also makes sense to merge this PR as there is a real use-case for this and I am not sure how far apollo-server 3 is from being ready?

I found this PR because the documentation says

The options correspond to the express cors configuration with the following fields(all are optional):

but the cors configuration it links to (https://github.com/expressjs/cors#configuration-options) says that RegEx are supported

I might end up forking this repo and apply this PR to use it for now which I don't like

@glasser
Copy link
Member

glasser commented Apr 26, 2021

FWIW, you can combine apollo-server-express with @vendia/serverless-express today, though you have to figure out how to combine them yourselves. As far as I can tell it works just fine.

@jessgoldq4
Copy link

@glasser Hi there. Have you guys considered adding this functionality for Apollo Server 3? Thanks

@glasser
Copy link
Member

glasser commented May 12, 2022

@jessgoldq4 In AS3 we already did the thing I theorized above: apollo-server-lambda is just a thin wrapper around apollo-server-express and you can configure it with the same way you configure Express' cors:

exports.handler = server.createHandler({
  expressGetMiddlewareOptions: {
    cors: anyOptionsYouCanPassToTheExpressCorsMiddleware,
  }
});

In Apollo Server 4 we are moving even further in the direction of "it's not Apollo Server's job to take in configuration that is immediately passed to another library" and unbundling cors configuration. The Lambda integration will get its cors configuration ... well, there might be multiple Lambda integrations. One built on top of express + @vendia/serverless-express that lets you use the Express cors middleware, one built on top of Middy that lets you use @middy/http-cors, etc.

@glasser glasser closed this May 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[apollo-server-lambda] allow regex support for Acess-Control-Allow-Origin
5 participants