-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
Javascript Debug Terminal tab still created in untrusted workspace #122428
Comments
Adding @connor4312 and @Tyriar since I'm not sure where the prompt comes from and I don't get any prompts |
I'm not able to repro this Terminals don't require trust, so we don't either. We only require trust when debugging right now, so the prompt is shown when you run a Node program: https://memes.peet.io/img/21-04-de3afdff-c282-427f-bb03-db7ed58f8d16.mp4 |
this is what I saw with latest Insiders Screen.Recording.2021-04-27.at.2.06.37.PM.mov |
Also after cancelling twice, I got
in the terminal. This is not expected as restricted mode says debugging will be disabled. |
I'm guessing you use nvm, which runs a Node script in the The debugger still gets attached if trust isn't granted, since we need to do so in order to "unlock" the process. If trust isn't granted, we just send |
@connor4312 can you wait to attach til after the profile is initialized? |
yes I'm using |
edited as you wrote it: we cannot avoid attaching until trust is given, otherwise any node scripts in the terminal will hang indefinitely. However if trust isn't granted we detach. |
is it possible to make it so the node scripts that run as part of initialization don't wait for the debugger? I assume no since it would probably already be that way if it were easy |
and alternatively, if we can't do that, can you not prompt for trust on these init scripts and just attach/detach |
In both cases the question is to identify 'what is an init script'? There's not a good way to do that, as far as I know there's no signal that the terminal gives us to say "this shell prompt is now ready." It would be technically possible to special-case nvm's init, but that could change and there could be other init scripts that run for other reasons -- so this isn't a can of worms I have been eager to tackle. |
I don't think the terminal should do anything about this, we expose the contribution and then try run the command when clicked, it's up to the extension to activate and handle that on demand of trusted workspaces to block the activation.
I don't think this is a problem, we're just attaching to the shell's standard script, not something inside the workspace?
@connor4312 there is no signal right now, perhaps we could cover almost all cases by having an event that's after the terminal has started writing and it stops processing output for 500ms or something (with a max of 2s?). What are your thoughts on this? It's a pretty common request from extensions like Python. |
I think that could work well. We would still attach to processes, since the bootloader doesn't know anything about the state of VS Code, but could similarly detach if the terminal isn't yet 'ready' |
Created #122471 for that ready idea. |
With #127717 js-debug was able to get smarter and not attach to the init scripts. I think that covers the outstanding queries on this issue. |
Testing #122252
The text was updated successfully, but these errors were encountered: