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

feat(dotenv): allow reading from .env files without granting env access #3306

Merged
merged 3 commits into from
Apr 19, 2023
Merged

feat(dotenv): allow reading from .env files without granting env access #3306

merged 3 commits into from
Apr 19, 2023

Conversation

jsejcksn
Copy link
Contributor

@jsejcksn jsejcksn commented Apr 4, 2023

This PR improves security/privacy by allowing the user to read env data from .env files without granting actual environment permissions.

Continuation of #3221 (comment):

Note to self and for a potential follow up PR: After refactoring the new test to be stricter, I realized that there doesn't seem to be a way to load environment variables purely from a file: without granting access to at least some actual environment variables. In my view, this seems like an oversight and could be fixed rather easily by:

  • not using an empty array [] as the default parameter for restrictEnvAccessTo in most of the functions in this module, and
  • refactoring readEnv to interpret the case of an empty array as "allow access to no variables" (a no-op), and just return {} without attempting to use the Deno.env API at all

Technically, I think this is a breaking change — but if I'm wrong about that, please feel free to change the title. There are no tests to support my hypothesis that it's a breaking change, but I think that it could break existing code if a consumer previously provided an explicit empty array and expected for variables to be read from the environment.

@jsejcksn jsejcksn requested a review from kt3k as a code owner April 4, 2023 21:50
@kt3k kt3k changed the title BREAKING(dotenv): Allow reading from .env files without granting env access feat(dotenv): allow reading from .env files without granting env access Apr 19, 2023
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. Thanks for the suggestion!

Technically, I think this is a breaking change

I agree, but the default behavior and non-empty restrictEnvAccessTo usages are the same. I think this can be considered as a new feature.

@kt3k kt3k merged commit 29e2dc5 into denoland:main Apr 19, 2023
@jsejcksn jsejcksn deleted the feat/dotenv-permissions branch April 19, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants