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.

Sign up for GitHub

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

Jump to bottom

Opener Service: trusted domains, standalone editor, openExternal #79487

Closed
bpasero opened this issue Aug 20, 2019 · 3 comments
Closed

Opener Service: trusted domains, standalone editor, openExternal #79487

bpasero opened this issue Aug 20, 2019 · 3 comments
Assignees
@octref
Labels
debt Code quality issues
Milestone
August 2019

Comments

@bpasero
Copy link
Member

bpasero commented Aug 20, 2019 •
edited

Adding @alexandrudima for standalone editor.

@octref it looks like you added support for showing a dialog to the user whenever we open a URL externally into IOpenerService. I think this makes sense for the case of the workbench, but the way it works now, it will also trigger a dialog in the standalone editor (I am not even sure we have support for dialogs there). To make matters worse, we store the result into the storage service, which as far as I know is just implemented in-memory in the standalone editor case. So anytime a link opens, the user would get asked again after a reload of the page.

Also, I see some layering issues in the code: You seem to reference workbench.action.configureTrustedDomains from vs/editor which is not good.

Here is my suggestion:

  • remove this logic from the opener service in vs/editor/browser/services
  • move this logic, including the code that contributes the workbench.action.configureTrustedDomains into a single workbench contribution (url.contribution.ts for example)
  • leverage the IOpenerService.registerOpener() method from the workbench layer to participate in the opening and do the dialog + storage logic from there

On top of that: I recently added IOpenerService#openExternal() which is the code that gets triggered, e.g. when an extension calls vscode.env.openExternal(). I wonder: should we show the same dialog on that case for trusted domains? I would think so, but keep in mind that one of the URLs may have the vscode:// scheme which we should always trust.
@bpasero bpasero added the debt Code quality issues label Aug 20, 2019
@bpasero bpasero added this to the August 2019 milestone Aug 20, 2019
@alexdima
Copy link
Member

@octref Here is how you can run the standalone editor from source -- https://github.com/microsoft/monaco-editor/blob/master/CONTRIBUTING.md#running-the-editor-from-source

@bpasero
Copy link
Member Author

bpasero commented Aug 20, 2019 •
edited

@octref after working on #79453 today I think here is a path forward:

  • introduce a similar concept as we have today where you can register Opener to participate in the opening but call it Validator or similar that can signal with a boolean value if the opening should continue
  • contribute this validator for opening from your existing url.contribution.ts
  • move all the code that deals with picker, storage etc into there
  • revert the existing opener service to how it worked before

I would think it is fine to have these validators run before the opening handlers have a chance to get access. Your validator will only take care of URIs with schemes that we open external (mailto, http, https). This means that an extension that opens a link (via vscode.env.openExternal) will also benefit from your logic.

@octref
Copy link
Contributor

octref commented Aug 21, 2019

@bpasero Good suggestion, I went with this interface. PR at #79538.
I didn't realize I broke standalone editor (it'd error out with no product service). After the PR standalone editor is not covered by the link protection mechanism.

export interface IValidator {
	shouldOpen(resource: URI): Promise<boolean>;
}

export interface IOpenerService {
	/**
	 * Register a participant that can validate if the URI resource should be opened.
	 * validators are run before openers.
	 */
	registerValidator(uriScheme: string, validator: IValidator): IDisposable;
}

@octref octref closed this as completed in 62461ec Aug 21, 2019
octref added a commit that referenced this issue Aug 21, 2019
@octref
Merge pull request #79538 from microsoft/pine/openerservice
9be3eb5 
Refactor OpenerService. Fix #79487
@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@octref octref

Labels
debt Code quality issues
Projects
None yet
Milestone
August 2019
Development

No branches or pull requests
3 participants
@bpasero @octref @alexdima