From a0e928e43b1b30691695ca70b6d335d042b46be1 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 7 Dec 2021 22:47:51 +0000 Subject: [PATCH 1/7] Add tests for relativeRoot --- src/node/http.ts | 23 ++++++++++++++++++----- test/unit/node/http.test.ts | 11 +++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 test/unit/node/http.test.ts diff --git a/src/node/http.ts b/src/node/http.ts index 1719aaba1ae1..e0fd02d49063 100644 --- a/src/node/http.ts +++ b/src/node/http.ts @@ -33,7 +33,7 @@ declare global { } export const createClientConfiguration = (req: express.Request): ClientConfiguration => { - const base = relativeRoot(req) + const base = relativeRoot(req.originalUrl) return { base, @@ -108,15 +108,28 @@ export const authenticated = async (req: express.Request): Promise => { /** * Get the relative path that will get us to the root of the page. For each - * slash we need to go up a directory. For example: + * slash we need to go up a directory. Will not have a trailing slash. + * + * For example: + * * / => . * /foo => . * /foo/ => ./.. * /foo/bar => ./.. * /foo/bar/ => ./../.. + * + * All paths must be relative in order to work behind a reverse proxy since we + * we do not know the base path. Anything that needs to be absolute (for + * example cookies) must get the base path from the frontend. + * + * All relative paths must be prefixed with the relative root to ensure they + * work no matter the depth at which they happen to appear. + * + * For Express `req.originalUrl` should be used as they remove the base from the + * standard `url` property making it impossible to get the true depth. */ -export const relativeRoot = (req: express.Request): string => { - const depth = (req.originalUrl.split("?", 1)[0].match(/\//g) || []).length +export const relativeRoot = (originalUrl: string): string => { + const depth = (originalUrl.split("?", 1)[0].match(/\//g) || []).length return normalize("./" + (depth > 1 ? "../".repeat(depth - 1) : "")) } @@ -138,7 +151,7 @@ export const redirect = ( } }) - const relativePath = normalize(`${relativeRoot(req)}/${to}`, true) + const relativePath = normalize(`${relativeRoot(req.originalUrl)}/${to}`, true) const queryString = qs.stringify(query) const redirectPath = `${relativePath}${queryString ? `?${queryString}` : ""}` logger.debug(`redirecting from ${req.originalUrl} to ${redirectPath}`) diff --git a/test/unit/node/http.test.ts b/test/unit/node/http.test.ts new file mode 100644 index 000000000000..87e8e04199b1 --- /dev/null +++ b/test/unit/node/http.test.ts @@ -0,0 +1,11 @@ +import { relativeRoot } from "../../../src/node/http" + +describe("http", () => { + it("should construct a relative path to the root", () => { + expect(relativeRoot("/")).toStrictEqual(".") + expect(relativeRoot("/foo")).toStrictEqual(".") + expect(relativeRoot("/foo/")).toStrictEqual("./..") + expect(relativeRoot("/foo/bar ")).toStrictEqual("./..") + expect(relativeRoot("/foo/bar/")).toStrictEqual("./../..") + }) +}) From f70a32f4e721063f981bc2a161e635b320d76361 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 7 Dec 2021 23:18:03 +0000 Subject: [PATCH 2/7] Remove path.posix.join Since this is for file system paths it feels incorrect to use it on URL paths as they are different in many ways. --- src/common/util.ts | 6 ++++++ src/node/http.ts | 7 ++++--- src/node/routes/vscode.ts | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/common/util.ts b/src/common/util.ts index 30fb8387a549..28795d7768c6 100644 --- a/src/common/util.ts +++ b/src/common/util.ts @@ -23,6 +23,12 @@ export const generateUuid = (length = 24): string => { /** * Remove extra slashes in a URL. + * + * This is meant to fill the job of `path.join` so you can concatenate paths and + * then normalize out any extra slashes. + * + * If you are using `path.join` you do not need this but note that `path` is for + * file system paths, not URLs. */ export const normalize = (url: string, keepTrailing = false): string => { return url.replace(/\/\/+/g, "/").replace(/\/+$/, keepTrailing ? "/" : "") diff --git a/src/node/http.ts b/src/node/http.ts index e0fd02d49063..928da621d7a3 100644 --- a/src/node/http.ts +++ b/src/node/http.ts @@ -3,7 +3,6 @@ import * as express from "express" import * as expressCore from "express-serve-static-core" import * as http from "http" import * as net from "net" -import path from "path" import * as qs from "qs" import { Disposable } from "../common/emitter" import { CookieKeys, HttpCode, HttpError } from "../common/http" @@ -18,7 +17,9 @@ import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString, es */ export interface ClientConfiguration { codeServerVersion: string + /** Relative path from this page to the root. No trailing slash. */ base: string + /** Relative path from this page to the static root. No trailing slash. */ csStaticBase: string } @@ -36,8 +37,8 @@ export const createClientConfiguration = (req: express.Request): ClientConfigura const base = relativeRoot(req.originalUrl) return { - base, - csStaticBase: normalize(path.posix.join(base, "_static/")), + base: base, + csStaticBase: base + "/_static", codeServerVersion, } } diff --git a/src/node/routes/vscode.ts b/src/node/routes/vscode.ts index c9f4976554fc..c78543e6e2d8 100644 --- a/src/node/routes/vscode.ts +++ b/src/node/routes/vscode.ts @@ -24,7 +24,7 @@ export class CodeServerRouteWrapper { const isAuthenticated = await authenticated(req) if (!isAuthenticated) { - return redirect(req, res, "login/", { + return redirect(req, res, "login", { // req.baseUrl can be blank if already at the root. to: req.baseUrl && req.baseUrl !== "/" ? req.baseUrl : undefined, }) From 92de2aec39e38190f5800063ba9a8540d709f075 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 7 Dec 2021 23:14:36 +0000 Subject: [PATCH 3/7] Rewrite cookie path logic Before we relied on the client to resolve the base given to it by the backend against the path. Instead have the client pass that information along so we can resolve it on the backend. This means the client has to do less work. --- src/browser/pages/login.html | 7 ++++--- src/node/http.ts | 33 +++++++++++++++++++++++++++++++++ src/node/routes/login.ts | 12 ++---------- src/node/routes/logout.ts | 15 ++++----------- src/node/util.ts | 2 +- vendor/package.json | 2 +- vendor/yarn.lock | 4 ++-- 7 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/browser/pages/login.html b/src/browser/pages/login.html index 75aa86dc2523..6149ecf11cd6 100644 --- a/src/browser/pages/login.html +++ b/src/browser/pages/login.html @@ -30,7 +30,8 @@

Welcome to code-server