withastro / roadmap

Ideas, suggestions, and formal RFC proposals for the Astro project.
290 stars 29 forks source link

Astro Env #894

Closed florian-lefebvre closed 1 week ago

florian-lefebvre commented 4 months ago

Summary

astro:env - Improve DX and security around environment variables in Astro

Links

jdtjenkins commented 4 months ago

Hey @florian-lefebvre I love the general idea! Tbh I don't know much about env at the moment so I can't really comment on all of this.

I like the idea of the zod-lite validation approach and creating an envSchema. I also really like how integrations could enforce certain environment variables too! That's really great.

I just wonder: with the separation between static and dynamic, are devs supposed to know and keep track of which env variables will be static or dynamic? Or can all env variables be accessed by both astro:env/static and astro:env/dynamic and basically it's just under the hood works differently?

Also just while we're on the topic of environment variables. Is there a reason astro uses the PUBLIC_ prefix for public variables, rather than the VITE_ prefix from Vite itself? Is it just to abstract Vite away a little bit more?

florian-lefebvre commented 4 months ago

I just wonder: with the separation between static and dynamic, are devs supposed to know and keep track of which env variables will be static or dynamic? Or can all env variables be accessed by both astro:env/static and astro:env/dynamic and basically it's just under the hood works differently?

No variables won't be available as both static and dynamic, it's either one or another. I think it's great this way for technical reasons and also so that devs are aware where their variables end up (eg. static public can end up on the client). Do you see a possible downside to such approach?

Is there a reason astro uses the PUBLIC prefix for public variables, rather than the VITE prefix from Vite itself? Is it just to abstract Vite away a little bit more?

I don't know the reason behind this, I just reused the existing convention. IMO it's more explicit that it can end up on the client than having VITE_ as a prefix

matthewp commented 4 months ago

I don't think this idea of static/dynamic and public/private is quite right. Any "static" variable, meaning one that is set at build time, is always "public", in the sense that anyone with access to the source code can see the value. I would recommend a new distinction here:

With these definitions, I don't think the .dynamic() and .static() helpers are needed at all.

Additionally, I don't think the /static and /dynamic subpaths are needed at all. They can all be part of astro:env, but the exports of astro:env will differ in the client where only the public items will be available.

If we want to restrict some public environment variables to only be available in the server or client I would recommend .client() and .server() helpers. If we went with this, .client().private() should be a type (and runtime) error.

florian-lefebvre commented 4 months ago

I think that makes sense, let me summarize how the api looks and what it means:

Now let's say we have such config:

import { defineConfig, envField } from "astro/config"

export default defineConfig({
    env: {
        schema: {
            API_PORT: envField.server().private().number(),
            API_URL: envField.server().public().string(),
            PUBLIC_FLAG: envField.client().public().boolean()
        }
    }
})

If we have a single module like below, there's no issue on the server but on the client because we have no way to update types based on where it's called:

import { API_URL, PUBLIC_FLAG, getSecret } from "astro:env"

// On the server, no issues
API_URL // string
PUBLIC_FLAG // boolean
getSecret("API_PORT") // number
getSecret("UNKNOWN") // string | undefined

// But on the client...
API_URL // typed as string, returns undefined
PUBLIC_FLAG // boolean
getSecret("API_PORT") // typed as number, returns undefined
getSecret("UNKNOWN") // typed as string | undefined, returns undefined

I think it will cause too much confusion for users, because you can't guess it's going to be undefined if not correctly typed. Instead it could make sense to at least have 2 imports like astro:env/server and astro:env/client:

import { API_URL, getSecret } from "astro:env/server"
import { PUBLIC_FLAG } from "astro:env/client"

Let me know what you think, and if I summarized correctly at the top!

bluwy commented 4 months ago

I think that's also an interesting way to look at it. I would prefer chaining .public()/.private() first, and then chain .client()/.server(), as the bonus with this is that we can say .client()/.server() is only available after chaining .public(). .private() variables are always to be used in servers only. Personally I quite like this API too.

About combining astro:env, I also think we need a better way to warn misused env var rather than undefined (I also touched at here). I think what Florian suggested with two imports seems like the best way to warn in that case. (EDITed away some outdated comment)

bholmesdev commented 4 months ago

I agree with Blu on the ordering! I also have a question on API design. Astro DB uses a base function with an object parameter (column.number({ optional: true })), while Astro Env is using a chain (envField.private().static()). Ideally, Astro's custom validators would be aligned. Astro DB is pre-1.0, so that API choice is not set in stone. I'm curious what reasons y'all have for preferring one format over another?

