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 <[email protected]>

Author: Teffen

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",
+}

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<boolean> => {
       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"],
       }

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<void> => {
   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)

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",
       })
 

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 })
 })

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) {

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() : ""

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", () => {

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,

vendor/package.json

@@ -7,6 +7,6 @@
     "postinstall": "./postinstall.sh"
   },
   "devDependencies": {
-    "code-oss-dev": "cdr/vscode#a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6"
+    "code-oss-dev": "cdr/vscode#5e0c6f3b95ed032e62c49101ae502a46c62ef202"
   }
 }