Spec for Elevation QOL improvements

[zadjii-msft]

⇒ doc link ⇐

Summary of the Pull Request

Despite my best efforts to mix elevation levels in a single Terminal window, it seems that there's no way to do that safely. With the dream of mixed elevation dead, this spec outlines a number of quality-of-life improvements we can make to the Terminal today. These should make using the terminal in elevated scenarios better, since we can't have M/E.

Abstract

For a long time, we've been researching adding support to the Windows Terminal
for running both unelevated and elevated (admin) tabs side-by-side, in the same
window. However, after much research, we've determined that there isn't a safe
way to do this without opening the Terminal up as a potential
escalation-of-privilege vector.

Instead, we'll be adding a number of features to the Terminal to improve the
user experience of working in elevated scenarios. These improvements include:

• A visible indicator that the Terminal window is elevated ([UI Styling to Clearly Indicate Elevated (admin) Window #1939])
• Configuring the Terminal to always run elevated ([[Question] Configuring Windows Terminal profile to always launch elevated #632])
• Configuring a specific profile to always open elevated ([[Question] Configuring Windows Terminal profile to always launch elevated #632])
• Allowing new tabs, panes to be opened elevated directly from an unelevated
  window
• Dynamic profile appearance that changes depending on if the Terminal is
  elevated or not. ([UI Styling to Clearly Indicate Elevated (admin) Window #1939], [Suggestion: Enable default theme based on elevation status #8311])

PR Checklist

• Specs: Megathread: sudo, runas, mixed elevation of tabs, etc. #1032, [Question] Configuring Windows Terminal profile to always launch elevated #632
• References: Scenario: Windows Terminal 2.0 Process Model Improvements #5000, Add support for wt.exe to run commands in an existing Terminal Window #4472, Feature Request: Option to only allow one instance #2227, Spec for Windows Terminal Process Model 2.0 #7240, Spec for Windows Terminal Window Management #8135, Suggestion: Enable default theme based on elevation status #8311
• I work here

Detailed Description of the Pull Request / Additional comments

*^*_* read the spec _*^**

Why are these two separate documents?

I felt that the spec that is currently in review in #7240 and this doc should remain separate, yet closely related documents. #7240 is more about showing how this large set of problems discussed in #5000 can all be solved technically, and how those solutions can be used together. It establishes that none of the proposed solutions for components of #5000 will preclude the possibility of other components being solved. What it does not do however is drill too deeply on the user experience that will be built on top of those architectural changes.

This doc on the other hand focuses more closely on a pair of scenarios, and establishes how those scenarios will work technically, and how they'll be exposed to the user.

TODO:

• Discuss how a cross-elevation splitPane should work
• Add a warning dialog before auto-elevating a profile. Otherwise someone might inject malicious.exe into your auto-elevate profile, and have you run it without knowing it was changed.
• Get someone from the security team's eyes on this
• There's Allow for custom profiles when running as admin vs user #3637 tracking "different profiles if we're elevated or unelevated". I'm not sure if the current solution will work for this.
  • I should add a section on "hiddenWhenElevated"/"hiddenWhenUnelevated" to future considerations for this

[skyline75489]

Should the roadmap be updated accordingly? I mean in the “open tab as admin” section in
https://github.com/microsoft/terminal/blob/main/doc/terminal-v2-roadmap.md#20-scenarios

[zadjii-msft]

Should the roadmap be updated accordingly?

meh, probably. Could use some editorializing to reflect the current state of the #5000 plans. Thanks for bringing that up!

[zadjii-msft]

Note to reviewers: I'm probably going to be changing this from elevated: bool? to elevate: bool, where false does nothing and true will elevate if it's not already elevated. TURNS OUT that centenial doesn't play super well with de-elevating a process.

[miniksa]

Especially given our roadmap has us replacing the default story with the Terminal. And we don't want to be vulnerable by default. Individually choosing to be vulnerable... that's different.

[skyline75489]

Should we maybe display the actual command before auto-elevate the profile? Malicious software could possibly modify the elevated profiles and perform some kind of injection. Users will not notice the difference if the profile is auto-elevated, and naturally say "yes" to some malwares unconsciously.

[zadjii-msft]

I believe UAC will do that already, though, it's in the "show details". I suppose we'll only be showing the profile... I guess that means someone could edit the settings so the profile was "commandline": "malicious.exe", and there's nothing preventing them from doing that. That's not good. Yea there should probably be another internal warning.

[miniksa]

Okay, I'm good with this. Thanks for the additional evaluation of the previous batch of comments, especially the future considerations once we get a bit further so we can refer back.

[carlos-zamora]

Just wanna iron out a few more thoughts.

Did we ever have a spec meeting on this? Do you want to have one? It's been a while haha

[carlos-zamora]

By making it a part of themes, that means it'll act like a global setting right? We won't have a scenario where you can have a different icon on each tab?

[zadjii-msft]

Correct. This icon's only in the top-left of the entire window, not the tab.

[carlos-zamora]

It'd be cool if you could upload your own icon too (similar to profile icons). That should be pretty straightforward.

[zadjii-msft]

I mean, yea, sure, we could do that if someone really wanted that

[DHowett]

I'd swing hard the other way -- why let them choose mono or color? Just "on or off", with us controlling the visual treatment. for now

[carlos-zamora]

Could/Should we add something like wt --admin new-tab ...?

Note --admin is off of wt not new-tab. It would be annoying to have to set the elevate member on NewTerminalArgs for each command.

[carlos-zamora]

This might also solve the split-pane issue below? I feel like it's more straightforward to thing of the entire wt.exe commandline as "do all of this in an admin window" vs "do some of these commands in an admin window"

[zadjii-msft]

That's maybe a good tangent to the split-pane discussion. With that discussion, I'm more concerned about someone trying to splitPane with a keybinding in an unelevated window, a Profile that's elevate=true. That's where it's a little weird.

wt --admin [args...] would be interpreted as basically "if I'm not elevated, launch an elevated wt [args...]". wt -w X --admin [args...] would then run [args...] in the elevated X window

[carlos-zamora]

Can we have an issue to track the discussion on "elevated" vs "unfocused" appearance configs?

I'm wondering if we could just do the following:

	Focused	Unfocused
Regular	regular settings	unfocusedAppearance
Elevated	elevatedAppearance	elevatedAppearance

Reasoning: "elevated" is a long-term state whereas focus constantly shifts

idk, either way, a thread on this might be nice to have.

[zadjii-msft]

I mean, we might as well just use #1939 for that, yea?

[carlos-zamora]

Crazy idea: admin-settings.json?

[zadjii-msft]

Honestly I'm just thinking newTabDropdown and adminNewTabDropdown, if you really want them to be different

[Rosefield]

I also think that having completely separate settings would prevent security issues in the future. It may not be the best UX, but the safest thing would probably be to only have a OpenNewAdminWindow that just does runas wt.exe, and then within the admin window they can interact with the terminal however they'd like with the admin settings. I think that marshaling any amount of data into an admin context could cause problems. This is assuming that the actual wt.exe is only modifiable by the administrator.

consider if the attacker tries to run something like (edit: On reflection, the fact that you can just run a command as administrator is something that probably can't be prevented)

wt.exe "cmd.exe {1000 spaces/newlines to hide the rest of the string} && evil.exe"

Separate settings does have benefits too

• admin specific startup actions
• admin only profiles for free (including styling)

[carlos-zamora]

I'm good with this. Thanks!

[zadjii-msft]

Did we ever have a spec meeting on this? Do you want to have one? It's been a while haha

Lol yea, I'm pretty sure we actually did have one one this. IIRC it was during the other process model one? In like, March 😝 The new stuff we discussed Monday, so I figured I'd document and #shipit

[DHowett]

Fix these and then GO OG OG

[github-actions]

@check-spelling-bot Report

Unrecognized words, please review:

• incrememntally

Previously acknowledged words that are now absent

SPACEBAR Unregister xIcon yIcon

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/migrie/s/1032-elevation-qol branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/1b6e6bd6dd03b72e2918a076eea8f701a71455ab.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/905733004" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spelling/allow/names.txt.
• ... APIs, you can add them to a file in .github/actions/spelling/allow/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).