fix(auth): harden device-auth poll (atomic consume + blocked re-check)#4328
fix(auth): harden device-auth poll (atomic consume + blocked re-check)#4328markijbema wants to merge 2 commits into
Conversation
pollDeviceAuthRequest read the approved request, minted a token, then issued an unconditional 'expired' update — so two concurrent polls could each mint a long-lived API token from one approval. Replace it with a guarded compare-and-swap that flips 'approved' -> 'expired' and returns the row only to the winning caller; losers normalize to 'expired'. The token is minted only after the row is consumed, so at most one token is issued per approval.
…(Finding 2) Authorization was only checked when the user approved the device request; the later poll minted a ~5-year API token without re-checking. A user could approve while in good standing, get blocked, then poll and receive a fresh token still accepted by pepper-only downstream services. Re-validate the user at mint time (blocked_reason or isUserBlacklistedByDomain) and return 'denied' instead of minting. The approval is already consumed, so a blocked user cannot retry the code into a token.
| // user approves the request, but they may have been blocked or blacklisted | ||
| // between approval and this poll. The approval is already consumed above, so | ||
| // a now-blocked user cannot retry this code into a fresh long-lived token. | ||
| if (user.blocked_reason || (await isUserBlacklistedByDomain(user))) { |
There was a problem hiding this comment.
Low, Security: When this re-check trips, the function returns { status: 'denied' } silently. The canonical auth path (validateUserAuthorization / authError in lib/user/server.ts:1077) emits an AUTH-FAIL 403 warn line for blocked and blacklisted users, so those denials are visible in backend logs. This branch has no equivalent, so a blocked or blacklisted user who approved before being blocked and then polls (exactly the abuse case this branch defends against) leaves no server-side trace.
This is backend observability only; it does not change the response the device receives. Suggest logging before the return, matching the existing AUTH-FAIL format and distinguishing the two reasons, e.g.:
if (user.blocked_reason || (await isUserBlacklistedByDomain(user))) {
const reason = user.blocked_reason ? 'blocked' : 'blacklisted-domain';
console.warn(`AUTH-FAIL 403 (${kiloUserId}): device-auth poll denied (${reason})`);
return { status: 'denied' };
}
| // long-lived token from one approval. | ||
| const consumed = await db | ||
| .update(device_auth_requests) | ||
| .set({ status: 'expired' }) |
There was a problem hiding this comment.
Nit, Functional: For the blocked/blacklisted case this row is set to expired, but pollDeviceAuthRequest returns denied (line 211), which the route maps to 403 while a consumed or race-lost code returns expired / 410. Two things worth a look:
- The persisted row state (
expired) does not match the response (denied), which can be confusing when debugging from the database. - More substantively, that distinct 403-vs-410 lets a polling client tell "you were blocked" apart from "code is gone." If you'd rather not signal the block to the device, returning
expiredhere (still consumed, still terminal) would make the blocked case indistinguishable from a normal consumed code while the backend log from the comment above preserves the real reason for operators.
Not blocking; the current behavior is intentional per the description.
2 participants
Summary
Follow-up to #4314 (block rotates pepper). That PR was Workstream B; this is Workstream A — server-side hardening of the device-authorization poll path in
apps/web/src/lib/device-auth/device-auth.ts. It closes two findings from the device-auth review:pollDeviceAuthRequestread the approved request, minted a token, then issued an unconditionalstatus = 'expired'update. Two concurrent polls could each pass the read and each mint a long-lived API token from one approval. Replaced with a guarded compare-and-swap (UPDATE ... SET status='expired' WHERE code = ? AND status='approved' RETURNING kilo_user_id); the token is minted only by the caller that won the swap, and losers normalize toexpired.blocked_reasonandisUserBlacklistedByDomain, returningdeniedinstead of minting. Because the approval is consumed first, a blocked user can't retry the code into a token.No schema change, no migration. The poll API route already maps
denied-> 403 andexpired-> 410, so no route change was needed.Verification
apps/web/src/lib/device-auth/device-auth.test.ts(24 tests pass), including two new cases:expired);deniedwith no token, and a retry returnsexpired.ifas the blocked-reason check; I did not add a dedicated test for it becausegetBlacklistedDomainsis Redis-cached (60s) and not reliably seedable in a unit test.Visual Changes
N/A
Reviewer Notes
deniedwhile the row is set toexpiredby the consume; both are terminal/non-retryable, anddenied(403) is the more informative signal to the polling device.