M1 security: close the password backdoor, apply re-review, pin E.164
Acts on an adversarial review of the M1 auth and authorization code. Five findings fixed; the rest recorded in SECURITY-FINDINGS.md as the M1 exit criteria rather than left in a tool transcript. - auth: serve /phone-number/request-password-reset, /phone-number/ reset-password and /sign-in/phone-number as 404. better-auth's phoneNumber() registers all three unconditionally -- they are NOT gated on emailAndPassword.enabled:false. Left live they form a silent second credential path: request-password-reset stores an OTP and sends no SMS (sendPasswordResetOTP was never configured, so the owner is never told), reset-password mints a bcrypt credential row, and sign-in/phone-number then accepts it forever with no OTP. The OTP gate still applies, so this is not remote unauthenticated takeover -- it converts one momentary OTP compromise into permanent access the victim cannot see or rotate. - auth: drop bearer(). It accepts the plaintext sessions.token column as an Authorization credential, making any single leaked row a replayable login. The mobile client it was added for is hypothetical. - auth: pin E.164 via phoneNumberValidator, and add toE164/isE164 to @linkder/shared. phone is UNIQUE and bans are per-account, so "+34600111222" and "0034600111222" being separately storable meant one handset could hold two accounts and a ban was escapable by retyping. 15 tests. - auth: NEXT_PUBLIC_APP_URL now throws in production instead of falling back to localhost, which was silently dropping Secure and the __Secure- prefix from the production session cookie. - pro.upsertProfile: actually apply requiresReReview. It was computed, returned to the client and never acted on, so a verified plumber could become a verified electrician in another city by ignoring a response flag. Now demotes to pending in the same transaction and audits it. Trade changes count as material (they did not before) -- the licence is per-trade. Needed a verified -> pending edge in VERIFICATION_GRAPH, which did not exist. Removed two untracked scratch repro files. The impersonation repro depended on bearer() for transport and no longer applies as written; the underlying finding (resolveSession drops impersonatedBy, so admin actions are audited as the victim) is open and documented. typecheck, lint, build clean; 127 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -67,11 +67,44 @@ export const proRouter = router({
|
||||
|
||||
// Editing a live profile sends it back for review — a verified plumber must
|
||||
// not be able to quietly become an unverified electrician.
|
||||
const materiallyChanged =
|
||||
//
|
||||
// The trade list is part of "material": it is the single most important
|
||||
// thing verification actually checks (the licence is per-trade), so a
|
||||
// change of trade matters more than a change of radius.
|
||||
const previousCategoryIds = existing
|
||||
? (
|
||||
await ctx.db
|
||||
.select({ categoryId: schema.proCategories.categoryId })
|
||||
.from(schema.proCategories)
|
||||
.where(eq(schema.proCategories.proId, ctx.session.userId))
|
||||
)
|
||||
.map((c) => c.categoryId)
|
||||
.sort()
|
||||
: [];
|
||||
const nextCategoryIds = [...input.categoryIds].sort();
|
||||
|
||||
const materiallyChanged = Boolean(
|
||||
existing &&
|
||||
(existing.serviceRadiusM !== input.serviceRadiusM ||
|
||||
existing.baseLocation.lat !== input.location.lat ||
|
||||
existing.baseLocation.lng !== input.location.lng);
|
||||
(existing.serviceRadiusM !== input.serviceRadiusM ||
|
||||
existing.baseLocation.lat !== input.location.lat ||
|
||||
existing.baseLocation.lng !== input.location.lng ||
|
||||
previousCategoryIds.length !== nextCategoryIds.length ||
|
||||
previousCategoryIds.some((id, i) => id !== nextCategoryIds[i])),
|
||||
);
|
||||
|
||||
/**
|
||||
* Act on it. Computing `materiallyChanged` and returning it to the client
|
||||
* without changing anything server-side is exactly the bug the comment
|
||||
* above was written to prevent: the client is free to ignore the flag.
|
||||
*
|
||||
* Only a currently-verified pro needs demoting. A draft/pending/rejected
|
||||
* profile is already not on the deck, and demoting a `suspended` pro to
|
||||
* pending would quietly undo a moderator's suspension.
|
||||
*/
|
||||
const sendBackForReview = materiallyChanged && existing?.verificationStatus === 'verified';
|
||||
if (sendBackForReview) {
|
||||
assertTransition('verification', 'verified', 'pending');
|
||||
}
|
||||
|
||||
await ctx.db.transaction(async (tx) => {
|
||||
const values = {
|
||||
@@ -83,6 +116,7 @@ export const proRouter = router({
|
||||
baseLocation: input.location,
|
||||
serviceRadiusM: input.serviceRadiusM,
|
||||
updatedAt: new Date(),
|
||||
...(sendBackForReview ? { verificationStatus: 'pending' as const } : {}),
|
||||
};
|
||||
|
||||
await tx
|
||||
@@ -96,9 +130,19 @@ export const proRouter = router({
|
||||
await tx.insert(schema.proCategories).values(
|
||||
input.categoryIds.map((categoryId) => ({ proId: ctx.session.userId, categoryId })),
|
||||
);
|
||||
|
||||
if (sendBackForReview) {
|
||||
await tx.insert(schema.auditLog).values({
|
||||
actorId: ctx.session.userId,
|
||||
action: 'verification.re_review_required',
|
||||
entity: 'pro_profile',
|
||||
entityId: ctx.session.userId,
|
||||
ip: ctx.ip,
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
return { saved: true, requiresReReview: Boolean(materiallyChanged) };
|
||||
return { saved: true, requiresReReview: sendBackForReview };
|
||||
}),
|
||||
|
||||
/** Attach an uploaded photo. The file itself went straight to R2. */
|
||||
@@ -150,7 +194,7 @@ export const proRouter = router({
|
||||
.values({
|
||||
proId: ctx.session.userId,
|
||||
kind: input.kind,
|
||||
fileUrl: input.fileUrl,
|
||||
fileKey: input.fileKey,
|
||||
issuer: input.issuer ?? null,
|
||||
expiresAt: input.expiresAt ?? null,
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user