diff --git a/src/node/http.ts b/src/node/http.ts index 87a0be10..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)}'`) @@ -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 index dfd07ce9..999b8dfa 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,11 +36,12 @@ const getRoot = async (req: Request, error?: Error): Promise => { } else if (req.args.usingEnvHashedPassword) { passwordMsg = "Password was set from $HASHED_PASSWORD." } + return replaceTemplates( req, content .replace(/{{PASSWORD_MSG}}/g, passwordMsg) - .replace(/{{ERROR}}/, error ? `
${error.message}
` : ""), + .replace(/{{ERROR}}/, error ? `
${escapeHtml(error.message)}
` : ""), ) } @@ -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 index 5cb5e3cd..40ae9cef 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..d93cbd37 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(`
"'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 2a2e2046..ba4fcac8 100644 --- 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") + }) + }) })