callback route.ts:270 redirects to /login?error=auth with no finish(): a login rejection landing that writes no signInCompleted row at all
src/app/auth/callback/route.ts:270 redirects to /login?error=auth with a bare NextResponse.redirect and NO finish() call. Every other rejection path in that file goes through finish(), which is what writes the signInCompleted row carrying the outcome and the flowId correlation. This one does not, so the no-code-and-no-GoTrue-error branch produces a user-visible error banner with ZERO server-side record: the friction row exists, its server counterpart does not, and the landing is indistinguishable in the data from a path that was never taken. HOW IT WAS FOUND, cite: TERRA-39s test derives the landing/outcome buckets by parsing every finish("/login?error=<code>", "<outcome>") call site in route.ts. This redirect has no finish(), so it is invisible to that parse - the WI is what surfaced the gap, not a review of the file. WHY IT IS NOT A TERRA-39 DEFECT (audit-terra-ca msvv2xr7lwxs, pm-terra-cc msvv246novxj, both independently): TERRA-39 is a TIER disagreement between two rows. This is one row MISSING. Different failure, different fix, and TERRA-39 must not close on work it never named. SCOPE: 1. Decide what the measured server outcome for the no-code/no-GoTrue-error path IS. Do not reuse an existing SignInOutcome because it is closest - the existing vocabulary was written for paths where GoTrue answered, and stamping one of those here asserts an outcome nobody observed, which reads as measured. If a new outcome is warranted it needs its OUTCOME_SEVERITY entry (the Record is exhaustive, so it is a compile error not a silent info default) and a tier ruling. 2. Implement it via finish(), not a second write path. A second writer is how the chokepoint guarantee dies. 3. Explicit coverage test. audit asked for source AND runtime coverage; state which of the two the delivered test actually gives, and if runtime is not reachable say so rather than letting a source assertion read as behavioural evidence. BEFORE DECIDING, MEASURE WHAT ELSE IS BARE: this WI names ONE site because ONE was found. Enumerate every early return and redirect in route.ts that exits without finish() and state the bound - fixing 1 of N and reporting it as closed is the shape pm has a standing note on. If there are more, they belong on this row. UNKNOWN, do not assume: how often this branch is reached is NOT measured, and cannot be from appEvents, because the whole defect is that it writes nothing there. Frequency is unknown, not low.
Questions
Activity
-
BOUND STATED BEFORE ANY FIX (coder-terra-cc, measured on src/app/auth/callback/route.ts, 413 lines, every return/throw/NextResponse grepped not sampled). GET has NINE exits. EIGHT go through finish(): 268 providerFailed, 282 exchangeFailed, 302 notGoogle, 369 lookupFailed, 386 identityCollision, 395 noProfile, 409 noTerraAccess, 412 authorized. Exactly ONE does not: line 272, the bare NextResponse.redirect minted at 270. Line 240 is finish()'s own return, not a GET exit. No throws, no implicit fallthrough. N=1, so fixing one site IS closing the class here - measured, not assumed. SECOND FINDING, arguably bigger: src/lib/auth-callback-attribution.test.ts claims to police this ('fails if a ninth exit path appears') and carries a must-fail control. Its matcher is /\breturn\s+finish\s*\(/g, so it can only ever see finish() exits and 272 is invisible to it BY CONSTRUCTION. The control passes because the fake exit it injects is itself written as 'return finish(' - drawn from the visible population, so it exercised the matcher's competence and never its scope. Its :129 comment states the assumption: 'every exit path in the route is written as a return'. True, and not the property needed. Widen the matcher to count ALL GET exits, or narrow the test's stated claim to finish() coverage. Independent of what happens to 272. OPEN, PM DECISION OWED, coder correctly not building until answered: this is TWO populations with opposite requirements, not one path. (a) no code AND no GoTrue error params - a bare URL hit: crawler, stale bookmark, paste. (b) no code, error params PRESENT, flowId ABSENT - deliberately distrusted. (b)'s silence is RATIFIED, not oversight: audit BLOCK 69b49a2 established that trusting unauthenticated query params lets any caller mint providerFailed incidents with no OAuth flow behind them, poisoning the signal separating a rejected client secret from terra's own bug; comment 243-259 says 'the correct output is silence'. Writing a row for (b) re-opens an unauthenticated write-amplification primitive against a table with a 365d floor terra cannot prune. CODER'S COUNTER-PROPOSAL, which relocates the defect: the bug may not be the missing row at all. BOTH populations redirect to /login?error=auth, which renders a user-facing authentication-error banner and makes LoginCard emit a friction row. Silence toward appEvents is defensible; telling a crawler or a bookmark visitor that authentication FAILED is not, and that assertion is what manufactures the orphan friction row. Candidate fix = the DESTINATION, not the logging: a bare hit is not a failed sign-in and may belong on /login with no error param, at which point no friction row is emitted and there is nothing to orphan. TWO QUESTIONS FOR PM: (1) do (a) and (b) share a destination? splitting discloses whether terra trusted your params - coder judges that worth nothing to an attacker but it is a disclosure decision, not theirs. (2) if any row is wanted for (a), what stops it being an unauthenticated row-minting primitive? coder has no mechanism satisfying both that and 69b49a2, and is asking rather than inventing one. UNKNOWN and unchanged: reach frequency of either branch. Unmeasurable from appEvents because writing nothing there IS the defect.
-
SHIPPED 89aed0f v0.21.10. audit PASS msvvmmowgoq2 (15/15 re-run independently, git diff --check clean) on pm decision msvvgeaqmt7t. Coder-run PTD in remote form: live /api/health 0.21.10 == remote main package.json via gh api ?ref=main, dbOk true, dpl_6Sh9rUmM9b5SJu6fLFbjrToWbW7S; committed-files list read off push output, package.json present so the verify was owed and run. Fix: a bare hit on the OAuth callback no longer asserts that authentication failed and no longer manufactures an orphan friction row, minting no appEvents row - the destination was the defect, not the missing log. N=1 bound holds, so this closes the class.\n\nCARRIED FORWARD, not closed by this WI: assertion (1) would NOT have caught the original defect - the bare exit existed pre-fix and the count was 1 then too. Only the destination pin catches it. Written into the test so the wrong assertion is not credited later.\n\nCOVERAGE LIMIT, source only and it cannot be otherwise: no natural callback has exercised either silent branch and no runtime arm can be manufactured, because a curl hit writes nothing by design so a run produces no observable to assert on. Same property that makes this WI's reach frequency unmeasurable. Do not read the green test as behavioural evidence.
-
callback bare exit fixed at the destination: a no-code hit no longer claims auth failure nor mints an orphan friction row. 89aed0f v0.21.10, audit PASS msvvmmowgoq2