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

BREAKING(dotenv): Fix dot env permissions #3578

Merged
merged 13 commits into from
Aug 29, 2023

Conversation

cknight
Copy link
Contributor

@cknight cknight commented Aug 25, 2023

Addresses #3572

Headline changes

  • BREAKING: remove restrictEnvAccessTo configuration option
  • Additional documentation added, including commentary on permissions
  • Removed all module dependencies
  • Fixed bug (see below)
  • Refactored tests, added additional tests

Overview

This is a breaking change, primarily to remove the restrictEnvAccessTo configuration option. To allow expansion of .env variables to use values from the process environment, the original implementation read the entire process environment into memory. This mean all users of dotenv required the --allow-env permission. Accessing all process variables was undesirable if you only needed to expand a single env variable, so restrictEnvAccessTo was added to restrict which env variables dotenv had access to and allow the granular --allow-env=SOME_VAR to work properly. This setup deviates away from how Deno handles permissions.

This change now allows for per-env-var access with no code configuration required and if you don't export any variables or expand any process variables then no --allow-env permissions are needed at all. The below examples show before and after permission handling:

Permission example 1

#.env
HELLO=world
// mod.ts
import { load } from "https://deno.land/std@0.199.0/dotenv/mod.ts";
await load();

Before change: Run with deno run --allow-read --allow-env mod.ts

After change: Run with deno run --allow-read mod.ts

Permission example 2

#.env
HELLO=${A}
// app.ts
import { load } from "https://deno.land/std@0.199.0/dotenv/mod.ts";
await load();

Before change: Run with A=world deno run --allow-read --allow-env mod.ts (full env access still required)

After change: Run with A=world deno run --allow-read --allow-env=A mod.ts (granular access allowed)

Permission example 3

#.env
HELLO=${A}

Before:

import { load } from "https://deno.land/std@0.199.0/dotenv/mod.ts";
await load({restrictEnvAccessTo: ["A"]});

Run with A=world deno run --allow-read --allow-env=A mod.ts (granular access only works with code grant to A via restrictEnvAccessTo)

After change:

import { load } from "./mod.ts";
await load();

Run with A=world deno run --allow-read --allow-env=A mod.ts (no code change required for granular access)

Bug fix

When expanding a process environment var who value was blank, the existing code was setting the configuration value to undefined rather than empty string.

#.env
HELLO=${A}

Before:

// mod.ts
import { load } from "https://deno.land/std@0.199.0/dotenv/mod.ts";
console.log(await load());

Run with A= deno run --allow-read --allow-env mod.ts yields:

{ HELLO: "undefined" }

After change:

// mod.ts
import { load } from "./mod.ts";
console.log(await load());

Run with A= deno run --allow-read --allow-env=A mod.ts yields:

{ HELLO: "" }

Additional consideration

As this is a breaking change, it may be desirable to consider #3561 as well, which would also be a breaking change to dotenv. However, as there is no feedback on that proposal to date no PR for that change has been progressed.

@cknight cknight requested a review from kt3k as a code owner August 25, 2023 22:26
@cknight cknight changed the title Fix dot env permissions BREAKING(dotenv): Fix dot env permissions Aug 25, 2023
@cknight
Copy link
Contributor Author

cknight commented Aug 25, 2023

Any ideas why the linter is failing? Also failing locally for me, but I'm not sure how to fix it.

@iuioiua
Copy link
Collaborator

iuioiua commented Aug 25, 2023

std/streams/readable_stream_from_iterable.ts needs to be deleted as it's deprecated. Best doing that in another PR.

@github-actions github-actions bot added the wasi label Aug 27, 2023
* looked up. This allows to permit access to only specific Env variables with
* `--allow-env=ENV_VAR_NAME`.
*/
restrictEnvAccessTo?: StringList;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this option only in typing in a few versions to prevent immediately breaking users code (in type checks)

* Load environment variables from a `.env` file.
* Load environment variables from a `.env` file. Loaded variables are accessible
* in a configuration object returned by the `load()` function, as well as optionally
* exporting them to the process environment using the `export` option.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these lines.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice clean up and fix!

@kt3k kt3k merged commit f6d0d8a into denoland:main Aug 29, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants