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

Problem when using a library that uses debug in Deno #981

Open
krlwlfrt opened this issue Nov 28, 2024 · 8 comments
Open

Problem when using a library that uses debug in Deno #981

krlwlfrt opened this issue Nov 28, 2024 · 8 comments

Comments

@krlwlfrt
Copy link

I use jsdom in a project in Deno. jsdom uses debug as a dependency:

└─┬ [email protected]
  ├─┬ [email protected]
  │ ├─┬ [email protected]
  │ │ └── [email protected] deduped
  │ └── [email protected]
  └─┬ [email protected]
    └── [email protected] deduped

Deno asks for permission for access to environment variables. Unfortunately debug accesses process.env directly and on requiring the module itself which leads to the following request by Deno.

┏ ⚠️  Deno requests env access.
┃  ├─ Object.toObject (ext:runtime/30_os.js:134:12)
┃  ├─ Object.ownKeys (ext:deno_node/_process/process.ts:54:40)
┃  ├─ Function.keys (<anonymous>)
┃  ├─ Object.<anonymous> (file:///home/krlwlfrt/work/krlwlfrt/calendar/node_modules/debug/src/node.js:124:30)
┃  ├─ Object.<anonymous> (file:///home/krlwlfrt/work/krlwlfrt/calendar/node_modules/debug/src/node.js:267:4)
┃  ├─ Module._compile (node:module:745:34)
┃  ├─ loadMaybeCjs (node:module:770:10)
┃  ├─ Object.Module._extensions..js (node:module:755:12)
┃  ├─ Module.load (node:module:662:32)
┃  └─ Function.Module._load (node:module:534:12)
┠─ Learn more at: https://docs.deno.com/go/--allow-env
┠─ Run again with --allow-env to bypass this prompt.
┗ Allow? [y/n/A] (y = yes, allow; n = no, deny; A = allow all env permissions) > 

This goes against the principle of least privilege. Denying the request leads to an exception, because the code can't handle a rejection of that request with Deno's API.

Long story short: Could you please change the behaviour of your library, so that the process.env is not accessed on module load?

@Qix-
Copy link
Member

Qix- commented Dec 1, 2024

Er, No sorry. The whole point is that debug uses the environment to configure itself. Very common thing for logging libraries to do.

@Qix- Qix- closed this as completed Dec 1, 2024
@krlwlfrt
Copy link
Author

krlwlfrt commented Dec 1, 2024

Yes, sure. I get that. Totally valid point. I'm just asking if you could change it, so that the acccess to process.env does not happen on module load and rather on a function/method call.

Or that you access the variables that you need in process.env directly - like process.env.NODE_ENV or similar. And then parse the whole process.env when it is needed for debugging purposes.

Deno starts scripts without any permissions and then grants permissions as needed which is a huge security benefit. This is completely negated when I have to grant permission to access all environment variables.

@Qix-
Copy link
Member

Qix- commented Dec 1, 2024

I'm not quite sure what the difference is; environment variables are still being accessed. How does delaying their access provide any security benefit?

@krlwlfrt
Copy link
Author

krlwlfrt commented Dec 2, 2024

I'm not completely certain on how your module works, but I can see from glancing over the code, that inspectOpts, which contains the variables of process.env, is only accessed when certain functions are called.
I assume that these some/most of these functions are only called when the developer of a module that wants to debug it enables a the debug mode (via environment variable). When I changed the code to inspectOpts = {}, JSDOM worked completely fine.
So maybe it would only be necessary to access the full process.env when it is needed... Or you could build inspectOpts only when one of the referencing functions is called instead of on module load.

Deno is designed in a way, that the end user, who is running the script can grant access per environment variable, which is useful if you have data in your ENV that you don't want exposed to potentially thousands of node modules that might be installed in a project. If a module access process.env instaed of process.env.VARIABLE the end user has to grant access to all variables at once.
Node.js is the complete opposite, where I/O, access to network, environment variables, etc. is unrestricted and any node module could do harmful things, without the end user even noticing.

I'm not sure if I'm able to communicate clearly, what I mean...

@Qix-
Copy link
Member

Qix- commented Dec 6, 2024

I mean I'm not really sure that anything I do is going to fix Deno's case. There are still millions of packages that will never upgrade to the latest version. This will always be a problem. It's also a questionable way to achieve security, if you ask me.

@krlwlfrt
Copy link
Author

krlwlfrt commented Dec 8, 2024

There are still millions of packages that will never upgrade to the latest version.

Valid point.

It's also a questionable way to achieve security, if you ask me.
Yes, sure. It would be better to always run code you don't own in an unprivileged container. But it's not convenient and most people don't do it. And Deno's approach of asking for permissions is one layer of security.

I get that it's not that important to you. Would you consider it, if I'd open a PR with the changes I'm proposing?

@Qix-
Copy link
Member

Qix- commented Dec 8, 2024

It really depends on the changes but sure, I can take a look.

@Qix- Qix- reopened this Dec 8, 2024
@eikowagenknecht
Copy link

This would probably also solve another problem I'm encountering:

I'm running a Node.js application bundled by Vite and want to configure debug logging for the grammy library (which uses debug internally) based on runtime configuration. Currently, I set process.env.DEBUG right after startup when my config is parsed, but this doesn't work after bundling - likely because the env var is read during import, and the bundler hoists all imports to the top.

The proposed delayed loading would be a great solution for this use case, since it would allow setting the debug configuration before the actual environment variable reading happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants