Finding 1: the fail-closed bypass
The starter runs, you are signed in as the admin, and the findings/ directory is sitting there with eight empty skeletons. Time to fill the first one. Your goal for this lesson is narrow and concrete: document the fail-closed violation that lives in src/lib/admin/transfer-ownership.ts as findings/001-fail-closed.md, with all four template sections filled.
But finding 1 is doing more work than the seven that follow it. It is the gift finding — the worked example whose shape every later finding copies, and the lesson where you set the audit method for the whole pass. When a finding three lessons from now feels stuck, this is the one you come back and re-read. The finished deliverable is a single Markdown file that names a precise rule, points at the exact lines that break it, spells out the user-visible damage, and proposes the fix — and it does all of that off a command you can re-run, never a hunch.
The deliverable is a single Markdown file, and the whole lesson is the move from its left state to its right state — from a skeleton with four empty headers to a finding that names a rule, points at lines, and proposes a fix. Switch between the two tabs below to see the transformation you are about to perform.
# 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”Here is the rhythm every finding in this pass reuses, and finding 1 is where you practice it: open the running app, open the source, and hold the two side-by-side. Then walk one category to the end and write the finding before you touch anything else. Switching categories mid-finding is how reports fragment — you end up with eight half-written stubs and no confidence that any of them is complete. One finding, start to finish, then move on. The category for this lesson is fail-closed checks, the rule from the fail-closed lesson in chapter 80.
The audit is grep-driven, and that is the part to internalize as method, not as the answer. You do not read every file hoping a defect jumps out. You run a command that names suspects, then read each hit. Two greps land this finding, and they are not redundant: one for Server Actions that skip the canonical authedAction wrapper, and one for the requireRole('owner') call so you can read the try/catch around each hit. The first grep’s hits all turn out legitimate by design — the public sign-up and sign-in actions handle authorization differently because there is no logged-in actor yet, sign-out needs only a session, and one more is pure grep noise. The defect is not among them, because it lives inside a properly wrapped action; the second grep is what lands it. A finding names which commands ran, how many hits each returned, and which of those hits are not findings and why. Naming the non-findings is half the discipline; a command that returns hits and a finding that quietly triages only some of them is a finding you cannot trust.
The target ships every canonical seam healthy — authedAction, requireRole, safeLimit, logAudit, tenantDb, the typed env. Read those first. Not because they are broken, but because reading a correct seam calibrates your eye: findings live in the call sites that bypass a seam, never in the seam itself. requireRole is fine. The two places that wrap it in a swallowing try/catch are the defect — and note that transfer-ownership.ts carries the bug twice, in the wrapped Server Action and in a direct variant, which is exactly why the grep surfaces it no matter which entry point you came in through.
The trap to stay clear of is the “code review opinion” — a finding that says a file looks off with no command behind it. Every finding’s Location names the command that surfaced it. And one more constraint on the Consequence section, because it is the section inexperienced auditors get wrong: it is read aloud at a launch review by someone who has not read the code. So it has to 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 the operator failure mode (a console.warn-then-continue that is fail-open dressed up as logging) as the secondary note. No “could potentially” hedging; the defect is real, describe it as real.
Two things are out of scope. You do not patch the target — the fix is a paragraph, not a diff, and the target stays green and unchanged at the end of this lesson and every lesson after it. And the finding follows findings/template.md exactly: Category, Severity, then ## Rule, ## Location, ## Consequence, ## Fix.
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. The worked finding below is the reference shape; reading it before you attempt the work robs you of the rep that makes the next seven findings yours.
Reference solution and walkthrough
The defect, in source
Section titled “The defect, in source”Here is the file the audit lands on. Read it the way the audit does — the seam it should lean on, and the try/catch that throws the seam’s 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') is supposed to be the whole story — it 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. A console.warn-then-continue is 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, because control reached it — the owner-only mutation goes through 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 going through 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 around it. 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. The same try { await requireRole('owner') } catch { console.warn(...) } shape, 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 update. And because there is no wrapper here, even removing the catch leaves nothing to convert the throw — that gap is the seam of the senior fix below.
The two-variant design is the callout: a defect that lives at one entry point is easy to miss if you only test the UI. The grep finds it at every entry point, which is the whole reason the audit is command-driven rather than click-driven.
The seam, for contrast
Section titled “The seam, for contrast”Now read what the call sites are throwing away. requireRole is healthy — and its own 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 seam throws by design. The authedAction wrapper that surrounds transferOwnershipAction is built to catch that throw in one place and turn it into a typed refusal — its closing catch returns err('unauthorized', …) exactly so a thrown check becomes a { ok: false } Result instead of a 500. That is the machinery the try/catch in the call site sabotages. For the rule itself and the wrapper’s job, lean on the fail-closed lesson in chapter 80 and the seams catalog rather than re-deriving them here.
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.A few decisions worth pausing on.
The Location is the load-bearing section, and the non-finding hits are most of it. Grep 1 returns four files and all four are fine — the defect is not even among them; the finding records each by name, with the one-line reason each is legitimate (including the comment false-positive), and scores none of them. That is not padding. An auditor who lists only the hit they acted on has given you no way to check their work — you cannot tell a thorough pass from a lucky one, and you cannot tell that they knew to reach for the second grep. The discipline is to make the command reproducible and the triage visible: anyone can re-run the grep and confirm the same four files, then read the same four reasons.
The Fix names the conversion, not a re-throw. The seam already owns error handling: authedAction’s closing catch turns a thrown check into err('unauthorized', …). So the fix is to remove machinery from the call site, not add it. The tempting wrong answer is catch (e) { throw e } — re-throwing inside the catch — which technically restores fail-closed behavior but misses the point that the call site should hold no error-handling at all. The wrapper is the one place that converts; leave it that one place.
The duplicated logic is real but it is not this finding. transfer-ownership.ts ships the transfer flow twice, and that duplication is a maintainability problem — but duplication is a code-quality observation, not one of the eight audit categories. It does not belong in finding 1, and it does not get its own scored finding either. Park it in findings/out-of-scope.md so the next refactor ticket has a home and the finding count stays honest: one fail-closed defect, one finding, plus one parked note.
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 the observable shape of your finding — that all four sections carry real content (a leftover TODO comment does not count), that the Rule names “fail-closed” and cites chapter 80 lesson 1, that the Severity line picks one value rather than leaving the critical | high | medium | low menu, that the Location names the target file and a grep command, and that the Fix names the authedAction conversion without prescribing a re-throw inside a catch. It also probes the audit target itself: the seeded try { await requireRole('owner') } must still be present in src/lib/admin/transfer-ownership.ts. The target is read-only, 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 test asserts the file’s shape; it cannot read prose for quality. 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.