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

Fix #48403 broken UNC paths in markdown images #74332

Merged
merged 1 commit into from Jul 3, 2019

Conversation

@lmcarreiro
Copy link
Contributor

commented May 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

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 mjbvz added this to the June 2019 milestone May 29, 2019

@mjbvz

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

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 mjbvz modified the milestones: June 2019, July 2019 Jul 1, 2019

@mjbvz mjbvz merged commit abe0c06 into microsoft:master Jul 3, 2019

5 checks passed

VS Code Build #20190623.6 succeeded
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
VS Code (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
@mjbvz

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.