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

Handling server-side/sensitive/runtime variables #3176

Closed
GrygrFlzr opened this issue Apr 27, 2021 · 4 comments · Fixed by #5404
Closed

Handling server-side/sensitive/runtime variables #3176

GrygrFlzr opened this issue Apr 27, 2021 · 4 comments · Fixed by #5404
Labels

Comments

@GrygrFlzr
Copy link
Member

GrygrFlzr commented Apr 27, 2021

Context

Currently, Vite indiscriminately replaces process.env. with ({}).:

const replacements: Record<string, string | undefined> = {
'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV || config.mode),
...userDefine,
...importMetaKeys,
'process.env.': `({}).`
}

I'm not sure if this behavior is Vite is attempting to protect SPA authors from accidentally using process.env.SECRET in their code. Confirmed with Evan that this was to support libraries that indiscriminately use process.env.NODE_ENV.

The Problem

Applications that have a server-side encounter a common issue of needing to be able to access sensitive information from environment variables, e.g. database or API credentials. Aside from secrets, it is also possible to require runtime variables that you don't know at build time. Currently people are working around this using process.env['SECRET'] to avoid the string replacement, but it is entirely possible that in the future Vite decides to similarly replace process.env[ with ({})[ given the above code. I am unsure if this is the "blessed" way to access secrets or an oversight, considering it completely circumvents the concerns written above.

Using VITE_ prefixed environment variables is not a solution. It is explicitly documented that it should not contain sensitive information.

Using define is not a solution:

  • That would be baked in at build time, which is not appropriate for all use cases.
    (e.g. heroku database URLs can change across application reboots, so runtime ENV vars are required)
  • That exposes it to client side code because it blindly does a find-and-replace.

Potential Solutions

(in no particular order)

  • This could instead be implemented individually by SSR providers. This seems counterproductive if the solution can be framework-agnostic, but would be a necessity if it is determined to be outside of Vite's scope or capabilities.
  • Declare process.env['SECRET'] as a "blessed" way to access runtime variables, although then it is unclear why process.env. is replaced in the first place.
  • Stop replacing process.env. - This replacement behavior is not currently written in the docs.
  • Create a "store" somewhere (in the Vite config?) that could contain such information, although I'm not sure how to expose this to the user. Vite doesn't really "know" about server vs client code aside from SSR'd code, and not all SSR code should have access to secrets. This also doesn't really solve the runtime non-secret situation.
  • Have a config option to disable the process.env. replacement.

Non-solutions

  • Start replacing process.env[ with ({})[ if you're sadistic
@GrygrFlzr GrygrFlzr changed the title Handling server-side/sensitive secrets Handling server-side/sensitive/runtime secrets Apr 27, 2021
@GrygrFlzr GrygrFlzr changed the title Handling server-side/sensitive/runtime secrets Handling server-side/sensitive/runtime variables Apr 27, 2021
@Conduitry
Copy link

It seems to me that, at best, what the current ({}). behavior achieves is hiding problems from people who end up with process.env. in code that's destined for the browser. ({}).FOO doesn't seem like it would result in correct behavior in any case, other than just that it wouldn't crash like process.env.FOO in the browser would. I would argue though that crashing is better than quietly providing unexpected behavior.

If removing this replacement isn't a viable option for compatibility reasons, then I think this should be configurable in some way - if not in all cases, then at least in SSR situations, where there is a legitimate reason to be using process.env..

@b-fuze
Copy link

b-fuze commented May 15, 2021

Why not replace it with optional chaining and globalThis? e.g. process.env. becomes

(globalThis.process?.env ?? {}).

And ofc, making this configurable is another option

@arxpoetica
Copy link

arxpoetica commented May 31, 2021

Been futzing around with process.env.* all evening, and I can't understand why Vite doesn't make server-side process.env.* a first-class consideration.

The current lack of solutions is highly frustrating.

rmunn added a commit to sillsdev/web-languagedepot-api that referenced this issue Jul 5, 2021
Due to vitejs/vite#3176, we have to use
`process.env['FOO']` rather than `process.env.FOO`.
rmunn added a commit to sillsdev/web-languagedepot-api that referenced this issue Jul 5, 2021
Due to vitejs/vite#3176, we have to use
`process.env['FOO']` rather than `process.env.FOO`.
@hatemjaber
Copy link

Until there's a solution for this, is safe to import dotenv in the hooks file and use process.env['WHATEVER'] in server side code? I tried to access those variables in svelte files and it seems that vite does ignore them by default; I'm assuming it's safe and the only workaround at the moment.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
6 participants