florian-lefebvre commented 4 months ago

My initial proposal was to have chaining first for the important stuff (so server/client and public/private) and then object syntax per type to match astro db:

envField.private().static().string({ optional: true })

I think Blu suggested to match zod api with full chaining

envField.private().static().string().optional()

I'm in favor to keep at least server/client and public/private as chained calls and then the type as an object

matthewp commented 4 months ago

I agree with @bholmesdev that we should align the APIs. I think I tend to prefer making public/private/client/server properties of the options object rather than chaining. This mostly comes down to the discussion above about what order chaining should happen.

In a fluent API (another name for chaining) the expectation is that they can be chained in any order as they are operating on a common this value. If that's not the case and ordering matters I would find using chaining to be unintuitive.

If we go with an options object we can make the types work by making them be a discriminated union where private and client can't be used together.

matthewp commented 4 months ago

Some questions I have on the client and server options are:

  1. Should it be required that you provide at least one of these?
  2. If not required then what is the default? That it be applied to both?

I think I lean towards making at least one of these be required rather than picking a default, because accidentally bringing in values into the client, even if "public" would be a footgun.

matthewp commented 4 months ago

On the topic of getSecret() in the client, I think it should throw if you call that function. The only reason for it existing at all is because of TypeScript. There's no use-case for calling it on the client, so we should throw.

bluwy commented 4 months ago

In a fluent API (another name for chaining) the expectation is that they can be chained in any order as they are operating on a common this value. If that's not the case and ordering matters I would find using chaining to be unintuitive.

I don't think we should strictly follow this. Instead I think we should follow a zod-like API so it feels unified. Zod also has certain cases where certain chains can only be used after another, for example z.string().datetime(). If we opt for an pure-options based config, then it doesn't feel as unified (to me).

Also, Zod supports .optional() and .default() instead of { default/optional: boolean }.

EDIT: The fluent API wiki also doesn't explicitly mention that methods need to / can be in any order.

Princesseuh commented 4 months ago

No comments from me, looks neat. I like the builder API for the .public() and stuff, always looks cute. I agree with bluwy regarding fluent vs options

matthewp commented 4 months ago

I think we should have the fluent vs. options API discussion externally. It will probably block this RFC from being fully accepted but shouldn't affect implementation very much. I'll start a thread elsewhere.

Tc-001 commented 3 months ago

Hmmm... one thing that is not quite clear to me is how you would define an env variable for both client and server.

Something like PUBLIC_SOME_THIRD_PARTY_API="https://example.com/api" that gets imported into custom_fetch.ts that then gets used by both server and client

florian-lefebvre commented 3 months ago

I agree it's not super clear but a client variable can be imported on both client and server

Tc-001 commented 3 months ago

Oh I see! Then you probably need to change this: https://github.com/withastro/roadmap/pull/894/files#diff-3c58a592f2b3662c3cda9ddfdd25a71de2a989fff0a5fcebfcd805dee36be9baR68

Otherwise looks really nice!

florian-lefebvre commented 3 months ago

I've updated the RFC to match the current implementation PR state and make things clearer

ematipico commented 3 months ago

@florian-lefebvre thank you for this RFC. I think you mostly covered everything. However, something important that is missing is how an adapter gets to provide their "getter" function: process.env VS Deno.env.

theoephraim commented 3 months ago

Hello fellow Astronauts!

We over @dmno-dev have been very deep in this specific problem space for the last few months, building out a platform-agnostic configuration tool meant to be a unified system for your entire stack. You can read more at dmno.dev and specifically check out our Astro integration. While our solution may be overkill for some folks, we hope it becomes the go-to solution for anyone working in a monorepo or wanting some tools with a bit more than what is built-in.

Regardless, we are super happy to see the progress on this RFC and for Astro to have amazing first-class env var handling.

Apologies for being a little late to the party, as I know some excellent implementation work by @florian-lefebvre is already underway, but I wanted to share a few thoughts here (that I’ve shared with him already) from what we've learned while building out DMNO.

Please understand this is all meant to be constructive, simply to provoke discussion, and get the best possible outcome for Astro users.


Challenges of hybrid rendering

In any SSR app (output server or hybrid) it can be very difficult to reason about where/when certain code will be run and/or bundled. In fact it often changes because the parent page/component or the site settings rather than what is happening in the component itself.

This presents challenges on two fronts - preventing leaks, and reasoning about if config will be actually dynamic (vary at boot time) or not.

