From c505fc45a84623dc76dfb2884bf8229f17d50af1 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 29 Jun 2021 15:28:44 -0700 Subject: [PATCH 1/5] feat: add escapeHtml function This can be used to escape any special characters in a string with HTML before sending from the server back to the client. This is important to prevent a cross-site scripting attack. --- src/node/routes/login.ts | 7 ++++-- src/node/util.ts | 14 ++++++++++++ test/unit/node/util.test.ts | 8 +++++++ test/unit/routes/login.test.ts | 40 ++++++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 2 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index dfd07ce9..2b160f25 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, sanitizeString } from "../util" +import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString, escapeHtml } from "../util" export enum Cookie { Key = "key", @@ -36,6 +36,7 @@ const getRoot = async (req: Request, error?: Error): Promise => { } else if (req.args.usingEnvHashedPassword) { passwordMsg = "Password was set from $HASHED_PASSWORD." } + return replaceTemplates( req, content @@ -111,6 +112,8 @@ router.post("/", async (req, res) => { throw new Error("Incorrect password") } catch (error) { - res.send(await getRoot(req, error)) + const html = await getRoot(req, error) + const escapedHtml = escapeHtml(html) + res.send(escapedHtml) } }) diff --git a/src/node/util.ts b/src/node/util.ts index 5cb5e3cd..09e439de 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -508,3 +508,17 @@ export const isFile = async (path: string): Promise => { 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, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'") +} diff --git a/test/unit/node/util.test.ts b/test/unit/node/util.test.ts index 8fae54b7..d089908b 100644 --- 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(`
"Hello & world"
`)).toBe( + "<div class="error">"Hello & world"</div>", + ) + }) +}) diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts index 2a2e2046..9d68799b 100644 --- a/test/unit/routes/login.test.ts +++ b/test/unit/routes/login.test.ts @@ -1,3 +1,6 @@ +import * as httpserver from "../../utils/httpserver" +import * as integration from "../../utils/integration" + import { RateLimiter } from "../../../src/node/routes/login" describe("login", () => { @@ -34,4 +37,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(() => { + process.env.PASSWORD = previousEnvPassword + }) + + it("should return escaped 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).not.toContain(">") + expect(htmlContent).not.toContain("<") + expect(htmlContent).not.toContain('"') + expect(htmlContent).not.toContain("'") + expect(htmlContent).toContain("<div class="error">Missing password</div>") + }) + }) }) From 22a22a8f7a2f2d5fd74d5763d4a18875bcd13452 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 30 Jun 2021 09:53:04 -0700 Subject: [PATCH 2/5] fix: escape error.message on login failure --- src/node/routes/login.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index 2b160f25..63991165 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -41,7 +41,7 @@ const getRoot = async (req: Request, error?: Error): Promise => { req, content .replace(/{{PASSWORD_MSG}}/g, passwordMsg) - .replace(/{{ERROR}}/, error ? `
${error.message}
` : ""), + .replace(/{{ERROR}}/, error ? `
${escapeHtml(error.message)}
` : ""), ) } @@ -112,8 +112,7 @@ router.post("/", async (req, res) => { throw new Error("Incorrect password") } catch (error) { - const html = await getRoot(req, error) - const escapedHtml = escapeHtml(html) - res.send(escapedHtml) + const htmlToRender = await getRoot(req, error) + res.send(htmlToRender) } }) From 2092f82270b32522b67a4677d00b06f48cf46e09 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 30 Jun 2021 10:37:08 -0700 Subject: [PATCH 3/5] fixup! fix: escape error.message on login failure --- src/node/util.ts | 2 +- test/unit/node/util.test.ts | 4 ++-- test/unit/routes/login.test.ts | 8 ++------ 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/node/util.ts b/src/node/util.ts index 09e439de..40ae9cef 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -520,5 +520,5 @@ export function escapeHtml(unsafe: string): string { .replace(//g, ">") .replace(/"/g, """) - .replace(/'/g, "'") + .replace(/'/g, "'") } diff --git a/test/unit/node/util.test.ts b/test/unit/node/util.test.ts index d089908b..d93cbd37 100644 --- a/test/unit/node/util.test.ts +++ b/test/unit/node/util.test.ts @@ -448,8 +448,8 @@ describe("onLine", () => { describe("escapeHtml", () => { it("should escape HTML", () => { - expect(util.escapeHtml(`
"Hello & world"
`)).toBe( - "<div class="error">"Hello & world"</div>", + expect(util.escapeHtml(`
"'ello & world"
`)).toBe( + "<div class="error">"'ello & world"</div>", ) }) }) diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts index 9d68799b..c6e131bd 100644 --- a/test/unit/routes/login.test.ts +++ b/test/unit/routes/login.test.ts @@ -60,18 +60,14 @@ describe("login", () => { process.env.PASSWORD = previousEnvPassword }) - it("should return escaped HTML with 'Missing password' message", async () => { + 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).not.toContain(">") - expect(htmlContent).not.toContain("<") - expect(htmlContent).not.toContain('"') - expect(htmlContent).not.toContain("'") - expect(htmlContent).toContain("<div class="error">Missing password</div>") + expect(htmlContent).toContain("Missing password") }) }) }) From 2ba03c3424144ce625a328c95251553597d9c460 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 30 Jun 2021 10:37:46 -0700 Subject: [PATCH 4/5] docs: clarify redirect function in http.ts usage --- src/node/http.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node/http.ts b/src/node/http.ts index 87a0be10..76eb5207 100644 --- a/src/node/http.ts +++ b/src/node/http.ts @@ -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 = ( From c0e123a8012c8fd0aa0bbce3edbcf1e25f63eea4 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 30 Jun 2021 10:39:48 -0700 Subject: [PATCH 5/5] fix(http): escape req.query.to in replaceTemplates --- src/node/http.ts | 4 ++-- src/node/routes/login.ts | 4 ++-- test/unit/routes/login.test.ts | 9 ++++++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/node/http.ts b/src/node/http.ts index 76eb5207..d7ffa1f1 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, 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 = ( ...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)}'`) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index 63991165..999b8dfa 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -112,7 +112,7 @@ router.post("/", async (req, res) => { throw new Error("Incorrect password") } catch (error) { - const htmlToRender = await getRoot(req, error) - res.send(htmlToRender) + const renderedHtml = await getRoot(req, error) + res.send(renderedHtml) } }) diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts index c6e131bd..ba4fcac8 100644 --- a/test/unit/routes/login.test.ts +++ b/test/unit/routes/login.test.ts @@ -1,8 +1,7 @@ +import { RateLimiter } from "../../../src/node/routes/login" import * as httpserver from "../../utils/httpserver" import * as integration from "../../utils/integration" -import { RateLimiter } from "../../../src/node/routes/login" - describe("login", () => { describe("RateLimiter", () => { it("should allow one try ", () => { @@ -56,8 +55,12 @@ describe("login", () => { _codeServer = await integration.setup(["--auth=password"], "") }) - afterEach(() => { + afterEach(async () => { process.env.PASSWORD = previousEnvPassword + if (_codeServer) { + await _codeServer.close() + _codeServer = undefined + } }) it("should return HTML with 'Missing password' message", async () => {