Handle cookies more robustly

If you visit /login/ instead of /login the cookie will be set at /login
instead of / which means the cookie can't be read at the root. It will
redirect to the login page which *can* read the cookie at /login and
redirect back resulting in an infinite loop.

The previous solution relied on setting the cookie at / (any invalid
value works) which then overrode the login page cookie since
parseCookies only kept a single value. So the login page would see the
same cookie the root was seeing and not redirect back. However, that
behavior depends on the cookies being in the right order which I'm not
sure is guaranteed.

This new method tests all available cookies and always sets the cookie
so the root path will be able to read it in case the login page is
seeing a cookie the root can't.

It also goes a step further and explicitly sets the path on the cookie
which fixes the case where there is a permanent misconfiguration
redirecting /login to /login/. Otherwise the cookie would continually be
set on /login only and you'd have another loop. It also means you only
need to delete one cookie to log out.

Lastly add some properties to make the cookies a bit more secure.
This commit is contained in:
Asher 2019-11-07 12:41:06 -06:00
parent 727ac6483b
commit a1d6bcb8e5
No known key found for this signature in database
GPG key ID: D63C1EF81242354A

View file

@ -98,7 +98,7 @@ export interface Response {
} }
export interface LoginPayload { export interface LoginPayload {
password?: string; password?: string[] | string;
} }
export class HttpError extends Error { export class HttpError extends Error {
@ -298,10 +298,7 @@ export abstract class Server {
return response; return response;
} }
if (!this.authenticate(request)) { if (!this.authenticate(request)) {
return { return { redirect: "/login" };
redirect: "/login",
headers: { "Set-Cookie": `password=` }
};
} }
break; break;
case "/static": case "/static":
@ -360,16 +357,22 @@ export abstract class Server {
} }
private async tryLogin(request: http.IncomingMessage): Promise<Response> { private async tryLogin(request: http.IncomingMessage): Promise<Response> {
if (this.authenticate(request) && (request.method === "GET" || request.method === "POST")) { const redirect = (password?: string | string[] | true) => {
return { redirect: "/" }; return {
redirect: "/",
headers: typeof password === "string"
? { "Set-Cookie": `password=${password}; Path=${this.options.basePath || "/"}; HttpOnly; SameSite=strict` }
: {},
};
};
const providedPassword = this.authenticate(request);
if (providedPassword && (request.method === "GET" || request.method === "POST")) {
return redirect(providedPassword);
} }
if (request.method === "POST") { if (request.method === "POST") {
const data = await this.getData<LoginPayload>(request); const data = await this.getData<LoginPayload>(request);
if (this.authenticate(request, data)) { if (this.authenticate(request, data)) {
return { return redirect(data.password);
redirect: "/",
headers: { "Set-Cookie": `password=${data.password}` }
};
} }
console.error("Failed login attempt", JSON.stringify({ console.error("Failed login attempt", JSON.stringify({
xForwardedFor: request.headers["x-forwarded-for"], xForwardedFor: request.headers["x-forwarded-for"],
@ -429,7 +432,7 @@ export abstract class Server {
: Promise.resolve({} as T); : Promise.resolve({} as T);
} }
private authenticate(request: http.IncomingMessage, payload?: LoginPayload): boolean { private authenticate(request: http.IncomingMessage, payload?: LoginPayload): string | boolean {
if (this.options.auth !== "password") { if (this.options.auth !== "password") {
return true; return true;
} }
@ -437,15 +440,26 @@ export abstract class Server {
if (typeof payload === "undefined") { if (typeof payload === "undefined") {
payload = this.parseCookies<LoginPayload>(request); payload = this.parseCookies<LoginPayload>(request);
} }
return !!this.options.password && safeCompare(payload.password || "", this.options.password); if (this.options.password && payload.password) {
const toTest = Array.isArray(payload.password) ? payload.password : [payload.password];
for (let i = 0; i < toTest.length; ++i) {
if (safeCompare(toTest[i], this.options.password)) {
return toTest[i];
}
}
}
return false;
} }
private parseCookies<T extends object>(request: http.IncomingMessage): T { private parseCookies<T extends object>(request: http.IncomingMessage): T {
const cookies: { [key: string]: string } = {}; const cookies: { [key: string]: string[] } = {};
if (request.headers.cookie) { if (request.headers.cookie) {
request.headers.cookie.split(";").forEach((keyValue) => { request.headers.cookie.split(";").forEach((keyValue) => {
const [key, value] = split(keyValue, "="); const [key, value] = split(keyValue, "=");
cookies[key] = decodeURI(value); if (!cookies[key]) {
cookies[key] = [];
}
cookies[key].push(decodeURI(value));
}); });
} }
return cookies as T; return cookies as T;