Finding 1: the fail-closed bypass
The starter runs, you are signed in as the admin, and findings/ holds eight empty skeletons.
Your goal this lesson is the first one: document the fail-closed violation in src/lib/admin/transfer-ownership.ts as findings/001-fail-closed.md, with all four template sections filled.
Switch between the two tabs below to see the move from an empty skeleton to the finished file — a precise rule, the exact lines that break it, the user-visible damage, and the fix.
# Finding 001 — <short title>
**Category:** one of the eight audit categories.**Severity:** critical | high | medium | low (justified in two lines).
<!-- TODO(L2) — document the fail-closed bypass in lib/admin/transfer-ownership.ts -->
## Rule
## Location
## Consequence
## FixFour empty headers and a TODO. Category and Severity are still the template menu; the four sections are bare. Nothing here names a defect yet.
# Finding 001 — Fail-closed bypass on the ownership-transfer role check
**Category:** Fail-closed checks (error discipline).**Severity:** critical — an owner-only mutation runs when the gatecannot prove the actor is an owner, reachable from a real admin action.
## RuleAny check that gates access fails closed (chapter 080, lesson 1)…
## Location`src/lib/admin/transfer-ownership.ts`, lines 29–35 and 64–68.Surfaced by two greps — a `'use server'`-without-`authedAction`sweep and `rg "requireRole('owner')"` — counted, with the fourlegitimate non-finding hits named.
## ConsequenceThe transfer goes through when the check cannot prove ownership…
## FixRemove the try/catch; let authedAction convert the throw…Every section populated, off a command. A precise rule, the exact lines, the grep that found them, the user-visible damage, and the fix — never a hunch.
Your mission
Section titled “Your mission”Every finding in this pass reuses one rhythm, and finding 1 is where you practice it: open the running app and the source side by side, walk one category to the end, and write the finding before you touch anything else. Switching categories mid-finding fragments the report into half-written stubs you can’t trust. The category here is fail-closed checks, the rule from the fail-closed lesson in chapter 80.
The audit is grep-driven, and that is the method to internalize: you don’t read every file hoping a defect surfaces, you run a command that names suspects, then read each hit.
Two greps land this finding — one for Server Actions that skip the canonical authedAction wrapper, one for requireRole('owner') so you can read the try/catch around each hit.
A finding names which commands ran, how many hits each returned, and which hits are not findings and why; a grep whose legitimate hits go untriaged is a finding you can’t trust.
Read the healthy seams first — authedAction, requireRole, safeLimit, logAudit — because a correct seam calibrates your eye: defects live in the call sites that bypass a seam, never in the seam itself.
Here requireRole is fine; the two places that wrap it in a swallowing try/catch are the defect, and transfer-ownership.ts carries the bug twice.
Two constraints on the write-up. The Consequence is read aloud at a launch review by someone who hasn’t seen the code, so name the user-visible failure mode in plain terms — an owner-only mutation slipping through when the role check can’t prove ownership during a database blip — with no “could potentially” hedging. And the fix is a paragraph, not a diff: you document the defect, you never patch the target.
src/lib/admin/transfer-ownership.ts is located with a line range and the grep command(s) that surfaced it, including the legitimate hits the command also returned and why they are not findings.requireRole('owner') throws during a Postgres blip — with the fail-open-dressed-as-logging operator note alongside it, and no “could potentially” hedging.try/catch, let requireRole throw, and let the outer authedAction wrapper convert the throw to { ok: false, error: { code: 'unauthorized' } }.findings/001-fail-closed.md has all four template sections filled and the audit target still runs unchanged.Coding time
Section titled “Coding time”Write findings/001-fail-closed.md now, against findings/template.md and the brief above — run the greps, read both call sites, fill the four sections — before you open the walkthrough below.
Attempting the work first is the rep that makes the next seven findings yours.
Reference solution and walkthrough
The defect, in source
Section titled “The defect, in source”Read this file the way the audit does: the seam it should lean on, and the try/catch that throws that guarantee away.
export const transferOwnershipAction = authedAction( 'admin', z.strictObject({ nextOwnerId: z.string().min(1) }), async ({ nextOwnerId }, ctx): Promise<Result<{ ok: true }>> => { try { await requireRole('owner'); } catch (error) { console.warn('[transfer-ownership] role check failed, continuing', error); }
await withTenant(ctx.orgId, async (tx) => { await tx .update(organization) .set({ ownerId: nextOwnerId }) .where(eq(organization.id, ctx.orgId));
await logAudit(tx, { organizationId: ctx.orgId, actorUserId: ctx.user.id, action: 'org.ownership-transferred', subjectType: 'organization', subjectId: ctx.orgId, payload: { nextOwnerId }, }); });
return ok({ ok: true }); },);The gate. await requireRole('owner') throws when the actor is not an owner, and a thrown access check is a refusal.
export const transferOwnershipAction = authedAction( 'admin', z.strictObject({ nextOwnerId: z.string().min(1) }), async ({ nextOwnerId }, ctx): Promise<Result<{ ok: true }>> => { try { await requireRole('owner'); } catch (error) { console.warn('[transfer-ownership] role check failed, continuing', error); }
await withTenant(ctx.orgId, async (tx) => { await tx .update(organization) .set({ ownerId: nextOwnerId }) .where(eq(organization.id, ctx.orgId));
await logAudit(tx, { organizationId: ctx.orgId, actorUserId: ctx.user.id, action: 'org.ownership-transferred', subjectType: 'organization', subjectId: ctx.orgId, payload: { nextOwnerId }, }); });
return ok({ ok: true }); },);The swallow. The catch logs the thrown refusal and lets control continue — fail-open dressed up as logging.
export const transferOwnershipAction = authedAction( 'admin', z.strictObject({ nextOwnerId: z.string().min(1) }), async ({ nextOwnerId }, ctx): Promise<Result<{ ok: true }>> => { try { await requireRole('owner'); } catch (error) { console.warn('[transfer-ownership] role check failed, continuing', error); }
await withTenant(ctx.orgId, async (tx) => { await tx .update(organization) .set({ ownerId: nextOwnerId }) .where(eq(organization.id, ctx.orgId));
await logAudit(tx, { organizationId: ctx.orgId, actorUserId: ctx.user.id, action: 'org.ownership-transferred', subjectType: 'organization', subjectId: ctx.orgId, payload: { nextOwnerId }, }); });
return ok({ ok: true }); },);The fall-through. The organization.ownerId update runs anyway, with no proof the actor is an owner.
The same shape repeats lower in the file, in a direct variant the admin console calls server-side without a Server Action at all.
export const transferOwnership = async ( orgId: string, nextOwnerId: string,): Promise<void> => { try { await requireRole('owner'); } catch (error) { console.warn('[transfer-ownership] role check failed, continuing', error); }
await db .update(organization) .set({ ownerId: nextOwnerId }) .where(eq(organization.id, orgId));};A plain async function — no authedAction boundary. The admin console calls this server-side, so the wrapped Server Action is not the only way in.
export const transferOwnership = async ( orgId: string, nextOwnerId: string,): Promise<void> => { try { await requireRole('owner'); } catch (error) { console.warn('[transfer-ownership] role check failed, continuing', error); }
await db .update(organization) .set({ ownerId: nextOwnerId }) .where(eq(organization.id, orgId));};The identical swallow, which is why one grep surfaces the defect at both entry points.
export const transferOwnership = async ( orgId: string, nextOwnerId: string,): Promise<void> => { try { await requireRole('owner'); } catch (error) { console.warn('[transfer-ownership] role check failed, continuing', error); }
await db .update(organization) .set({ ownerId: nextOwnerId }) .where(eq(organization.id, orgId));};The same fall-through. And with no wrapper here, even removing the catch leaves nothing to convert the throw — the gap the senior fix below closes.
A defect at one entry point is easy to miss when you only test the UI, which is why this audit is command-driven: the grep finds it at every entry point.
The seam, for contrast
Section titled “The seam, for contrast”Now read what the call sites throw away. requireRole is healthy, and its doc comment tells callers, in writing, not to catch it.
import 'server-only';
import { requireOrgUser } from '@/lib/auth';import type { Role } from '@/lib/auth/roles';import { roleAtLeast } from '@/lib/auth/roles';
// The fail-closed role gate. Resolves the request's { user, orgId, role } from the// validated session and THROWS when the actor's role is below `required`. A thrown// check is a refusal, never a pass (080 L1). Callers run it for its throw and let the// outer authedAction wrapper convert the throw into { ok: false, error } — they must// NOT swallow it in a try/catch (that is the fail-open anti-pattern seeded defect #1// plants in lib/admin/transfer-ownership.ts).export const requireRole = async ( required: Role,): Promise<{ user: Awaited<ReturnType<typeof requireOrgUser>>['user']; orgId: string; role: Role;}> => { const { user, orgId, role } = await requireOrgUser(); if (!roleAtLeast(role, required)) { throw new Error(`requireRole: ${required} required, actor is ${role}`); } return { user, orgId, role };};The authedAction wrapper around transferOwnershipAction catches that throw in one place and returns err('unauthorized', …), turning it into a { ok: false } Result instead of a 500 — the machinery the call site’s try/catch sabotages.
For the rule and the wrapper’s job, lean on the fail-closed lesson in chapter 80 and the seams catalog.
The finding
Section titled “The finding”This is the completed findings/001-fail-closed.md as it lands in the repo.
# Finding 001 — Fail-closed bypass on the ownership-transfer role check
**Category:** Fail-closed checks (error discipline).**Severity:** critical — an owner-only mutation runs when the gate cannot prove the actor is an owner, and it is reachable from a real admin Server Action, so an unauthorized ownership transfer is one thrown role check away.
## Rule
Any check that gates access fails closed: a thrown access check is a refusal, never a pass, and the action body never runs when the check threw (chapter 080, lesson 1 — Refuse by default; the canonical fail-open anti-pattern named there is `try { await requireRole(...) } catch { /* log and continue */ }`).
## Location
`src/lib/admin/transfer-ownership.ts`:
- `transferOwnershipAction` — the `try { await requireRole('owner') } catch (error) { console.warn(...) }` at lines 29–35, then the membership update at lines 37–51.- `transferOwnership` (the direct, non-action variant the admin console calls server-side) — the same swallowing `try/catch` at lines 64–68, then the update at lines 70–73.
How it surfaced — the audit method this finding sets for every later one: open the running admin surface, open the source, and let a command name the suspect. Two greps land it.
```# 1. Server Actions that do not route through the canonical wrapper.rg -l "'use server'" --glob '*.ts' src | xargs rg --files-without-match 'authedAction'# 2. The fail-open shape itself — a role check inside a try/catch.rg -n "requireRole\('owner'\)" src --glob '*.ts'```
Grep 1 returns four files, and all four are legitimate non-findings — recorded as such, not scored: `src/app/(auth)/sign-up/actions.ts` (the public account-creation path — gated by Better Auth's own `signUpEmail`, not by a role, by design), `src/app/(auth)/sign-in/actions.ts` (same — the pre-auth sign-in path), `src/app/(protected)/sign-out-action.ts` (sign-out needs only a session, no role gate), and `src/lib/billing/require-plan.ts` — a false positive: it matches only because a comment line contains the literal text `'use server'` (`import 'server-only'` — NOT 'use server'). It is not a Server Action at all; it is a `server-only` plan gate that throws a `BillingError`, fail-closed by design. The defect file is *not* among grep 1's hits, because `transfer-ownership.ts` correctly imports and routes through `authedAction` — a wrapper-bypass grep alone misses it. Naming the hits a command returns that are *not* findings is half the discipline — a finding is a defect named against a rule, never "this file looked unusual."
Grep 2 is what lands `transfer-ownership.ts` directly, because the defect lives *inside* a properly wrapped action. Reading both call sites confirms the `requireRole('owner')` throw is caught and discarded, and control falls through to the `organization.ownerId` update.
## Consequence
The ownership transfer goes through when the role check cannot prove the actor is an owner. A below-owner member who reaches the admin action, or any caller during a Postgres blip while the membership row is read, has their thrown refusal swallowed by the `catch`, and the next line reassigns the organization's owner. In user-visible terms: an account that should never have been allowed to transfer ownership transfers it, and the legitimate owner can be locked out of their own organization. The secondary, operator read is worse for being plausible — the code looks careful (it `console.warn`s the failure before continuing), so this reads as discipline when it is fail-open dressed up: logging a refusal and then proceeding is not logging, it is allowing.
## Fix
Remove the `try/catch` around `requireRole('owner')` at both call sites and let the throw propagate. `requireRole` is declared to throw on a below-owner actor and on its own internal failure (it reads the membership row and compares the role); the caller's job is to run it for its throw, not to interpret it. With the catch gone, the throw reaches the `authedAction` boundary that wraps `transferOwnershipAction`, which converts it to the refusal branch of the carried-in `Result` — mapped to the `unauthorized` code from the seven-code set — so the user gets a 403-shaped outcome and the action body never runs (chapter 080, lesson 1, the structural-shape section: the check throws on its own failure, the wrapper catches in one place and converts to a refusal).
```ts// The whole gate. No try, no catch, no fall-through.await requireRole('owner');await withTenant(ctx.orgId, async (tx) => { /* update + logAudit */ });```
The direct `transferOwnership` variant has no `authedAction` boundary; the senior reach is to delete its duplicated logic and route the admin console through the wrapped action so the one fail-closed seam is the only path, rather than leaving a second copy to drift. Either way the rule holds: when `requireRole` throws, nothing downstream runs. Do not re-introduce a re-throw inside a catch — the point is that the call site holds no error-handling machinery at all; the wrapper owns it.Three decisions worth pausing on.
The non-finding hits are most of the Location, by design. An auditor who lists only the hit they acted on gives you no way to check the pass — a thorough sweep and a lucky one look identical. Recording all four legitimate hits with their one-line reasons makes the command reproducible and the triage visible.
The Fix removes machinery from the call site, it does not add a re-throw.
The tempting wrong answer, catch (e) { throw e }, restores fail-closed behavior but keeps error handling where it does not belong.
authedAction’s closing catch is the one place that converts a thrown check into err('unauthorized', …).
The duplicated logic is real, but it is not this finding.
Duplication is a code-quality observation, not one of the eight audit categories, so it gets parked in findings/out-of-scope.md rather than scored as a second finding.
The canonical weakness ID for the exact defect you are naming — a swallowed refusal that falls back to the more permissive path. Cite it in the finding's Rule line.
The industry-standard category for a caught exception that continues anyway — useful for grounding the Severity justification a launch reviewer will accept.
Moment of truth
Section titled “Moment of truth”Run this lesson’s gate:
pnpm test:lesson 2The suite reads findings/001-fail-closed.md off disk and checks its shape: all four sections carry real content (not a leftover TODO), the Rule names “fail-closed” and cites chapter 80 lesson 1, the Severity picks one value, the Location names the target file and a grep command, and the Fix names the authedAction conversion without prescribing a re-throw.
It also checks that the seeded try { await requireRole('owner') } is still present in src/lib/admin/transfer-ownership.ts, so a passing gate proves you documented the defect rather than patched it.
A green run looks like this:
$ node scripts/test-lesson.mjs 2
RUN v4.1.8 …/projects/Chapter 082/solution
✓ tests/lessons/Lesson 2.test.ts (14 tests) 9ms
Test Files 1 passed (1) Tests 14 passed (14)The gate checks which words are present, not whether your reasoning holds. Confirm these by hand:
authedAction wrapper convert the throw, and explicitly warns against a re-throw inside the catch — the call site should hold no error handling.findings/out-of-scope.md as a code-quality note, not scored as a second finding.