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

Support the "file" URI scheme #7526

Merged
merged 8 commits into from
Feb 19, 2021
Merged

Support the "file" URI scheme #7526

merged 8 commits into from
Feb 19, 2021

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Sep 3, 2020

We now support the file URI schemes where the hostname is either
"localhost" or the empty string

References #5001

Fixes #7699

doc

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in comment

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 4, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 4, 2020
DHowett
DHowett previously approved these changes Sep 4, 2020
src/cascadia/TerminalApp/TerminalPage.h Outdated Show resolved Hide resolved
@j4james
Copy link
Collaborator

j4james commented Sep 4, 2020

Surely this has all the same problems as OSC 7 (#3158)? Realistically it's most likely going to be used in a WSL shell, so the file path will be in the WSL filesystem, and thus won't be understood by ShellExecute. Until we have a way to appropriately convert file paths based on the active shell, this just doesn't seem practical.

Also, using ShellExecute on a file url seems risky from a security point of view. If the file path points to an executable then that executable is going to be run. I'm not sure how much damage could be done that way, but I wouldn't feel comfortable knowing an attacker could run any executable on my system just by tricking me into clicking somewhere.

@DHowett DHowett dismissed their stale review September 4, 2020 22:08
  • pending discussion -
@WSLUser
Copy link
Contributor

WSLUser commented Sep 5, 2020

From security point of view, I expect Windows Security or third party vendors for A/V, FW, HIDS/HIPS to handle the protection. Additionally there's a ransomware setting that basically denies all executables from doing anything to protected parts of the filesystem. Its not completely up to WT to handle the security here. People want this functionality and should understand the security implications. If there's a more secure way for files to get opened including executables then great. Just as long as it's understood we're inheriting most of the security from Windows.

@Lo0oG
Copy link

Lo0oG commented Sep 6, 2020

#7420 should land before this, so that people can see what they are going to be clicking on.

@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Sep 11, 2020

The Uri class has a property called Extension() which in the case of the file scheme, we can use to get the file extension.

So perhaps one way of solving this would be by having a dialog that pops up when a user clicks a link with the .exe extension - the dialog would inform them that the link is an executable (the URI will be provided too) and give them the option of opening the link anyway or canceling the operation. We could also have a setting to prevent such dialogs from appearing (in which case the link will just open).

Thoughts?

EDIT: Nevermind, there are too many types of executables to account for

@WSLUser
Copy link
Contributor

WSLUser commented Sep 11, 2020

I agree with this. Dialogs by default with a setting to disable the dialog and simply run the executable.

@WSLUser
Copy link
Contributor

WSLUser commented Sep 11, 2020

technically we also have to consider scripts as well. Malicious programs written in go, python, ruby, etc can still run if the programming language is installed

@DHowett
Copy link
Member

DHowett commented Sep 11, 2020

No.

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Sep 22, 2020
@DHowett
Copy link
Member

DHowett commented Jan 21, 2021

@j4james Alright, I finally get to come back with a bit of an answer to this.

Also, using ShellExecute on a file url seems risky from a security point of view.

We chatted with the defender/application security folks about this. At the end of the day, ShellExecute ends up using the same reputation services as the rest of Windows. Defender/ATM/"Sense" gets a chance to intervene if the executable is unsigned or untrustworthy.

Looking at it all-up:

  • this doesn't let a would-be attacker specify command-line arguments (ala "cmd.exe /s /c do_a_bad_thing") (using somebody else's reputation to cause mayhem)
  • Links require affirmative (Ctrl+click) action to activate, and I will staunchly refuse to add support for opening one without a modifier key
  • For links that the user has vetted:
    • A local application that can overwrite a linked region with another link is running at-or-above user permission and can just do_the_bad_thing() (or the reputation service kicks in)
    • A remote application that can overwrite a linked region either...
      1. can manipulate a UNC path and replace an executable (network permission required; reputation service kicks in)
      2. can make a user launch an otherwise-acquired local executable (reputation service kicks in)
  • ShellExecute de-elevates because it bounces a launch request off the shell

I'm less concerned about the security aspect of this feature.

