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

Allow wsl$ in file URIs; generally allow all URI schemes #14993

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

zadjii-msft
Copy link
Member

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

discuss

@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Mar 14, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Mar 14, 2023
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.

I'm cool with this.

@DHowett DHowett changed the title Allow WSL$ in file:// URIs, and generally allow all URI schemes Allow wsl$ in file URIs; generally allow all URI schemes Mar 16, 2023
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Mar 17, 2023
@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Mar 17, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Mar 17, 2023
@carlos-zamora
Copy link
Member

carlos-zamora commented Mar 17, 2023

@msftbot merge this in 10 minutes

EDIT: or ignore me, that works too, I guess

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 17, 2023
@carlos-zamora carlos-zamora merged commit 65640f6 into main Mar 17, 2023
@carlos-zamora carlos-zamora deleted the dev/migrie/b/allow-all-the-uris branch March 17, 2023 18:29
@Araxeus
Copy link

Araxeus commented Mar 17, 2023

This is great, so many possibilities !

I'm just a random guy wondering if the first if is necessary in this function

if (parsedUri.SchemeName() == L"http" || parsedUri.SchemeName() == L"https")
{
return true;
}

the function would return true anyways right?

@heaths
Copy link
Member

heaths commented Mar 17, 2023

@Araxeus necessary, no, but IMO it's better to maintain. You're still explicitly allowing HTTP/S but that fallback may change in the future. Will future maintainers remember to explicitly handle HTTP/S again? A lot would be broken if not. Also allows more understandable comments, IMO, and is neither a hot path nor all that slow anyway so a refactor doesn't seem worth the potential maintenance issues.

Araxeus added a commit to Araxeus/ls-interactive that referenced this pull request Mar 29, 2023
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-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file:// links with WSL$ are not supported [DISCUSSION] Allow more URI schemes
5 participants