wasp-lang / wasp

The fastest way to develop full-stack web apps with React & Node.js.
https://wasp-lang.dev
MIT License
13.76k stars 1.18k forks source link

Configure CORS to be more restrictive #94

Open Martinsos opened 4 years ago

Martinsos commented 4 years ago

Not really a bug, but a deficiency that we are aware of and want to improve.

In the node server that Wasp is generating, we currently just set CORS with app.use(cors()), which allows all CORS requests. This is not restrictive enough, it should allow only requests from the domain of the client I believe. Should we also configure something else? I am not actually what it takes to say that CORS is well configured, that is the first thing that we should investigate, and then we can make a plan for implementing that.

If you are reading this and have a good idea of how CORS should be configured, do share the thoughts!

bashovski commented 3 years ago

@Martinsos I've thought about the best possible implementation for CORS. So I came up with a couple of ideas:

Now, you might ask yourself, why?

App could access those environment variables and configure CORS options according to the allowed origins array. Wasp could additionally allow the setting of other CORS options if needed.

Martinsos commented 3 years ago

@bashovski Awesome, that's a good analysis!

  1. So right now, in generated Wasp code, we look for those ENV vars that you listed. We could add more as needed, for example we can add CORS_ORIGINS, or CORS_OPTIONS or smth like that. That is sometihng we would define in Wasp, since CORS part is also managed by Wasp.
  2. AWS SSM / Hashicorp's Vault -> ha interesting! That makes sense, I believe AWS SSM basically just exposes the variables as env vars if I remember it correctly, so maybe we don't need explicit support for it? I haven't used Hashicorp's Vault but I am sure we can figure it out. I guess we will want to add support for both of these at some point, but probably later when we refine the API that Wasp exposes for CORS.

For a moment, let's look at the whole thing from the "Wasp" perspective. If I am defining my app in Wasp, right now it is a SPA + API server, both generated by Wasp. API is currently ready to be consumed by the external client, it is here just to be consumed by the SPA. So, is there anything user might want to configure regarding CORS or not? I think we should be able to configure it all for them on our own right now. In time, if things change and we realize they need more control, we can figure out how to give them more control (expose CORS options, ...). Right now, the only thing we don't know is what is the domain to which SPA is deployed (we don't have staging right now, so only production matters). Ideally , we would specify that in Wasp code, in deploy block, where the rest of the deployment would also be described. However, we don't have that right now. We could implement it thought, if needed, for the purpose of this. Then, when developing locally, we could generate code that has only localhost as an origin, and when building for production, we could hardcode the production origin.

Or, we could just implement it so that origins are pulled from CORS_ORIGINS env var, when running in dev mode localhost is also added, and we tell users when deploying to set CORS_ORIGINS to the correct value on the hosting provider. Maybe that is the easiest for now? In the future, we could have more advanced approach as I described above, where we ask them about the domain of the SPA and take care of these details for them.

bashovski commented 3 years ago

Hey, I think SSM and Vault could be implemented later, as they are nice to have. When it comes to the most basic implementation, I'd suggest on adding a global property inside an App element:

app testApp {
    title: "test app"
    domain: "test-app.com"
}

We'd automatically allow localhost, as well as the above-mentioned domain "test-app.com".

What else could be done is to allow more domains simultaneously by allowing users to specify domains in a certain array (perhaps in an environment variable):

app testApp {
    title: "test app"
    domain: "test-app.com",
    allowed_origins: ["test-application.com", "test-app.org", "test-app.dev", "test-app.io"]
}

or for more advanced users:

app testApp {
    title: "test app"
    domain: "test-app.com",
    allowed_origins: parseArrayFromEnv("env_var_name_here")
}

parseArrayFromEnv should be a wasp-specific fn, which would read a string, and then deserialize it into an array or a similar, adequate structure.

Martinsos commented 3 years ago

Makes sense, I like the direction!

Obviously there are different levels of complexity here we could go with, and I agree with you that specifying domain in the .wasp code might be the easiest way to start. I think we will want to do that at some point anyway, so why not add it right now.

allowed_origins also sounds good (both with array directly in .wasp code and with reading it from the env var) as a next step, but it is something we can postpone for later and figure out better at that point.

One thing I am also thinking about in the meantime is "hardcoding" the env var that can be used to set cors origins directly -> same like PORT and JWT_TOKEN. So it would be CORS_ALLOWED_ORIGINS and if you set it on Heroku or AWS or wherever, it would be read by the deployed Wasp app and used. How this plays with app.allowed_origins? Well, the main difference is that with CORS_ALLOWED_ORIGINS you can set the value based on the environment, directly from the environment. We could also do that with app.allowed_origins if we do the parseArrayFromEnv approach, assuming that parseArrayFromEnv means it will be read from environment once deployed, and not while building a Wasp app.

Anyway, I am in for app.domain approach for now! One thing here: the idea is to at some moment introduce deployment block, looking smth like:

deployment production {
  domain: "test-app.com",
  provider: "heroku",
  ...
}

and in that case, as shown above, it would make sense to put the domain in this block. So I was thinking, we could take a step in that direction right now and instead of having a domain in app, we could have:

deployment {
  domain: "test-app.com"
}

But, we can also just go with app.domain and move domain to deployment once we make deployment more prominent.

Sorry for a lot of text, I am thinking through about different approaches as I type this.

Soooo, what now? Adding domain to app would require modifying Haskell code (mostly copying), so if you are not ready to play with Haskell this might not be the best fit. What you could do right now is configure CORS to use localhost + whatever is written in env CORS_ALLOWED_ORIGINS, so we have that working for us now, and later we can add app.domain.

bashovski commented 3 years ago

I like the suggestions, I might as well take a look at the Haskell part of the code and see how it's been implemented for the title, maybe it could be implemented similarly for the app domain. Nonetheless, I'll add a basic CORS configuration as soon as I grab some free time tomorrow.

Martinsos commented 3 years ago

Awesome :)! Ping me if you need any help (maybe best to do it on Discord), we can maybe even throw some pair programming if you need help with Haskell.

Martinsos commented 2 years ago

We did decent progress here https://github.com/wasp-lang/wasp/pull/206 but never finalized it -> let's continue from there once we are ready to continue with this issue!