toastdotdev / toast

The best place to stack your JAM. Toast is a Jamstack framework
153 stars 13 forks source link

process.env.* replacement should scope down to only used vars #7

Open ChristopherBiscardi opened 4 years ago

ChristopherBiscardi commented 4 years ago

The environment variable handling for process.env.* string replacements implemented here is extremely broad and results in errors like the following if an "impossible" env var is encountered. This is a cross platform issue (reports on Mac and Windows).

failed to parse global variable ITERM_ORIG_PS1=`'\[\e[33m\]\u\[\e[m\]@\[\e[32m\]\h\[\e[m\]:\W `parse_git_branch`$ '` as module

I'm not sure what the best fix is. We could safelist anything with a TOAST_ prefix and then we still need a solution for rando process.envs. Possibly a config option in toast.js to safelist more env vars.

reported by @jbolda and @lannonbr

ChristopherBiscardi commented 4 years ago

The env vars are restricted to TOAST_* which works. We could probably use a safelist in toast.js as well. The esinstall config will also be going in toast.js

jlengstorf commented 4 years ago

this seems like the right idea since you should deliberately opt in to inject an env var into client code. I do like the idea of a config setting to explicitly allow vs. prefixing, though

ChristopherBiscardi commented 4 years ago

@jlengstorf you think we should remove the TOAST_* prefix? I've never loved it myself in other frameworks. Makes some code "non-portable" when it doesn't need to be. I think the explicit config setting to safelist is solid though.

ChristopherBiscardi commented 4 years ago

noting that the env vars were scoped down in a recent PR, and that the TOAST_ prefix is already working while the manual safelisting is not

jlengstorf commented 3 years ago

I think supporting a safelist is a great idea

maybe it could be a toast.js export?

export const exportEnvToClient = ['PUBLIC_KEY', 'CLOUDINARY_CLOUD_NAME'];

// or a function

export const exportEnvToClient = () => Object.keys(process.env).filter((k) => k.startsWith('PUBLIC_'));
ChristopherBiscardi commented 3 years ago

a function could be enumerated at instantiation time, so I don't think we need to offer that as API surface.

export const exportEnvToClient = () => Object.keys(process.env).filter((k) => k.startsWith('PUBLIC_'));

would do the same as this, but need an additional function call.

export const exportEnvToClient = Object.keys(process.env).filter((k) => k.startsWith('PUBLIC_'));