Merge pull request #3695 from cdr/jsjoeio-sanitize-error-msg

jsjoeio · web-flow · commit 975dd13d125c · 2021-07-01T14:50:32.000-07:00
fix: escape error.message on login failure
diff --git a/src/node/http.ts b/src/node/http.ts
@@ -7,7 +7,7 @@ import { normalize, Options } from "../common/util"
 import { AuthType, DefaultedArgs } from "./cli"
 import { commit, rootPath } from "./constants"
 import { Heart } from "./heart"
-import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString } from "./util"
+import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString, escapeHtml } from "./util"
 
 declare global {
   // eslint-disable-next-line @typescript-eslint/no-namespace
@@ -35,7 +35,7 @@ export const replaceTemplates = <T extends object>(
     ...extraOpts,
   }
   return content
-    .replace(/{{TO}}/g, (typeof req.query.to === "string" && req.query.to) || "/")
+    .replace(/{{TO}}/g, (typeof req.query.to === "string" && escapeHtml(req.query.to)) || "/")
     .replace(/{{BASE}}/g, options.base)
     .replace(/{{CS_STATIC_BASE}}/g, options.csStaticBase)
     .replace(/"{{OPTIONS}}"/, `'${JSON.stringify(options)}'`)
@@ -100,7 +100,8 @@ export const relativeRoot = (req: express.Request): string => {
 }
 
 /**
- * Redirect relatively to `/${to}`. Query variables will be preserved.
+ * Redirect relatively to `/${to}`. Query variables on the current URI will be preserved.
+ * `to` should be a simple path without any query parameters
  * `override` will merge with the existing query (use `undefined` to unset).
  */
 export const redirect = (
diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts
@@ -4,7 +4,7 @@ import { RateLimiter as Limiter } from "limiter"
 import * as path from "path"
 import { rootPath } from "../constants"
 import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http"
-import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString } from "../util"
+import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString, escapeHtml } from "../util"
 
 export enum Cookie {
   Key = "key",
@@ -36,11 +36,12 @@ const getRoot = async (req: Request, error?: Error): Promise<string> => {
   } else if (req.args.usingEnvHashedPassword) {
     passwordMsg = "Password was set from $HASHED_PASSWORD."
   }
+
   return replaceTemplates(
     req,
     content
       .replace(/{{PASSWORD_MSG}}/g, passwordMsg)
-      .replace(/{{ERROR}}/, error ? `<div class="error">${error.message}</div>` : ""),
+      .replace(/{{ERROR}}/, error ? `<div class="error">${escapeHtml(error.message)}</div>` : ""),
   )
 }
 
@@ -111,6 +112,7 @@ router.post("/", async (req, res) => {
 
     throw new Error("Incorrect password")
   } catch (error) {
-    res.send(await getRoot(req, error))
+    const renderedHtml = await getRoot(req, error)
+    res.send(renderedHtml)
   }
 })
diff --git a/src/node/util.ts b/src/node/util.ts
@@ -508,3 +508,17 @@ export const isFile = async (path: string): Promise<boolean> => {
     return false
   }
 }
+
+/**
+ * Escapes any HTML string special characters, like &, <, >, ", and '.
+ *
+ * Source: https://stackoverflow.com/a/6234804/3015595
+ **/
+export function escapeHtml(unsafe: string): string {
+  return unsafe
+    .replace(/&/g, "&amp;")
+    .replace(/</g, "&lt;")
+    .replace(/>/g, "&gt;")
+    .replace(/"/g, "&quot;")
+    .replace(/'/g, "&apos;")
+}
diff --git a/test/unit/node/util.test.ts b/test/unit/node/util.test.ts
@@ -445,3 +445,11 @@ describe("onLine", () => {
     expect(await received).toEqual(expected)
   })
 })
+
+describe("escapeHtml", () => {
+  it("should escape HTML", () => {
+    expect(util.escapeHtml(`<div class="error">"'ello & world"</div>`)).toBe(
+      "&lt;div class=&quot;error&quot;&gt;&quot;&apos;ello &amp; world&quot;&lt;/div&gt;",
+    )
+  })
+})
diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts
@@ -1,4 +1,6 @@
 import { RateLimiter } from "../../../src/node/routes/login"
+import * as httpserver from "../../utils/httpserver"
+import * as integration from "../../utils/integration"
 
 describe("login", () => {
   describe("RateLimiter", () => {
@@ -34,4 +36,41 @@ describe("login", () => {
       expect(limiter.removeToken()).toBe(false)
     })
   })
+  describe("/login", () => {
+    let _codeServer: httpserver.HttpServer | undefined
+    function codeServer(): httpserver.HttpServer {
+      if (!_codeServer) {
+        throw new Error("tried to use code-server before setting it up")
+      }
+      return _codeServer
+    }
+
+    // Store whatever might be in here so we can restore it afterward.
+    // TODO: We should probably pass this as an argument somehow instead of
+    // manipulating the environment.
+    const previousEnvPassword = process.env.PASSWORD
+
+    beforeEach(async () => {
+      process.env.PASSWORD = "test"
+      _codeServer = await integration.setup(["--auth=password"], "")
+    })
+
+    afterEach(async () => {
+      process.env.PASSWORD = previousEnvPassword
+      if (_codeServer) {
+        await _codeServer.close()
+        _codeServer = undefined
+      }
+    })
+
+    it("should return HTML with 'Missing password' message", async () => {
+      const resp = await codeServer().fetch("/login", { method: "POST" })
+
+      expect(resp.status).toBe(200)
+
+      const htmlContent = await resp.text()
+
+      expect(htmlContent).toContain("Missing password")
+    })
+  })
 })
