From 62b3a6fd9f00ce182d12a357de076008f5838fd0 Mon Sep 17 00:00:00 2001 From: Teffen Date: Wed, 1 Dec 2021 19:21:52 -0500 Subject: [PATCH] Proxy path fixes (#4548) * Fix issue where HTTP error status codes are not read. * Fix issues surrounding sessions when accessed from a proxy. - Updated vscode args to match latest upstream. - Fixed issues surrounding trailing slashes affecting base paths. - Updated cookie names to better match upstream's usage, debuggability. * Bump vendor. * Update tests. * Fix issue where tests lack cookie key. Co-authored-by: Asher --- src/common/http.ts | 6 +++++- src/node/http.ts | 4 ++-- src/node/main.ts | 9 ++++++--- src/node/routes/login.ts | 11 ++++------- src/node/routes/logout.ts | 14 +++++++++----- src/node/routes/vscode.ts | 9 ++++++--- src/node/util.ts | 2 +- test/unit/common/http.test.ts | 2 +- test/utils/globalSetup.ts | 3 ++- vendor/package.json | 2 +- vendor/yarn.lock | 4 ++-- 11 files changed, 39 insertions(+), 27 deletions(-) diff --git a/src/common/http.ts b/src/common/http.ts index c08c8673b..5709c455c 100644 --- a/src/common/http.ts +++ b/src/common/http.ts @@ -13,8 +13,12 @@ export enum HttpCode { * used in the HTTP response. */ export class HttpError extends Error { - public constructor(message: string, public readonly status: HttpCode, public readonly details?: object) { + public constructor(message: string, public readonly statusCode: HttpCode, public readonly details?: object) { super(message) this.name = this.constructor.name } } + +export enum CookieKeys { + Session = "code-server-session", +} diff --git a/src/node/http.ts b/src/node/http.ts index 461aefc0d..09f472ba9 100644 --- a/src/node/http.ts +++ b/src/node/http.ts @@ -6,7 +6,7 @@ import * as net from "net" import path from "path" import qs from "qs" import { Disposable } from "../common/emitter" -import { HttpCode, HttpError } from "../common/http" +import { CookieKeys, HttpCode, HttpError } from "../common/http" import { normalize } from "../common/util" import { AuthType, DefaultedArgs } from "./cli" import { version as codeServerVersion } from "./constants" @@ -93,7 +93,7 @@ export const authenticated = async (req: express.Request): Promise => { const passwordMethod = getPasswordMethod(hashedPasswordFromArgs) const isCookieValidArgs: IsCookieValidArgs = { passwordMethod, - cookieKey: sanitizeString(req.cookies.key), + cookieKey: sanitizeString(req.cookies[CookieKeys.Session]), passwordFromArgs: req.args.password || "", hashedPasswordFromArgs: req.args["hashed-password"], } diff --git a/src/node/main.ts b/src/node/main.ts index 32dd48a29..1bda3b74b 100644 --- a/src/node/main.ts +++ b/src/node/main.ts @@ -1,6 +1,6 @@ import { field, logger } from "@coder/logger" -import * as os from "os" import http from "http" +import * as os from "os" import path from "path" import { Disposable } from "../common/emitter" import { plural } from "../common/util" @@ -37,8 +37,11 @@ export const runVsCodeCli = async (args: DefaultedArgs): Promise => { try { await spawnCli({ ...args, - // For some reason VS Code takes the port as a string. - port: typeof args.port !== "undefined" ? args.port.toString() : undefined, + /** Type casting. */ + "accept-server-license-terms": true, + help: !!args.help, + version: !!args.version, + port: args.port?.toString(), }) } catch (error: any) { logger.error("Got error from VS Code", error) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index c0b5fde88..dc3b59480 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -3,14 +3,11 @@ import { promises as fs } from "fs" import { RateLimiter as Limiter } from "limiter" import * as os from "os" import * as path from "path" +import { CookieKeys } from "../../common/http" import { rootPath } from "../constants" import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http" import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString, escapeHtml } from "../util" -export enum Cookie { - Key = "key", -} - // RateLimiter wraps around the limiter library for logins. // It allows 2 logins every minute plus 12 logins every hour. export class RateLimiter { @@ -62,7 +59,7 @@ router.get("/", async (req, res) => { res.send(await getRoot(req)) }) -router.post("/", async (req, res) => { +router.post<{}, string, { password: string; base?: string }, { to?: string }>("/", async (req, res) => { const password = sanitizeString(req.body.password) const hashedPasswordFromArgs = req.args["hashed-password"] @@ -87,13 +84,13 @@ router.post("/", async (req, res) => { if (isPasswordValid) { // The hash does not add any actual security but we do it for // obfuscation purposes (and as a side effect it handles escaping). - res.cookie(Cookie.Key, hashedPassword, { + res.cookie(CookieKeys.Session, hashedPassword, { domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]), // Browsers do not appear to allow cookies to be set relatively so we // need to get the root path from the browser since the proxy rewrites // it out of the path. Otherwise code-server instances hosted on // separate sub-paths will clobber each other. - path: req.body.base ? path.posix.join(req.body.base, "..") : "/", + path: req.body.base ? path.posix.join(req.body.base, "..", "/") : "/", sameSite: "lax", }) diff --git a/src/node/routes/logout.ts b/src/node/routes/logout.ts index d1a19dfef..5c8311266 100644 --- a/src/node/routes/logout.ts +++ b/src/node/routes/logout.ts @@ -1,17 +1,21 @@ import { Router } from "express" +import { CookieKeys } from "../../common/http" import { getCookieDomain, redirect } from "../http" -import { Cookie } from "./login" + +import { sanitizeString } from "../util" export const router = Router() -router.get("/", async (req, res) => { +router.get<{}, undefined, undefined, { base?: string; to?: string }>("/", async (req, res) => { + const path = sanitizeString(req.query.base) || "/" + const to = sanitizeString(req.query.to) || "/" + // Must use the *identical* properties used to set the cookie. - res.clearCookie(Cookie.Key, { + res.clearCookie(CookieKeys.Session, { domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]), - path: req.query.base || "/", + path: decodeURIComponent(path), sameSite: "lax", }) - const to = (typeof req.query.to === "string" && req.query.to) || "/" return redirect(req, res, to, { to: undefined, base: undefined }) }) diff --git a/src/node/routes/vscode.ts b/src/node/routes/vscode.ts index 06272ba86..c9f497655 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, }) @@ -88,9 +88,12 @@ export class CodeServerRouteWrapper { try { this._codeServerMain = await createVSServer(null, { - connectionToken: "0000", + "connection-token": "0000", + "accept-server-license-terms": true, ...args, - // For some reason VS Code takes the port as a string. + /** Type casting. */ + help: !!args.help, + version: !!args.version, port: args.port?.toString(), }) } catch (createServerError) { diff --git a/src/node/util.ts b/src/node/util.ts index a55ae9a6d..d12ba6f0c 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -321,7 +321,7 @@ export async function isCookieValid({ * - greater than 0 characters * - trims whitespace */ -export function sanitizeString(str: string): string { +export function sanitizeString(str: unknown): string { // Very basic sanitization of string // Credit: https://stackoverflow.com/a/46719000/3015595 return typeof str === "string" && str.trim().length > 0 ? str.trim() : "" diff --git a/test/unit/common/http.test.ts b/test/unit/common/http.test.ts index fd49ae183..ba4981377 100644 --- a/test/unit/common/http.test.ts +++ b/test/unit/common/http.test.ts @@ -19,7 +19,7 @@ describe("http", () => { const httpError = new HttpError(message, HttpCode.BadRequest) expect(httpError.message).toBe(message) - expect(httpError.status).toBe(400) + expect(httpError.statusCode).toBe(400) expect(httpError.details).toBeUndefined() }) it("should have details if provided", () => { diff --git a/test/utils/globalSetup.ts b/test/utils/globalSetup.ts index eace7f9b2..7b19d8829 100644 --- a/test/utils/globalSetup.ts +++ b/test/utils/globalSetup.ts @@ -1,4 +1,5 @@ import { Cookie } from "playwright" +import { CookieKeys } from "../../src/common/http" import { hash } from "../../src/node/util" import { PASSWORD, workspaceDir } from "./constants" import { clean } from "./helpers" @@ -27,7 +28,7 @@ export default async function () { domain: "localhost", expires: -1, httpOnly: false, - name: "key", + name: CookieKeys.Session, path: "/", sameSite: "Lax", secure: false, diff --git a/vendor/package.json b/vendor/package.json index 21a862451..40fe2a2a7 100644 --- a/vendor/package.json +++ b/vendor/package.json @@ -7,6 +7,6 @@ "postinstall": "./postinstall.sh" }, "devDependencies": { - "code-oss-dev": "cdr/vscode#a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6" + "code-oss-dev": "cdr/vscode#5e0c6f3b95ed032e62c49101ae502a46c62ef202" } } diff --git a/vendor/yarn.lock b/vendor/yarn.lock index d4886aba5..2f6522058 100644 --- a/vendor/yarn.lock +++ b/vendor/yarn.lock @@ -296,9 +296,9 @@ clone-response@^1.0.2: dependencies: mimic-response "^1.0.0" -code-oss-dev@cdr/vscode#a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6: +code-oss-dev@cdr/vscode#5e0c6f3b95ed032e62c49101ae502a46c62ef202: version "1.61.1" - resolved "https://codeload.github.com/cdr/vscode/tar.gz/a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6" + resolved "https://codeload.github.com/cdr/vscode/tar.gz/5e0c6f3b95ed032e62c49101ae502a46c62ef202" dependencies: "@microsoft/applicationinsights-web" "^2.6.4" "@vscode/sqlite3" "4.0.12"