vite

vite copied to clipboard

Provide an option to disable to warning and subsequent error when configuring Vite with an empty envPrefix option.

Description

Current behaviour of Vite is to warn and exit with an error when supplying '' as a value for the envConfig configuration option.

In certain cases a project owner should be able to accept the risk and ensure the project isn't exposed to security risks in another way, such as containerised build environment with limited variables.

Alternatives Considered

One can technically list the first couple of characters of all of the environment variables that they would like to allow, e.g.:

defineConfig({
  // ...
  envPrefix: [
    "A",
    "B",
    "C",
    "D",
    "E",
    "F",
    "G",
    "H",
    "I",
    "J",
    "K",
    "L",
    "M",
    "N",
    "O",
    "P",
    "Q",
    "R",
    "S",
    "T",
    "U",
    "V",
    "W",
    "X",
    "Y",
    "Z",
  ],
  // ...
});

But a much more pragmatic way to solve this is to allow the user to pass in an option that explicitly disables this check. With this pull request I propose the addition of dangerouslyAllowEmptyEnvPrefix option, which defaults to false and when set to true allows the usage of '' as a value for envPrefix.

Additional context

In docs, I purposefully did not refer to this option in the description for envPrefix option. This new option should be something people stumble upon as a way to solve their problem rather than considered as just something to set to true to remove an inconvenience.

---

What is the purpose of this pull request?

• [ ] Bug fix
• [x] New Feature
• [x] Documentation update
• [ ] Other

Before submitting the PR, please make sure you do the following

• [x] Read the Contributing Guidelines.
• [x] Read the Pull Request Guidelines and follow the Commit Convention.
• [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• [x] Ideally, include relevant tests that fail without this PR but pass with it.

I'm not sure of an option to allow this. Is there a reason to not add the env prefixes in the containerized build environment? I'm assuming that you're controlling it so it could be achievable.

If we ever do this, I think we could go with envPrefix: false. But I agree with @bluwy, and it is in line with what we discussed when we added the custom prefix option. We think it is better to force a prefix, so we should get a very strong use case to justify the new option.

Here's an example scenario, where this change might be useful. Your variables might not always come from your own software, they may be generated by an external service or process, so with this current limitation you end up having to explicitly map these to PREFIX_, which gives you an additional point of failure.

The sentiment of this feature is to communicate to developers:

"yes we have this security measure in place to protect you from exposing your variables by accident, but also, we understand that this might not suit every single application, so rather than you having to work around this security measure, here's a way to turn if off"

The feature is documented but not linked where envPrefix is described deliberately to discourage people from jumping straight to using it, but if using it is the most pragmatic solution to your problem, you'll easily be able to stumble upon this solution, either via StackOverflow or a Google search. The name starts with dangerously to depict the fact that using this option you should take extra care to protect yourself from unwanted consequences, much like dangerouslySetInnerHTML in React noting that you're turning off a safety feature. Not only that but you also have to pair this option with envPrefix: "", otherwise it does nothing.

@patak-dev about using envPrefix: false, while that also addresses the same problem, I believe it would remove a layer of communication for developers who may not be acutely aware of environment leaking and would only keep protection in place for people accidentally setting envPrefix: "", which I predict is a much smaller number of users compared to the amount of people wanting to turn it off to begin with.

@danielkov do you have an example of a real setup with this issue? What specific software is causing this problem that you don't control?

I've added this PR to the team board for discussion. To set the expectations, we have a bit of a backlog of features to review so it may take a while to get back to you.

@patak-dev yes, so in our system, there's an amount of preset environment variables for each container regardless of what tooling they use, e.g.: VERSION and APP_NAME. These are ran in k8s pods and are only accessible on local network or behind VPN by employees, so leaking secrets isn't as pressing of a security concern as one would have in case of a public-facing application.

@danielkov I wonder if a better approach would be to have a new envAllowed config option that lets you specify a safe list. So you can add VERSION and APP_NAME to it. And maybe we want to have env: { prefix: 'CUSTOM_', allowed: ['VERSION', 'APP_NAME' ] }

Note: maybe explicit instead of allowed? 🤔

What about manually reading from process.env and setup define for them? You can do:

define: {
  'import.meta.env.VITE_APP_NAME': JSON.stringify(process.env.APP_NAME)
}

for example, so we can avoid another option, while being explicit that it's not a normal thing to do.

I think this may be a common enough use case that it could justify a straightforward option. I also like having the possibility of later letting users disable the feature with env: false.

I think a envPrefix: false would be good too, but I'm on the side that this still isn't a common case 😅