From 3b50bfc17d031661c83a0a029bdb22a7b391e7b0 Mon Sep 17 00:00:00 2001
From: Joe Previte <jjprevite@gmail.com>
Date: Mon, 7 Jun 2021 14:46:59 -0700
Subject: [PATCH] fix: sanitize password and cookie key

---
 ci/build/build-standalone-release.sh |  2 +-
 ci/build/npm-postinstall.sh          |  2 +-
 src/node/http.ts                     |  4 ++--
 src/node/routes/login.ts             |  4 ++--
 src/node/util.ts                     | 11 +++++++++++
 test/unit/node/util.test.ts          | 13 +++++++++++++
 6 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/ci/build/build-standalone-release.sh b/ci/build/build-standalone-release.sh
index a8072a5a5..3afde2ead 100755
--- a/ci/build/build-standalone-release.sh
+++ b/ci/build/build-standalone-release.sh
@@ -3,7 +3,7 @@ set -euo pipefail
 
 # This is due to an upstream issue with RHEL7/CentOS 7 comptability with node-argon2
 # See: https://github.com/cdr/code-server/pull/3422#pullrequestreview-677765057
-npm_config_build_from_source=true
+export npm_config_build_from_source=true
 
 main() {
   cd "$(dirname "${0}")/../.."
diff --git a/ci/build/npm-postinstall.sh b/ci/build/npm-postinstall.sh
index ae1be016a..5e413d7a6 100755
--- a/ci/build/npm-postinstall.sh
+++ b/ci/build/npm-postinstall.sh
@@ -20,7 +20,7 @@ detect_arch() {
 ARCH="${NPM_CONFIG_ARCH:-$(detect_arch)}"
 # This is due to an upstream issue with RHEL7/CentOS 7 comptability with node-argon2
 # See: https://github.com/cdr/code-server/pull/3422#pullrequestreview-677765057
-npm_config_build_from_source=true
+export npm_config_build_from_source=true
 
 main() {
   # Grabs the major version of node from $npm_config_user_agent which looks like
diff --git a/src/node/http.ts b/src/node/http.ts
index 8b4c9d8c8..87a0be108 100644
--- 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 } from "./util"
+import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString } from "./util"
 
 declare global {
   // eslint-disable-next-line @typescript-eslint/no-namespace
@@ -72,7 +72,7 @@ export const authenticated = async (req: express.Request): Promise<boolean> => {
       const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
       const isCookieValidArgs: IsCookieValidArgs = {
         passwordMethod,
-        cookieKey: req.cookies.key as string,
+        cookieKey: sanitizeString(req.cookies.key),
         passwordFromArgs: req.args.password || "",
         hashedPasswordFromArgs: req.args["hashed-password"],
       }
diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts
index 6b8ab4d23..dfd07ce95 100644
--- 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 } from "../util"
+import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString } from "../util"
 
 export enum Cookie {
   Key = "key",
@@ -61,7 +61,7 @@ router.get("/", async (req, res) => {
 })
 
 router.post("/", async (req, res) => {
-  const password = req.body.password
+  const password = sanitizeString(req.body.password)
   const hashedPasswordFromArgs = req.args["hashed-password"]
 
   try {
diff --git a/src/node/util.ts b/src/node/util.ts
index 5a3d0b701..6fbabd0f7 100644
--- a/src/node/util.ts
+++ b/src/node/util.ts
@@ -274,6 +274,17 @@ export async function isCookieValid(isCookieValidArgs: IsCookieValidArgs): Promi
   return isValid
 }
 
+/** Ensures that the input is sanitized by checking
+ * - it's a string
+ * - greater than 0 characters
+ * - trims whitespace
+ */
+export function sanitizeString(str: string): string {
+  // Very basic sanitization of string
+  // Credit: https://stackoverflow.com/a/46719000/3015595
+  return typeof str === "string" && str.trim().length > 0 ? str.trim() : ""
+}
+
 const mimeTypes: { [key: string]: string } = {
   ".aac": "audio/x-aac",
   ".avi": "video/x-msvideo",
diff --git a/test/unit/node/util.test.ts b/test/unit/node/util.test.ts
index 0397fbff9..14a674455 100644
--- a/test/unit/node/util.test.ts
+++ b/test/unit/node/util.test.ts
@@ -7,6 +7,7 @@ import {
   hashLegacy,
   isHashLegacyMatch,
   isCookieValid,
+  sanitizeString,
 } from "../../../src/node/util"
 
 describe("getEnvPaths", () => {
@@ -382,3 +383,15 @@ describe.only("isCookieValid", () => {
     expect(isValid).toBe(false)
   })
 })
+
+describe.only("sanitizeString", () => {
+  it("should return an empty string if passed a type other than a string", () => {
+    expect(sanitizeString({} as string)).toBe("")
+  })
+  it("should trim whitespace", () => {
+    expect(sanitizeString(" hello   ")).toBe("hello")
+  })
+  it("should always return an empty string", () => {
+    expect(sanitizeString("   ")).toBe("")
+  })
+})