Skip to content

Commit a1d6bcb

Browse files
committed
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.
1 parent 727ac64 commit a1d6bcb

File tree

1 file changed

+29
-15
lines changed

1 file changed

+29
-15
lines changed

src/node/server.ts

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ export interface Response {
9898
}
9999

100100
export interface LoginPayload {
101-
password?: string;
101+
password?: string[] | string;
102102
}
103103

104104
export class HttpError extends Error {
@@ -298,10 +298,7 @@ export abstract class Server {
298298
return response;
299299
}
300300
if (!this.authenticate(request)) {
301-
return {
302-
redirect: "/login",
303-
headers: { "Set-Cookie": `password=` }
304-
};
301+
return { redirect: "/login" };
305302
}
306303
break;
307304
case "/static":
@@ -360,16 +357,22 @@ export abstract class Server {
360357
}
361358

362359
private async tryLogin(request: http.IncomingMessage): Promise<Response> {
363-
if (this.authenticate(request) && (request.method === "GET" || request.method === "POST")) {
364-
return { redirect: "/" };
360+
const redirect = (password?: string | string[] | true) => {
361+
return {
362+
redirect: "/",
363+
headers: typeof password === "string"
364+
? { "Set-Cookie": `password=${password}; Path=${this.options.basePath || "/"}; HttpOnly; SameSite=strict` }
365+
: {},
366+
};
367+
};
368+
const providedPassword = this.authenticate(request);
369+
if (providedPassword && (request.method === "GET" || request.method === "POST")) {
370+
return redirect(providedPassword);
365371
}
366372
if (request.method === "POST") {
367373
const data = await this.getData<LoginPayload>(request);
368374
if (this.authenticate(request, data)) {
369-
return {
370-
redirect: "/",
371-
headers: { "Set-Cookie": `password=${data.password}` }
372-
};
375+
return redirect(data.password);
373376
}
374377
console.error("Failed login attempt", JSON.stringify({
375378
xForwardedFor: request.headers["x-forwarded-for"],
@@ -429,23 +432,34 @@ export abstract class Server {
429432
: Promise.resolve({} as T);
430433
}
431434

432-
private authenticate(request: http.IncomingMessage, payload?: LoginPayload): boolean {
435+
private authenticate(request: http.IncomingMessage, payload?: LoginPayload): string | boolean {
433436
if (this.options.auth !== "password") {
434437
return true;
435438
}
436439
const safeCompare = localRequire<typeof import("safe-compare")>("safe-compare/index");
437440
if (typeof payload === "undefined") {
438441
payload = this.parseCookies<LoginPayload>(request);
439442
}
440-
return !!this.options.password && safeCompare(payload.password || "", this.options.password);
443+
if (this.options.password && payload.password) {
444+
const toTest = Array.isArray(payload.password) ? payload.password : [payload.password];
445+
for (let i = 0; i < toTest.length; ++i) {
446+
if (safeCompare(toTest[i], this.options.password)) {
447+
return toTest[i];
448+
}
449+
}
450+
}
451+
return false;
441452
}
442453

443454
private parseCookies<T extends object>(request: http.IncomingMessage): T {
444-
const cookies: { [key: string]: string } = {};
455+
const cookies: { [key: string]: string[] } = {};
445456
if (request.headers.cookie) {
446457
request.headers.cookie.split(";").forEach((keyValue) => {
447458
const [key, value] = split(keyValue, "=");
448-
cookies[key] = decodeURI(value);
459+
if (!cookies[key]) {
460+
cookies[key] = [];
461+
}
462+
cookies[key].push(decodeURI(value));
449463
});
450464
}
451465
return cookies as T;

0 commit comments

Comments
 (0)