mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-19 09:15:19 +03:00
217e82a9a8
## What landed
**Email is the root of an account.** A `user` is created by verifying an
email
address and nothing else. Every Steam account is now a connection
hanging off a
user that already exists, capped at four.
**And it is the only thing that creates one.** Signing in with a gaming
account
or with an SSH key are both unwired from the issuer. Each could mint a
user,
which makes an account only as recoverable as the thing that made it and
gives
one person as many accounts as they have gaming logins. The providers
still
exist under `packages/auth/src/provider/` and can be wired back;
connecting a
Steam account is unaffected, because that runs through `POST
/steam/link` in
`apps/api` against a user who already exists. A test asserts the two
routes are
not served, so they cannot come back quietly.
**The pin-code provider is wired**, and email delivery refuses rather
than
guesses. **The device authorization grant is served**, and it now ends
at a
question somebody has to answer.
## The review found five real defects. All five are fixed, and so are
six more
The first pass of this branch shipped a device flow that handed out
tokens
without asking anybody, an email path whose pin could be guessed
outright, and
three read-then-write races. Each fix below has a test that fails
without it —
verified by reverting the fix and watching the test go red, not by
assertion.
### Signing in was mistaken for saying yes
`GET /device?user_code=…` started a provider flow and provider success
approved
the grant. So the whole attack was: ask for a device code, mail somebody
the
pre-filled link, keep the device code, poll. They see an ordinary
sign-in
prompt, complete it correctly, and you hold their access **and refresh**
tokens.
They were never asked a question, because there wasn't one.
There is now. Signing in establishes who the browser belongs to; it does
not
establish that the person meant to hand an account to a program running
somewhere else. The flow ends at a page that names the client, shows the
user
code back so it can be compared against what the device is displaying,
and
offers Approve and Deny. Approving is a POST carrying a value placed in
the
cookie alongside it, so another site cannot submit it on their behalf.
Denial moved onto that page too. It was a `GET` anybody could fire with
no
authentication: a link scanner or a chat unfurler would cancel real
sign-ins,
and anyone who learned a user code could grief one.
### A six-digit pin with unlimited guesses and a day to use them
The code travelled in an encrypted cookie held by the caller,
verification
compared against that cookie, and a wrong answer re-rendered the form.
Nobody
has to be the person the code was mailed to — type someone else's
address into
the first screen and the code goes to their mailbox while the cookie
stays with
you. At that point the only thing between a stranger and an account was
a
million requests. The constant-time comparison was guarding a door you
could
keep knocking on.
Guesses are counted on the server now, under a name that rotates with
every
code. The placement is the point: a counter kept beside the code, in the
cookie,
is one the guesser winds back by replaying an older copy. Starting over
is still
allowed and still costs a fresh code sent to the mailbox being aimed at,
where
somebody notices. The cookie's twenty-four hour life is ten minutes.
Resend is
spaced and bounded, because it was otherwise a way to mail a stranger as
fast as
requests go out. Both refusals say the same thing, since which one it
was is a
fact about someone else's mailbox.
The user codes on the other side of the flow are rate limited too —
eight
characters over a twenty-five character alphabet is a large space but a
fixed
one, and the endpoint had no opinion about how often you asked.
### Three read-then-write races
- **A poll could erase an approval.** The grant was read, modified and
written
back whole, so a poll that read a pending record and then wrote its
bookkeeping undid an approval that landed in between, leaving the client
polling a dead grant until it aged out. The same window let an approval
overwrite a denial.
- **The connection cap counted nothing.** `select … for update` over the
connections a user already had locks the rows it finds, and finding none
locks
nothing — there are no gap locks under read committed. Six concurrent
links
against a cap of four produced six; the test asserts that.
- **Concurrent email sign-ins returned a driver error.** Two tabs
finishing the
same sign-in both found no user, and the loser got a raw constraint
violation
instead of the account the winner had just made.
The cap now counts under a lock on the account's own row, which is the
one thing
every caller for that account contends on. The email paths let the
unique index
arbitrate and read back what the winner wrote. Device grants moved out
of the
key-value store into a table, where approving is one conditional update
and
redeeming is one delete that returns what it deleted.
### Three more the review did not raise
- **`client_id` was never checked at either end.** Anyone could mint a
grant
naming any client, and any holder of a leaked device code could redeem
it. It
is validated at issue and has to match at redemption.
- **Tokens were minted at approval** and left in storage until
collected, so the
lifetime reported to the client overstated what was left, and a grant
nobody
collected still left a usable refresh token lying around. They are
minted at
redemption.
- **The device code was stored as written.** It is the credential the
tokens are
handed to, so what is kept is now its hash: enough to recognise it, not
enough
to present it.
### And the environment check that decided none of this mattered
Mail delivery threw only when the environment said `production`, and
logged the
recipient and the live code otherwise. The deployment sets no such
marker — see
`alchemy.run.ts` before this change — so production took the developer
branch,
printed every code to a retained log, and reported success while nobody
received
anything.
That is the cost of a fail-open default: the deployment that forgets its
mail
settings is exactly the one with no marker saying it is real, so it gets
the
lenient branch precisely when it should not. Turned around. Printing a
code is
asked for by name; absence of configuration is a refusal; two settings
out of
three is an error rather than a fallback. Stages anyone else can reach
are
checked at deploy time, so a missing setting stops the deploy with the
name of
the variable it wanted.
## Where device grants live, and why it is a table
Short-lived state that would sit happily in a cache, in Postgres anyway.
The
reason is not durability. Every transition has to happen exactly once
while two
parties touch the same record — a browser somebody is clicking through
and a
program polling every few seconds — and a store that can only read and
write
whole records cannot promise that. The key-value store behind the rest
of the
issuer has no compare-and-swap, so on it the poll/approval race and
single
redemption can be narrowed and never closed.
The issuer cannot reach the database, so the store is an interface
(`packages/auth/src/device.ts`) with two implementations: one in memory
for
tests, one in `packages/core/src/auth/device-grant.ts`. Every method is
a single
operation and no caller reads a grant, decides, and writes it back.
Migration
`0010` adds the table. Rows are swept when a grant is created rather
than on a
schedule, since a grant lives ten minutes and that is the only statement
that
adds one.
## `session.claim_token` is here on another lane's behalf
Migration `0009` adds `session.claim_token`, nullable `text`, no default
and no
backfill. **It is not identity work and carries no identity reason** —
do not go
looking for one. It records which attempt holds a session run; the
endpoint that
reads and writes it arrives separately. It is in this migration only
because a
schema change has one owner at a time. The column is declared in
`session.sql.ts` and the snapshot, so the next `drizzle-kit generate`
will not
try to drop it — `0010` was generated clean, which is the proof.
## The tests
```
$ bun test
268 pass
0 fail
738 expect() calls
Ran 268 tests across 21 files.
```
Baseline on `dev` before any of this, measured on the same database:
**198 pass,
0 fail, 531 expect() calls.**
The tests that matter are the ones that would have caught the defects,
so each
was checked by putting the defect back:
| Reverted | What goes red |
|---|---|
| the confirmation step | signing in through the link leaves the grant
pending |
| the guess counter | four tests, including one that replays an older
cookie |
| the lock on the account row | six connections against a cap of four |
| the unique-violation handling | a driver error where a sentence should
be |
| the field-level poll write | an approval erased by a poll behind it |
| the verification rate limit | four tests |
Concurrency is exercised by running the same call several times at once
against
a real database, because run one at a time all of it passes whether or
not any
of the protection exists.
## The migration, against a database built to be awkward
`packages/core/script/verify-migration-0009.sh` builds a database
containing the
rows that make the statements do work — an account with no address, two
accounts
holding one address in different cases, an account already over the cap,
a
soft-deleted row holding a live row's address — applies everything
before `0009`,
applies it, and checks each case. All nineteen checks still pass. The
negative
control still dies where it should: with the de-duplication statement
neutered,
`create unique index` fails on the first duplicate pair.
## What this still does not verify
**1. No real email has ever been sent.** The mailer is tested against a
stubbed
`fetch`: the URL, the bearer header, the body it builds, and that it
refuses
when unconfigured. The body shape follows the common `{from, to,
subject, text}`
convention and may need a field the first real provider wants. This now
fails
closed and the deploy refuses without settings, so the failure mode is
loud
rather than silent — but it is still untested against a provider.
**2. The migration is verified against a database I built, not the one
that
matters.** I do not know whether production has duplicate addresses, how
many
rows carry case or whitespace, or whether any account is already over
the cap.
The fixtures cover those because they are *possible*. Worth three
`select`s
against production before this is applied.
**3. The verification rate limit is approximate.** The counter is in the
key-value store, so a caller spread across a distributed edge can exceed
the
budget somewhat. The number that decides the question is whether
somebody is
working through the code space, and a handful either way does not change
it.
**4. The single-redemption and conditional-transition properties are
held
against Postgres, and the in-memory store is trusted rather than
proved.** The
memory implementation gets its atomicity from nothing suspending inside
a
method, which is true of it and is not a promise the interface makes. It
is for
tests and local runs.
**5. A person who signed up by email carries an empty connected-account
id in
their token.** The subject schema requires the field and an account with
no
connection has nothing to put there, so it gets `''` — the same value a
server-to-server caller has always carried, and the one consumer already
falls
back to `''`. The honest shape is an optional field, and making it
optional
touches `subjects.ts`, the actor model and the API middleware. Flagging
rather
than doing.
**6. The desktop client cannot actually complete this flow yet.** Its
`DeviceCode` struct has no field for the device code, so it has nothing
to poll
with. That is a different lane's file and nothing here touches it, but
the grant
is not reachable end to end until it does.
**7. The four-account cap and the user-code alphabet are asserted, not
measured.** Four comes from a household's size and a screen's width. The
alphabet excludes look-alikes on the same reasoning. No measurement
decides
either, and no test here pretends one does.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR makes verified email the root account identity, turns Steam into
an attached account connection, adds provider-neutral email-code
delivery, and implements an RFC 8628 device authorization flow backed by
atomic PostgreSQL grant transitions. It also aligns the related
identity, session, and device-grant migrations and adds concurrency and
flow tests.
- Removes Steam and SSH as direct auth-worker sign-in providers.
- Adds email-code account creation and deployment-time mail
configuration checks.
- Adds explicit device approval, denial, polling throttling, client
binding, and one-time redemption.
- Serializes Steam-link cap enforcement and handles concurrent email
uniqueness conflicts.
- Adds and aligns migrations, snapshots, durable device-grant storage,
and `session.claim_token`.
- One non-blocking resend-limit replay issue remains.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge, with one non-blocking email-delivery abuse
limitation that should be hardened.
The prior device-token theft, polling race, Steam-link concurrency,
email uniqueness, and session-schema findings are fixed in the current
code; the five corresponding threads were manually resolved without
explanatory replies. The remaining new issue permits bypassing the
intended email send cap through replay of an older provider cookie, but
the resend interval still bounds its rate and it does not compromise
account authentication.
**Files Needing Attention:** packages/auth/src/provider/code.ts
<details open><summary><h3>Security Review</h3></summary>
The device flow now requires an explicit, CSRF-protected confirmation
and uses atomic, client-bound redemption. One lower-impact abuse issue
remains: replaying an older code-provider cookie can bypass the intended
email send cap.
</details>
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| packages/auth/src/issuer.ts | Adds an explicitly confirmed RFC 8628
device flow with client-bound, one-time token redemption. |
| packages/auth/src/provider/code.ts | Adds server-side attempt and send
accounting, but replacement flows leave earlier cookie-referenced
counters replayable. |
| packages/core/src/auth/device-grant.ts | Implements durable device
grants using atomic conditional approval, denial, polling updates, and
consumption. |
| packages/core/src/user/identity.ts | Makes email identity creation
conflict-aware and serializes Steam-link cap enforcement on the user
row. |
| apps/auth/src/index.ts | Reconfigures the deployed issuer around email
sign-in and PostgreSQL-backed desktop device authorization. |
| alchemy.run.ts | Requires complete mail configuration for permanent
stages and explicitly enables code logging only for ephemeral
development stages. |
| packages/core/migrations/0010_device_authorization_grant.sql | Adds
the device-grant enum, table, and unique indexes in alignment with the
Drizzle model and snapshot. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant D as Desktop client
participant A as Auth issuer
participant B as Browser
participant E as Email provider
participant DB as PostgreSQL
D->>A: POST /device/authorize
A->>DB: Create pending grant
A-->>D: device_code, user_code, interval
B->>A: Enter user_code
A->>E: Send email verification code
B->>A: Verify email code
A-->>B: Display client and user-code confirmation
B->>A: Approve or deny
A->>DB: Atomic terminal transition
loop Until terminal
D->>A: Poll /token
end
A->>DB: Delete-and-return approved grant
A-->>D: Access and refresh tokens
```
<details><summary>Prompt To Fix All With AI</summary>
`````markdown
### Issue 1
packages/auth/src/provider/code.ts:291-297
**Cookie replay bypasses send cap**
Replaying an earlier encrypted provider cookie bypasses `maxSends`. A resend creates a new flow with an incremented counter but leaves the old flow and its lower counter valid, so the old cookie can call `sendCode` again after each resend interval. This permits repeated unsolicited sign-in emails to an attacker-selected address, although the resend interval still limits their rate.
**How this was verified:** The resend check reads only the flow named by the presented cookie, while creating its replacement neither updates nor removes that old flow.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
`````
</details>
<sub>Reviews (3): Last reviewed commit: ["fix(auth): stop a caller
working
through..."](fc825f5219)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=60581000)</sub>
> Greptile also left **1 inline comment** on this PR.
<details><summary><h4>Context used (4)</h4></summary>
- Knowledge Base — [Authentication
platform](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/auth-platform.md)
- Knowledge Base — [Auth providers, sessions, and
storage](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/auth-providers-and-storage.md)
- Knowledge Base — [Core domain and
persistence](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-domain-data.md)
- Knowledge Base — [Users, identity, and game
libraries](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-identity-and-library.md)
</details>
<!-- /greptile_comment -->