Fix #48403 broken UNC paths in markdown images

[lmcarreiro]

Fix #48403.

It's my first PR in this repo, I don't know if it could be fixed in a better way, if so, please tell me.

The problem is:

contents.session.protocol.registerBufferProtocol(protocol, (request, callback: any) => {
	const requestPath = URI.parse(request.url).path;
	//request.url = "vscode-resource://desktop-9su47m5/test-vscode-issue/350x150.png"
	//requestPath = /test-vscode-issue/350x150.png
	const normalizedPath = URI.file(requestPath);
	for (const root of getRoots()) {
		if (!startsWith(normalizedPath.fsPath, root.fsPath + sep)) {
			continue;
		}
		// ...

normalizedPath.fsPath won't have the //desktop-9su47m5 part of the path, because normalizedPath variable is constructed with URI.file("/test-vscode-issue/350x150.png"), so the condition !startsWith(normalizedPath.fsPath, root.fsPath + sep) will never be false with UNC paths.

[lmcarreiro]

I just added another commit. The previous one was just handling UNC paths.

With project in the root of a drive, this code:

if (!startsWith(normalizedPath.fsPath, root.fsPath + sep)) {

When root.fsPath is a folder, it doesn't have a trailing slash, like c:\folder\subfolder, but when the project root is at the root of a drive, root.fsPath is c:\ with a trailing slash.

So, as it always concatenates the sep separator, the condition was this:

!startsWith('c:\image.png', 'c:\' + '\')

Resulting in a c:\\ second parameter passed to startsWith.

[mjbvz]

Thanks for looking into this. Marking for June since it is too late to merge for the May release.

@lmcarreiro Can you please quickly overview how you tested that this change works and that it does not cause regressions?

[lmcarreiro]

@mjbvz I know that it works because I created a shared folder in my PC with the example of a simple .md file with a local image and I opened it both from the UNC path and from a mapped network drive (of the same UNC path) with the .md file and the image on the root of this drive. Doing these tests that I was able to identify this other bug (#74362) that happens in the same scenario.

I ran the automated tests with and without my changes and the results were the same.

About the possibility of regressions, I don't think that would be any, because of the kind of these changes:

• The first change, it will just concatenate a //<authority> if there is an authority on the URI parsed request.url. I took a look at the URI.parse code and regex, and I saw that it just happens when we pass a URI that starts with something:// and a non-slash character after the two slashes. The only two cases I can think are when we are dealing with UNC paths or absolute URLs.
  • The case of a UNC path was the bug itself, that I already tested.
  • The case of a URL, I tested and saw that this code never executes, because it doesn't start with vscode-resource:.
• The second change, the change in the actual behavior is only in the case when it was concatenating a slash at the end of a path that already ends with a slash, clearly not the intention of who wrote the code and probably expected a path that would never have a trailing slash.

Anyway, I don't know the architecture of this project. The only resource I found about this was the Code-Organization that doesn't help too much. If there is anything that could be made in a better way, or if it would be better to be fixed in other place of the code, just give me directions and I'll try to do it.

[mjbvz]

Hey can you please take a quick look at resolving the merge conflicts for this PR. The webview code has been reworked quite a bit recently to support the web. I intended to merge this PR earlier but it fell off my radar

[lmcarreiro]

I saw that the code I changed was moved into the new file resourceLoader.ts. So, I moved my changes to this file, it worked for paths at the root of a network mapped drive, but it didn't for an UNC path.

It is missing a slash after the vscode-resource:

contents.session.protocol.registerBufferProtocol(protocol, (request, callback: any) => {
// request.uri for UNC paths:
// It used to be like: "vscode-resource://desktop-9su47m5/test-vscode-issue/350x150.png"
// Now it is like:     "vscode-resource:/desktop-9su47m5/test-vscode-issue/350x150.png"

I did a git bisect to find the commit that changed this behavior, and it was the merge of this pull request: #75741

[lmcarreiro]

I added a comment there in the line that introduced the change: https://github.com/microsoft/vscode/pull/75741/files#r296498579

-<base href="${markdownDocument.uri.with({ scheme: 'vscode-resource' }).toString(true)}">
+<base href="${toResoruceUri(markdownDocument.uri)}">

-<!--<base href="vscode-resource://desktop-9su47m5/test-vscode-issue/test.md">-->
+<!--<base href="vscode-resource:/test-vscode-issue/test.md">-->

Anyway, I rewrote my changes in the new file resourceLoader.ts, but to work with UNC paths, you need to review this change. As you used this new method toResoruceUri in other places too, I think it's best that you fix this <base>'s href attribute yourself.

[mjbvz]

Thanks. Will fix the markdown case as part of #76489