The auth bypass

The starter runs, you have read the /plan surface in the browser, and reviews/chapter 104.md is sitting there with the pass-order header on top and a <!-- TODO(L2) --> marker where the first comment goes. Your goal for this lesson is narrow: write that first comment — the missing authedAction wrapper on the plan-label mutation — as a correctly-shaped, correctly-severed blocking: finding.

Finding 1 is doing more work than the four that follow it. It is the worked example whose cadence every later comment copies, and the lesson where you set the rhythm for the whole review. When a comment three lessons from now feels stuck, this is the one you come back and re-read. The finished deliverable is a single comment block under the pass-order header: it pins the defect to a file and a line range, names the rule it breaks with a lesson ID so the comment is portable to whoever wrote the code, and proposes the move — not the line-by-line patch, the move. You will see the block in Coding time; there is nothing to screenshot, because the deliverable is markdown.

Your mission

The first finding is the gift: you walk it end to end so the cadence is set before you tackle the rest on your own. Open src/app/(app)/plan/actions.ts and read the mutation against the canonical wrapper in src/lib/authed-action.ts. The reflex an experienced reviewer runs here is one question — what was the established surface, and where does this bypass it? The surface is authedAction(role, schema, fn): every privileged Server Action in this codebase routes through it, and it enforces a role gate, a parse-and-deny path, the tenant scope, and the rate limit in one place. updatePlanLabel doesn’t route through it. It carries a 'use server' directive and then a hand-rolled const session = await getSession() with an if (!session) throw, which means it runs no role check at all — any signed-in member can rewrite the org’s plan label — and it reaches past the tenantDb facade to mutate the store record directly, and it runs a write-side mutation with no rate limit. As a bonus tell, the if (!session) guard is dead code: getSession() returns a Session or throws, it never returns null, so that branch can never run.

That is one bypass dropping several guarantees the wrapper enforces — role, tenant scope, rate limit — which is why it lands as blocking: and not as a style preference. A lone style nit would be suggestion:; a write-side mutation any member can call against the wrong tenant scope is a security-and-correctness defect, and the severity has to say so. The comment you write addresses the code, not the author — “updatePlanLabel hand-rolls the session check” reads cleaner than “you hand-rolled the session check,” and the author reads it without bracing for a fight. And the principle/pattern line carries the load: naming the rule with a lesson ID is what makes the comment portable, where “this is wrong” alone is the failure mode the comment-anatomy lesson in chapter 103 calls out. Work the cadence in order — read the file, find the bypass, name the principle and the pattern, set the severity, write the comment in the template — and resist switching files mid-comment, which fragments the review into half-written stubs.

The blocking-versus-suggesting cut is in scope, and the wrapper bypass is the clearest blocker in the surface. Two things are not. The missing audit-log write lives in this same actions.ts file, but that is finding 5 — leave it for the next lesson; one finding, start to finish. And you do not edit the source. The fix you propose lives in the comment body, never in a diff; the audit target stays unchanged at the end of this lesson and every lesson after it.

The review file carries a comment pinned to the file and the line range of the hand-rolled session check in src/app/(app)/plan/actions.ts (L18-21).

untested

The comment carries a blocking: severity label, justified by the security-and-correctness consequence rather than preference.

untested

The observation names the bypass in code terms and the guarantees it drops: the role check and the tenant-facade scope (the dead if (!session) guard and the missing rate limit are fair extra tells).

untested

The comment cites SaaS pattern #2 (lesson 2 of chapter 57) and Principle #5 (chapter 29 / chapter 42) as the violated rule, with the lesson ID.

untested

The comment’s action proposes wrapping the mutation in authedAction('admin', updatePlanLabelSchema, fn) so the role, tenant, and rate-limit gaps close at once, in one sentence.

untested

The comment is phrased in the address-the-code-not-the-author voice.

untested

Coding time

Write comment 1 into reviews/chapter 104.md now — under the shipped pass-order header, in place of the <!-- TODO(L2) --> marker — against the brief above. Use reviews/template.md for the four-part shape. Read the walkthrough below only after your attempt; reading the reference before you write robs you of the rep that makes the next four findings yours.

Reference solution and walkthrough

The defect, in source

Here is the file the comment lands on. Read it the way the review does — the seam it should lean on, and the three places it doesn’t.

'use server';

import { updateTag } from 'next/cache';
import { orgPlanEntitlementTag } from '@/lib/cache/tags';
import { err, ok, type Result } from '@/lib/result';
import { getSession } from '@/server/session';
import { findOrganization } from '@/server/store';
import type { Organization } from '@/server/types';

// The plan-label mutation. It rolls its own session check by hand, reaches past
// the tenant facade to mutate the org record directly, runs no role check and no
// rate limit, and records nothing to the compliance trail. It compiles, runs, and
// rewrites the label against the in-memory store — working but wrong. The proposed
// fixes live in the review, never in this file.
export const updatePlanLabel = async (
  formData: FormData,
): Promise<Result<Organization>> => {
  const session = await getSession();
  if (!session) {
    throw new Error('Not signed in');
  }

  const planLabel = String(formData.get('planLabel') ?? '');
  if (planLabel.length === 0) {
    return err('validation', 'Plan label is required.');
  }

  const org = findOrganization(session.orgId);
  if (!org) {
    return err('not_found', 'Organization not found.');
  }

  org.planLabel = planLabel;

  updateTag(orgPlanEntitlementTag(session.orgId));
  return ok(org);
};

The hand-rolled gate. const session = await getSession() followed by if (!session) throw is the whole authorization story this action tells, and it is no story at all: it proves the caller is signed in, nothing more. No role check means any member, not just an admin, can rewrite the org’s plan label. And the guard is dead — getSession() returns a Session or throws, so !session is never true.

'use server';

import { updateTag } from 'next/cache';
import { orgPlanEntitlementTag } from '@/lib/cache/tags';
import { err, ok, type Result } from '@/lib/result';
import { getSession } from '@/server/session';
import { findOrganization } from '@/server/store';
import type { Organization } from '@/server/types';

// The plan-label mutation. It rolls its own session check by hand, reaches past
// the tenant facade to mutate the org record directly, runs no role check and no
// rate limit, and records nothing to the compliance trail. It compiles, runs, and
// rewrites the label against the in-memory store — working but wrong. The proposed
// fixes live in the review, never in this file.
export const updatePlanLabel = async (
  formData: FormData,
): Promise<Result<Organization>> => {
  const session = await getSession();
  if (!session) {
    throw new Error('Not signed in');
  }

  const planLabel = String(formData.get('planLabel') ?? '');
  if (planLabel.length === 0) {
    return err('validation', 'Plan label is required.');
  }

  const org = findOrganization(session.orgId);
  if (!org) {
    return err('not_found', 'Organization not found.');
  }

  org.planLabel = planLabel;

  updateTag(orgPlanEntitlementTag(session.orgId));
  return ok(org);
};

The unscoped write. org.planLabel = planLabel mutates the store record directly, reaching past the tenantDb(orgId) facade that exists so the org boundary is enforced in one place instead of re-derived at every call site. This is the dropped tenant scope.

'use server';

import { updateTag } from 'next/cache';
import { orgPlanEntitlementTag } from '@/lib/cache/tags';
import { err, ok, type Result } from '@/lib/result';
import { getSession } from '@/server/session';
import { findOrganization } from '@/server/store';
import type { Organization } from '@/server/types';

// The plan-label mutation. It rolls its own session check by hand, reaches past
// the tenant facade to mutate the org record directly, runs no role check and no
// rate limit, and records nothing to the compliance trail. It compiles, runs, and
// rewrites the label against the in-memory store — working but wrong. The proposed
// fixes live in the review, never in this file.
export const updatePlanLabel = async (
  formData: FormData,
): Promise<Result<Organization>> => {
  const session = await getSession();
  if (!session) {
    throw new Error('Not signed in');
  }

  const planLabel = String(formData.get('planLabel') ?? '');
  if (planLabel.length === 0) {
    return err('validation', 'Plan label is required.');
  }

  const org = findOrganization(session.orgId);
  if (!org) {
    return err('not_found', 'Organization not found.');
  }

  org.planLabel = planLabel;

  updateTag(orgPlanEntitlementTag(session.orgId));
  return ok(org);
};

The shape that should have been here. There is no authedAction wrap anywhere — no role gate, no parse-and-deny, no rate limit, no audit write (that last one is finding 5; leave it). One bypassed seam, several guarantees gone.

← Previous 1 / 1 Next →

The seam, for contrast

Now read what the action is throwing away. authedAction is the only privileged Server Action shape in this codebase: it resolves the session, checks the role, parses the input, and only then runs your function — and its body is wrapped in a try/catch that defaults to deny, so an unexpected error becomes a typed refusal instead of a 500.

export const authedAction =
  <TSchema extends z.ZodType, TOut>(
    role: Role,
    schema: TSchema,
    fn: (input: z.infer<TSchema>, ctx: AuthedCtx) => Promise<Result<TOut>>,
  ) =>
  async (
    _prev: Result<TOut> | null,
    formData: FormData,
  ): Promise<Result<TOut>> => {
    try {
      const session = await getSession();

      if (!roleAtLeast(session.role, role)) {
        return err('forbidden', 'You do not have permission to do this.');
      }

      const parsed = schema.safeParse(Object.fromEntries(formData));
      if (!parsed.success) {
        return err(
          'validation',
          'Check the highlighted fields.',
          z.flattenError(parsed.error).fieldErrors as Record<string, string[]>,
        );
      }

      return await fn(parsed.data, {
        session,
        orgId: session.orgId,
        userId: session.userId,
        role: session.role,
      });
    } catch {
      return err('internal', 'Something went wrong. Please try again.');
    }
  };

