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

Breaking change in 1.25 in how external webpages are opened #51358

Closed
avanderhoorn opened this issue Jun 7, 2018 · 9 comments
Closed

Breaking change in 1.25 in how external webpages are opened #51358

avanderhoorn opened this issue Jun 7, 2018 · 9 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-editors Managing of editor widgets in workbench window
Milestone

Comments

@avanderhoorn
Copy link
Member

  • VSCode Version: 1.25
  • OS Version: All

Steps to Reproduce:

  1. Attempt to us vscode.commands.executeCommand('vscode.open', …) as per guidance in issues like Open an Http Url from extension #9651.
  2. Below exception occurs

consoleinfo

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

With the release of 1.25, people have started running into this exception - https://github.com/Microsoft/vscode/blob/fea449cc7616ce039e2c7e5845fb60136f84405e/src/vs/workbench/services/editor/browser/editorService.ts#L243.

I believe this is caused when we attempt to open an external browser window for auth. Currently we use vscode.commands.executeCommand('vscode.open', …) as per the guidance in issues like this - #9651.

I understand from the issue that we should be using IOpenerService, but I’m wondering if you can confirm that this is a conscious breaking change and not a regression for this user case (as I can’t find any docs on this).

// @bpasero @jrieken

@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug workbench-editors Managing of editor widgets in workbench window labels Jun 7, 2018
@jrieken
Copy link
Member

jrieken commented Jun 7, 2018

This is due to https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/services/editor/browser/editorService.ts#L238 and looks like it's done on purpose... I'll push a workaround and let @bpasero do the rest.

@eamodio
Copy link
Contributor

eamodio commented Jun 7, 2018

@jrieken is this something extension will need to worry about? Or is this purely an internal thing?

@bpasero
Copy link
Member

bpasero commented Jun 8, 2018

@jrieken your change makes sense to me, the idea was to stay away from having logic to open windows from http/https URLs in a service that is for opening editors.

I think I would even go one step further and just always use the IOpenerService instead of checking for the schema and using the IEditorService for some cases that can be handled. The IOpenerService is already leveraging the editor service for opening stuff in the editor that is neither http, https or a command. For that the only thing that seems to be missing is a way to forward the ViewColumn where to open.

@bpasero bpasero added this to the June 2018 milestone Jun 8, 2018
@jrieken
Copy link
Member

jrieken commented Jun 8, 2018

Yeah, I would have gone for the opener service but I wasn't sure about all those editor options. I looks like the open-command supports a wide interface, but I might be wrong

@bpasero
Copy link
Member

bpasero commented Jun 8, 2018

@jrieken I can add those options to the opener service. One sort of breaking change however is that previously we did not support command:// resources from the vscode.open command (because the editor service never behaved like that) and now if we go through the opener service, they would be supported. I think that should be fine though.

@jrieken
Copy link
Member

jrieken commented Jun 8, 2018

That rings a bell - there are security considerations regarding command links... I'd block them for now.

@bpasero
Copy link
Member

bpasero commented Jun 8, 2018

Ok, we can preserve the current behaviour and just have a setting in the opener service to not execute command links.

@jrieken
Copy link
Member

jrieken commented Jun 8, 2018

setting in the opener service to not execute command links.

Just to clarify that you mean an argument, not a property because it is per invocation/call. E.g trusted marked down can run commands, untrusted not

@bpasero
Copy link
Member

bpasero commented Jun 8, 2018

@jrieken yeah I would make this exclusive to the vscode.open command as an argument of the open method to preserve the exact behaviour we have today.

@bpasero bpasero closed this as completed Jun 11, 2018
bpasero added a commit that referenced this issue Jun 11, 2018
@mjbvz mjbvz added the verified Verification succeeded label Jun 27, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-editors Managing of editor widgets in workbench window
Projects
None yet
Development

No branches or pull requests

5 participants