Fix relative paths

ci/dev/watch.ts

@@ -1,5 +1,4 @@
 import { spawn, fork, ChildProcess } from "child_process"
-import del from "del"
 import { promises as fs } from "fs"
 import * as path from "path"
 import { CompilationStats, onLine, OnLineCallback } from "../../src/node/util"
@@ -57,8 +56,6 @@ class Watcher {
       process.on(event, () => this.dispose(0))
     }
 
-    this.cleanFiles()
-
     for (const [processName, devProcess] of Object.entries(this.compilers)) {
       if (!devProcess) continue
 
@@ -121,15 +118,6 @@ class Watcher {
 
   //#region Utilities
 
-  /**
-   * Cleans files from previous builds.
-   */
-  private cleanFiles(): Promise<string[]> {
-    console.log("[Watcher]", "Cleaning files from previous builds...")
-
-    return del(["out/**/*"])
-  }
-
   /**
    * Emits a file containing compilation data.
    * This is especially useful when Express needs to determine if VS Code is still compiling.

package.json

@@ -52,7 +52,6 @@
     "@typescript-eslint/parser": "^5.0.0",
     "audit-ci": "^5.0.0",
     "codecov": "^3.8.3",
-    "del": "^6.0.0",
     "doctoc": "^2.0.0",
     "eslint": "^7.7.0",
     "eslint-config-prettier": "^8.1.0",

src/browser/pages/login.html

@@ -30,7 +30,8 @@ <h1 class="main">Welcome to code-server</h1>
         <div class="content">
           <form class="login-form" method="post">
             <input class="user" type="text" autocomplete="username" />
-            <input id="base" type="hidden" name="base" value="/" />
+            <input id="base" type="hidden" name="base" value="{{BASE}}" />
+            <input id="href" type="hidden" name="href" value="" />
             <div class="field">
               <input
                 required
@@ -51,9 +52,9 @@ <h1 class="main">Welcome to code-server</h1>
     <script>
       // Inform the backend about the path since the proxy might have rewritten
       // it out of the headers and cookies must be set with absolute paths.
-      const el = document.getElementById("base")
+      const el = document.getElementById("href")
       if (el) {
-        el.value = window.location.pathname
+        el.value = location.href
       }
     </script>
   </body>

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 ? "/" : "")
@@ -35,21 +41,6 @@ export const trimSlashes = (url: string): string => {
   return url.replace(/^\/+|\/+$/g, "")
 }
 
-/**
- * Resolve a relative base against the window location. This is used for
- * anything that doesn't work with a relative path.
- */
-export const resolveBase = (base?: string): string => {
-  // After resolving the base will either start with / or be an empty string.
-  if (!base || base.startsWith("/")) {
-    return base ?? ""
-  }
-  const parts = location.pathname.split("/")
-  parts[parts.length - 1] = base
-  const url = new URL(location.origin + "/" + parts.join("/"))
-  return normalize(url.pathname)
-}
-
 /**
  * Wrap the value in an array if it's not already an array. If the value is
  * undefined return an empty array.

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
 }
 
@@ -33,11 +34,11 @@ declare global {
 }
 
 export const createClientConfiguration = (req: express.Request): ClientConfiguration => {
-  const base = relativeRoot(req)
+  const base = relativeRoot(req.originalUrl)
 
   return {
-    base,
-    csStaticBase: normalize(path.posix.join(base, "_static/")),
+    base: base,
+    csStaticBase: base + "/_static",
     codeServerVersion,
   }
 }
@@ -108,15 +109,28 @@ export const authenticated = async (req: express.Request): Promise<boolean> => {
 
 /**
  * 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 +152,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}`)
@@ -241,3 +255,36 @@ export function disposer(server: http.Server): Disposable["dispose"] {
     })
   }
 }
+
+/**
+ * Get the options for setting a cookie.  The options must be identical for
+ * setting and unsetting cookies otherwise they are considered separate.
+ */
+export const getCookieOptions = (req: express.Request): express.CookieOptions => {
+  // Normally we set paths relatively.  However browsers do not appear to allow
+  // cookies to be set relatively which means we need an absolute path.  We
+  // cannot be guaranteed we know the path since a reverse proxy might have
+  // rewritten it.  That means we need to get the path from the frontend.
+
+  // The reason we need to set the path (as opposed to defaulting to /) is to
+  // avoid code-server instances on different sub-paths clobbering each other or
+  // from accessing each other's tokens (and to prevent other services from
+  // accessing code-server's tokens).
+
+  // When logging in or out the request must include the href (the full current
+  // URL of that page) and the relative path to the root as given to it by the
+  // backend.  Using these two we can determine the true absolute root.
+  let domain = req.headers.host || ""
+  let pathname = "/"
+  const href = req.query.href || req.body.href
+  if (href) {
+    const url = new URL(req.query.base || req.body.base || "/", href)
+    domain = url.host
+    pathname = url.pathname
+  }
+  return {
+    domain: getCookieDomain(domain, req.args["proxy-domain"]),
+    path: normalize(pathname) || "/",
+    sameSite: "lax",
+  }
+}

src/node/routes/login.ts

@@ -5,7 +5,7 @@ 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 { authenticated, getCookieOptions, redirect, replaceTemplates } from "../http"
 import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString, escapeHtml } from "../util"
 
 // RateLimiter wraps around the limiter library for logins.
@@ -84,15 +84,7 @@ router.post<{}, string, { password: string; base?: string }, { to?: string }>("/
     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(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, "..", "/") : "/",
-        sameSite: "lax",
-      })
+      res.cookie(CookieKeys.Session, hashedPassword, getCookieOptions(req))
 
       const to = (typeof req.query.to === "string" && req.query.to) || "/"
       return redirect(req, res, to, { to: undefined })

