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

Implement vscode-file:// support and drop node.js for window loading #107437

Closed
wants to merge 20 commits into from

Conversation

bpasero
Copy link
Member

@bpasero bpasero commented Sep 25, 2020

Continuation of #101837

Fixes #98682

Open issues:

@bpasero
Copy link
Member Author

bpasero commented Sep 25, 2020

After a bit more investigation who is depending on file protocol, I found this place to follow up:

  • markdownRenderer.ts: @mjbvz are you the owner? I see a few Schemas.file references and wonder if we need to look into it. We seem to be allowing file paths in markdown but I am not sure who is crafting these. Some clients seem to be custom hover, notebooks, comments.

if (options.baseUrl && hrefAsUri.scheme === Schemas.file) { // absolute or relative local path, or file: uri

const allowedSchemes = [Schemas.http, Schemas.https, Schemas.mailto, Schemas.data, Schemas.file, Schemas.vscodeRemote, Schemas.vscodeRemoteResource];

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 25, 2020

@bpasero The allowed schemes ones is probably fine. That would be used for cases where markdown from an extension contains a link such as [link](file:///path/to/file.png), which our opener service can handle.

I believe that markdown content can load images using the file: protocol (although I will check that because our content security policy may block it)

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 25, 2020

Yes it looks like our markdown content does allow loading images using the file: protocol:

Screen Shot 2020-09-25 at 4 42 53 PM

This is problematic because it allows reading images at any path, while the vscode-file scheme only allows reading from specific directories.

A first step may be to see how often file images are actually being used in markdown content. My guess it that it would not be very common, but maybe there are some extensions that rely on this functionality

@bpasero
Copy link
Member Author

bpasero commented Sep 27, 2020

@mjbvz ok, this will no longer work once we enable to block file requests. I gave it a quick try with putting vscode-file URI instead but that was also not working, so I wonder where we actually convert that into HTML and if possibly that scheme is not allowed?

@bpasero bpasero modified the milestones: September 2020, October 2020 Sep 27, 2020
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 29, 2020

@bpasero Yes we limit the allowed scheme in a few places:

@bpasero bpasero modified the milestones: October 2020, November 2020 Oct 6, 2020
@bpasero
Copy link
Member Author

bpasero commented Nov 25, 2020

I just noticed that on Windows, when you cd into a UNC path via PowerShell and you try to run code or code-insiders, Windows will leave the UNC path to a non-UNC path because *.cmd do not support UNC paths. So we maybe OK here to proceed even if UNC paths fail. The only situation where VSCode would fail to start from UNC path is if someone was to start our exe directly.

@bpasero
Copy link
Member Author

bpasero commented Jan 6, 2021

This PR is replaced by 169a0ec which gives us a way to enable vscode-file protocol in selfhost via:

  • either setting export ENABLE_VSCODE_BROWSER_CODE_LOADING=bypassHeatCheck
  • or argv.json entry: "enable-browser-code-loading": "bypassHeatCheck"

Other supported values are: 'none' | 'code' | 'bypassHeatCheck' | 'bypassHeatCheckAndEagerCompile'

I have enabled bypassHeatCheck for process explorer and issue reporter who were already using sandbox and vscode-file.

Turning this on by default is still blocked by electron/electron#27075

@bpasero bpasero closed this Jan 6, 2021
@bpasero bpasero deleted the vscode-file branch January 6, 2021 14:47
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sandbox Running VSCode in a node-free environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on node require for startup code path
3 participants