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

fix: escape error.message on login failure
This commit is contained in:
Joe Previte 2021-07-01 14:50:32 -07:00 committed by GitHub
commit 975dd13d12
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 70 additions and 6 deletions

View File

@ -7,7 +7,7 @@ import { normalize, Options } from "../common/util"
import { AuthType, DefaultedArgs } from "./cli" import { AuthType, DefaultedArgs } from "./cli"
import { commit, rootPath } from "./constants" import { commit, rootPath } from "./constants"
import { Heart } from "./heart" import { Heart } from "./heart"
import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString } from "./util" import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString, escapeHtml } from "./util"
declare global { declare global {
// eslint-disable-next-line @typescript-eslint/no-namespace // eslint-disable-next-line @typescript-eslint/no-namespace
@ -35,7 +35,7 @@ export const replaceTemplates = <T extends object>(
...extraOpts, ...extraOpts,
} }
return content 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(/{{BASE}}/g, options.base)
.replace(/{{CS_STATIC_BASE}}/g, options.csStaticBase) .replace(/{{CS_STATIC_BASE}}/g, options.csStaticBase)
.replace(/"{{OPTIONS}}"/, `'${JSON.stringify(options)}'`) .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). * `override` will merge with the existing query (use `undefined` to unset).
*/ */
export const redirect = ( export const redirect = (

View File

@ -4,7 +4,7 @@ import { RateLimiter as Limiter } from "limiter"
import * as path from "path" import * as path from "path"
import { rootPath } from "../constants" import { rootPath } from "../constants"
import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http" 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 { export enum Cookie {
Key = "key", Key = "key",
@ -36,11 +36,12 @@ const getRoot = async (req: Request, error?: Error): Promise<string> => {
} else if (req.args.usingEnvHashedPassword) { } else if (req.args.usingEnvHashedPassword) {
passwordMsg = "Password was set from $HASHED_PASSWORD." passwordMsg = "Password was set from $HASHED_PASSWORD."
} }
return replaceTemplates( return replaceTemplates(
req, req,
content content
.replace(/{{PASSWORD_MSG}}/g, passwordMsg) .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") throw new Error("Incorrect password")
} catch (error) { } catch (error) {
res.send(await getRoot(req, error)) const renderedHtml = await getRoot(req, error)
res.send(renderedHtml)
} }
}) })

View File

@ -508,3 +508,17 @@ export const isFile = async (path: string): Promise<boolean> => {
return false 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;")
}

View File

@ -445,3 +445,11 @@ describe("onLine", () => {
expect(await received).toEqual(expected) 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;",
)
})
})

View File

@ -1,4 +1,6 @@
import { RateLimiter } from "../../../src/node/routes/login" import { RateLimiter } from "../../../src/node/routes/login"
import * as httpserver from "../../utils/httpserver"
import * as integration from "../../utils/integration"
describe("login", () => { describe("login", () => {
describe("RateLimiter", () => { describe("RateLimiter", () => {
@ -34,4 +36,41 @@ describe("login", () => {
expect(limiter.removeToken()).toBe(false) 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")
})
})
}) })