Now- for WSL paths. "Works predictably for 15% of applications" (h/t to PhMajerus' AXSH, and other on-Windows requestors) is better in so many ways than "Works for 0% of applications", in my estimation. Incremental progress 😄 while we work on features that'll make it even more broadly applicable.

I'm inclined to call in favor of merging this.

{
return true;
}
// TODO: by the OSC 8 spec, if a hostname (other than localhost) is provided, we _should_ be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be filing a follow-up for this? I'd imagine that this would involve returning a different URL that the one emitted. That would let the client emit file://localhost/foo/bar, and then let us ShellExecute file://foo/bar.

@j4james
Copy link
Collaborator

j4james commented Jan 21, 2021

We chatted with the defender/application security folks about this.

I'm still not completely comfortable with this myself, but if your security folk are satisfied that it's not going to be a problem, then that is at least some reassurance. For those of us that are more paranoid, it might be nice to have an option to disable some or all of the hyperlink functionality, but that could always be a follow-up PR.

@skyline75489
Copy link
Collaborator

skyline75489 commented Jan 25, 2021

Do we have a separate issue for the auto-hyperlinking of plain file paths, for example D:\, just like we did for http & https links in #7691 ? This one is admittedly harder but also quite useful.

I observed an interesting behavior in iTerm2 (I just really used iTerm2 a lot back in the days as iOS developer): it actually detects if the file path is legal before hyperlinking it. For example, if the path is /Library/Apple, you can ctrl+click the path to open it in Finder. But the if the path is /Library/Appl, there's no hyperlink at all. I haven't tested how ssh works, though.

@skyline75489
Copy link
Collaborator

Inspired by #8166 (comment), we can also make the result of ls clickable if the CWD is provided. This is also possible in iTerm2. I'm not sure which sequence or technology it uses to detect the CWD, though. Anyway, with OSC9;9 recently added, this is also possible in WT.

Comment on lines +1807 to 1810
if (_IsUriSupported(parsed))
{
ShellExecute(nullptr, L"open", eventArgs.Uri().c_str(), nullptr, nullptr, SW_SHOWNORMAL);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to use the "edit" verb here for file URLs? Perhaps having two separate conditions, like IsUriSupportedForOpen, and IsUriSupportedForEdit. Because if a user is clicking on a link to a .py file, I think it's more likely they would want the file edited rather than executed (which is what the open verb tends to do).

I don't know if there are any downsides to using the edit verb though. I did bring this up in the #7562 discussion thread, but I don't think anyone commented on the merits of the idea one way or the other.

Copy link
Member

@DHowett DHowett Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in love with the idea of using the edit verb. I just gave it a test on a couple of the files I had laying around:

extension open behavior edit behavior
ps1 notepad powershell ISE (?!)
txt notepad notepad
cpp visual studio preview, which is not my default VS (argh) notepad
no extension "how do you want to open this?" dialog an error message from ShellExecute
.gitconfig, .wslconifg, etc same as above same as above

It's great if somebody's registered for it, but I'm bothered that the OS doesn't give it the same "do what with?" treatment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extensions that concerned me were things like .py, .cmd, .bat, and .vbs. In all cases, the open verb executed the script, and the edit verb opened the file in notepad. Those all seem like files that would not be good to execute when you click on them.

If we're worried about the cases where a file extension doesn't have a registered handler, maybe we can handle that as a special case check. But wanting to click on a link to a file that you've never registered a handler for, seems a far less likely use case than clicking on something like a .py file, and expecting to be able to edit it.

Although maybe I'm the odd one out here. I just assumed nobody would want scripts to execute when clicking on their links.

@github-actions
Copy link

Misspellings found, please review:

  • hostnames
To accept these changes, run the following commands from this repository on this branch
pushd $(git rev-parse --show-toplevel)
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('"aspnet boostorg COINIT dahall fde fea fmtlib isocpp mintty NVDA pinam QOL remoting unte vcrt what3words xamarin "');
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/c07553cb57e1b5c709ff90a4e0ada7052d04d9a3.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"coinit hostnames Remoting "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd
✏️ 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/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
  • ... 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 and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/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).

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Feb 19, 2021
@ghost ghost requested review from carlos-zamora and leonMSFT February 19, 2021 12:54
@DHowett
Copy link
Member

DHowett commented Feb 19, 2021

I'm going to merge this with no backport to get Preview into the hands of more folks who can help us evaluate whether this is a risk and if we should back it out or restrict it further. I do, though, hear & value James' concerns about autoexecuting a script on click(!)

@DHowett DHowett merged commit dfeb855 into main Feb 19, 2021
@DHowett DHowett deleted the dev/pabhoj/file_scheme branch February 19, 2021 22:32
@ghost
Copy link

ghost commented Mar 1, 2021

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

carlos-zamora pushed a commit that referenced this pull request Mar 17, 2023
Does two things related to URLs emitted via OSC8. 

* Allows `wsl$` and `wsl.localhost` as the hostname in `file://` URIs
* Generally allows _all_ URIs that parse as a URI. 

The relevant security comments: #7526 (comment)
> this doesn't let a would-be attacker specify command-line arguments (ala "cmd.exe /s /c do_a_bad_thing") (using somebody else's reputation to cause mayhem)
> 
> `ShellExecute` de-elevates because it bounces a launch request off the shell
> 
> "Works predictably for 15% of applications" (h/t to PhMajerus' AXSH, and other on-Windows requestors) is better in so many ways than "Works for 0% of applications", in my estimation. Incremental progress 😄 while we work on features that'll make it even more broadly applicable.

Closes #10188
Closes #7562
DHowett pushed a commit that referenced this pull request Mar 31, 2023
Does two things related to URLs emitted via OSC8.

* Allows `wsl$` and `wsl.localhost` as the hostname in `file://` URIs
* Generally allows _all_ URIs that parse as a URI.

The relevant security comments: #7526 (comment)
> this doesn't let a would-be attacker specify command-line arguments (ala "cmd.exe /s /c do_a_bad_thing") (using somebody else's reputation to cause mayhem)
>
> `ShellExecute` de-elevates because it bounces a launch request off the shell
>
> "Works predictably for 15% of applications" (h/t to PhMajerus' AXSH, and other on-Windows requestors) is better in so many ways than "Works for 0% of applications", in my estimation. Incremental progress 😄 while we work on features that'll make it even more broadly applicable.

Closes #10188
Closes #7562

(cherry picked from commit 65640f6)
Service-Card-Id: 88719284
Service-Version: 1.17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hyperlinks: Please allow "file" schema
9 participants