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

Only OSC 8 terminal hyperlinks with http/https scheme seem be to working #176812

Closed
jaminthorns opened this issue Mar 10, 2023 · 18 comments · Fixed by #209376
Closed

Only OSC 8 terminal hyperlinks with http/https scheme seem be to working #176812

jaminthorns opened this issue Mar 10, 2023 · 18 comments · Fixed by #209376
Assignees
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes terminal-links verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@jaminthorns
Copy link

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.76.1
  • OS Version: macOS 13.2.1 (22D68)

Steps to Reproduce

  1. Print an OSC 8 hyperlink that uses a schema other than http or https. For example, the vscode URL scheme:
printf '\e]8;;vscode://file//Users/jaminthornsberry/.gitconfig:10\e\\Take me to git config!\e]8;;\e\n'
  1. The text renders as a hyperlink (dotted underline), but isn't cmd/ctrl+clickable like other like OSC 8 hyperlinks with the http/https scheme. Other OSC 8-capable terminals (like WezTerm) will open the link correctly.

Demonstration

Here's a video demonstrating the issue with several other URL schemes:

Screen.Recording.2023-03-10.at.11.28.03.AM.mov

As you can see, only http and https URL schemes result in clickable links.

Comments

If I remember correctly, this regressed in the 1.74 -> 1.75 update, which leads me to believe it might be related the link improvements introduced in that version.

This functionality can be super useful for tools like Delta, which let you add hyperlinks to line numbers in diffs. When this functionality worked in VS Code 1.74, those links would be clickable:

Screenshot 2023-03-10 at 11 47 14 AM

@Tyriar Tyriar added feature-request Request for new features or functionality terminal-links labels Aug 3, 2023
@Tyriar Tyriar added this to the Backlog milestone Aug 3, 2023
@flying-sheep
Copy link

Since everything supported this, I relied on it, damn.

Would be amazing to have this back. Only non-heuristic way to get clickable file links in the terminal that handle all paths (e.g. paths with spaces)

@Tyriar
Copy link
Member

Tyriar commented Aug 31, 2023

We currently do not allow non-http(s) links due to allowNonHttpProtocols here:

this._xterm.options.linkHandler = {
activate: (_, text) => {
this._openers.get(TerminalBuiltinLinkType.Url)?.open({
type: TerminalBuiltinLinkType.Url,
text,
bufferRange: null!,
uri: URI.parse(text)
});
},
hover: (e, text, range) => {
activeHoverDisposable?.dispose();
activeHoverDisposable = undefined;
activeTooltipScheduler?.dispose();
activeTooltipScheduler = new RunOnceScheduler(() => {
const core = (this._xterm as any)._core as IXtermCore;
const cellDimensions = {
width: core._renderService.dimensions.css.cell.width,
height: core._renderService.dimensions.css.cell.height
};
const terminalDimensions = {
width: this._xterm.cols,
height: this._xterm.rows
};
activeHoverDisposable = this._showHover({
viewportRange: convertBufferRangeToViewport(range, this._xterm.buffer.active.viewportY),
cellDimensions,
terminalDimensions
}, this._getLinkHoverString(text, text), undefined, (text) => this._xterm.options.linkHandler?.activate(e, text, range));
// Clear out scheduler until next hover event
activeTooltipScheduler?.dispose();
activeTooltipScheduler = undefined;
}, this._configurationService.getValue('workbench.hover.delay'));
activeTooltipScheduler.schedule();
}
};

https://github.com/Tyriar/xterm.js/blob/48184cc94f8d16f9dc240a30edae85973ecd7c92/src/browser/OscLinkProvider.ts#L70-L81

This may just be a case of adding that property to linkHandler, but we need to make sure it won't introduce a security issue.

@Tyriar Tyriar modified the milestones: Backlog, September 2023 Aug 31, 2023
@flying-sheep
Copy link

flying-sheep commented Sep 1, 2023

Hm, running code in a terminal generally means one trusts it. After all at this point it has access to a lot of things. Displaying data from untrusted sources without stripping ANSI escapes is more a problem of the tool displaying the data.

I think when a terminal link is hovered, VS Code should just show a little link target floaty thing that shows where the link points to. Other terminal emulators do that.

If you’re being paranoid, show a little ⚠ when a link label looks like an URI but the corresponding link points to another URI. But that’s a situation people have in the web all the time, so they should know it can happen.

@Tyriar Tyriar added the help wanted Issues identified as good community contribution opportunities label Sep 7, 2023
@Tyriar Tyriar modified the milestones: September 2023, Backlog Sep 7, 2023
@Tyriar
Copy link
Member

Tyriar commented Sep 11, 2023

@flying-sheep sure, but you can cat some file and it will evaluate sequences which is a common attack vector. The user is typically unaware that cat could result in execution and/or malicious links being inserted. The hover is probably sufficient, I would want to double check with someone on the team who has done some general URI handling when looking at the PR.

@jaminthorns
Copy link
Author

