From a5c35af81b335412522a8c25a75b1c4379d631ef Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 6 Apr 2020 19:06:12 -0500 Subject: [PATCH] Fix encoding issues with folder and workspace params The raw value is now passed back to VS Code so it can do the parsing with its own URI class rather than trying to parse using Node's url module first since that has no guarantee of working the same way. It also lets us keep the vscode-remote bit internal to VS Code. Removed the logic that keeps trying paths until it finds a valid one because it seems confusing to open a path and silently get some other path instead of an error for the one you tried to open. Now it'll just use exactly what you specified or fail trying. Fixes #1488. The problem here was that url.parse was encoding the spaces then the validation failed looking for a literal %20. --- ci/vscode.patch | 56 ++++++++++++++++++++++++++++++++++-------- src/node/app/vscode.ts | 55 +++++++++++------------------------------ 2 files changed, 60 insertions(+), 51 deletions(-) diff --git a/ci/vscode.patch b/ci/vscode.patch index d26b9d4f..96579193 100644 --- a/ci/vscode.patch +++ b/ci/vscode.patch @@ -261,16 +261,32 @@ index 2c64061da7..c0ef8faedd 100644 // Do nothing. If we can't read the file we have no // language pack config. diff --git a/src/vs/code/browser/workbench/workbench.ts b/src/vs/code/browser/workbench/workbench.ts -index 45f6f17ce0..4d1a590a7c 100644 +index 45f6f17ce0..102289c147 100644 --- a/src/vs/code/browser/workbench/workbench.ts +++ b/src/vs/code/browser/workbench/workbench.ts +@@ -16,6 +16,7 @@ import product from 'vs/platform/product/common/product'; + import { Schemas } from 'vs/base/common/network'; + import { posix } from 'vs/base/common/path'; + import { localize } from 'vs/nls'; ++import { encodePath } from 'vs/server/node/util'; + + interface ICredential { + service: string; +@@ -237,7 +238,6 @@ class WorkspaceProvider implements IWorkspaceProvider { + } + + private createTargetUrl(workspace: IWorkspace, options?: { reuse?: boolean, payload?: object }): string | undefined { +- + // Empty + let targetHref: string | undefined = undefined; + if (!workspace) { @@ -246,12 +246,18 @@ class WorkspaceProvider implements IWorkspaceProvider { // Folder else if (isFolderToOpen(workspace)) { - targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_FOLDER}=${encodeURIComponent(workspace.folderUri.toString())}`; + const target = workspace.folderUri.scheme === Schemas.vscodeRemote -+ ? encodeURIComponent(workspace.folderUri.path).replace(/%2F/g, "/") ++ ? encodePath(workspace.folderUri.path) + : encodeURIComponent(workspace.folderUri.toString()); + targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_FOLDER}=${target}`; } @@ -279,7 +295,7 @@ index 45f6f17ce0..4d1a590a7c 100644 else if (isWorkspaceToOpen(workspace)) { - targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_WORKSPACE}=${encodeURIComponent(workspace.workspaceUri.toString())}`; + const target = workspace.workspaceUri.scheme === Schemas.vscodeRemote -+ ? encodeURIComponent(workspace.workspaceUri.path).replace(/%2F/g, "/") ++ ? encodePath(workspace.workspaceUri.path) + : encodeURIComponent(workspace.workspaceUri.toString()); + targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_WORKSPACE}=${target}`; } @@ -290,7 +306,7 @@ index 45f6f17ce0..4d1a590a7c 100644 const config: IWorkbenchConstructionOptions & { folderUri?: UriComponents, workspaceUri?: UriComponents } = JSON.parse(configElementAttribute); + // Strip the protocol from the authority if it exists. -+ const normalizeAuthority = (authority: string): string => authority.replace(/^https?:\/\//, ""); ++ const normalizeAuthority = (authority?: string): string => authority && authority.replace(/^https?:\/\//, ""); + if (config.remoteAuthority) { + (config as any).remoteAuthority = normalizeAuthority(config.remoteAuthority); + } @@ -2346,10 +2362,10 @@ index 0000000000..3c74512192 +} diff --git a/src/vs/server/node/server.ts b/src/vs/server/node/server.ts new file mode 100644 -index 0000000000..d6dcfe1fe7 +index 0000000000..52311bf756 --- /dev/null +++ b/src/vs/server/node/server.ts -@@ -0,0 +1,257 @@ +@@ -0,0 +1,269 @@ +import * as net from 'net'; +import * as path from 'path'; +import { Emitter } from 'vs/base/common/event'; @@ -2429,10 +2445,22 @@ index 0000000000..d6dcfe1fe7 + await this.servicesPromise; + const environment = this.services.get(IEnvironmentService) as IEnvironmentService; + const startPath = options.startPath; ++ const parseUrl = (url: string): URI => { ++ // This might be a fully-specified URL or just a path. ++ try { ++ return URI.parse(url, true); ++ } catch (error) { ++ return URI.from({ ++ scheme: Schemas.vscodeRemote, ++ authority: options.remoteAuthority, ++ path: url, ++ }); ++ } ++ }; + return { + workbenchWebConfiguration: { -+ workspaceUri: startPath && startPath.workspace ? URI.parse(startPath.url) : undefined, -+ folderUri: startPath && !startPath.workspace ? URI.parse(startPath.url) : undefined, ++ workspaceUri: startPath && startPath.workspace ? parseUrl(startPath.url) : undefined, ++ folderUri: startPath && !startPath.workspace ? parseUrl(startPath.url) : undefined, + remoteAuthority: options.remoteAuthority, + logLevel: getLogLevel(environment), + }, @@ -2639,10 +2667,10 @@ index 0000000000..fc69441cf0 +}; diff --git a/src/vs/server/node/util.ts b/src/vs/server/node/util.ts new file mode 100644 -index 0000000000..06b080044c +index 0000000000..dd7fdf7b58 --- /dev/null +++ b/src/vs/server/node/util.ts -@@ -0,0 +1,9 @@ +@@ -0,0 +1,17 @@ +import { getPathFromAmdModule } from 'vs/base/common/amd'; +import { URITransformer, IRawURITransformer } from 'vs/base/common/uriIpc'; + @@ -2652,6 +2680,14 @@ index 0000000000..06b080044c + const rawURITransformer = rawURITransformerFactory(remoteAuthority); + return new URITransformer(rawURITransformer); +}; ++ ++/** ++ * Encode a path for opening via the folder or workspace query parameter. This ++ * preserves slashes so it can be edited by hand more easily. ++ */ ++export const encodePath = (path: string): string => { ++ return path.split("/").map((p) => encodeURIComponent(p)).join("/"); ++}; diff --git a/src/vs/workbench/api/browser/extensionHost.contribution.ts b/src/vs/workbench/api/browser/extensionHost.contribution.ts index e69aa80159..71a899d37b 100644 --- a/src/vs/workbench/api/browser/extensionHost.contribution.ts diff --git a/src/node/app/vscode.ts b/src/node/app/vscode.ts index 5759213c..6c9d10bb 100644 --- a/src/node/app/vscode.ts +++ b/src/node/app/vscode.ts @@ -1,11 +1,9 @@ import { field, logger } from "@coder/logger" import * as cp from "child_process" import * as crypto from "crypto" -import * as fs from "fs-extra" import * as http from "http" import * as net from "net" import * as path from "path" -import * as url from "url" import { CodeServerMessage, Options, @@ -168,15 +166,12 @@ export class VscodeHttpProvider extends HttpProvider { private async getRoot(request: http.IncomingMessage, route: Route): Promise { const remoteAuthority = request.headers.host as string const { lastVisited } = await settings.read() - const startPath = await this.getFirstValidPath( - [ - { url: route.query.workspace, workspace: true }, - { url: route.query.folder, workspace: false }, - this.args._ && this.args._.length > 0 ? { url: path.resolve(this.args._[this.args._.length - 1]) } : undefined, - lastVisited, - ], - remoteAuthority, - ) + const startPath = await this.getFirstPath([ + { url: route.query.workspace, workspace: true }, + { url: route.query.folder, workspace: false }, + this.args._ && this.args._.length > 0 ? { url: path.resolve(this.args._[this.args._.length - 1]) } : undefined, + lastVisited, + ]) const [response, options] = await Promise.all([ await this.getUtf8Resource(this.rootPath, "src/browser/pages/vscode.html"), this.initialize({ @@ -209,41 +204,19 @@ export class VscodeHttpProvider extends HttpProvider { } /** - * Choose the first valid path. If `workspace` is undefined then either a - * workspace or a directory are acceptable. Otherwise it must be a file if a - * workspace or a directory otherwise. + * Choose the first non-empty path. */ - private async getFirstValidPath( + private async getFirstPath( startPaths: Array<{ url?: string | string[]; workspace?: boolean } | undefined>, - remoteAuthority: string, ): Promise { for (let i = 0; i < startPaths.length; ++i) { const startPath = startPaths[i] - if (!startPath) { - continue - } - const paths = typeof startPath.url === "string" ? [startPath.url] : startPath.url || [] - for (let j = 0; j < paths.length; ++j) { - const uri = url.parse(paths[j]) - try { - if (!uri.pathname) { - throw new Error(`${paths[j]} is not a valid URL`) - } - const stat = await fs.stat(uri.pathname) - if (typeof startPath.workspace === "undefined" || startPath.workspace !== stat.isDirectory()) { - return { - url: url.format({ - protocol: uri.protocol || "vscode-remote", - hostname: remoteAuthority.split(":")[0], - port: remoteAuthority.split(":")[1], - pathname: uri.pathname, - slashes: true, - }), - workspace: !stat.isDirectory(), - } - } - } catch (error) { - logger.warn(error.message) + const url = + startPath && (typeof startPath.url === "string" ? [startPath.url] : startPath.url || []).find((p) => !!p) + if (startPath && url) { + return { + url, + workspace: !!startPath.workspace, } } }