Fix relative paths (#4594)

* Add tests for relativeRoot

* 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.

* 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.

* Do not remove out directory before watch

This is re-used for incremental compilation.

Also remove del since that was the only use (and we can use fs.rmdir in
the future if we need something like this).

* Remove unused function resolveBase

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 @@
         <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 @@
     <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/")),
+    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 => {
+export const relativeRoot = (originalUrl: string): string => {
-  const depth = (req.originalUrl.split("?", 1)[0].match(/\//g) || []).length
+  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,32 @@ 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.
+  const url = new URL(
+    req.query.base || req.body.base || "/",
+    req.query.href || req.body.href || "http://" + (req.headers.host || "localhost"),
+  )
+  return {
+    domain: getCookieDomain(url.host, req.args["proxy-domain"]),
+    path: normalize(url.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, {
+      res.cookie(CookieKeys.Session, hashedPassword, getCookieOptions(req))
-        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",
-      })
 
       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, {
+  res.clearCookie(CookieKeys.Session, getCookieOptions(req))
-    domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
-    path: decodeURIComponent(path),
-    sameSite: "lax",
-  })
 
-  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"

yarn.lock

@@ -615,14 +615,6 @@ agent-base@6, agent-base@^6.0.0, agent-base@^6.0.2:
   dependencies:
     debug "4"
 
-aggregate-error@^3.0.0:
-  version "3.1.0"
-  resolved "https://registry.yarnpkg.com/aggregate-error/-/aggregate-error-3.1.0.tgz#92670ff50f5359bdb7a3e0d40d0ec30c5737687a"
-  integrity sha512-4I7Td01quW/RpocfNayFdFVk1qSuoh0E7JrbRJ16nH01HhKFQ88INq9Sd+nd72zqRySlr9BmDA8xlEJ6vJMrYA==
-  dependencies:
-    clean-stack "^2.0.0"
-    indent-string "^4.0.0"
-
 ajv@^6.10.0, ajv@^6.12.4:
   version "6.12.6"
   resolved "https://registry.yarnpkg.com/ajv/-/ajv-6.12.6.tgz#baf5a62e802b07d977034586f8c3baf5adf26df4"
@@ -977,11 +969,6 @@ chownr@^2.0.0:
   resolved "https://registry.yarnpkg.com/chownr/-/chownr-2.0.0.tgz#15bfbe53d2eab4cf70f18a8cd68ebe5b3cb1dece"
   integrity sha512-bIomtDF5KGpdogkLd9VspvFzk9KfpyyGlS8YFVZl7TGPBHL5snIOnxeshwVgPteQ9b4Eydl+pVbIyE1DcvCWgQ==
 
-clean-stack@^2.0.0:
-  version "2.2.0"
-  resolved "https://registry.yarnpkg.com/clean-stack/-/clean-stack-2.2.0.tgz#ee8472dbb129e727b31e8a10a427dee9dfe4008b"
-  integrity sha512-4diC9HaTE+KRAMWhDhrGOECgWZxoevMc5TlkObMqNSsVU62PYzXZ/SMTjzyGAFF1YusgxGcSWTEXBhp0CPwQ1A==
-
 cliui@^7.0.2:
   version "7.0.4"
   resolved "https://registry.yarnpkg.com/cliui/-/cliui-7.0.4.tgz#a0265ee655476fc807aea9df3df8df7783808b4f"
@@ -1235,20 +1222,6 @@ degenerator@^3.0.1:
     esprima "^4.0.0"
     vm2 "^3.9.3"
 
-del@^6.0.0:
-  version "6.0.0"
-  resolved "https://registry.yarnpkg.com/del/-/del-6.0.0.tgz#0b40d0332cea743f1614f818be4feb717714c952"
-  integrity sha512-1shh9DQ23L16oXSZKB2JxpL7iMy2E0S9d517ptA1P8iw0alkPtQcrKH7ru31rYtKwF499HkTu+DRzq3TCKDFRQ==
-  dependencies:
-    globby "^11.0.1"
-    graceful-fs "^4.2.4"
-    is-glob "^4.0.1"
-    is-path-cwd "^2.2.0"
-    is-path-inside "^3.0.2"
-    p-map "^4.0.0"
-    rimraf "^3.0.2"
-    slash "^3.0.0"
-
 delegates@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/delegates/-/delegates-1.0.0.tgz#84c6e159b81904fdca59a0ef44cd870d31250f9a"
@@ -2082,10 +2055,10 @@ globals@^13.6.0, globals@^13.9.0:
   dependencies:
     type-fest "^0.20.2"
 
-globby@^11.0.1, globby@^11.0.4:
+globby@^11.0.3:
-  version "11.0.4"
+  version "11.0.3"
-  resolved "https://registry.yarnpkg.com/globby/-/globby-11.0.4.tgz#2cbaff77c2f2a62e71e9b2813a67b97a3a3001a5"
+  resolved "https://registry.yarnpkg.com/globby/-/globby-11.0.3.tgz#9b1f0cb523e171dd1ad8c7b2a9fb4b644b9593cb"
-  integrity sha512-9O4MVG9ioZJ08ffbcyVYyLOJLk5JQ688pJ4eMGLpdWLHq/Wr1D9BlriLQyL0E+jbkuePVZXYFj47QM/v093wHg==
+  integrity sha512-ffdmosjA807y7+lA1NM0jELARVmYul/715xiILEjo3hBLPTcirgQNnXECn5g3mtR8TOLCVbkfua1Hpen25/Xcg==
   dependencies:
     array-union "^2.1.0"
     dir-glob "^3.0.1"
@@ -2094,10 +2067,10 @@ globby@^11.0.1, globby@^11.0.4:
     merge2 "^1.3.0"
     slash "^3.0.0"
 
-globby@^11.0.3:
+globby@^11.0.4:
-  version "11.0.3"
+  version "11.0.4"
-  resolved "https://registry.yarnpkg.com/globby/-/globby-11.0.3.tgz#9b1f0cb523e171dd1ad8c7b2a9fb4b644b9593cb"
+  resolved "https://registry.yarnpkg.com/globby/-/globby-11.0.4.tgz#2cbaff77c2f2a62e71e9b2813a67b97a3a3001a5"
-  integrity sha512-ffdmosjA807y7+lA1NM0jELARVmYul/715xiILEjo3hBLPTcirgQNnXECn5g3mtR8TOLCVbkfua1Hpen25/Xcg==
+  integrity sha512-9O4MVG9ioZJ08ffbcyVYyLOJLk5JQ688pJ4eMGLpdWLHq/Wr1D9BlriLQyL0E+jbkuePVZXYFj47QM/v093wHg==
   dependencies:
     array-union "^2.1.0"
     dir-glob "^3.0.1"
@@ -2123,7 +2096,7 @@ graceful-fs@^4.1.2:
   resolved "https://registry.yarnpkg.com/graceful-fs/-/graceful-fs-4.2.6.tgz#ff040b2b0853b23c3d31027523706f1885d76bee"
   integrity sha512-nTnJ528pbqxYanhpDYsi4Rd8MAeaBA67+RZ10CM1m3bTAVFEDcd5AuA4a6W5YkGZ1iNXHzZz8T6TBKLeBuNriQ==
 
-graceful-fs@^4.1.6, graceful-fs@^4.2.0, graceful-fs@^4.2.4:
+graceful-fs@^4.1.6, graceful-fs@^4.2.0:
   version "4.2.8"
   resolved "https://registry.yarnpkg.com/graceful-fs/-/graceful-fs-4.2.8.tgz#e412b8d33f5e006593cbd3cee6df9f2cebbe802a"
   integrity sha512-qkIilPUYcNhJpd33n0GBXTB1MMPp14TxEsEs0pTrsSVucApsYzW5V+Q8Qxhik6KU3evy+qkAAowTByymK0avdg==
@@ -2471,16 +2444,6 @@ is-number@^7.0.0:
   resolved "https://registry.yarnpkg.com/is-number/-/is-number-7.0.0.tgz#7535345b896734d5f80c4d06c50955527a14f12b"
   integrity sha512-41Cifkg6e8TylSpdtTpeLVMqvSBEVzTttHvERD741+pnZ8ANv0004MRL43QKPDlK9cGvNp6NZWZUBlbGXYxxng==
 
-is-path-cwd@^2.2.0:
-  version "2.2.0"
-  resolved "https://registry.yarnpkg.com/is-path-cwd/-/is-path-cwd-2.2.0.tgz#67d43b82664a7b5191fd9119127eb300048a9fdb"
-  integrity sha512-w942bTcih8fdJPJmQHFzkS76NEP8Kzzvmw92cXsazb8intwLqPibPPdXf4ANdKV3rYMuuQYGIWtvz9JilB3NFQ==
-
-is-path-inside@^3.0.2:
-  version "3.0.3"
-  resolved "https://registry.yarnpkg.com/is-path-inside/-/is-path-inside-3.0.3.tgz#d231362e53a07ff2b0e0ea7fed049161ffd16283"
-  integrity sha512-Fd4gABb+ycGAmKou8eMftCupSir5lRxqf4aD/vd0cD2qc4HL07OjCeuHMr8Ro4CoMaeCKDB0/ECBOVWjTwUvPQ==
-
 is-plain-obj@^1.1.0:
   version "1.1.0"
   resolved "https://registry.yarnpkg.com/is-plain-obj/-/is-plain-obj-1.1.0.tgz#71a50c8429dfca773c92a390a4a03b39fcd51d3e"
@@ -3186,13 +3149,6 @@ p-locate@^4.1.0:
   dependencies:
     p-limit "^2.2.0"
 
-p-map@^4.0.0:
-  version "4.0.0"
-  resolved "https://registry.yarnpkg.com/p-map/-/p-map-4.0.0.tgz#bb2f95a5eda2ec168ec9274e06a747c3e2904d2b"
-  integrity sha512-/bjOqmgETBYB5BoEeGVea8dmvHb2m9GLy1E9W43yeyfP6QQCZGFNa+XRceJEuDB6zqr+gKpIAmlLebMpykw/MQ==
-  dependencies:
-    aggregate-error "^3.0.0"
-
 p-try@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/p-try/-/p-try-1.0.0.tgz#cbc79cdbaf8fd4228e13f621f2b1a237c1b207b3"