@Tyriar I understand the reasoning for disabling this, as VS Code focuses pretty strongly on security given its popularity (workspace trust, confirmations when opening links, well-defined extension API), but would it be possible to allow non-HTTP hyperlinks in the terminal via a setting? Something like terminal.integrated.allowNonHttpHyperlinks = true?

It seems like implementing this would be a simple pass-through to xterm.js's allowNonHttpProtocols property on ILinkHandler (which you previously linked to). The setting could even have a security warning in its description. It'd be really nice to have this functionality back.

@flying-sheep
Copy link

Who needs to sign off on a decision here? This is affecting me pretty much daily (since my testing framework uses this and I simply can’t make VS Code behave)

@gaborcsardi
Copy link

If would be nice to support at least file:// links, so that one could open files within VS Code, by clicking on a link in a terminal.

@michprev
Copy link

michprev commented Feb 4, 2024

+1, our framework uses OSC 8 links as one of the exclusive features vs competitors. Missing support in VS Code makes it irrelevant and degrades our productivity.

@Tyriar
Copy link
Member

Tyriar commented Apr 2, 2024

Easy testing below, only the first 2 work:

printf '\e]8;;http://github.com\e\\http scheme\e]8;;\e\n'
printf '\e]8;;https://github.com\e\\https scheme\e]8;;\e\n'
printf '\e]8;;file:///Users/tyriar/dev/microsoft/vscode\e\\file scheme\e]8;;\e\n'
printf '\e]8;;vscode://file/Users/tyriar/dev/microsoft/vscode:10\e\\vscode scheme (with line number)\e]8;;\e\n'
printf '\e]8;;mailto:foo@bar.com\e\\mailto scheme\e]8;;\e\n'

Note that we wouldn't be able to support any arbitrary link without additional work as there are security concerns.

@Tyriar Tyriar modified the milestones: Backlog, April 2024 Apr 2, 2024
Tyriar added a commit that referenced this issue Apr 2, 2024
@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Apr 3, 2024
@VSCodeTriageBot VSCodeTriageBot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Apr 4, 2024
@Tyriar Tyriar added the verification-needed Verification of issue is requested label Apr 22, 2024
@andreamah
Copy link
Contributor

andreamah commented Apr 24, 2024

Easy testing below, only the first 2 work:

printf '\e]8;;http://github.com\e\\http scheme\e]8;;\e\n'
printf '\e]8;;https://github.com\e\\https scheme\e]8;;\e\n'
printf '\e]8;;file:///Users/tyriar/dev/microsoft/vscode\e\\file scheme\e]8;;\e\n'
printf '\e]8;;vscode://file/Users/tyriar/dev/microsoft/vscode:10\e\\vscode scheme (with line number)\e]8;;\e\n'
printf '\e]8;;mailto:foo@bar.com\e\\mailto scheme\e]8;;\e\n'

Note that we wouldn't be able to support any arbitrary link without additional work as there are security concerns.

Which terminal type should we test this in to verify the issue? @Tyriar

@Tyriar
Copy link
Member

Tyriar commented Apr 24, 2024

@andreamah bash should work

@rzhao271 rzhao271 added the verified Verification succeeded label Apr 24, 2024
@jackzzs
Copy link

jackzzs commented Apr 25, 2024

Is it possible to open "file:///Users/tyriar/dev/microsoft/vscode" link in vscode by default? In WSL, this link will do nothing.

@Tyriar
Copy link
Member

Tyriar commented Apr 25, 2024

@jackzzs yeah the file needs to exist for it to work

@Tyriar Tyriar added the on-release-notes Issue/pull request mentioned in release notes label Apr 25, 2024
@jackzzs
Copy link

jackzzs commented Apr 25, 2024

In Windows it works, but doesn't seem to work in WSL2.

image

This may be due to the fact that accessing the file link calls the system's open command, and wsl2 does not handle this kind of command.

Considering that the link is opened in vscode, is it a better way to open it in the current vscode editor instead of calling the system's open command to open another vscode window? And this will also fix the WSL problem.

@Tyriar
Copy link
Member

Tyriar commented Apr 25, 2024

@jackzzs are you connected to the window via the Remote - WSL extension? If not this is as expected currently.

@jackzzs
Copy link

jackzzs commented Apr 25, 2024

Yes,

  1. If I connect to the window using WSL remote extension, clicking the file:// url link will do nothing despite the file is existing. This is not so convenient.

  2. If I connect to the window without remote extensions, clicking the file:// url link will open the file using system open call, resulting in a new vscode window. This is also not so convenient, especially for some rich logging libraries (that use file:// rather than vscode://file/ url for source file location)

@Tyriar
Copy link
Member

Tyriar commented Apr 26, 2024

@jackzzs created #211443 to track your 1. For 2 right now proper WSL link support only works within Remote - WSL windows, so making this work well in local windows would be a new feature request.

@jackzzs
Copy link

jackzzs commented Apr 27, 2024

Thank you very much for your patient inspection and answer, looking forward to the fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes terminal-links verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants