hapipal / boilerplate

A friendly, proven starting place for your next hapi plugin or deployment
https://hapipal.com
183 stars 27 forks source link

Consider using a better env file library #91

Open sholladay opened 4 years ago

sholladay commented 4 years ago

The boilerplate currently uses the dotenv package, which is less than ideal because it's easy to make mistakes with it and accidentally commit secrets to the git repository, among other issues.

It would be great if the boilerplate could use a safer library, such as envy or dotenv-safe, which provide various levels of validation.

devinivy commented 4 years ago

I am actually a fan of envy. If we did decide to go that direction, could we contribute to update envy's tests for some recent versions of node? (I realize even dotenv's test suite is a little bit behind too!) What do others think?

Here are some additional thoughts:

sholladay commented 4 years ago

I created envy after a relatively junior developer at my company accidentally committed sensitive secrets, including database credentials, and pushed them to the git repo, where they were subsequently seen by people who shouldn't have had access to them. I then spent all day rolling passwords and API keys, combing through logs to check for misuse, and educating people on what happened, etc. It was a bad time and I set out to ensure that would not happen again (and it hasn't).

The workflow went something like this:

  1. I sent them the credentials on an end-to-end encrypted messaging system with a brief description of what to do with them
  2. They put the credentials in .env.example instead of .env, apparently because they misunderstood its purpose
  3. They ran cp .env.example .env after realizing their mistake
  4. They started the app, it loaded successfully, and they proceeded with their development work, thinking nothing of it
  5. At some point later, they did git commit -a and git push, followed by a PR

I prefer to look at this not as a problem with the developer but as a problem with the system. It was too easy to make this mistake. In some sense, this is actually a failing of the .env.example pattern. At a glance, it looks like you're supposed to put your values in there. Yet, I didn't want to get rid of having a .env.example file because it has been genuinely useful as a form of documentation and lightweight validation.

Thus, envy was created to help catch mistakes while still supporting .env.example. It performs a number of sanity checks:

Amazingly, no other library does these checks. Even dotenv-safe only checks for missing variables, but makes no effort to prevent committing secrets.

I don't see how it is easier to commit secrets with our setup versus envy's or dotenv-safe's. We identically gitignore server/.env and keep a server/.env-keep as an example file. Do dotenv-safe and envy confirm that the file is gitignored at runtime, or something along those lines?

There are two main ways secrets could end up in the repo, both of which envy detects at runtime:

  1. Developers may screw up the .gitignore in various ways, e.g. omitting the .env file or ignoring the wrong file path by making a typo or using the wrong directory
  2. Developers copy their secrets into .env.example instead of .env

The main difference with dotenv-safe seems to be that it ensures the .env file and the example file are in sync, and is stricter about the example file's contents being present in the environment.

Correct. dotenv-safe is a small improvement over dotenv but I still find it to be a useful addition in the sense that secrets being out of sync with the example file can be a pain. To be clear, that library considers "safety" to mean that the .env.example file is treated as a list of required variables. Preventing leaking of secrets is not a goal of that library.

The main difference with envy (aside from not mutating the environment) seems to be that you only have access to the variables that you explicitly opt-in to using the .env file and example file.

Yes, envy returns an object that contains:

At the moment, envy does not return extraneous entries in process.env that are missing in both .env and .env.example. This is a design decision that I was on the fence about for a while. I'm open to discussing changes to that behavior or adding an API that includes all process.env entries. Would definitely like some feedback on this. Ultimately, I chose not to include those entries because it's a little more flexible - the user can still access process.env manually if they need to.

I prefer envy's approach to dotenv-safe because it isn't as pedantic about .env and the example file matching each other, and makes more room for defaults to be applied within the app. I also like that envy doesn't modify the environment itself, which reinforces that all configuration for the app should be determined in/near the app's entrypoint and passed down as e.g. plugin options.

Agreed. There's no need for .env and .env.example to be strictly in sync, so long as the variables defined in .env.example are available globally in process.env. In fact, if envy finds all of the required variables in process.env, it short-circuits and avoids reading the .env file. This is another design decision I am open to refining further. My philosophy here is that process.env takes precedence because values in there may come from someone running MY_VAR=foo node app.js. It's convenient to be able to override variables on the command line that way, so envy respects that.

I appreciate that dotenv does not have any dependencies, and dotenv-safe only depends on dotenv. On the other hand, envy has multiple dependencies. I do aim to keep the boilerplate dependencies as light as possible in order to introduce minimal transitive dependencies outside of the hapi ecosystem, so it is a consideration.

dotenv lacks dependencies because its behavior is overly simplistic. Every dependency that envy has is for a meaningful purpose, either related to ergonomics of the API or the sanity checks.

I care a lot about dependencies. I thoroughly investigate all of my dependencies, including deeply nested ones, and aim to read and understand all of their code. No small task. One of the ways I make this manageable is to only use modules by Sindre Sorhus, when possible. He keeps his code lean and simple.

Here is the entire dependency tree for envy:

❯ npm ls --production
envy@2.0.0 /Users/sholladay/Code/personal/envy
├── camelcase@5.3.1
├─┬ camelcase-keys@5.2.0
│ ├── camelcase@5.3.1 deduped
│ ├── map-obj@3.1.0
│ └── quick-lru@1.1.0
├── filter-obj@1.1.0
├── is-wsl@1.1.0
└─┬ path-type@3.0.0
  └── pify@3.0.0

Every single one of these modules is made by Sindre. They are actively maintained, stable, and have high quality tests, documentation, etc.

Also notice that most of them are simply utilities that arguably ought to be in JavaScript's standard library. The pify dependency will be removed when I update to path-type@4, further simplifying things.

I like the idea of using a dependency that is more related to the hapi community :)

Awesome, well it's definitely meant to play nicely with hapi. One of the things I strongly dislike about the Express ecosystem is the prevalent use of process.env and I'm so thankful that hapi doesn't do that. envy very much encourages the hapi way of doing things. I use envy for all of my production applications. I dogfood it with hapi, too, It even secures the Stripe secrets for my own personal website.

YoannMa commented 4 years ago

I think it's overall a good idea, having a "safer" default is always good (plus it follows the hapi "way")

My only struggle when using envy is the behavior :

Validates that .env.example does not have any values because they could be actual secrets

Because I mostly want to use .env.exemple as the exemple with default non-production values, that way you can quickly get the project working with a single cp .env.exemple .env. But I agree with why envy does not allow that behavior at all.

sholladay commented 4 years ago

My suggested workaround is to either:

These solutions aren't as bad as they might sound because I would urge you to only do the above for simple, non-sensitive config values like a port number or apiUrl. It's not appropriate for things like API keys.

YoannMa commented 4 years ago

That's mostly what I do indeed, using confidence (this project use it as well now that I looked at it) for default values.

devinivy commented 4 years ago

@sholladay I am playing with envy in a baseline pal boilerplate, and am running into an issue I can't resolve myself: the boilerplate only has one env variable (PORT) which also happens to be optional. I am happy to mark it as optional by commenting it out and applying the default with joi, but envy doesn't allow an empty example file. I am sort of at an impasse right now, as I don't want users to have to fill-in any environment variables in order to run their new project, but I also would like envy to be all setup for them. Do you have any recommendations either for either 1. envy feature requests that you would be open to receiving a PR for or 2. adjusting my usage of envy? For example: would you be open to allowing example files without an entry, possibly behind a flag?

sholladay commented 4 years ago

At the time I was thinking that an empty example file would likely be a mistake. But your use case seems valid. Let's remove that assertion. PR welcome.

I'm happy to help work on this if you run into any issues, but I'm currently in the process of selling my house so fairly busy for a few weeks.

devinivy commented 3 years ago

Just an update here for those interested: I am favoring an eventual move to envy, but there are some loose ends regarding Windows support that are still in progress, and I don't expect it to land for the upcoming version of the boilerplate. Before moving to envy I would also like to alter how environment variables are handled in confidence, and that work is also in progress but not finalized. Here are the relevant issues: https://github.com/sholladay/envy/issues/4 https://github.com/sholladay/envy/pull/8 https://github.com/hapipal/confidence/issues/106 https://github.com/hapipal/confidence/pull/110.