starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Prompt before launching commandlines when elevated ; cache answers in elevated state.json #15025

New Issue
Closed
opened 2026-01-31 04:26:23 +00:00 by claunia · 1 comment
claunia commented 2026-01-31 04:26:23 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Aug 31, 2021).

Originally assigned to: @zadjii-msft on GitHub.

This is part of https://github.com/microsoft/terminal/projects/5, and a pre-req for #632. I'm promoting this to a full issue since I've got a lot of edge-case-y work that needs doing and tracking somewhere.

  • Hot reload elevated-state.json
  • Find the right place to store the file
  • Figure out the right way to do the permissions on this file.
    • SYSTEM: GENERIC_ALL, Everyone: GENERIC_READ - Didn't work. Elevated Terminal couldn't write the file.
    • Admins: GENERIC_ALL, Everyone: GENERIC_READ - Didn't work. Sublime could just delete the file and write something else in it's place. Un-elevated Terminal could even write the file!
    • DOMAIN_USER_RID_ADMIN "Admin", not "Admins"?
    • Manual experimentation says "MIGRIE-HOME\Administrators": Full Control + "Everyone": Read seems to work as I want. Now how do I get that?
    • Something else?
    • IT WAS SOMETHING ELSE. WriteFileAtomic was the problem - the rename would discard the elevated only version of the file, then write a new one in it's place. Presumably, this doesn't really matter. We should only be calling into that function from an elevated window, but it's unfortunate that it doesn't prevent it from working when unelevated.
  • Maybe decide to still do WriteUTF8FileAtomic. We don't need the link handling, so I'm not worried about that...
  • Move the isElevated() logic to a common place to make sure we have one implementation
  • Make a call to the common isElevated() in WriteUTF8File*(), so that we can bail early if we're unelevated.
  • Check that the file is only writable by admins when opening. If it isn't, blow it away and recreate it.
  • Use unique_sid and in general, make sure to clean up the things allocated for us.
  • Only pop the dialog when running elevated
  • Don't pop the dialog if we're elevated (but UAC is disabled) (this is probably just #7754)
  • Don't pop the dialog for commandlines that are literally just a path to a file in C:\windows\system32.
    • The file must be a full path
    • The file must be in C:\windows\system32
  • UHG also resolve environment variables, so %SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe will work.
  • UHG WSL distros too
  • Pop the dialog for panes too.
  • Actually format the dialog message with the commandline
  • Resolve what happens when multiple controls should be added at once (See Ideas A and B below)
    • What happens when you reject the creation of a pane? Probably should just close
    • What happens during startup for this dialog? Do we just immediately go to the Complete state, then process the actions one at a time?
    • What happens when running a wt action in an elevated window?
    • Rejecting the creation of the first&only (or all) tabs should just close the window.
    • What happens if you wt nt ; sp, then reject the first tab? There are no tabs now to split!
    • Approving a commandline in one pane should automatically update the other ones.
  • Don't add the same commandline multiple times
  • There's some weird focus issues - Replacing the content of the pane doesn't update the tab title (and presumably other properties as well)
  • What happens when the user isn't an admin, but runs the terminal as another user that's an admin (that also has the Terminal installed)?
  • Make sure to add a "why am I seeing this" link
  • Add the TermControl's background to the admin warning placeholder's background.
  • After 64d02f2b2, fix the hot-reloading for elevated windows

Engineering deets

  • Break into two PRs:
    • Elevation State
    • Warning Dialog
  • Fix #7754 in it's own PR
  • send dev/migrie/uac-shield
  • send dev/migrie/f/632-attempt-2

what happens when multiple controls should be added at once

Idea A: for scenarios where we're going to perform multiple actions, check all of them for commandlines. If we find one action that has an unapproved commandline, then ask the user if they want to run all of the actions. They either approve them all, or none of them.

Idea B: Use a fake content dialog as a placeholder control, until the connection is approved.

  • B.1: We use non-terminal content in the Pane. We use a fake ContentDialog as a placeholder for the TermControl, then swap it out with the TermControl if the control is allowed.
    • This is in the branch from c66a56656 onwards
    • How do we handle closing? I think in my initial implementation, there was a IPaneContent interface that exposed all the things a TermControl might do.
    • Like, what happens when the dialog is rejected. It raises a Closed()? How does the pane listen for that...
    • I suppose the AdminWarningPlaceholder will be holding the Pane, the
    • NO it won't hold the pane. That's A: bad circular refs; and B: the Panes will toss the controls around to other panes. So that's bad.
  • B.2: We tell the TermControl to display the dialog itself. It doesn't Start() the connection immediately, instead it starts with it's fake contentdialog.
    • How does this work with oop controls? It wouldn't. The ContentProcess needs to be started before the TermControl. So even if we told the TermControl "you need to ask permission", we'd have already started the content process. It would be harder for the Control to then tell the app "yes I have permission for the thing you asked", then pass it the ContentProcess.
Originally created by @zadjii-msft on GitHub (Aug 31, 2021). Originally assigned to: @zadjii-msft on GitHub. This is part of https://github.com/microsoft/terminal/projects/5, and a pre-req for #632. I'm promoting this to a full issue since I've got a lot of edge-case-y work that needs doing and tracking somewhere. * [x] Hot reload `elevated-state.json` * [x] Find the right place to store the file * [x] Figure out the right way to do the permissions on this file. * [x] SYSTEM: `GENERIC_ALL`, Everyone: `GENERIC_READ` - Didn't work. Elevated Terminal couldn't write the file. * [x] Admins: `GENERIC_ALL`, Everyone: `GENERIC_READ` - Didn't work. Sublime could just delete the file and write something else in it's place. Un-elevated Terminal could even write the file! * [x] `DOMAIN_USER_RID_ADMIN` "Admin", not "Admins"? * [x] Manual experimentation says "MIGRIE-HOME\Administrators": Full Control + "Everyone": Read seems to work as I want. Now how do I get that? * [x] Something else? * [x] IT WAS SOMETHING ELSE. `WriteFileAtomic` was the problem - the rename would discard the elevated only version of the file, then write a new one in it's place. Presumably, this doesn't really matter. We should only be calling into that function from an elevated window, but it's unfortunate that it doesn't prevent it from working when unelevated. * [x] Maybe decide to still do `WriteUTF8FileAtomic`. We don't _need_ the link handling, so I'm not worried about that... * [x] Move the `isElevated()` logic to a common place to make sure we have one implementation * [x] Make a call to the common `isElevated()` in `WriteUTF8File*()`, so that we can bail early if we're unelevated. * [x] Check that the file is only writable by admins when opening. If it isn't, blow it away and recreate it. * [x] Use `unique_sid` and in general, make sure to clean up the things allocated for us. * [x] Only pop the dialog when running elevated * [x] Don't pop the dialog if we're elevated (but UAC is disabled) (this is probably just #7754) * [x] Don't pop the dialog for commandlines that are literally just a path to a file in `C:\windows\system32`. * The file must be a full path * The file must be in `C:\windows\system32` * [x] UHG also resolve environment variables, so `%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe` will work. * [x] UHG WSL distros too * [x] Pop the dialog for panes too. * [x] Actually format the dialog message with the commandline * [x] Resolve what happens when multiple controls should be added at once (See Ideas **A** and **B** below) * [x] What happens when you reject the creation of a pane? Probably should just close * [x] What happens during startup for this dialog? Do we just immediately go to the `Complete` state, then process the actions one at a time? * [x] What happens when running a `wt` action in an elevated window? * [x] Rejecting the creation of the first&only (or all) tabs should just close the window. * [x] What happens if you `wt nt ; sp`, then reject the first tab? There are no tabs now to split! * [x] Approving a commandline in one pane should automatically update the other ones. * [x] Don't add the same commandline multiple times * [x] There's some weird focus issues - Replacing the content of the pane doesn't update the tab title (and presumably other properties as well) * [ ] What happens when the user isn't an admin, but runs the terminal as another user that's an admin (that also has the Terminal installed)? * [ ] Make sure to add a "why am I seeing this" link * [x] Add the TermControl's background to the admin warning placeholder's background. * [x] After 64d02f2b2, fix the hot-reloading for elevated windows ### Engineering deets * [ ] Break into two PRs: * [x] Elevation State * [ ] Warning Dialog * [x] Fix #7754 in it's own PR * [x] send `dev/migrie/uac-shield` * [ ] send `dev/migrie/f/632-attempt-2` ### what happens when multiple controls should be added at once Idea **A**: for scenarios where we're going to perform multiple actions, check all of them for commandlines. If we find one action that has an unapproved commandline, then ask the user if they want to run _all_ of the actions. They either approve them all, or _none of them_. Idea **B**: Use a fake content dialog as a placeholder control, until the connection is approved. - B.1: We use non-terminal content in the Pane. We use a fake ContentDialog as a placeholder for the TermControl, then swap it out with the TermControl if the control is allowed. - This is in the branch from c66a56656 onwards - How do we handle closing? I think in my initial implementation, there was a `IPaneContent` interface that exposed all the things a TermControl might do. - Like, what happens when the dialog is rejected. It raises a `Closed()`? How does the pane listen for that... - I suppose the `AdminWarningPlaceholder` will be holding the Pane, the - NO it won't hold the pane. That's A: bad circular refs; and B: the Panes will toss the controls around to other panes. So that's bad. - B.2: We tell the TermControl to display the dialog itself. It doesn't Start() the connection immediately, instead it starts with it's fake contentdialog. - How does this work with oop controls? It wouldn't. The ContentProcess needs to be started before the TermControl. So even if we told the TermControl "you need to ask permission", we'd have already started the content process. It would be harder for the Control to then tell the app "yes I have permission for the thing you asked", then pass it the ContentProcess.
claunia commented 2026-01-31 04:26:24 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Jan 10, 2022):

We're cutting this for the time being. We'll wait for a more official plan before we ship something like this.

@zadjii-msft commented on GitHub (Jan 10, 2022): We're cutting this for the time being. We'll wait for a more official plan before we ship something like this.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-UserInterface Issue-Task Product-Terminal Resolution-Won't-Fix
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#15025
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?