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

Suggest: Deno.env.get("NODE_DEBUG") should not permission prompt #2507

Closed
dsherret opened this issue Aug 10, 2022 · 9 comments
Closed

Suggest: Deno.env.get("NODE_DEBUG") should not permission prompt #2507

dsherret opened this issue Aug 10, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@dsherret
Copy link
Member

dsherret commented Aug 10, 2022

I think probably Deno.env.get("NODE_DEBUG") should not permission prompt and do a permission check for the env var before requesting it (using Deno.permissions.query).

https://github.com/denoland/deno_std/blob/cac7764e3878ac8c2a3c26a1201f3dfcc15e6910/node/internal/util/debuglog.ts#L108

@dsherret dsherret added enhancement New feature or request good first issue Good for newcomers labels Aug 10, 2022
@devadeka
Copy link

@dsherret I'd like to try my hand at this one if that's alright...

@devadeka
Copy link

So had a go at it...
I found out how to check whether the permissions for the NODE_DEBUG environment variables have been granted used Deno.permissions.query.

Could I please get more clarification on expected behaviour? I am assuming line 108 is only run when permissions are granted. What happens for the other permission states? Should it permission prompt? If so could you point me to docs on how to trigger permission prompts.

Thanks

@kt3k
Copy link
Member

kt3k commented Aug 13, 2022

Introducing TLA in std/node has undesired effects on some esm.sh modules. See denoland/deno#14243 and #2097 for the past discussions.

Because Deno.permissions.query is async, querying env permission before getting it doesn't work in this case. (And actually we removed Deno.permissions.query call before to avoid the above issues. See #2098 )

@dsherret
Copy link
Member Author

Oh yeah, I forgot about that. I’m going to close this one, but we should come up with a way to remove this env var check eventually.

@dsherret dsherret closed this as not planned Won't fix, can't repro, duplicate, stale Aug 13, 2022
@kt3k kt3k removed the good first issue Good for newcomers label Aug 13, 2022
@kt3k
Copy link
Member

kt3k commented Aug 13, 2022

but we should come up with a way to remove this env var check eventually.

I agree. Maybe we can enhance/change CLI somehow to achieve this

@iuioiua
Copy link
Contributor

iuioiua commented Aug 13, 2022

Introducing TLA in std/node has undesired effects on some esm.sh modules. See denoland/deno#14243 and #2097 for the past discussions.

Because Deno.permissions.query is async, querying env permission before getting it doesn't work in this case. (And actually we removed Deno.permissions.query call before to avoid the above issues. See #2098 )

I see. I’ll keep that in mind. Thanks!

@nayeemrmn
Copy link
Contributor

Maybe we can enhance/change CLI somehow to achieve this

@kt3k What do you think about just not checking permissions for NODE_DEBUG in cli? Excluding any number of select environment variables from the permission system, as long as they are select, shouldn't be a problem since the alternative is just creating a separate permissionless binding for it which is the same thing.

@kt3k
Copy link
Member

kt3k commented Aug 22, 2022

What do you think about just not checking permissions for NODE_DEBUG in cli?

@nayeemrmn I'm open to that solution

since the alternative is just creating a separate permissionless binding for it which is the same thing.

What does 'the alternative' mean here?

@rivy
Copy link

rivy commented Jan 28, 2023

@dsherret , @kt3k , @nayeemrmn , I believe this could now be solved with gating, using the newly merged synchronous permission functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants