fix: findings from the code review of the mail and sharing work
Two reviews, both dimensions. The serious ones, verified before fixing:
Sessions never expired server-side and survived a password reset. `Max-Age` is
a request to the browser, not a rule for the server, and `readSession` only
checked the signature -- a leaked cookie stayed valid indefinitely, and the very
reset meant to rescue a compromised account left the intruder logged in. A
demoted admin also kept admin, because the role came from the cookie. Now the
cookie carries a session epoch, `verifySession` checks the account against the
database on every request and takes the role from there, and the epoch is
bumped on password reset and on any role change. This was pre-existing, but the
reset feature promised safety it did not deliver.
"Allow download: off" did nothing for images with transparency -- exactly the
cutouts and stickers this tool exists for. The view path skipped resizing when
`has_alpha`, so the untouched original went out at full resolution, right-click
saveable. Verified: now 1500x2000 instead of 2400x3200, and the download route
returns 403.
The share title went into outgoing mail HTML unescaped while the message was
escaped -- a foreign link could be posted through the account's own
SPF/DKIM-signed mailserver. Both escaped now, plus length limits.
argon2 ran on every request for passphrase-protected links, including every
image: 130 ms and 64 MiB each. A 40-image gallery would have walked into the
memory limit, and a few dozen wrong guesses could have stalled the process.
Checked once, then a short-lived signed cookie scoped to that one link. The
passphrase moved from the query string to POST (it was landing in history and
proxy logs) and wrong guesses are now rate limited.
`days` accepted nonsense and quietly produced an unlimited link -- `Number('abc')`
is NaN and `NaN > 0` is false. `[]` coerced to 0, same result. Now rejected
outright, which matters when the pictures are of children.
Rate limits keyed on `clientAddress`, which is the *first* X-Forwarded-For
value -- set by the caller. Counting from the right instead, with the number of
trusted proxies configurable (TRUSTED_PROXIES, default 1), plus per-username and
per-recipient counters, a real limit on outgoing mail, and a limiter that
expires entries individually instead of clearing everyone's counter.
Also: expired links could still be mailed; missing PUBLIC_BASE_URL sent dead
links while reporting success; the reset endpoint's response time revealed
whether an account existed (send is no longer awaited); invalid UUIDs produced
500s; the email uniqueness race returned an HTML error page; share_views grew
without bound and is now one row per visitor per hour, hashed per link, user
agent reduced to browser and system, deleted after 90 days; double provenance
marking appended a second XMP packet with the older model instead of replacing.
21 new tests (97 total) for shares, mail and provenance -- all three were
completely untested, and that is where both reviews found the most.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BNhLunz586g6DeYMa7Qsd1
This commit is contained in:
22 files changed
+752
-108
No files matched your search
@@ -1,10 +1,14 @@
|
||||
import type { APIRoute } from 'astro';
|
||||
import { login, makeSessionCookie, rateLimited, noteFailure, clearFailures } from '../../../lib/auth';
|
||||
import { echteIp, zuOft } from '../../../lib/limit';
|
||||
|
||||
export const prerender = false;
|
||||
|
||||
export const POST: APIRoute = async ({ request, clientAddress }) => {
|
||||
const ip = clientAddress || 'unknown';
|
||||
// Nicht `clientAddress` direkt: Das ist der **erste** Wert aus
|
||||
// X-Forwarded-For und damit vom Aufrufer selbst gesetzt — die Sperre nach
|
||||
// fünf Fehlversuchen ließe sich sonst mit einem wechselnden Header umgehen.
|
||||
const ip = echteIp(request, clientAddress);
|
||||
if (rateLimited(ip)) {
|
||||
return new Response(JSON.stringify({ error: 'Zu viele Versuche. Bitte in 15 Minuten erneut.' }),
|
||||
{ status: 429, headers: { 'Content-Type': 'application/json' } });
|
||||
@@ -19,15 +23,22 @@ export const POST: APIRoute = async ({ request, clientAddress }) => {
|
||||
username = String(f.get('username') || ''); password = String(f.get('password') || '');
|
||||
}
|
||||
|
||||
const user = await login(username.trim(), password);
|
||||
if (!user) {
|
||||
// Zusätzlich je Benutzername zählen — sonst hilft die IP-Sperre nichts, wenn
|
||||
// der Angreifer aus vielen Netzen kommt.
|
||||
if (zuOft('login-user:' + username.trim().toLowerCase(), 10, 15 * 60_000)) {
|
||||
return new Response(JSON.stringify({ error: 'Zu viele Versuche. Bitte in 15 Minuten erneut.' }),
|
||||
{ status: 429, headers: { 'Content-Type': 'application/json' } });
|
||||
}
|
||||
|
||||
const angemeldet = await login(username.trim(), password);
|
||||
if (!angemeldet) {
|
||||
noteFailure(ip);
|
||||
return new Response(JSON.stringify({ error: 'Benutzername oder Passwort falsch.' }),
|
||||
{ status: 401, headers: { 'Content-Type': 'application/json' } });
|
||||
}
|
||||
clearFailures(ip);
|
||||
return new Response(JSON.stringify({ ok: true, role: user.role }), {
|
||||
return new Response(JSON.stringify({ ok: true, role: angemeldet.user.role }), {
|
||||
status: 200,
|
||||
headers: { 'Content-Type': 'application/json', 'Set-Cookie': makeSessionCookie(user) },
|
||||
headers: { 'Content-Type': 'application/json', 'Set-Cookie': makeSessionCookie(angemeldet.user, angemeldet.epoch) },
|
||||
});
|
||||
};
|
||||
+20
-17
@@ -1,8 +1,9 @@
|
||||
import type { APIRoute } from 'astro';
|
||||
import { randomBytes, createHash } from 'node:crypto';
|
||||
import { one, query } from '../../../lib/db';
|
||||
import { hashPassword } from '../../../lib/auth';
|
||||
import { sendMail, mailLayout, mailReady, istMail } from '../../../lib/mail';
|
||||
import { hashPassword, bumpSessionEpoch } from '../../../lib/auth';
|
||||
import { sendMail, mailLayout, mailReady, istMail, esc } from '../../../lib/mail';
|
||||
import { zuOft, echteIp } from '../../../lib/limit';
|
||||
|
||||
export const prerender = false;
|
||||
const json = (b: unknown, s = 200) =>
|
||||
@@ -12,17 +13,6 @@ const GUELTIG_MINUTEN = 60;
|
||||
const hashToken = (t: string) => createHash('sha256').update(t).digest('hex');
|
||||
const basis = () => (process.env.PUBLIC_BASE_URL || '').replace(/\/+$/, '');
|
||||
|
||||
// Ein Zähler je Adresse, damit niemand über diese Route Mails verschicken kann.
|
||||
const versuche = new Map<string, { n: number; bis: number }>();
|
||||
function zuOft(schluessel: string, max = 5, fensterMs = 15 * 60_000): boolean {
|
||||
const jetzt = Date.now();
|
||||
const e = versuche.get(schluessel);
|
||||
if (!e || e.bis < jetzt) { versuche.set(schluessel, { n: 1, bis: jetzt + fensterMs }); return false; }
|
||||
e.n++;
|
||||
if (versuche.size > 5000) versuche.clear(); // simpler Schutz gegen Zuwachs
|
||||
return e.n > max;
|
||||
}
|
||||
|
||||
/**
|
||||
* Rücksetz-Link anfordern (`{ email }`) oder neues Passwort setzen
|
||||
* (`{ token, password }`).
|
||||
@@ -31,6 +21,7 @@ function zuOft(schluessel: string, max = 5, fensterMs = 15 * 60_000): boolean {
|
||||
* Adresse gibt. Sonst ließe sich hier ablesen, wer ein Konto hat.
|
||||
*/
|
||||
export const POST: APIRoute = async ({ request, clientAddress }) => {
|
||||
const ip = echteIp(request, clientAddress);
|
||||
const b = await request.json().catch(() => ({}));
|
||||
|
||||
// --- Neues Passwort setzen -------------------------------------------------
|
||||
@@ -38,7 +29,7 @@ export const POST: APIRoute = async ({ request, clientAddress }) => {
|
||||
const token = String(b.token);
|
||||
const pw = String(b.password || '');
|
||||
if (pw.length < 10) return json({ error: 'Das Passwort muss mindestens 10 Zeichen haben.' }, 400);
|
||||
if (zuOft('einloesen:' + clientAddress, 20)) return json({ error: 'Zu viele Versuche. Bitte später erneut.' }, 429);
|
||||
if (zuOft('reset-einloesen:' + ip, 20, 15 * 60_000)) return json({ error: 'Zu viele Versuche. Bitte später erneut.' }, 429);
|
||||
|
||||
const r = await one<{ user_id: string }>(
|
||||
`SELECT user_id FROM password_resets
|
||||
@@ -48,13 +39,22 @@ export const POST: APIRoute = async ({ request, clientAddress }) => {
|
||||
await query('UPDATE users SET password_hash=$2 WHERE id=$1', [r.user_id, await hashPassword(pw)]);
|
||||
// Einmalnutzung, und alle anderen offenen Links desselben Kontos verfallen mit.
|
||||
await query('UPDATE password_resets SET used_at = now() WHERE user_id=$1 AND used_at IS NULL', [r.user_id]);
|
||||
// Entscheidend: **alle bestehenden Sitzungen entwerten**. Wer das Passwort
|
||||
// zurücksetzt, tut das meist, weil jemand anderes hineingekommen ist — ohne
|
||||
// diesen Schritt bliebe dessen Cookie bis zu 30 Tage gültig und der ganze
|
||||
// Vorgang wäre wirkungslos.
|
||||
await bumpSessionEpoch(r.user_id);
|
||||
return json({ ok: true });
|
||||
}
|
||||
|
||||
// --- Rücksetz-Link anfordern ----------------------------------------------
|
||||
const email = String(b.email || '').trim().toLowerCase();
|
||||
if (!istMail(email)) return json({ error: 'Bitte eine gültige E-Mail-Adresse eingeben.' }, 400);
|
||||
if (zuOft('anfordern:' + clientAddress)) return json({ ok: true }); // still schlucken
|
||||
// Zwei Grenzen: je Absender **und** je Zieladresse. Ohne die zweite ließe sich
|
||||
// ein bestimmtes Postfach von wechselnden Adressen aus zumüllen — auf Kosten
|
||||
// des Zustellrufs der eigenen Domain.
|
||||
if (zuOft('reset-ip:' + ip, 5, 15 * 60_000)) return json({ ok: true });
|
||||
if (zuOft('reset-mail:' + email, 3, 60 * 60_000)) return json({ ok: true });
|
||||
if (!(await mailReady())) return json({ error: 'Auf diesem Server ist kein E-Mail-Versand eingerichtet.' }, 503);
|
||||
|
||||
const u = await one<{ id: string; display_name: string | null; username: string }>(
|
||||
@@ -69,14 +69,17 @@ export const POST: APIRoute = async ({ request, clientAddress }) => {
|
||||
|
||||
const url = `${basis()}/passwort?token=${token}`;
|
||||
const name = u.display_name || u.username;
|
||||
await sendMail({
|
||||
// Nicht abwarten: Ein SMTP-Versand dauert Hunderte Millisekunden. Wartet die
|
||||
// Antwort darauf, verrät allein die Dauer, ob es das Konto gibt — und die
|
||||
// gleichlautende Antwort unten wäre wertlos.
|
||||
void sendMail({
|
||||
to: email,
|
||||
subject: 'Passwort für Klarbild zurücksetzen',
|
||||
text: `Hallo ${name},\n\nüber den folgenden Link kannst du dir ein neues Passwort für Klarbild setzen. `
|
||||
+ `Der Link gilt ${GUELTIG_MINUTEN} Minuten und lässt sich nur einmal verwenden.\n\n${url}\n\n`
|
||||
+ `Wenn du das nicht angefordert hast, kannst du diese Nachricht ignorieren — es ändert sich dann nichts.`,
|
||||
html: mailLayout('Neues Passwort setzen', [
|
||||
`Hallo ${name},`,
|
||||
`Hallo ${esc(name)},`,
|
||||
`über den Knopf unten setzt du dir ein neues Passwort für Klarbild. Der Link gilt <b>${GUELTIG_MINUTEN} Minuten</b> und lässt sich nur <b>einmal</b> verwenden.`,
|
||||
`Wenn du das nicht angefordert hast, kannst du diese Nachricht ignorieren — es ändert sich dann nichts.`,
|
||||
], { text: 'Neues Passwort setzen', url }),
|
||||
|
||||
Reference in new issue
Block a user