There just is no easy way to enforce that the script part of a component has access to secrets, but only when being run on the server... Or to ensure that a config item you want to vary dynamically at boot time was not used during a pre-render of a page, effectively making it static even though it was not "bundled" into the code.

When the line between client and server can be so blurry, I think it will lead to confusion to say an item is a “server” or “client” item, or vary behaviour depending on how it is accessed.

Instead I think we should rely on a clear config schema to express the user's intent, build the system to do the right thing whenever possible, and to protect the user from anything unexpected.

Config Schema

When we think about how to classify our config items, to me it really boils down to 2 concepts:

The important part here is that these 2 concepts are fairly easy to reason about and totally decoupled

In DMNO this is accomplished by marking items with sensitive: true/false and dynamic: true/false along with project-level settings to define the default behaviour (ex: default_static vs default_dynamic).

_It should be noted that most users probably won't care about this dynamic/static stuff, and the default behaviour can still be set to public_static, which matches what most are used to (sensitive items are dynamic and public items are static)._

The Edge cases

You can read more about DMNO's handling of this in our dynamic config guide.

Access patterns

The current proposal has multiple methods of accessing certain config items which is supposed to help achieve safety. But this means you must think about which access pattern to use, and changing the schema of an item may necessitate changing how it is accessed throughout your code.

By using a combination of proxy objects and build-time rewrites, the user gets to access everything exactly the same way, and it all just works as intended. In DMNO, we inject 2 globals (which could of course be imported from virtual modules instead)

NOTE - while the extra PUBLIC object is not strictly necessary, I do think it's helpful when browsing what items can be safely used on the client, and it's a bit closer to what folks are used to with the `PUBLIC` prefix._

The injected proxy objects mostly let us give the user more helpful error messages (ex: “that item doesn’t exist” vs “that item exists but it is sensitive”), and it handles loading of any dynamic-public items. Loading these dynamic config items in the client uses a blocking http request so that we can maintain the same DX, rather than needing to do something like await getConfigItem(‘DYNAMIC_THING’).

Protection from footguns

Once we have a full schema to express the user's intent, we can effectively protect the user from doing the wrong thing, without having to think too much:

PUBLIC_ prefix

One more note on the current proposal. Once we've introduced a proper schema, I don't think it is helpful to enforce a naming convention. I'd recommend keeping one or the other as it just adds another source of truth and opportunity for confusion. Personally with the rest of the system keeping you safe, I'd recommend relying on the schema, as it means you only update the schema, not where the config is accessed.

In DMNO, our type system allows extendable types to be marked as sensitive - for example any instance of a StripeSecretTokenDataType is always going to be sensitive - so we must use the schema


Hopefully this is helpful, at the very least to kick off some discussion. And please if this sounds interesting, head over to our Astro integration guide and give it a try. We're happy to get deep into the weeds with folks and to help in any way we can!

Also please don't hesitate to reach out by email or on the Astro Discord.

Cheers!

theoephraim commented 3 months ago

In doing a larger review of other tools, it looks like sveltkit is thinking along very similar lines to what I have proposed above - see https://kit.svelte.dev/docs/modules#$env-dynamic-private They let you import from 4 different virtual modules for the combos of public/private and static/dynamic

They use the prefix only as the marker of public/private, and the import method to help control static/dynamic behaviour.

florian-lefebvre commented 3 months ago

Yep we looked at it during the stage 2 rfc but thanks for bringing it again!

theoephraim commented 2 months ago

Maybe if this is something y'all have already thought through, and maybe I'm overestimating it's importance, but I think you want to make sure users can still benefit from build-time injection and the constant folding / tree-shaking that it enables.

I think anything that looks like import { SOME_VAR } from 'astro:env' is going to make this very difficult.

On the other hand, using anything like import AstroEnv from 'astro:env' and then using AstroEnv.SOME_VAR makes it fairly trivial to add extra entries in the vite's define configuration. Of course you probably won't want to automatically enable this for all env items have some rules about which ones can be replaced, based on the schema, or (less optimally) on how it was imported/used.

florian-lefebvre commented 2 months ago

Just to make sure I understand correctly, if I have this:

import { FOO } from 'astro:env/client'

if (FOO) {
    // do something
}

and FOO is false, you expect the whole thing to be removed on build.

I have to admit I'm not familiar enough with vite to know if it currently works, we're doing some pretty simple stuff for public variables so maybe it works already? I think @bluwy may know

bluwy commented 2 months ago

Using import { FOO }, import * as AstroEnv, or import AstroEnv (if we support this) will all work with Rollup's treeshaking, so I don't think we have to change the syntax to accommodate it.

