Skip to content

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

Merged
carlos-zamora merged 1 commit intomainfrom
dev/migrie/b/allow-all-the-uris
Mar 17, 2023
Merged

Allow wsl$ in file URIs; generally allow all URI schemes#14993
carlos-zamora merged 1 commit intomainfrom
dev/migrie/b/allow-all-the-uris

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 14, 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

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-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Mar 14, 2023
DHowett
DHowett approved these changes Mar 16, 2023
View reviewed changes
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 •
edited
Loading

@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

terminal/src/cascadia/TerminalApp/TerminalPage.cpp

Lines 2463 to 2466 in be62c22

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
@Araxeus
[Windows] fix My Computer link in dirview
3315cca 
depends on microsoft/terminal#14993
@Araxeus Araxeus mentioned this pull request Mar 29, 2023
Merged
DHowett pushed a commit that referenced this pull request Mar 31, 2023
@zadjii-msft @DHowett
Allow wsl$ in file URIs; generally allow all URI schemes (#14993)
31aa23f 
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
This was referenced Apr 2, 2023
Closed
Closed
@zadjii-msft zadjii-msft mentioned this pull request May 5, 2023
Closed
@j4james j4james mentioned this pull request May 7, 2023
Open
@inoyakaigor inoyakaigor mentioned this pull request Jul 25, 2024
Closed
@davidanthoff davidanthoff mentioned this pull request Aug 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

@DHowett DHowett Awaiting requested review from DHowett

@lhecker lhecker Awaiting requested review from lhecker

Assignees

No one assigned

Labels

Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-UserInterface 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@zadjii-msft @carlos-zamora @Araxeus @heaths @DHowett