src/node/routes/logout.ts

@@ -1,21 +1,14 @@
 import { Router } from "express"
 import { CookieKeys } from "../../common/http"
-import { getCookieDomain, redirect } from "../http"
-
+import { getCookieOptions, redirect } from "../http"
 import { sanitizeString } from "../util"
 
 export const router = Router()
 
 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(CookieKeys.Session, {
-    domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
-    path: decodeURIComponent(path),
-    sameSite: "lax",
-  })
+  res.clearCookie(CookieKeys.Session, getCookieOptions(req))
 
-  return redirect(req, res, to, { to: undefined, base: undefined })
+  const to = sanitizeString(req.query.to) || "/"
+  return redirect(req, res, to, { to: undefined, base: undefined, href: 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,
       })

src/node/util.ts

@@ -324,7 +324,7 @@ export async function isCookieValid({
 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() : ""
+  return typeof str === "string" ? str.trim() : ""
 }
 
 const mimeTypes: { [key: string]: string } = {

test/unit/common/util.test.ts

@@ -74,42 +74,6 @@ describe("util", () => {
     })
   })
 
-  describe("resolveBase", () => {
-    beforeEach(() => {
-      const location: LocationLike = {
-        pathname: "/healthz",
-        origin: "http://localhost:8080",
-      }
-
-      // Because resolveBase is not a pure function
-      // and relies on the global location to be set
-      // we set it before all the tests
-      // and tell TS that our location should be looked at
-      // as Location (even though it's missing some properties)
-      global.location = location as Location
-    })
-
-    it("should resolve a base", () => {
-      expect(util.resolveBase("localhost:8080")).toBe("/localhost:8080")
-    })
-
-    it("should resolve a base with a forward slash at the beginning", () => {
-      expect(util.resolveBase("/localhost:8080")).toBe("/localhost:8080")
-    })
-
-    it("should resolve a base with query params", () => {
-      expect(util.resolveBase("localhost:8080?folder=hello-world")).toBe("/localhost:8080")
-    })
-
-    it("should resolve a base with a path", () => {
-      expect(util.resolveBase("localhost:8080/hello/world")).toBe("/localhost:8080/hello/world")
-    })
-
-    it("should resolve a base to an empty string when not provided", () => {
-      expect(util.resolveBase()).toBe("")
-    })
-  })
-
   describe("arrayify", () => {
     it("should return value it's already an array", () => {
       expect(util.arrayify(["hello", "world"])).toStrictEqual(["hello", "world"])

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("./../..")
+  })
+})

vendor/package.json

@@ -7,6 +7,6 @@
     "postinstall": "./postinstall.sh"
   },
   "devDependencies": {
-    "code-oss-dev": "cdr/vscode#c2a251c6afaa13fbebf97fcd8a68192f8cf46031"
+    "code-oss-dev": "cdr/vscode#478224aa345e9541f2427b30142dd13ee7e14d39"
   }
 }

vendor/yarn.lock

@@ -296,9 +296,9 @@ clone-response@^1.0.2:
   dependencies:
     mimic-response "^1.0.0"
 
-code-oss-dev@cdr/vscode#c2a251c6afaa13fbebf97fcd8a68192f8cf46031:
+code-oss-dev@cdr/vscode#478224aa345e9541f2427b30142dd13ee7e14d39:
   version "1.61.1"
-  resolved "https://codeload.github.com/cdr/vscode/tar.gz/c2a251c6afaa13fbebf97fcd8a68192f8cf46031"
+  resolved "https://codeload.github.com/cdr/vscode/tar.gz/478224aa345e9541f2427b30142dd13ee7e14d39"
   dependencies:
     "@microsoft/applicationinsights-web" "^2.6.4"
     "@vscode/sqlite3" "4.0.12"