close three cross-account holes found reading the multi-user path
reset-password accepted any valid signed jwt as a reset token, including a 30-day session token — its sibling verify-token.ts already gated on purpose === 'reset-password' and this handler did not. forgot-password mints that claim, so the gate costs the legitimate flow nothing. notify's DELETE /_officer/devices/:token deleted by token with no user predicate: a token is the address of a device, not a secret, so any account holding the notify capability could deregister another's device. deletePushDevice now takes an optional userId — the route passes it, the APNs/FCM dead-token paths deliberately do not. POST /_officer/notify let a request body's userId override the proxy-injected X-Officer-User. The header now wins where present, which is what separates a signed-in browser from a loopback producer that has no session to speak from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -37,9 +37,21 @@ export async function getPushDevices(userId: number, appSlug?: string): Promise<
|
|||||||
return db.select().from(pushDevices).where(where);
|
return db.select().from(pushDevices).where(where);
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Sign-out, or a hard rejection from Apple/Google (410 Unregistered, UNREGISTERED). */
|
/**
|
||||||
export async function deletePushDevice(token: string): Promise<void> {
|
* Sign-out, or a hard rejection from Apple/Google (410 Unregistered, UNREGISTERED).
|
||||||
await db.delete(pushDevices).where(eq(pushDevices.token, token));
|
*
|
||||||
|
* `userId` is optional because the two callers are genuinely different. The APNs/FCM paths delete a
|
||||||
|
* token the provider has just declared dead, which is true for whoever owns it — they pass nothing. The
|
||||||
|
* `DELETE /_officer/devices/:token` route is reached by a signed-in account naming a token in the URL,
|
||||||
|
* so it MUST pass its own id: a token is the address of a device, not a secret, and without the
|
||||||
|
* predicate any account could deregister another's device by guessing or replaying one.
|
||||||
|
*/
|
||||||
|
export async function deletePushDevice(token: string, userId?: number): Promise<void> {
|
||||||
|
const where =
|
||||||
|
userId === undefined
|
||||||
|
? eq(pushDevices.token, token)
|
||||||
|
: and(eq(pushDevices.token, token), eq(pushDevices.userId, userId));
|
||||||
|
await db.delete(pushDevices).where(where);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -6,11 +6,18 @@ import argon2 from 'argon2';
|
|||||||
import * as errors from '@@/custom-errors';
|
import * as errors from '@@/custom-errors';
|
||||||
import { validatePassword } from './validate-password';
|
import { validatePassword } from './validate-password';
|
||||||
|
|
||||||
|
// The token must be a reset-password token and nothing else. `verifyToken` — which only renders the form
|
||||||
|
// — has always checked `purpose`; this handler, which actually rewrites the password, did not, so any
|
||||||
|
// valid signed JWT was accepted here. A 30-day session token is a valid signed JWT, which made this a
|
||||||
|
// route from "holds a session token" to "owns the account": it rewrites the password (locking the real
|
||||||
|
// owner out, since passwordChangedAt invalidates their other tokens) and forces `status: 'Active'`,
|
||||||
|
// re-enabling an account somebody had deliberately suspended. The two handlers now agree.
|
||||||
export const resetPasswordHandler: Handler = async function (ctx) {
|
export const resetPasswordHandler: Handler = async function (ctx) {
|
||||||
const { password, verificationCode } = ctx.get('body');
|
const { password, verificationCode } = ctx.get('body');
|
||||||
validatePassword(password);
|
validatePassword(password);
|
||||||
const userInfo = (await verify(verificationCode)) as User;
|
const userInfo = (await verify(verificationCode)) as User & { purpose?: string };
|
||||||
if (!userInfo) throw errors.UNAUTHORIZED();
|
if (!userInfo?.id) throw errors.UNAUTHORIZED();
|
||||||
|
if (userInfo.purpose !== 'reset-password') throw errors.UNAUTHORIZED();
|
||||||
const passwordHash = await argon2.hash(password);
|
const passwordHash = await argon2.hash(password);
|
||||||
await updateUser(userInfo.id, { password: passwordHash, status: 'Active', passwordChangedAt: new Date() });
|
await updateUser(userInfo.id, { password: passwordHash, status: 'Active', passwordChangedAt: new Date() });
|
||||||
|
|
||||||
|
|||||||
@@ -2,8 +2,11 @@ import { upsertPushDevice, getPushDevices, deletePushDevice } from 'officerdb';
|
|||||||
|
|
||||||
// Device registration, reached through the platform's authenticated proxy.
|
// Device registration, reached through the platform's authenticated proxy.
|
||||||
//
|
//
|
||||||
// The owner comes from X-Officer-User, injected by createSidecarProxy and trusted because this server
|
// The caller comes from X-Officer-User, injected by createSidecarProxy and trusted because this server
|
||||||
// binds loopback only. Nothing else can reach it.
|
// binds loopback only. Nothing else can reach it.
|
||||||
|
//
|
||||||
|
// Every route here is scoped to that id, including the DELETE — which takes a token from the URL, and a
|
||||||
|
// token is the address of a device rather than a secret.
|
||||||
|
|
||||||
const PLATFORMS = new Set(['ios', 'android']);
|
const PLATFORMS = new Set(['ios', 'android']);
|
||||||
const ENVIRONMENTS = new Set(['production', 'sandbox']);
|
const ENVIRONMENTS = new Set(['production', 'sandbox']);
|
||||||
@@ -72,7 +75,7 @@ export async function handleDeviceRoute(req: Request, url: URL): Promise<Respons
|
|||||||
|
|
||||||
const match = url.pathname.match(/^\/_officer\/devices\/(.+)$/);
|
const match = url.pathname.match(/^\/_officer\/devices\/(.+)$/);
|
||||||
if (match && req.method === 'DELETE') {
|
if (match && req.method === 'DELETE') {
|
||||||
await deletePushDevice(decodeURIComponent(match[1]!));
|
await deletePushDevice(decodeURIComponent(match[1]!), userId);
|
||||||
return Response.json({ ok: true });
|
return Response.json({ ok: true });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -61,12 +61,17 @@ const server = Bun.serve({
|
|||||||
if (!body.type || !VALID_TYPES.includes(body.type)) {
|
if (!body.type || !VALID_TYPES.includes(body.type)) {
|
||||||
return Response.json({ error: `type must be one of ${VALID_TYPES.join(', ')}` }, { status: 400 });
|
return Response.json({ error: `type must be one of ${VALID_TYPES.join(', ')}` }, { status: 400 });
|
||||||
}
|
}
|
||||||
// Producers inside the tailnet POST directly and say who to notify. A browser reaching this
|
// Producers inside the tailnet POST directly over loopback and say who to notify — the queue and
|
||||||
// through /api/notify cannot know its own id, so the proxy's injected header stands in — the
|
// the email sidecar have no session to speak from, so the body is their only way to name a user.
|
||||||
// platform already authenticated whoever sent it.
|
//
|
||||||
|
// The header WINS where it is present, and that ordering is the whole access control here. A
|
||||||
|
// request carrying X-Officer-User arrived through createSidecarProxy, meaning a signed-in browser
|
||||||
|
// sent it; letting its body override the id the platform authenticated would let any account with
|
||||||
|
// the `notify` capability push to any other account's devices. A direct producer sets no header,
|
||||||
|
// so its body is still honoured.
|
||||||
const headerUser = Number(req.headers.get('X-Officer-User'));
|
const headerUser = Number(req.headers.get('X-Officer-User'));
|
||||||
const userId = typeof body.userId === 'number' ? body.userId : headerUser;
|
const userId = Number.isFinite(headerUser) && headerUser > 0 ? headerUser : body.userId;
|
||||||
if (!Number.isFinite(userId) || userId <= 0) {
|
if (typeof userId !== 'number' || !Number.isFinite(userId) || userId <= 0) {
|
||||||
return Response.json({ error: 'userId is required (body or X-Officer-User)' }, { status: 400 });
|
return Response.json({ error: 'userId is required (body or X-Officer-User)' }, { status: 400 });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user