The roleAtLeast gate is what updatePlanLabel skips. The typed Result refusals are what its bare throw skips. The tenantDb-scoped write is what org.planLabel = planLabel skips. Every guarantee the action drops is a line you can point at right here — which is exactly why the fix is to route through this seam, not to bolt a role check onto the hand-rolled version. For the wrapper’s full job, the role taxonomy, and the tenant-scoped facade it reaches past, lean on the authedAction lesson in chapter 57 rather than re-deriving them here.

The comment

This is the completed comment block as it lands under the pass-order header in reviews/chapter 104.md.

**blocking:** `src/app/(app)/plan/actions.ts` L18-21 — `updatePlanLabel` hand-rolls `getSession()` with an `if (!session) throw`, so it accepts any signed-in user (no role check), drops the tenant scope on the org update, and runs a write-side mutation with no rate limit.
Principle/pattern: SaaS pattern #2 (lesson 2 of chapter 057 — `authedAction(role, schema, fn)`) and Principle #5 use-framework-conventions (chapter 029 / chapter 042).
Action: replace the manual auth and parse with `authedAction('admin', updatePlanLabelSchema, async (input, ctx) => { ... })`, which closes the role, tenant, and rate-limit gaps in one named seam.

A few decisions worth pausing on.

The action wraps in authedAction('admin', ...) rather than adding a role check by hand. The wrapper closes the role gate, the parse-and-deny path, the tenant scope, and the rate limit in one named seam. Adding only a role check to the hand-rolled version re-introduces the very thing Principle #5 warns against — a bespoke copy of a seam the codebase already owns, free to drift from it. Naming the seam is the fix; reaching for 'admin' rather than 'member' is the call that a plan-label change is an administrative mutation, not a member-level one.

Naming the dropped guarantees is what calibrates the severity. The observation does not say “this looks unsafe.” It says no role check, tenant scope dropped, no rate limit — three concrete guarantees, each one a thing the wrapper would enforce. That list is the difference between blocking: and suggestion:: a reviewer who can name what breaks has earned the label, and an author reading the comment can verify each claim against the seam. The dead if (!session) guard rides along as a tell — it is what tipped you off that the auth here was hand-built rather than borrowed — but it is not the reason the comment blocks. The missing guarantees are.

The Action: proposes the move, not the patch. A review comment names the seam and stops. authedAction('admin', updatePlanLabelSchema, async (input, ctx) => { ... }) tells the author exactly which wrapper, which role, and which schema — and leaves the body to them. Spelling out every line inside the arrow function is writing their code for them, which is not what a review comment is for. Name the move; the author writes the diff.

For the four-part comment anatomy, the five severity labels, and the address-the-code-not-the-author reflex, the comment lesson in chapter 103 owns them — this lesson applies them rather than re-deriving them.

Conventional Comments

conventionalcomments.org

The label-plus-decoration format this comment follows — blocking:, suggestion:, and the one-subject shape.

Google eng-practices — How to write code review comments

google.github.io

The address-the-code-not-the-author rule and severity labeling, from Google's reviewer guide.

Moment of truth

There is no automated checker for this chapter. lesson-verification/ ships empty, so there is no pnpm test:lesson to run — the deliverable is prose, and prose is hand-graded. Verify the comment against the four-part anatomy first: the block is pinned to a file and a line range, and it carries all four parts — a severity label, an observation, a Principle/pattern: line, and an Action: line. Any part missing and the comment is not portable.

Then hand-check the parts that take judgment, ticking each off as you confirm it:

The severity is blocking:, not suggestion:. Mis-labeling is the partial-credit miss the reference penalizes even when the finding is correctly located — a correct defect with the wrong severity still drops the severity half.

untested

The observation names the dropped guarantees — the role gate and the tenant scope — not just one of them. The dead if (!session) guard and the missing rate limit are fair extra tells, but the role and tenant gaps are the load-bearing ones.

untested

The cited rule is SaaS pattern #2 with the lesson ID, and the action proposes the authedAction('admin', ...) wrap rather than a hand-added role check.

untested

The comment addresses the code, not the author — “updatePlanLabel hand-rolls the session check,” never “you hand-rolled it.”

untested

The reference deliverable lives under solution/reviews/chapter 104.md, but you are on the honor system: don’t open it until the review and the ADR are both written in the ADR lesson at the end of this chapter. A real PR review has no answer key, and the reviewer who runs the pass under “no peeking” is the one who trains the reflex.