theoephraim commented 2 months ago

Ah - right of course because it's not rewritten by vite/rollups define... However this does also mean it needs to be an actual constant and not some proxy or function call. Thanks for confirming!

narration-sd commented 2 months ago

Very glad to hear about this, as I think it will allow me to unwind all the complexity made when Astro 4 summarily removed ability for import.met.env in astro.config, for a package adding Sanity live Presentation editing on Astro.

I have no idea why the discussion I raised a the time didn't inform me about this -- you 'd have gotten early feedback, and I would have had a great deal less trouble.

The trouble came because of the ferocious, non-traceable deployment-only errors which occur if you allow the client to one way or another access any code which had touched what looked innocent, the server-side variables when produced by the recommended method. This confuses Vite, in very bad ways.

narration-sd commented 2 months ago

I'd like to ask -- when do you foresee this becoming a more-than-experimental feature (months, etc.)??

florian-lefebvre commented 2 months ago

This feature should go out of experimental in a few minors, hard to tell at this time

florian-lefebvre commented 2 months ago

New related RFC https://github.com/withastro/roadmap/discussions/956

florian-lefebvre commented 2 months ago

New related RFC #957

braden-w commented 1 month ago

@florian-lefebvre Thank you for your work on this RFC! It's exciting to see Astro moving towards improved environment variable handling. I have a few questions that I hope can contribute to the discussion:

  1. Cross-context Usage:

The current proposal seems to focus primarily on using environment variables within the Astro application context. However, in many projects, we might need to access these variables in non-Astro contexts as well (e.g., standalone scripts where we still want database connections, debugging astro actions in isolation, etc.). Is there a way we could extend this proposal to allow for seamless use of environment variables across all contexts?

  1. Comparison with T3 env:

The T3 env package offers some nice features like runtime validation, type inference, and the ability to use environment variables outside of the main application. How does this proposal compare to T3 env in terms of features and ergonomics? After using the RFC, I found the ergonomics feel slightly more awkward, especially since I was unable to use it in my scripts due to point 1. If we're going to recommend Astro users to use Astro env, what benefits does it provide that aren't provided in using the alternatives?

This might instead be going into the direction of a standalone module that can be imported in both Astro and non-Astro contexts. It might look something like this:

   import { createEnv } from 'astro/env';

   export const env = createEnv({
     clientPrefix: 'PUBLIC_',
     server: {
       DATABASE_URL: z.string().url(),
       API_KEY: z.string().min(1),
     },
     client: {
       PUBLIC_API_URL: z.string().url(),
     },
     runtimeEnv: process.env,
   });

Where env is imported as a typesafe object whenever we need to use it.

This approach would allow for consistent environment variable usage across all parts of a project, including scripts and other non-Astro files.

florian-lefebvre commented 3 weeks ago

Hey @braden-w thanks for bringing this up! It's out of scope of this RFC but we're not against it! That's something often asked and source of confusion so I think that's something we could tackle. Would you mind creating a new roadmap discussion for this with as much info/context as you can? Then we can share it to gather more feedback and usecases

hxjo commented 3 weeks ago

Hello there,

First of all, thank you very much for your work, I love Astro and the people behind it.

I have a question regarding environment variables, why are we currently only reading those passed from .env.* and with the CLI command ? It means all environment variables present on the machine are ignored ; this is very impractical when those are handled by, for example, kubernetes and injected in the pod ; which makes me rely on process.env for now.

While I understand exposing the whole process.env is not desirable, maybe something similar to Vite with a prefix might do the trick ?

Again, thanks a lot for your work, and I'm eager to try this new api. Have a great day

florian-lefebvre commented 3 weeks ago

There are a few scenarios:

If you think that's a bug, feel free to open an issue on the main repo and ping me! Happy to look into this

hxjo commented 3 weeks ago
  • In SSR, it uses whatever thing the adapter specified, eg. process.env for node, Deno.env.get() for deno etc

Ooh, that makes sense. I'm in SSR with node which explains the need to use process.env then.

  • You can access this implementation directly by calling getSecret('KEY'). It's literally an interface for the point above

I'll try that today then !

Does not seem like a bug at all, just a misunderstanding from me. Thanks for the clarification!

florian-lefebvre commented 2 weeks ago

This is a call for consensus, and we aim to close the RFC in the next three days. Let us know if you have any important concerns.

florian-lefebvre commented 1 week ago

The final comment period has ended and this RFC is accepted. The feature will be unflagged in Astro 5.0.