mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-19 17:25:19 +03:00
04210cc9f10be5c119863790876ce30534d56f13
445 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Wanjohi
|
04210cc9f1 |
refactor(auth): describe sign-in screens as data, draw them in one place (#339)
Providers stop returning `Response`. Each says what it needs from the
person — an address, a pin, a yes-or-no — as a `Screen`, and one
`Renderer` draws it.
The old shape made every provider a small web framework: it had to know
about markup, about the stylesheet's attribute names, about how a page
is assembled. So each grew its own callback signature, its own copy, and
its own `new Response(jsx.toString())` — and there was no shared
vocabulary left to style.
### What that had already cost
- **The device flow never got a design.** Its two pages were built by
concatenating HTML strings, with an inline `style` on the user code. Six
more replies were `text/plain` — including the one a person gets when
their sign-in cookie expires.
- **The password screens were already broken.** They render
`data-component="input"`, `data-component="link"`,
`data-component="form-footer"` against a stylesheet that was rewritten
for the code flow. Nobody noticed because password sign-in is not
switched on. They are deleted here, not repaired.
- **A provider could not be named or marked without editing the
library.** `DiscordProvider` has existed all along; wiring it up
rendered **"Continue with discord"**, lowercase, no icon, because the
marks and display names were two hardcoded `const` records inside the
code that drew the chooser.
### What changed
`ui/screen.ts` — four screen kinds (`choose`, `form`, `confirm`,
`message`) and a field vocabulary, as plain data. No JSX, no hono.
`ui/render.tsx` — the only file that knows what a button looks like.
`Renderer` is one method, so replacing the presentation layer wholesale
means implementing that and nothing else.
`Provider.display` — each provider carries its own name and mark
(`ui/mark.ts`, raw SVG strings so `provider/*.ts` never imports a
rendering library). The chooser is built from what providers declare;
`issuer({ chooser })` is left with only the two decisions a deployment
makes that a provider cannot — whether to offer it, and what to put
first.
The theme global is gone. It was `globalThis`, with a comment conceding
as much: every component depended on something invisible at the call
site, untestable in isolation, and shared mutable state on a runtime
that keeps one module instance across requests. It is a closure argument
now, and `Theme` shrinks to the values a deployment sets that the
stylesheet cannot.
`kind: 'segments'` is named by meaning rather than widget — a code read
off one screen and typed into another. The emailed pin and the device
user code are the same field now; they used to describe it separately,
in different files.
### Adding things, after
```ts
providers: {
code: CodeProvider({ ... }),
discord: DiscordProvider({ clientID, clientSecret }), // icon and name included
password: PasswordProvider(PasswordUI({ ... })), // in the design, because it has none of its own
}
```
Neither touches CSS. Neither touches the renderer.
### Notes
- **No Tailwind.** I floated it and then dropped it: `packages/auth`
deploys straight from `src/` both ways (`wrangler.jsonc` points `main`
at `src/index.ts`; `server.ts` runs the same handler under Bun), so
adding a CSS toolchain would fight both. The stringly-typed
`data-component` problem is solved by the typed components instead —
nobody adding a screen writes one. The stylesheet grew ~95 lines for the
new primitives and that is the last CSS this change needs.
- **Design tokens stay independent** of the website rather than shared,
since auth is a separate Worker on a separate hostname with its own
release cadence. The brand values are copied, with a comment naming the
site as the source. Easy to reverse if you'd rather couple them.
- `src/provider/oauth2.ts` has a pre-existing `TS2578: Unused
'@ts-expect-error'` on `dev`. Untouched — verified it fails the same way
without this branch.
### Verification
- `packages/auth`: 66/66 pass. The 27 device tests are **unmodified**
and still pass, which is the behaviour argument — statuses, cookies and
the confirmation step are unchanged.
- `apps/auth`: 23/23 pass.
- `tsc --noEmit` clean on both, apart from the pre-existing error above.
- Every screen rendered and eyeballed in a browser.
Net −757 lines.
|
||
|
Wanjohi
|
cfb8ec26a0 |
feat(machine): mint a public name, and stop routing on the id
A host's hostname was its primary key. That worked and disclosed three things it should not have: ids here are monotonic, so an id in a hostname tells anyone who reads a URL roughly when that machine was registered and where it falls among its owner's others; an id is the primary key, so a name that had to change could only change by re-registering the machine, which is changing its identity to fix its name; and the hostname is also the OAuth audience and the cookie scope, so the id travelled into redirect URLs and browser history. Machines now carry a minted name -- two words and four digits, unique across the fleet, DNS-safe by construction, which an id was not. The words are a curated list rather than a dictionary, because every pair is shown to strangers. Names the fleet's own infrastructure answers on are refused at mint time: one minted onto the edge's own label would take the published key path away from every host at once. Minting retries on the unique index rather than checking first, because two registrations in the same instant both read "free" and both write. Only a name collision retries; a duplicate id or secret means something a new name cannot fix. The migration adds the column in three steps. Generated as a NOT NULL column it fails outright against a populated table, and a default would be worse: every row would share one value on a routing key. |
||
|
Wanjohi
|
eedb143b46 |
refactor(auth): describe sign-in screens as data, draw them in one place
Providers no longer return a `Response`. Each one says what it needs from the person — an address, a pin, a yes-or-no — as a `Screen`, and a single `Renderer` decides how that is drawn. The old arrangement made every provider a small web framework. It had to know about markup, about the stylesheet's attribute names, about how a page is assembled, so each grew its own callback signature and its own copy of `new Response(jsx.toString())`. Three consequences, all of them visible in the tree before this change: - The device flow never got a design at all. Its two pages were built by concatenating HTML strings, with an inline `style` on the user code, and six of its replies were `text/plain` — unstyled black-on-white in the middle of signing in, which is also what a person got when their sign-in cookie expired. - The password screens were drifting. They were written against attribute names the stylesheet no longer had, and nobody noticed because password sign-in is not switched on. They are deleted here rather than repaired; the flow is now six screen descriptions and no markup. - A provider could not be named or marked without editing the library. The brand marks and display names were two hardcoded records inside the code that drew the chooser, so anything missing from them rendered as its own lowercase identifier with no icon. Providers now declare `display` themselves, and the chooser is built from what they say. Also removes the theme global. It was `globalThis`, with a comment conceding as much, which made every component depend on something invisible at the call site — untestable in isolation, and shared mutable state on a runtime that keeps one module instance across requests. The theme is now a closure argument, and the same change shrinks `Theme` to the handful of values a deployment sets that the stylesheet cannot. Adding a screen now touches no CSS, and swapping the presentation layer means implementing one method. The code flow's tests demonstrate the second: they render screens as JSON. Behaviour is unchanged. Status codes, cookies and the confirmation step are the same, which the device tests cover unmodified. |
||
|
Wanjohi
|
aecae0c69e |
style(auth): tighten the sign-in field, and stop calling the runtime a worker
The email field sat a little taller than the action below it. Two comments also described this page as rendering inside a worker, which is not how the control plane runs; what they were reaching for is that nothing preprocesses this file, so the design tokens are written out longhand. |
||
|
Wanjohi
|
74391714d0 |
feat(auth): draw the sign-in screen in the product's design language
The screen was still the upstream template's: its font, its accent, its logo, and a theme that tried to serve a light and a dark scheme from one set of colours by deriving each one from the background's lightness. That derivation is replaced with stated values, and the page is dark only. Black, a neutral grey ramp, one brand accent, Mona Sans for the display line and Geist for anything read or typed; two dashed bands closing into a box on a wide screen, a dashed vertical either side of the column, the wordmark, and the line of copy the rest of the product opens with. The email field keeps the fix from the previous commit and takes the brand colour on focus, which is the only place it appears besides the wordmark. Measured against a render of the same design built from its own source: the band rules, the column rules, the wordmark box and the button height land on identical pixels. |
||
|
Wanjohi
|
4ed36e0d38 |
fix(auth): give the sign-in input a text colour, and restore the theme
The email field computed its own background one step lighter than the page and never set `color`. Form controls do not inherit it, so the text someone typed was the UA default — black, over a near-black field. There was no `color-scheme` either, so the browser rendered the control in light appearance to begin with. The theme was a second, separate loss: `issuer()` still takes one and calls `setTheme`, but nothing had passed one since `packages/auth` became a vendored fork, so every sign-in rendered as OpenAuth — its font, its periwinkle, its logo. Restored from the pre-fork config, with the logo and favicon repointed because both URLs it carried now 404 and a broken `logo` renders a broken image rather than falling back. |
||
|
Wanjohi
|
7859c8e13a |
ci: restart the issuer before the API
Units come back in the order their artefacts appear in the manifest, and the API reaches the issuer over AUTH_INTERNAL_URL -- so api-then-auth means the API spends a moment talking to a service that is restarting. |
||
|
Wanjohi
|
4668c98785 |
ci: a merge to prod releases the control plane, migrator included
`release-prod.yml` for api and auth, mirroring the edge's. The gate is not "you may not merge" — it is that merging does not deploy: tests run against a real Postgres, both binaries are executed, and a failure anywhere means no release exists, so the machine keeps serving what it has. The new piece is `nestri-migrate`, because the deploy runs migrations *before* it swaps a release into place and had nothing to run. `drizzle-kit migrate` reads the migrations folder at runtime, which is right on a laptop and wrong on a server: the deploy ships flat, checksummed files into bin/, and a migrator that needs a directory beside it can be pointed at the wrong directory. So the folder is baked into the binary — generated on every build rather than committed, so it cannot drift — and the artefact's checksum then covers every statement it will run. It reimplements drizzle's bookkeeping in thirty lines of SQL rather than calling into `db.dialect.migrate`: same table, same schema, same sha256 over the whole file, same high-water-mark comparison. That equivalence is the one thing that must not rot, because the production database was first migrated by drizzle-kit and a disagreement means a migration applied twice. Two checks hold it: CI applies the migrations with drizzle-kit and then asserts the embedded set reports nothing pending, and the hashes were verified by hand against the live database — all fourteen match to the byte. Proven before shipping, against the real database: nothing pending on the deployed schema, 14 unchanged rows, and a scratch database migrated from empty to the same 22 tables and the same high-water mark, idempotent on a second run. Refuses with exit 2 when DATABASE_URL is absent rather than defaulting to localhost, which would be a migrator reporting success having migrated nothing. setup-bun is pinned to a commit and not to `v2`. A moving major tag is fine everywhere else in this repository; this workflow is the only thing between a merge and a process serving users, and there is no first-party bun action to prefer instead. |
||
|
Wanjohi
|
b00f1064ae |
fix(api,auth)!: bind loopback by default, and let a container ask for more
Both servers bound `0.0.0.0`. That was harmless while the only deployment was docker-compose.yml, which publishes these ports on 127.0.0.1 and makes the container's own bind irrelevant. As ordinary processes on a rented machine there is no such wrapper, and `0.0.0.0` is a listener on the internet — in front of an issuer that sets cookies without `Secure` and mints sign-in codes, because it expects something else to be terminating TLS. So the default is `127.0.0.1` and `HOST` is there for the deployment that genuinely needs every interface. compose now sets `HOST: 0.0.0.0` explicitly, which is not a workaround: inside a container, binding loopback is what would make the published port unreachable. The right answer differs by deployment, which is why it is a variable rather than a constant. Both now log the address they bound, not the one they hoped for. BREAKING CHANGE: api and auth no longer listen on every interface by default. A deployment that relied on that must set HOST=0.0.0.0. |
||
|
Wanjohi
|
e3c9416170 |
docs(dns): the origin is a tunnel, and there is nothing to cut over from
`After the move off Workers` described proxied `A` records to the host and an ordering to avoid a gap while Worker routes were removed. Both are wrong. There is no `A` record because there is no address to publish: each name is a proxied CNAME to the tunnel, and the machine's address appears nowhere. And there is no ordering problem because `api.nestri.io` and `auth.nestri.io` have no DNS records and do not resolve — only the sandbox pair was ever deployed, so these names are created for the first time with no window to protect. The `nestri.link` section promised an origin certificate as the one key on our host. There is no key on our host at all. Measured 2026-09-16: a public HTTPS request served correctly while nothing listened on 443 or 8443 and both origins spoke plain HTTP on loopback; the certificate was Cloudflare's, covering the apex and one level, and a two-label name was refused at the handshake with no certificate presented. |
||
Kristian Ollikainen
|
15ad60bf49 |
feat: nescapture pacing, improvements and deps update (#334)
Make nescapture better with proper queue family checks, FPS limiting,
semaphore usage and other.. also updated deps like pollster and
pixelforge.
<!-- greptile_comment -->
<!-- greptile_summary -->
<h2><a
href="https://app.greptile.com/api/retrigger?id=64083904"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=1"><source
media="(prefers-color-scheme: light)"
srcset="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=1"><img
alt="Retrigger"
src="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=1"
align="right"></picture></a>Confidence Score: 5/5</h2>
The final review contains no accepted findings, so the PR appears safe
to merge.
<h3>Summary</h3>
- This PR reworks `nescapture` frame capture around a four-slot DMA-BUF
ring, tracks presentation queue families, adds semaphore-based
capture/present ordering, introduces capture frame-rate pacing, carries
presentation timestamps through encoding, and updates Vulkan-related
dependencies.
<sub>Reviews (1) · Last reviewed commit: ["feat: Update deps and remove
deprecated
..."](
|
||
|
Kristian Ollikainen
|
8246aa5538 |
feat: resident guest init (#333)
Get this thing going..
<!-- greptile_comment -->
<!-- greptile_summary -->
<h2><a
href="https://app.greptile.com/api/retrigger?id=63134761"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=1"><source
media="(prefers-color-scheme: light)"
srcset="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=1"><img
alt="Retrigger"
src="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=1"
align="right"></picture></a>Confidence Score: 5/5</h2>
The PR appears safe to merge; all previous findings are resolved and the
latest readiness change introduces no established actionable regression.
<h3>Summary</h3>
- Establishes required guest filesystems, runtime directories, device
permissions, and service processes.
- Reports initialization and service deaths over the lifecycle channel.
- Supports launch, restart, and shutdown commands for a resident guest.
- Separates service and workload identities and configures per-launch
runtime environments.
- Removes the currently inactive nescope screenshot option and makes
capture-chain verification fail explicitly when compositor readback is
unavailable.
- Reworks the guest image around `nesinit` as PID 1 without a
distribution service manager.
<h3>Diagram</h3>
```mermaid
sequenceDiagram
participant Host
participant Init as nesinit
participant FS as Guest filesystems
participant Services as Service stack
participant Workload
Init->>Host: Ready(protocol version)
Host->>Init: Boot(mount descriptors)
Init->>FS: Establish and mount shares
Init->>Services: Spawn services in order
Services-->>Init: Required sockets ready
Init->>Host: Initialized(service names)
Host->>Init: Launch(id, exec, on_exit)
Init->>Workload: Spawn with isolated UID/runtime
Init->>Host: Started(id)
Workload-->>Init: Exit status
Init->>Host: WorkloadExited(id, status)
Host->>Init: Launch / Restart / Shutdown
```
<sub>Reviews (4) · Last reviewed commit: ["fix(nesinit): readiness is a
socket
that..."](
|
||
|
Wanjohi
|
ec8b13d0c9 |
fix: A host-only session cookie, and a column for where a host actually is (#331)
Closes #330.
Two gaps that block a reverse proxy sitting in front of user-owned
hardware and
authenticating browsers on its behalf, plus one thing found on the way
that is
worse than either.
## 1. `machine` records where a host is
`machine` said who owns a host, which team it belongs to and when it was
last
seen, and nothing about how to reach it — so a request arriving for a
machine
could be authorised perfectly and then have nowhere to go.
`endpoint_id` is **reported, never assigned**: a host holds the secret
half of
that identity and is the only thing that can know the public half first,
so it
rides on the beat it already sends as itself. Migration `0013`.
- **Nullable**, because "has never reported one" is a real state that
every host
registered before today is in. Null reads as *not reachable yet*; a
default
would read as an address and route somewhere wrong.
- **Unique**, because an endpoint id belongs to one host. Two rows
claiming the
same one would send a request addressed to one machine to another
machine's
agent, which is the one mistake here the authorisation in front of it
cannot
catch.
- **Omitting the field leaves the stored value alone.** An agent that
does not
say where it is has not moved, and an absent field must never read as
"nowhere" — that would take every host shipped before this field off the
map
on its next beat. There is a test for exactly that.
## 2. The session cookie, and why it is not shaped the way the issue
asked
The issue asked for a `__Host-nestri-session` set on sign-in carrying
the access
token unwrapped. Building it turned up two reasons that cannot work, and
item 3
of the issue is the reason why:
1. **There is nowhere to set it.** The sign-in UI is served by the
issuer on its
own hostname, and the web client has no auth code at all. A `__Host-`
cookie
set at sign-in is host-only *to the issuer* — a host that does not need
one.
2. **The value would be a control-plane credential** sitting on hardware
the
control plane does not run.
Item 3 said the hand-off had to be settled from both ends and did not
pick a
shape. It is settled now, in the way the issue's own two rules point at:
**the
proxy is an ordinary public OAuth client, one per hostname**, and what
crosses
in the URL is an **authorization code** — single-use, sixty seconds
long,
redeemed exactly once by the store that already exists here, and
exchanged over
a back channel. A code in an access log is worthless by the time anybody
reads
it, which is the hazard the issue named. No credential in a URL, and no
caller-supplied return address.
What this repo owes that flow is one rule, and it is the whole of the
change to
the issuer: **a client id that is a single hostname under the host zone,
whose
`redirect_uri` is `https` and that same hostname at one reserved path,
is an
allowed client.** Everything else keeps the behaviour it had.
Making the client id the hostname is load-bearing rather than tidy. A
token's
`aud` is its client id, so the session that comes back is **bound to the
host it
will live on**, with no change to how tokens are minted — and it is not
a
credential on any other host, nor a control-plane credential at all.
That is a
better answer than "the access token, exactly", and it costs nothing.
## 3. `/authorize` was an open redirector
Found while testing the above. A refused client's `redirect_uri` was
still used
to deliver the refusal — and the check that approves that URI is the one
that
just failed:
```
GET /authorize?client_id=web&redirect_uri=https://somewhere.example/callback
-> 302 https://somewhere.example/callback?error=unauthorized_client
```
No sign-in required, on the hostname people are asked to type a password
into.
A refusal is now a page here. This is in `packages/auth` and is
independent of
everything above; it is in this PR because the flow above is built on
that path
and shipping one without the other would have been odd.
## Tests
Every case below fails against the unmodified code and passes after.
`379 pass,
0 fail` across `packages` and `apps`.
- `apps/auth/test/allow.test.ts` — the allowed case; a code is never
sent
anywhere but the client id; only the reserved path; `https` only; one
label,
because `a.b.zone` is not a host id; another zone does not get in by
using the
path; the three existing rules unchanged; and the refusal is a page
rather
than a redirect.
- `packages/core/src/machine/machine.test.ts` — reported and read back,
a
silent beat leaves it alone, four malformed ids refused, two machines
cannot
claim one id.
- `apps/api/test/heartbeat.test.ts` — a beat with no body is still a
beat, a
host cannot report where *another* host is, a malformed id is a `400`.
## What this does not verify
- **No end-to-end run against a real browser.** The flow is exercised
from the
proxy's side and from this side separately; nothing has driven a browser
through sign-in and out the other end.
- **The token's shape is asserted to the written contract, not captured
from a
running issuer.** If minting changes, these tests pass and a signed-in
person
is redirected to sign in again.
- **`machine_endpoint_id_unique` is not exercised under concurrency.**
Two hosts
reporting the same id in the same instant is a database-level race that
the
tests assert the *outcome* of, sequentially.
- **Nothing here makes a hand-off silent.** The issuer keeps no session
of its
own, so a second hostname asks for an email code again. That is not a
regression — nothing anywhere is silent today — but it is the next piece
of
work, and it is what the issue's item 1 will eventually be, on the
issuer's
own hostname.
- Pre-existing `tsc` errors remain in
`apps/api/app/utils/{hook,validator}.ts`
and `packages/core/src/session/session.test.ts`; none is touched here.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR adds a durable, host-reported endpoint identity and permits
tightly constrained OAuth callbacks for individual hostnames. It also
prevents rejected OAuth clients from controlling the error redirect.
- Adds a nullable, unique machine endpoint ID with migration and
heartbeat reporting.
- Preserves endpoint IDs when legacy agents send bodyless heartbeats.
- Converts duplicate endpoint claims into the API’s stable HTTP 409
conflict response.
- Allows HTTPS authorization callbacks only at the reserved path on the
matching single-label host.
- Returns unauthorized-client failures locally instead of redirecting to
an unapproved URI.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; the previously reported endpoint-conflict
failure is now translated and tested as a stable HTTP 409 response.
No actionable new failure remains. The prior endpoint-conflict finding
is fully fixed by translating PostgreSQL uniqueness failures into
`already_exists`, which the API maps to 409, with both domain-level and
route-level coverage.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/auth/src/index.ts | Adds strict host callback authorization while
retaining the existing same-domain and local-development rules. |
| packages/auth/src/issuer.ts | Prevents unauthorized clients from using
their rejected redirect URI as an open-redirect destination. |
| packages/core/src/machine/index.ts | Persists optional endpoint
reports and translates duplicate endpoint claims into a stable domain
conflict. |
| apps/api/app/routes/machine.ts | Extends machine heartbeats with
optional endpoint reporting and documents the 409 response. |
| packages/core/migrations/0013_machine_endpoint_id.sql | Adds the
nullable endpoint column and database-enforced uniqueness invariant. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant B as Browser
participant A as Auth issuer
participant P as Host proxy
participant H as Machine
participant D as Database
H->>A: Authenticated heartbeat with optional endpointId
A->>D: Update lastSeen and, when supplied, endpointId
D-->>A: Stored or unique conflict
A-->>H: 200 or typed 409
B->>A: /authorize for host.nestri.link
A->>A: Validate matching HTTPS reserved callback
A-->>B: Provider flow
B->>A: Complete authentication
A-->>P: Short-lived authorization code
P->>A: Exchange code
A-->>P: Host-audience session tokens
```
<sub>Reviews (2): Last reviewed commit: ["fix(machine): a taken endpoint
id is a
c..."](
|
||
|
Wanjohi
|
51d25f3e8c |
fix(machine): a taken endpoint id is a conflict, not a server fault
A host reporting an endpoint id another machine already holds hit the unique index, and the raw refusal reached the global handler as a 500 -- telling a host its beat broke the server rather than that the id is taken. It is now the 409 every other conflict here gives, and the route documents it. Checked-then-written would be worse rather than better: two hosts reporting the same id in the same instant both read "nobody holds it" and both write, which is precisely what the index is for. The read would add a query and remove nothing. Before: expect(res.status).toBe(409) Received: 500 |
||
|
Wanjohi
|
49ae45624e |
fix(auth): a host may receive a code at its own name, and a refusal is not a redirect
Two changes to who may start a flow here, and where a refusal is delivered. A host reached at its own hostname sits on a different registrable domain from this issuer, deliberately: that is what stops a cookie set there from ever reaching this one. The default rule allows a redirect back to whatever hostname the request arrived on, so it refused exactly the case the separation created. Which is a real problem rather than a theoretical one, because a session cookie without a Domain attribute is host-only, so a browser arriving at one of those hostnames for the first time carries no cookie whether or not it is signed in, and sending it here to sign in again changes nothing. So a client id that is a single hostname under that zone, whose redirect_uri is https and that same hostname at one reserved path, is allowed. Making the client id the hostname is the load-bearing part: a token's audience is its client id, so the session that comes back is bound to the host it will live on and is not a credential anywhere else. Separately, and worth its own paragraph: a refused client's redirect_uri was still used to report the refusal. The check that approves that URI is the one that just failed, so /authorize was an open redirector to anywhere at all -- no sign-in required, on the hostname people are asked to type a password into. It is now a page here. Before: GET /authorize?client_id=web&redirect_uri=https://somewhere.example/callback -> 302 https://somewhere.example/callback?error=unauthorized_client |
||
|
Wanjohi
|
6603383ad1 |
feat(machine): record where a host can be reached, as the host reports it
The machine table said who owns a host, which team it belongs to and when it was last seen, and nothing about how to reach it. Anything standing in front of a host and authenticating browsers on its behalf could therefore authorise a request perfectly and then have nowhere to send it. Reported, never assigned. A host holds the secret half of this identity and is the only thing that can know the public half first, so it rides on the beat it already sends as itself. Omitting the field leaves the stored value alone -- an agent that does not mention where it is has not moved, and an absent field must never read as "nowhere", which would take every host shipped before this field off the map on its next beat. Nullable, because "has never reported one" is a real state that every host registered before today is in. Unique, because an endpoint id belongs to one host: two rows claiming the same one would send a request addressed to one machine to another machine's agent, which is the one mistake here that the authorisation in front of it cannot catch. |
||
|
Wanjohi
|
0e94620808 |
feat(nesinit): carry the session's address out of the guest (#328)
## What was missing `neshub` serves the session's address on a socket, and its own flag has always said how that address gets out: > *"neshub listens; nesinit dials and carries the ticket to the host, because > the person who needs it is outside this VM and stdout here is a log file > inside one."* Nothing dialled it. `grep -rn ticket apps/nesinit/src` returned nothing at all, so the address never left the guest — and the one lifecycle message for it, `Ticket`, had no sender. This adds the carrier. ## Three decisions worth reading **Polled, not read once.** An address is not a value, it is the best answer so far: an endpoint discovers more ways to reach it after it binds — a local one immediately, a relayed one seconds later. Reading once means whoever asked first decides, and the first answer is the one that works on a local network and fails from anywhere else. Only a *changed* answer is forwarded, so an unchanged one costs nothing. **Dials, does not listen.** The opposite of the payload relay next door, and deliberately so. There the guest listens because the workload starts later; here the server is the long-lived one. Dialling also makes "not bound yet" an error to retry rather than a connection to wait for without knowing whether it is coming — which is the ordinary case at boot, since this starts before the server does. **The address is never logged.** It is a capability to reach the session, and a log inside the guest is the one place it has no reason to be. The log line says whether it is the first one and nothing else. ## Failing first The carrier's tests fail against unmodified code by not compiling: `ticket.rs` does not exist and `session::run` takes three arguments. Said plainly rather than manufactured. The behavioural gap is better shown as the `grep` above — nothing in the guest ever sent a `Ticket`, so the message had one end. `nesinit`: **35 tests passing**, up from 27. Four on the carrier itself (an address arrives; a better one replaces it; a socket that is not there yet is waited out rather than failed; an empty answer is not an address) and two on the session (an address reaches the caller as `Ticket`; a carrier that stops does not end the session). ## Verified in a real guest Built static for musl, run as PID 1 in a real microVM under a real VMM with a real vsock. It dialled out, completed the handshake at version 2, took a boot descriptor, started its workload, read the address that workload published and sent it up the channel — twice, the second time because a better one appeared. The caller saw `nestri:local-only` and then `nestri:with-relays`. Guest boot to init was **310 ms**. ## Review round ( |
||
|
Wanjohi
|
f74de9beb8 |
fix(nesinit): do not mount over the share tree, and check who serves an address
Four findings from review, all of them real. The relay's directory was mounted on the tree a session's shares live in. A fresh tmpfs there hides every directory the image prepared underneath it: the install, the user state, the work directory, and the mount point the log share is attached to from fstab. A box would have come up with a socket and without any of the places its workload looks for its files, and the exact-path check could not notice, because what fstab mounts is a directory inside that tree rather than the tree itself. It moves to /run, which is where a runtime socket belongs, is a tmpfs already, and has nothing else mounted inside it. It was also owned by this process and closed to everyone else, which stopped the workload traversing it to reach the relay at all. The directory is now readable and searchable, and still writable by nothing but this process, which is what makes the socket in it unreplaceable; the socket itself is what the workload is allowed to connect to. The permission belongs on the socket rather than on the path. The address served to a reader was built once at startup and served forever, so a reader that polls for a better one could only ever get the first. An endpoint does not know all of its own addresses when it binds: the first is the one that works on the same network and fails from anywhere else. It is now rebuilt per read, which is what makes polling for it worth doing. And the address was taken from whoever held a path in a directory the workload can write. Workload code could unlink the socket a service was listening on, bind its own, and every read afterwards would hand the client an address of its choosing -- a session given to somebody else rather than a session that fails. The peer's credentials are now checked before a byte is read, from the kernel rather than from anything the peer says about itself, and an address served by the workload's own user is refused and said loudly. That check is only worth something while the workload has a user of its own, so the image grows one. Two users, and they must stay two: one runs the services that ship in the image, the other is who a workload runs as. Sharing one does not weaken the check, it makes every session fail it. A workload running as root is every user at once and cannot be told apart from anything; the check stands down there and says so at boot instead, because refusing root would refuse whatever legitimately serves the address as well. Also bumps tinyvec by a patch release. It does not build on this toolchain -- `vec` resolves to the module and not the macro -- which made every crate that depends on an endpoint, including this one, unbuildable. Pre-existing and nothing to do with this change; the lockfile said the same version before it. |
||
KAAL1
|
00a2bdab30 |
fix(nesinit): give one look at the address socket a deadline
There was a cap on how much this would read and none on how long it would wait. The far end can accept a connection and then write nothing, and a read with no deadline turns that into a poll loop that never runs again: the address already forwarded stays correct, and the better one that arrives afterwards is never seen. That is the failure this polls to avoid, reached by a different route. Five seconds, against a two second interval, so an answer that is merely slow still lands and one that is never coming is abandoned. The test hangs against the code as it was, which is the whole point of it. |
||
|
KAAL1
|
18864b97f0 |
fix(nesinit): mount what the guest needs before anything asks for it
The root arrives read-only and this process is PID 1, so until it mounts them there is no /proc and nowhere in the filesystem to put a socket. Nothing else in the guest is an init system, so nothing else was going to. The symptom was three failures that look unrelated and share one cause. On a real box the payload relay could not bind, with EROFS; whatever serves the session's address could not bind either, the same way; and this process could not make itself ineligible for the OOM killer, because /proc was not there to write to. What the caller saw was a workload that ran and published nothing, which is true and says nothing about why. /proc is mounted first and unconditionally: finding out what an image already mounted requires it, and it is therefore the one entry that cannot be checked that way itself. Everything after it is skipped when it is already present, so an image that does this properly is not mounted over. Failures warn rather than abort. Refusing to boot would replace a session that fails with a reason by a guest that never dialled out at all, and the second is harder to diagnose from the outside. The relay's directory is named by the module that owns the socket rather than spelled again here, with a test tying the two together: a rename that reached one and not the other would put the relay back exactly as it was. |
||
|
Wanjohi
|
2a7be92a41 |
ci: run each half only when that half changes (#329)
## Why
Both jobs ran on every pull request. A change to a Rust binary waited on
a
Postgres service and a full TypeScript test run; a change to a
TypeScript route
spent a runner compiling Rust. Neither result told anyone anything.
## What changed
A `paths:` filter belongs to a **workflow**, not to a job — so the two
jobs
become two workflows. That is the entire cost of the change:
| | |
|---|---|
| `.github/workflows/web.yml` | the TypeScript half — `bun test` over
the control-plane apps and shared packages |
| `.github/workflows/nesdoctor.yml` | the Rust half — `fmt`, `clippy`,
`test`, and a no-network run |
| `.github/workflows/ci.yml` | deleted; it was the two of them together
|
**Both job bodies are carried over unchanged.** Parsed and compared
rather than
eyeballed:
```
web job body identical to ci.yml: True
nesdoctor job body identical to ci.yml: True
```
Only the triggers differ. `push` is untouched (see the first note
below).
## The filters, and where they come from
**`web`** — every TypeScript workspace member, plus the things that
reach all of
them. `packages/` is entirely TypeScript so it is taken whole; `apps/`
is mostly
Rust, so its two TypeScript members are named.
```
apps/api/** apps/auth/** packages/**
package.json bun.lock tsconfig.json oxlintrc.json
.github/workflows/web.yml
```
**`nesdoctor`** — its own directory, plus the workspace root and
lockfile, which
pin every version it builds against. No other member is listed because
it
depends on no other member; its dependency tree is four external crates
deep and
that is deliberate.
```
apps/nesdoctor/** Cargo.toml Cargo.lock
.github/workflows/nesdoctor.yml
```
## Verified by simulating the filters, not by reading them
The failure mode of a path filter is *silence* — a wrong pattern means
the job
never runs and the pull request goes green. So the globs were
implemented in
GitHub's dialect (`*` stops at a slash, `**` crosses them) and run
against real
change sets, including the actual file list of the last merged PR:
```
the enrolment PR, actual file list → web
a TS route only → web
a core module only → web
a migration only → web
the auth worker → web
the shared auth package → web
the bun lockfile → web
lint config → web
nesdoctor source → nesdoctor
nesdoctor README → nesdoctor
the Rust workspace root → nesdoctor
the Cargo lockfile → nesdoctor
another Rust app → (nothing)
a shared Rust crate → (nothing)
the web workflow itself → web
the nesdoctor workflow itself → nesdoctor
docs only → (nothing)
the root README → (nothing)
a stray root artefact → (nothing)
both halves at once → web, nesdoctor
```
`another Rust app` and `a shared Rust crate` firing nothing is correct
**today**
— CI covers `nesdoctor` alone, and the rest of the Rust half has never
been
under it. It stops being correct the moment a second member is added to
CI, and
each new member wants its own filter alongside its own job.
Both jobs were also run locally with the exact commands the workflows
use:
`nesdoctor` — fmt clean, clippy clean under `-D warnings`, 18 tests
pass, and
the binary runs; `web` — migrations apply and 363 tests pass, 0 fail.
## Three things found on the way, none of them fixed here
1. **`push: branches: [main]` is inert.** There is no `main` branch —
the
default is `dev` — so the push half of this trigger has never fired and
does
not fire now. I carried it over verbatim rather than "fixing" it to
`dev`,
because that would *add* CI runs and this PR exists to remove them. One
word
either way; your call.
2. **A new TypeScript app will silently not be tested** until someone
adds it to
`web.yml`'s list. Nothing detects this. It is written as a comment in
the
file, in the place someone editing that list will be looking.
3. **`nesdoctor.json` is committed at the repo root** and appears to be
a report
generated on someone's machine — it carries a specific CPU, kernel and
disk
layout. It is an output rather than an input, so no filter references
it.
Probably wants deleting and gitignoring, separately.
## Before turning on required status checks
Path-filtered workflows do not report at all when they do not match,
which
branch protection reads as *expected but missing* — a pull request that
touches
only docs would never become mergeable. There are no required checks on
`dev`
today (`required_status_checks: null`, checked), so nothing is broken by
this.
If you enable them later, the usual answer is a companion job that
always runs
and reports success under the same name.
## What this does not verify
- **The simulation implements GitHub's glob dialect; it is not GitHub.**
This
pull request is the first real exercise of it: it changes both workflow
files, each of which lists itself, so **both jobs should run here** —
which
is the intended behaviour, since a change to how the tests run is a
change
worth running. Anything else on the checks tab means a filter is wrong.
(An earlier draft of this section predicted *neither* would run. That
was
wrong, and the simulator says so: `this PR itself → web, nesdoctor`.
Left
visible because it is exactly the mistake path filters invite —
reasoning
about which files a change touches without checking.)
**Confirmed on the runner**, which is no longer a prediction:
```
nesdoctor / nesdoctor → success (pull_request)
web / web → success (pull_request)
```
- **`actionlint` was not available**, so the workflow files are
validated as
YAML and by parsing their trigger and job structure, not by a
schema-aware
linter.
- **Nothing is measured.** No before/after timings — the saving is "a
job that
had no reason to run does not run", not a number I benchmarked.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR splits the combined CI workflow into independently filtered web
and nesdoctor workflows while preserving their existing job bodies.
- Web tests now run for changes to current TypeScript workspace members
and their shared configuration.
- Nesdoctor checks now run for changes to its crate, Cargo workspace
inputs, or its workflow.
- Unrelated pull requests no longer start both test stacks.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; the new filters cover the current inputs
of both preserved CI jobs.
No actionable failure remains: current workspace members and build
inputs are covered, no in-repository consumer relies on the old workflow
identity, and the split does not increase permissions or action
exposure.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| .github/workflows/web.yml | Extracts the unchanged TypeScript test job
into a workflow filtered to all current web workspace members and
relevant shared inputs. |
| .github/workflows/nesdoctor.yml | Extracts the unchanged nesdoctor
checks into a workflow filtered to the crate and its Cargo workspace
inputs. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR[Pull request changes] --> W{Matches web paths?}
PR --> N{Matches nesdoctor paths?}
W -->|Yes| WT[Run Bun install, migrations, and tests]
W -->|No| WS[Skip web workflow]
N -->|Yes| NT[Run fmt, clippy, tests, and no-network smoke run]
N -->|No| NS[Skip nesdoctor workflow]
```
<sub>Reviews (1): Last reviewed commit: ["ci: run each half only when
that half
ch..."](
|
||
|
Wanjohi
|
f27ea3a132 |
ci: run each half only when that half changes
Both jobs ran on every pull request, so a change to a Rust binary waited on a Postgres service and a full TypeScript test run, and a change to a TypeScript route spent a runner compiling Rust. Neither told anyone anything. A `paths` filter belongs to a workflow rather than to a job, so the two jobs become two workflows. That is the whole cost of the change: both job bodies are carried over unchanged, and only the triggers differ. The filters are written from what each job actually reads. `packages/` is entirely TypeScript so it is taken whole, and the two TypeScript apps are named because the rest of `apps/` is Rust. The Rust job takes its own directory plus the workspace root and lockfile, which pin every version it builds against, and nothing else — it depends on no other member of the workspace. The failure mode of a path filter is silence: a job that does not run leaves a green pull request. So the one thing a future change has to remember is written where it will be read — adding a TypeScript app means adding it to that list. |
||
|
Wanjohi
|
b95d939aeb |
feat(api): record which host holds a Steam token for whom (#327)
## What this is
A host that signs someone into Steam ends up holding a refresh token.
The
control plane needs to know that happened; it must not know the
credential.
This adds the table, the domain module and the three
machine-authenticated
routes that record the outcome, and a test that the surface refuses a
token.
| | |
|---|---|
| `steam_enrolment` | `(machine_id, user_id)` primary key, `steam_id`,
`state`, `enrolled_at`, `last_ok_at`, `revoked_at`. **No token column.**
|
| `Enrolment.record` | upsert to `enrolled` — keeps the original
`enrolled_at`, clears a previous refusal |
| `Enrolment.markStale` | machine-scoped update; `null` when there is
nothing to mark |
| `Enrolment.listByMachine` | what this host is believed to hold, oldest
first |
| `POST /machine/enrolment` | `{userId, steamId}` → the enrolment |
| `POST /machine/enrolment/stale` | `{userId}` → the enrolment, now
stale; `404` if absent |
| `GET /machine/enrolment` | the list |
All three take the host from its own credentials, never from a body, so
a box
can neither report onto nor read another box's hardware. `data` is the
object
itself — never `{"data": {"enrolment": …}}`.
## The test failing first
Both new files, against unmodified code:
```
✗ POST /machine/enrolment > the outcome is recorded and `data` is the enrolment itself
✗ POST /machine/enrolment > the machine is taken from the credentials, never the body
✗ POST /machine/enrolment > re-enrolling keeps the first `enrolledAt` and adopts the new Steam account
✗ POST /machine/enrolment > one Steam account on two hosts is two enrolments
✗ POST /machine/enrolment > a user nobody has heard of is refused rather than crashing
✗ POST /machine/enrolment > a Steam id has to look like one
✗ POST /machine/enrolment > machine credentials are required
✗ POST /machine/enrolment/stale > a refused token moves the enrolment to stale
✗ POST /machine/enrolment/stale > re-enrolling after a refusal returns the row to enrolled
✗ POST /machine/enrolment/stale > an enrolment this host does not have is a 404
✗ POST /machine/enrolment/stale > a host cannot mark another host’s enrolment stale
✗ POST /machine/enrolment/stale > machine credentials are required
✗ GET /machine/enrolment > a host with no enrolments gets an empty list, not a 404
✗ GET /machine/enrolment > every enrolment this host is expected to hold, and no other host’s
✗ GET /machine/enrolment > machine credentials are required
✗ The enrolment surface refuses a token > POST /machine/enrolment rejects every credential-shaped field
✗ The enrolment surface refuses a token > POST /machine/enrolment/stale rejects every credential-shaped field
✗ The enrolment surface refuses a token > the published surface has exactly three enrolment routes and no field for a credential
error: Cannot find module './enrolment.js' from 'packages/core/src/steam/enrolment.test.ts'
0 pass
19 fail
1 error
Ran 19 tests across 2 files.
```
And after, against a database migrated from zero:
```
363 pass
0 fail
1047 expect() calls
Ran 363 tests across 29 files. [8.39s]
```
`bunx oxlint` clean; `oxfmt --check` clean on every file in the diff.
`tsc
--noEmit` on `apps/api` and `packages/core` reports exactly the same 5
and 2
pre-existing errors as `dev` does — none in files this touches.
## How the token is kept out, in three places rather than one
1. **The column list.** A core test asserts `information_schema.columns`
for
the table is exactly the seven contract columns. Adding `refresh_token`,
or
an `encrypted_token`, or a `secret`, fails it.
2. **The request bodies are `.strict()`**, derived from the domain
schema with
`Info.pick(...)` so they cannot drift from it. `refreshToken`, `token`,
`accessToken`, `challengeUrl` and `clientId` are each rejected with a
`400`.
3. **The published surface.** A test walks `/doc`, collects every
request-body
property and parameter under `/machine/enrolment*`, and asserts the set
is
exactly `{userId, steamId}` — so any new field on this surface has to be
argued for in that test, not only a token-shaped one. It also asserts
the
route list is exactly the two paths.
Checked by hand against a live server: the rejected key's **name**
reaches the
log, its **value** does not (`grep -c eyJsecret` over the server log →
0).
## Driven over real HTTP, not only `app.request`
A real `bun run apps/api/app/server.ts`, a real registered machine,
curl:
```
GET /machine/enrolment → {"data":[]} [200]
POST /machine/enrolment → {"data":{…,"state":"enrolled","lastOkAt":null}} [200]
POST again, new steam account → same enrolledAt, new steamId [200]
POST + refreshToken → Unrecognized key: "refreshToken" [400]
POST + challengeUrl+clientId → Unrecognized keys: "challengeUrl", "clientId" [400]
POST /machine/enrolment/stale → {"data":{…,"state":"stale"}} [200]
GET /machine/enrolment → the one row, stale [200]
no credentials / wrong secret → Machine credentials required [403]
stale for an absent enrolment → This machine has no enrolment for that user [404]
POST naming another machine → Unrecognized key: "machineId" [400]
```
## Two places the contract was read rather than followed literally, both
worth a look
- **`state` is a Postgres enum, not `text`.** The three values are the
whole
state machine, so the database refuses a fourth rather than storing it.
If
you would rather have `text`, say so and I will change it — but a typo'd
state is otherwise a silent write.
- **The row has no `id` and no `time_deleted`**, which departs from the
every-table convention in `packages/core/CLAUDE.md`. The pair *is* the
identity, and `enrolled_at` would make `time_created` a second answer to
the
same question. The row's life is bounded by the machine's and the
user's, and
both foreign keys cascade. Flagging it because it is the sort of thing a
reviewer should agree to on purpose.
## Review rounds: three findings, all real, all fixed
- **An overlong `userId` returned a 500.** Ids live in a `char(30)`
column, so
an overlong one is refused by Postgres with `22001` — not the `23503`
the
handler catches — and fell through to the global error boundary.
Measured
before fixing: 44 characters → `500`, absent-but-well-formed → `404`.
`Identifier.schema` had no callers anywhere in the tree, so it now
asserts
the exact width and the separator as well as the prefix, and
`Enrolment.Info`
uses it for both foreign keys. The route picks up the constraint through
its
existing `Info.pick(...)`, so the answer is a `400` naming `userId`.
Verified
against a live server: zero 500s across the run.
- **`user_id` was unindexed.** The key begins with `machine_id`, so
neither the
cascade behind deleting a user nor "which hosts hold a token for me" can
use
it. `steam_enrolment_user_idx` added. The migration has not been
released, so
it is folded in and regenerated through `drizzle-kit` rather than
followed by
a corrective `0013`.
- **Every documented id was one character short.** `Examples.Id` emitted
25
payload characters where an id has 26, so the published examples were 29
characters — invalid against the width the previous fix started
enforcing.
Never broken at runtime, since an example is not parsed; wrong in the
documentation people copy from. The width now comes from
`Identifier.LENGTH` rather than being typed out, in both places that had
counted it by hand, and the third hand-written copy of the same literal
in
`apps/api/app/routes/steam.ts` now calls the generator instead.
Each is covered by a test, including one that walks four differently
misshapen
ids. The existing "a user nobody has heard of" test now uses a
well-formed
absent id, so it exercises the foreign-key path it was written for
rather than
passing for the wrong reason.
## Files outside this lane's ownership
Four, each the smallest possible diff:
- `apps/api/app/index.ts` — one import, one `.route('/machine', …)`
line.
- `packages/core/src/examples.ts` — one `Examples.SteamEnrolment` block.
- `packages/core/src/id.ts` — `Identifier.schema` gains the width and
separator
checks described above, and `LENGTH` is exported so the examples can
derive
from it. `schema` had **no callers in the tree** before this branch, so
nothing else can be affected by the tightening; this lane is its first.
- `packages/core/src/id.test.ts` — new. Pins a generated id, the schema
for
one, and the documented example together, for every prefix.
- `apps/api/app/routes/steam.ts` — one line: a hand-written `usr_XXX…`
example
literal, wrong by the same character, replaced with
`Examples.Id('user')`.
Owned by no lane this week. Flagging it because it is the only change
here
outside enrolment's own surface.
- `packages/core/CLAUDE.md` — one row in the sub-module table, and the
sentence
saying `steam/` owns no table is now false, so it is reworded.
`packages/core/src/steam/index.ts` is this lane's, and the change there
is one
word: `STEAM_ID_RE` is exported so the enrolment schema uses the same
rule
rather than a second copy of the same regex.
## What this does not verify
- **No host has ever called these routes.** The other half of this seam
was
written from the same document without either side reading the other's
code.
A disagreement, if there is one, surfaces on first contact — not here.
- **Nothing enforces that a token never arrives.** The three guards
above fail
when somebody adds a field *to this surface*. They say nothing about a
route
added elsewhere, and no test can.
- **`revoked` has no writer.** The value exists in the enum and nothing
sets
it. Revocation is not built here.
- **`last_ok_at` has no writer**, so it is null in every row this
creates. It
has never been exercised with a value.
- **Authorisation is not tested here and is not this lane's.** Whether a
person
may reach a given box is decided before a request arrives; these routes
authenticate a *machine*, which is a different question. Nothing here
would
catch a mistake in the other one.
- **The `404` for an unknown user comes from catching a foreign-key
violation**
(`23503`), not from a lookup. It is exercised for a user id that does
not
exist. It has not been exercised against a user deleted concurrently
with the
insert, which is the same code path but a race I did not reproduce.
- **The index is not measured.** It is added because two readers exist
that
cannot use the primary key, not because a plan was compared. On a table
this
size neither would be slow yet.
- **Nothing else is measured.** No timing, no throughput, no load. The
only
numbers above are test counts and HTTP status codes.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR adds machine-authenticated Steam enrolment reporting without
transmitting or storing Steam credentials.
- Adds record, stale-state, and machine-scoped listing operations in the
core domain.
- Adds corresponding `/machine/enrolment` API routes with strict request
validation.
- Adds the enrolment table, state enum, foreign keys, user index, and
migration metadata.
- Aligns identifier validation and OpenAPI examples with the fixed
30-character identifier format.
- Adds domain, API, schema, authorization, and credential-exclusion
tests.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; the prior identifier-example issue is
fixed and no blocking correctness, security, or repository-rule
violations remain.
The current code fully addresses all previous findings: malformed
identifiers are rejected before database access, the user foreign key
has its own index, and shared examples now satisfy the identifier
schema. The changes since the previous review introduce no new
actionable failures.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/api/app/routes/enrolment.ts | Adds three strictly validated,
machine-authenticated enrolment routes scoped through the authenticated
machine actor. |
| packages/core/src/steam/enrolment.ts | Adds validated enrolment
recording, stale-state updates, machine-scoped listing, and
serialization. |
| packages/core/src/steam/enrolment.sql.ts | Defines credential-free
enrolment persistence with a composite key, cascading foreign keys, and
a user lookup index. |
| packages/core/migrations/0012_steam_enrolment_without_a_token.sql |
Creates the enrolment enum, table, constraints, and user index
consistently with the Drizzle model. |
| packages/core/src/id.ts | Tightens identifier validation to the exact
prefixed 30-character storage format. |
| packages/core/src/examples.ts | Corrects shared identifier examples to
produce schema-valid 30-character values. |
| apps/api/test/enrolment.test.ts | Covers route shape, authentication,
machine isolation, validation, lifecycle behavior, and rejection of
credential fields. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant H as Authenticated host
participant A as API
participant C as Core Enrolment domain
participant D as PostgreSQL
H->>A: "POST /machine/enrolment {userId, steamId}"
A->>A: Derive machineId from actor credentials
A->>C: record(machineId, userId, steamId)
C->>D: Upsert enrolment metadata
D-->>C: Enrolment row
C-->>A: Serialized enrolment
A-->>H: "{data: enrolment}"
H->>A: "POST /machine/enrolment/stale {userId}"
A->>C: markStale(authenticated machineId, userId)
C->>D: Machine-and-user-scoped update
D-->>H: Updated enrolment or 404
H->>A: GET /machine/enrolment
A->>C: listByMachine(authenticated machineId)
C->>D: Select this machine's rows
D-->>H: "{data: enrolments[]}"
```
<sub>Reviews (3): Last reviewed commit: ["fix(core): document an id that
is
actual..."](
|
||
|
Wanjohi
|
fe5297acbd |
fix(core): document an id that is actually a valid id
The example generator emitted twenty-five payload characters where an id has twenty-six, so every documented id was twenty-nine characters — one short of the width the column holds and, since last commit, one short of what the schema publishing it will accept. Nothing caught it because an example is never parsed: it is copied into documentation and read by people. The width now comes from the generator's own constant instead of being typed out, in the two places that had counted it by hand. Counting twenty-six of anything by eye is a thing people get right once and never re-check. A test pins the three together — a generated id, the schema for one, and the documented example must all agree, for every prefix. It fails on the off-by-one that prompted this, and on a prefix without its separator, which would otherwise read as an id of that type because it starts with the same three letters. |
||
|
Wanjohi
|
3d24a8e130 |
feat(nesinit): carry the session's address out of the guest
Whatever serves media in a box knows how it can be reached, and the person who needs to know is not in the box. Standard output here is a log file inside a VM, so the control channel is the delivery path rather than a convenience -- which makes this init's job and not a detail of whichever component happens to bind the port. The socket it reads was already documented as being read this way; nothing read it. Polled rather than read once, because an address is not a value but the best answer so far. An endpoint discovers more ways to reach it after it binds, so the first answer is the one that works on a local network and fails from anywhere else. Only a changed answer is forwarded. It dials rather than listens, which is the opposite of the relay next door and deliberate: there the guest listens because the workload starts later, and here the server is the long-lived one. Dialling also makes a server that has not bound yet something to retry rather than something to wait for without knowing whether it is coming. The address itself is never logged. It is a capability to reach the session, and a log inside the guest is the one place it has no reason to be. A carrier that stops does not end a session: whatever was already reported is still correct, and the workload's exit still has to be. |
||
|
Wanjohi
|
64a90abf75 |
fix(api): a misshapen id is bad input, not a server fault
Ids are stored in a fixed-width column, so an overlong one is refused by Postgres rather than simply matching nothing. That refusal is not a foreign-key violation, so it fell through to the global error boundary and reached the caller as a 500 — telling a host to retry something that can never succeed. Measured: a 44-character user id returned 500, where an absent but well-formed one correctly returned 404. `Identifier.schema` is the natural place for the check and had no callers yet, so it now asserts the exact width an id has as well as its prefix — including the separator, without which `usrsomething` reads as a user id. The enrolment schema uses it for both foreign keys, so the refusal happens where the input arrives and names the field. Also index `steam_enrolment.user_id`. The primary key begins with the machine, which answers what one host holds and nothing else, so neither of the two things that read by user alone can use it: the cascade behind deleting a user, and asking which hosts hold a token for one person. The table's migration has not been released, so this is folded into it rather than following it with a correction. |
||
|
Wanjohi
|
6429ec4ff7 |
feat(api): record which host holds a Steam token for whom
A host that signs a person into Steam ends up holding a refresh token. The control plane needs to know that happened — to show it, and so a host that lost its disk can find out what it is expected to hold — but it must not know the credential, because the token is bound to the address that obtained it and a copy anywhere else is the account-theft signal Steam watches for. So `steam_enrolment` stores the outcome and has no token column, no encrypted token column, and no column that could hold one later. The safeguard is that the credential is never sent here at all; a nullable column would be the first step in undoing it, so a test asserts the column list exactly and fails if one appears. Three machine-authenticated routes go with it: report a completed sign-in, report that Steam refused the token, and list what this host should have. All three take the host from its own credentials, so a box can neither report onto nor read another box's hardware. Their bodies are strict, so a host that sends a token is told it is wrong rather than quietly believed — which also keeps the value out of the request log. The Steam id is deliberately not unique. One account signed in on two hosts is two rows and two tokens, and a unique index there would look like hygiene while refusing somebody their second box. There is no `pending` state: a sign-in challenge lives about two minutes inside one process, and nothing outside it needs to know it exists. Nothing revokes yet, and `last_ok_at` has no writer — a successful logon happens where there is no credential to report it with — so the column exists with the shape it will need and stays null rather than being filled with the nearest event that was easy to observe. |
||
|
Wanjohi
|
7f7e39de60 |
docs: point the test database setting at the right database
Same correction as the one on the helper itself: "isolated" here means isolated from anything you care about, not isolated from DATABASE_URL. Filling this in with a second database name is a plausible reading that fails the suite in a way that does not point back here. |
||
|
Wanjohi
|
dae2990cbe |
docs(core): say which database the tests actually need
The helper told you to use "an isolated database for tests", which reads as a database of its own and is not what the suite wants. Route tests reach the database through the app and core tests reach it directly, so two different values put the fixtures in one database and the assertions in the other — around forty failures, none of them in the code that caused it, and nothing in the output naming the setting. Also drops a type import nothing uses. |
||
|
Wanjohi
|
b296918ab4 |
feat(api): record what a host says it is running
A host agent already sends a full inventory snapshot on a cadence, and nothing served the endpoint it sends it to — so every one of those calls answered 404. It fails quietly by design, because a dropped snapshot is meant to be corrected by the next one, which is exactly why nobody noticed: the only symptom is a line in the agent's own log. Kept separate from the heartbeat because the two have different loss tolerance. A dropped beat moves a host towards offline and unplaces it; a dropped snapshot costs nothing until the next one arrives. Folding them together would let a malformed inventory field make a healthy host look dead. Three rules decide what a snapshot may do, and the last two are why this is one core function rather than a loop in the route: - a box we know, that the snapshot names, takes the reported state - a box we know that was running, and that the snapshot omits, is stopped and says so — absence inside a snapshot is information - a box the snapshot names that is not placed on the calling host is never created, only reported back as a divergence The scope is in the `where` clause and not in the agent asking politely about its own boxes: a machine credential is a long-lived secret sitting on hardware in somebody's living room. `pid` and `uptimeS` are accepted and deliberately dropped. A pid is a number in another machine's namespace, and uptime is derivable from a run's start time, which is already stored and already trustworthy. |
||
|
Wanjohi
|
b6aae5c2ab |
fix(deploy): make bun dev actually start, and sign-in actually work (#326)
Follow-up to #325. Six defects, all found by running the thing rather
than reading it — #325 was verified by bundling, by tests, and by the
container images, and none of those start a Worker.
| | |
|---|---|
| `bun dev` never started | one multi-config process does not connect a
service binding between the workers it loads — the API reported `AUTH
[not connected]`. Two processes now, which is what the dev registry
connects |
| Neither server could bind | wrangler resolves `localhost` and takes
`::1` first; a host with no IPv6 on its loopback dies with a bind error
naming neither app nor port. `dev.ip` pinned, and `inspector_port` made
distinct — it is not derived from the port, so the second server died on
an address already in use |
| The API worker failed to evaluate | a specifier ending in `.sql` is
claimed by the bundler as its own module, so the schema file was emitted
verbatim beside the bundle and the runtime threw on a missing export |
| Sign-in failed on the second DB request | a pooled socket created
while handling one request may not be touched while handling another on
a Worker. The *first* request always succeeded, which is why nobody saw
it |
| The images named a base podman will not resolve | a short name needs a
registry; the database service alongside them already spelled one |
| Compose pinned container names | not scoped to the project, so `down`
in one checkout stops another's containers — it stopped a running
development database while this was being tested |
Two of these are worth a second look because they are not confined to
local development.
**The database pool one is a live bug on Workers**, and it predates #325
— it arrived with the pool cache in `6c1d407`. On a Worker an I/O object
created during one request cannot be used during another, so the cached
socket throws *"Cannot perform I/O on behalf of a different request"* on
the second request that reuses it. The cache is now kept only where a
process outlives its requests, which is the case it was added for; a
Worker goes back to a pool per invocation, which is what it did before.
**The `.sql` import was also a layering break.** A route was reaching
past the domain module into the schema to spell a download status. It
asks the domain module now, which is the rule everywhere else here and
happens to be what removes the bundler hazard.
## Verified
A real sign-in, end to end, against both dev servers: a code requested
over HTTP, read out of the issuer's log the way a person reads it out of
their mail, redeemed, exchanged for tokens, and presented to the API —
which resolved it to the account the sign-in had just created, and
refused the same request without it.
```
1. asked for a code -> 200
2. code from the log -> 811548
3. redeemed the code -> 302
4. exchanged for tokens-> access eyJhbGciOiJFUzI1NiIsImtp…
5. GET /user with it -> 200 {"data":{"id":"usr_071b56380001F9L3Wf8OQj8ogS", …}}
6. GET /user without -> 401
```
Also: 316 tests pass, both images build, and compose substitution
resolves with the required-variable guards refusing correctly when
`.env` is incomplete.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Improved game download status validation for more consistent API
behavior.
- Improved database connection handling in Cloudflare Workers by using
request-local connections.
- **Developer Experience**
- Added explicit local development and debugger ports for the API and
authentication services.
- Improved development process handling when running services together.
- **Chores**
- Updated container configuration to support multiple project checkouts
without naming conflicts.
- Standardized container image references for more reliable builds.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR repairs local development startup and sign-in behavior while
making containerized development safer across environments.
- Runs the authentication and API Workers as separate, jointly
supervised development processes.
- Assigns explicit IPv4 listener and distinct inspector ports.
- Prevents Cloudflare Workers from reusing database I/O objects across
requests.
- Exposes download statuses through the domain module rather than
importing a schema module from the route.
- Corrects the authentication app’s Hono JSX transform configuration.
- Uses fully qualified Bun image names and Compose-managed container
names.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge, with the previous process-supervision
issue resolved and no new actionable regressions identified.
The current development scripts stop the sibling server when either
process exits, and the changes since the previous review preserve server
entrypoint behavior while correcting the Hono JSX runtime selection. The
previous thread was manually resolved after the supervision fix.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| package.json | Starts and supervises the two development servers
independently; the follow-up direct entrypoint invocation preserves
their intended behavior. |
| apps/auth/tsconfig.json | Aligns authentication-server JSX
transformation with Hono and the repository’s existing TypeScript
configuration. |
| packages/core/src/db/index.ts | Limits database pool caching to
long-lived process environments so Worker requests do not reuse
request-bound I/O. |
| packages/core/src/game/download.ts | Exposes valid download statuses
through the domain namespace for API consumers. |
| apps/api/app/routes/game.ts | Uses the domain-level download status
export, avoiding a direct runtime import of the SQL schema module. |
| docker-compose.yml | Removes globally fixed container names so Compose
projects remain isolated between checkouts. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Dev["bun dev"] --> Auth["Auth Worker<br/>127.0.0.1:1337<br/>Inspector 9229"]
Dev --> API["API Worker<br/>127.0.0.1:3000<br/>Inspector 9230"]
API -->|AUTH service binding| Auth
Auth --> DB["Request-local DB pool<br/>on Cloudflare Workers"]
API --> DB
Supervisor["Process supervisor"] --> Dev
Auth -->|Either process exits| Supervisor
API -->|Either process exits| Supervisor
Supervisor -->|Stops sibling process| Dev
```
<sub>Reviews (3): Last reviewed commit: ["fix(deploy): make \`bun run
dev:server\`
s..."](
|
||
|
Wanjohi
|
46c3a67d4d |
fix(deploy): make bun run dev:server start and reach its settings
Two defects, both found by running it. Neither affects the container path, which is why the images passed: they run the same entrypoints from the repository root, and that turns out to be the load-bearing detail. **The issuer would not start at all.** `apps/auth/tsconfig.json` named React as the JSX runtime — left over from an earlier scaffold; there is no React anywhere in this repository. It only bit when something transpiled from that directory, and then the process died resolving `react/jsx-dev-runtime` from a sign-in screen in `packages/auth` before it bound a port. The package holding those components already said `hono/jsx`, and so did the root; this is the third place agreeing with them. **Neither process could see `.env`.** They ran with the working directory set to their own app, so the environment file the repository documents — the one at the root — was not the one they were offered. The issuer then correctly refused to send a sign-in code rather than logging one, which is the right behaviour and an opaque way to discover a path problem. Both now run from the root, which is also exactly what the images run. |
||
|
Wanjohi
|
b9b090a620 |
fix(deploy): stop bun dev when either half dies, not just the API
The issuer ran in the background and nothing watched it. If it failed to bind, or exited an hour later, the API kept serving and kept reporting itself ready — while every authenticated request failed, because there was no issuer to verify a token against. That is the worst shape for a development failure: the thing you are looking at looks fine. Both run in the background now, and the script waits for either to stop before killing the other. The previous form only handled the direction where the API was the one that exited. |
||
|
Wanjohi
|
9258c8dfef |
fix(deploy): make bun dev actually start, and sign-in actually work
Six defects found by running the thing rather than reading it. The previous change was verified by bundling, by tests, and by the container images — none of which start a Worker, so every one of these was invisible. **`bun dev` did not start.** It ran one multi-config process, which does not connect a service binding between the workers it loads; the API reported `AUTH [not connected]` and could not verify a token. It is two processes now, which is what the dev registry connects, and the second is backgrounded with the first killed on exit so stopping the pair stops both. **Neither server could bind.** Wrangler resolves `localhost` and takes `::1` first; a host with no IPv6 address on its loopback dies with a bind error from inside the runtime that names neither the app nor the port. `dev.ip` is pinned to `127.0.0.1`, and `inspector_port` is now distinct per app — it is not derived from the port above, so the second server to start died on an address already in use. **The API worker failed to evaluate.** A specifier ending in `.sql` is claimed by the bundler as a module of its own, so the schema file was emitted verbatim beside the bundle and the runtime threw on an export it could not find. The route was reaching past the domain module into the schema to spell a status; it now asks the domain module, which is the rule everywhere else here and happens to also avoid the hazard. **Signing in failed on the second request that touched the database.** A pool is cached per connection string, and on a Worker an I/O object created while handling one request may not be touched while handling another. The first request always succeeded, which is why it went unnoticed — a sign-in is several. The cache is now kept only where a process outlives its requests, which is the case it was added for. **The images named a base that podman will not resolve.** A short name needs a registry; the database service alongside them already spelled one. **Compose pinned container names.** The name is not scoped to the project, so a second checkout got the same three, and `down` in one stopped the other's containers. This is not hypothetical — it stopped a running development database while this was being tested. Verified by signing in end to end against both dev servers: a code requested over HTTP, read from the issuer's log, redeemed, exchanged for tokens, and presented to the API, which resolved it to the account the sign-in had just created. |
||
|
Wanjohi
|
8c80c025be |
feat(deploy): drop the IaC layer, and make both apps runnable as containers (#325)
Moving the issuer's state into Postgres (#324) removed the last thing
tying either app to one hosting provider. What was left was a deployment
tool describing resources that no longer existed — so this drops it in
favour of `wrangler`, which is what actually deploys a Worker, and adds
a second way to run each app that involves no provider at all.
## What changes
**Gone:** `alchemy.run.ts`, `docs/alchemy.md`, the `alchemy` and
`effect` root dependencies, and the two type imports that reached out of
`apps/api` into the infrastructure file.
**In its place**, per app:
| | |
|---|---|
| `wrangler.jsonc` | one environment per stage, custom-domain routes,
Hyperdrive, the `AUTH` service binding |
| `Dockerfile` + `server.ts` | the same handler behind a listening
socket |
The handler is the same one either way. What differs is only where its
settings come from, and two of them gained a second spelling so that
nothing has to branch on the runtime: Postgres arrives as a pooled
binding or as `DATABASE_URL`, and the route to the issuer is a service
binding or `AUTH_INTERNAL_URL`.
`docker-compose.yml` now brings up Postgres and both apps together,
which is both what a self-hoster runs and the shape this takes when it
stops being a set of Workers.
## `AUTH_INTERNAL_URL`, which is new
A service binding was quietly doing two jobs: routing to the issuer, and
letting the `iss` claim stay the issuer's public name. Nothing else can
do both with one setting — the public name is often not routable from
inside a deployment — so the name and the route are two settings now.
`AUTH_ISSUER_URL` is still compared literally against every token, and
is unchanged.
## DNS
Moves out of code and into [`docs/dns.md`](docs/dns.md): every hostname,
what it is for, and what answers it today. Six records that change
roughly never did not need a tool, and a table outlives whatever is
serving the names — which is the point, because some of them will stop
being Workers. `wrangler` keeps owning only the part that must stay in
step with a deploy, since a route and its hostname are one fact.
The one rule the table enforces is **one label deep on `nestri.io`**. A
certificate for `*.nestri.io` covers one level and not two, so the
sandbox names are hyphenated rather than nested —
`api-sandbox.nestri.io` can become an ordinary proxied origin later
without a certificate having to be ordered for it first. Production
hostnames are unchanged.
## Also
`EMAIL_DEV_LOG` moves from committed configuration into
`apps/auth/.dev.vars`, which `wrangler deploy` cannot upload. Printing a
live sign-in code to a log should not be one forgotten override away
from a stage somebody else can reach.
## Before this deploys
1. The Hyperdrive ids in both `wrangler.jsonc` files are placeholders.
`wrangler hyperdrive list` has the real ones. Local development works
without them.
2. The Worker names change, so the first deploy creates new Workers.
Confirm the custom domains answer, *then* remove the old Workers and
routes — that order, or the names resolve to nothing in between.
3. Set the secrets listed in [`docs/deploy.md`](docs/deploy.md). The
issuer refuses to sign anyone in without its three mail settings.
## Checks
- 316 tests pass, 0 fail, against a freshly migrated database.
- All four wrangler environments bundle with no warnings.
- Both images build, run, and report `healthy`; the API container
reaches the issuer container and rejects a bad token as 401 rather than
500.
- `AUTH_INTERNAL_URL` verified end to end: discovery and JWKS resolve
through the internal route while `iss` stays the public name.
- `oxlint` clean apart from one pre-existing unused import in
`packages/auth`.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR replaces the Alchemy deployment layer with direct Wrangler
configuration and container-based execution for both control-plane
applications.
- Adds Bun HTTP servers, production Dockerfiles, health checks, and a
Compose deployment for Postgres, auth, and API.
- Supports database and issuer routing through either Cloudflare
bindings or ordinary environment variables.
- Adds stage-specific Worker routes, service bindings, Hyperdrive
configuration, and deployment documentation.
- Moves DNS ownership and deployment guidance into dedicated
documentation.
- Removes unused Alchemy, Effect, and Steam API-key configuration.
<h3>Confidence Score: 5/5</h3>
The current changes appear safe to merge, with no established new
defects or outstanding previous findings.
The container and Worker configurations are internally consistent,
required privileged credentials no longer have Compose defaults, and
plaintext service ports are loopback-bound. The three previous threads
were manually resolved without explanation and therefore are not
outstanding.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| docker-compose.yml | Defines the self-hosted stack with required
credentials, loopback-bound ports, mail pass-through, health
dependencies, and internal issuer routing. |
| apps/api/app/middleware/auth.ts | Adds issuer access through either a
Worker service binding or AUTH_INTERNAL_URL while preserving the public
issuer used for token validation. |
| apps/api/app/server.ts | Exposes the existing API handler through
Bun’s HTTP server with a process-compatible execution context. |
| apps/auth/src/server.ts | Exposes the existing authentication handler
through Bun’s HTTP server. |
| apps/api/wrangler.jsonc | Configures API development, sandbox, and
production Workers with routes, Hyperdrive, issuer settings, and auth
service bindings. |
| apps/auth/wrangler.jsonc | Configures auth development, sandbox, and
production Workers with custom domains and Hyperdrive. |
| packages/core/src/env.ts | Supports database and issuer routing
through environment variables and removes an unused Steam API-key
setting. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[Clients] --> Proxy[Custom domain or TLS proxy]
Proxy --> API[API handler]
Proxy --> Auth[Auth issuer handler]
API -->|AUTH service binding| Auth
API -->|AUTH_INTERNAL_URL in containers| Auth
API --> DB[(Postgres)]
Auth --> DB
Wrangler[Cloudflare Wrangler runtime] --> API
Wrangler --> Auth
Compose[Docker Compose runtime] --> API
Compose --> Auth
Compose --> DB
```
<sub>Reviews (2): Last reviewed commit: ["fix(deploy): require every
credential,
a..."](
|
||
|
Wanjohi
|
f30a1432f8 |
fix(deploy): require every credential, and give sandbox its own domain
Three things review caught, and one shape correction. **No credential has a default any more.** The compose file shipped `ADMIN_SHARED_SECRET` falling back to a value written in this repository — and that header bypasses token verification entirely, so anyone reading the file could act as an operator against any deployment that had not overridden it. A default is worth less than it looks here: the deployment that never set the variable is exactly the one where the default is public. Every credential now comes from `.env`, and compose refuses to start naming the variable it wanted. That also takes the last literal password out of a tracked file. **The origin ports are on loopback.** Both services speak plain HTTP and mark no cookie `Secure`, because both expect to sit behind something that terminates TLS. Published on every interface they were a way to reach the issuer around that proxy, with sign-in codes and tokens in clear text. **Mail settings are passed through rather than fixed.** The issuer was pinned to printing sign-in codes to its log, and the three delivery settings never reached it — so the documented way to configure mail could not work, and every code and recipient went to the container log instead. Printing codes is now asked for in `.env` like everything else, and with nothing configured the issuer refuses to send rather than logging. **Sandbox becomes a domain rather than a prefix.** `api.sandbox.nestri.io` and `auth.sandbox.nestri.io`, because sandbox holds whatever is not production and that set grows. One certificate for `*.sandbox.nestri.io` then covers all of it, including unpredictable per-pull-request names, and cannot be presented for production's own domain — which the zone-wide wildcard the previous shape leaned on could. Also drops `STEAM_API_KEY`. It was declared in two type definitions and read by nothing: linking an account makes no outbound call that needs it. |
||
|
Wanjohi
|
51ababc900 |
feat(deploy): drop the IaC layer, and make both apps runnable as containers
Moving the issuer's state into Postgres removed the last thing that tied either app to one hosting provider. What was left was a deployment tool describing resources that no longer existed — so this replaces it with `wrangler`, which is what actually deploys a Worker, and adds a second way to run each app that involves no provider at all. Each app now has a `wrangler.jsonc` with an environment per stage, and a `Dockerfile` beside it. The handler is the same one in both cases; what differs is only where its settings come from. Two of them gained a second spelling so that nothing has to branch on the runtime: Postgres arrives as a pooled binding or as `DATABASE_URL`, and the route to the issuer is a service binding or `AUTH_INTERNAL_URL`. That last one is new, and it is a split the binding was already making without saying so. `AUTH_ISSUER_URL` has to be the issuer's public name, because it is compared literally against every token's `iss` claim — but the public name is often not routable from inside a deployment. So the name and the route are two settings now rather than one that cannot be both. DNS moves out of code and into `docs/dns.md`, which lists every hostname and what it is for. Six records that change roughly never did not need a tool, and the table outlives whatever is answering the names — which is the point, since some of them will stop being Workers. The sandbox hostnames are hyphenated rather than nested for the same reason: a certificate covering `*.nestri.io` covers one label and not two, so `api-sandbox.nestri.io` can become an ordinary origin later without a certificate having to be ordered for it first. Also drops `EMAIL_DEV_LOG` from committed configuration into `.dev.vars`, which `wrangler deploy` cannot upload. Printing a live sign-in code to a log should not be one forgotten override away from production. |
||
|
Wanjohi
|
3d0dcf3e46 |
feat(auth): move issuer state to Postgres (#324)
Moves the issuer's state out of Cloudflare KV and into Postgres, and
splits it by whether a record actually needs the guarantees a key-value
store cannot give.
## Why
The issuer kept everything behind one `get`/`set`/`remove`/`scan`
interface — which is what a library that has to run on any provider's
cache can offer. Three of the things kept there could not be served by
it:
- **Authorization codes** must be redeemable once. `get` → validate →
`remove` are separate steps, so two exchanges of one code arriving
together both pass every check, and each answer is a complete session.
- **Refresh tokens** must be spendable once. Reuse detection works by
recording *when* a token was first spent, so the check and the record
have to be the same operation. Split apart, two refreshes both look like
the first — and the reuse that reveals a stolen token is never recorded,
because recording it is the write the second caller overwrites.
- **Signing keys** don't race, but they're the one record whose loss
ends every session at once, and a cache is a place things may be evicted
from.
This is the same argument `device_grant` already made, applied to the
records that had it too.
## What's here
| table | why it exists |
|---|---|
| `authorization_code` | redeeming is one `delete … returning` |
| `refresh_token` | spending is one `update … where time_used is null
returning *`; `subject` indexed, so signing out everywhere is a query
rather than a prefix scan |
| `auth_key` | retired by setting `expired_at`, never deleted, so tokens
stay verifiable through a rotation |
| `auth_kv` | everything left: the rate-limit counters |
`auth_kv` stays deliberately generic. Those counters are written far
more often than read, meaningless within the hour, and allowed to be
approximate — a lost increment costs one guess out of ten. It's the one
place an unmigrated `jsonb` blob is the right answer rather than a
shortcut.
Both credential tables store a **hash and never the credential**, as
`device_grant` does. An authorization code travels in a query string, so
it passes through browser history, referrer headers and any log along
the redirect; a refresh token resumes a session outright.
`packages/auth` gains `keyStore`, `codeStore` and `refreshStore` as
optional issuer inputs, each defaulting to a storage-backed shim so the
library behaves exactly as before when they aren't passed — same shape
as the existing `deviceStore`.
## Portability
`AuthStorage` was the only stateful Cloudflare-proprietary primitive in
the control plane. It's gone, so the remaining CF surface is Hyperdrive
(a pooler over a plain `DATABASE_URL`), Workers and DNS. The self-host
story stays one service.
## Two behaviour changes worth reviewing
1. **A code that fails a check is now spent.** `consume` happens before
the redirect-URI, client and PKCE checks, because the operation that
decides which caller gets the code has to be the one that removes it.
RFC 6749 §4.1.2 asks for this, but it is a change: a code presented
wrongly no longer gets a second try.
2. **`legacySigningKeys` is removed.** It read a pre-ES256 `oauth:key`
prefix and stamped every key it returned with a hardcoded expiry of
2025-01-02 — so everything it produced has been expired for over a year,
and it only ever read from the store being left behind.
## ⚠️ Deploying this signs everyone out
The signing keys and refresh tokens live in the KV namespace this
removes. The issuer will start with a fresh key set, so every existing
access token stops verifying and every refresh token is gone. Worth
timing deliberately, or copying the key rows across first if that isn't
acceptable.
## Testing
- `packages/auth` — 66 pass (the issuer flows run through the
storage-backed shims, so the rewrite is behaviour-preserving on the
default path)
- `packages/core` — 130 pass, including 24 new ones
The concurrency claims are tested rather than asserted: five overlapping
`claim`s of one refresh token yield exactly one `fresh` and four
`reused`; five overlapping `consume`s of one code yield exactly one
non-null. Also covered: `LIKE` wildcard escaping in `scan` (keys are
built from email addresses and caller addresses, so `%` and `_` are not
hypothetical), and that `scan(['a'])` no longer reaches into `['ab']`.
Migration `0011` applies cleanly from empty.
## Not in scope
Pre-existing `tsc` errors — JSX config for `packages/auth/src/ui/*.tsx`,
`subject.ts`, `oauth2.ts`, `session.test.ts` — are untouched and left
for their own PR.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR moves issuer state from Cloudflare KV into dedicated PostgreSQL
stores and adds atomic operations for authorization-code redemption,
refresh-token claims, and signing-key bootstrap.
- Authorization codes are hashed and consumed with `DELETE ...
RETURNING`.
- Refresh tokens are hashed and claimed with a conditional atomic
update.
- Signing keys are persisted in PostgreSQL with one live key permitted
per kind.
- Remaining approximate rate-limit state uses a generic
PostgreSQL-backed adapter.
- The changes since the previous review preserve each stored key’s
algorithm and make concurrent key bootstrap converge on one live key.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; both findings from the previous review are
resolved and no new actionable failure remains.
Concurrent key bootstrap now converges through a partial unique index
and a post-insert reread, while imported keys retain their stored
algorithms. Both previous threads were manually resolved, and the
current code confirms their underlying issues are fixed.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| packages/auth/src/issuer.ts | Routes authorization codes, refresh
tokens, signing keys, and invalidation through specialized stores. |
| packages/auth/src/keys.ts | Preserves stored key algorithms and
rereads the store after bootstrap so concurrent issuers converge. |
| packages/core/src/auth/signing-key.ts | Persists key material and
safely ignores a concurrent insertion that already established the live
key. |
| packages/core/src/auth/authorization-code.ts | Implements atomic,
single-use authorization-code consumption. |
| packages/core/src/auth/refresh-token.ts | Implements atomic
refresh-token claims and subject-wide revocation. |
| packages/core/migrations/0011_auth_state_in_postgres.sql | Adds
PostgreSQL tables and constraints for issuer state, including one live
key per kind. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Issuer[OAuth issuer] --> Codes[Authorization code store]
Issuer --> Refresh[Refresh token store]
Issuer --> Keys[Signing key store]
Issuer --> KV[Generic auth state]
Codes -->|DELETE RETURNING| PG[(PostgreSQL)]
Refresh -->|Conditional claim| PG
Keys -->|Partial unique live-kind index| PG
KV -->|Counters and rate limits| PG
```
<sub>Reviews (2): Last reviewed commit: ["fix(auth): keep one live key
per kind,
a..."](
|
||
|
Wanjohi
|
f64f037574 |
fix(auth): keep one live key per kind, and report a key's own algorithm
Two problems found in review, both in the key store. Nothing stopped a kind from having two live keys, and the bootstrap path walks straight into it: two workers starting against an empty table both find no key and both insert one. From then on each signs and encrypts with its own. That is not the harmless split the comment here claimed — the issuer reaches for a single key rather than the published set when it decrypts a session cookie and when it verifies an access token, so a cookie written by one worker is unreadable to the other and a token minted by one is rejected by the other. It stays silent until someone cannot sign in. A partial unique index over the kind, where the key has not been retired, makes the second insert a dropped write instead. Both workers then read the table again and use the key that won, which is all that matters. The conflict clause stops naming a target: both indexes on the table mean the same thing at this call site, that the row already exists in some form. Creating a key is now attempted once rather than retried, because a store declining the write is an expected answer and spinning on it would hang the request instead of failing it. Separately, a key pair reported the algorithm the issuer currently uses rather than the one stored on the key it was built from, so a retained key would advertise the wrong algorithm in a token header and in the JWKS after a rotation — which defeats keeping it. The material was already being imported with the stored value; only what was handed back disagreed. Retiring a key and creating its replacement now have to happen together, so that a kind never has two live keys and never has none. |
||
|
Wanjohi
|
f25c9af545 |
feat(auth): keep issuer state in Postgres
The issuer kept everything behind one get/set/remove/scan interface, which is what a library that must run on any provider's cache can offer. Three of the things kept there could not actually be served by it. An authorization code must be redeemable once and a refresh token spendable once, and through get and set the check and the write are separate steps — so two requests arriving together both read an unspent record, and both mint a session. In the refresh case that also means the reuse which reveals a stolen token is never recorded, because recording it is the write that the second caller overwrites. Each now has a table and an interface of its own: redeeming is one `delete ... returning`, spending is one `update ... where time_used is null returning *`, so exactly one caller is ever told it went first. This is the same argument the device grant already made, applied to the two records that had it too. Signing keys move for a different reason. Nothing races for them; they are the one record whose loss ends every session at once, and a cache is a place things may be evicted from. They are retired by setting a column rather than deleted, so the tokens they signed stay verifiable until they expire. Both credential tables store a hash and never the credential, as the device grant does. An authorization code travels in a query string and so passes through history, referrer headers and any log along the redirect; a refresh token resumes a session outright. What is left in the generic store is the rate-limit counters — written far more often than read, meaningless within the hour, and allowed to be approximate, since a lost increment costs one guess out of ten. Those move to Postgres too, so the only key-value binding this deploys with is gone and the control plane's state is one database. That was the point: nothing here now depends on a primitive a self-hoster cannot run. The generic scan also gained the separator on its prefix, so scanning `a` cannot return what is under `ab` — subjects and email addresses are both prefixes of longer subjects and email addresses. Deploying this signs everyone out. The signing keys and refresh tokens are in a store that is being left behind, so the issuer starts with a fresh key set and every existing token stops verifying. |
||
|
Wanjohi
|
349305d0cc |
feat(api): hold a run to the attempt that claimed it (#323)
Closes the seam week 2 merged without. The agent sends a claim token on
every
write and this side rejected the field, so **every state report and
every ticket
publish answered 400** — neither end's tests could see it, because each
was
written against the document rather than against the other end.
## What lands
**Both agent bodies take `claimToken`.** That alone is what unblocks the
wire.
**The row remembers which attempt holds it.** Taking a claim requires
there to be
no holder; every write after it requires the caller to *be* the holder.
The guard
is in the `where` clause and not only in the read above it, so two
attempts that
both read an unheld row still leave with one winner.
**A report from an attempt that does not hold the run is 409 whatever
the state
is** — including the state the run is already in. That row is the whole
point: an
agent retrying after a lost response holds the token and is told nothing
broke;
an agent that lost the race does not and is told to stop. Both answers
are
decided by the request rather than by when it arrived.
**The ticket is held to the claim too**, for a worse reason than a
double start.
The client re-reads the address rather than caching it, so a ticket
published by
a losing attempt produces a client that connects, successfully, to a
machine
running nothing. A box started twice is waste and it is visible.
**The holder is never cleared**, including on terminal states, so a
settled claim
cannot be replayed and a finished run still records which attempt ran
it. It is
**not** in what goes out — holding one permits writing to a run, and the
owner
reading their own session is not the holder. There is a test asserting
the whole
response shape, so a column added later has to be added there before it
ships.
## One thing the contract asked for that cannot exist
The spec distinguishes *"claim, row already has a holder → 409"* from
*"report,
token is not the holder → 409"*. **Those are the same case.** Taking the
claim
and leaving `requested` are one write, so a rival never observes a
`requested`
row with a holder — it observes a `starting` row it does not hold. The
answer is
409 either way and nothing is lost, but the branch is unreachable and I
have not
written code pretending otherwise. Worth folding into the document.
## Failing first
The new tests against unmodified source:
```
Expected: 200 / Received: 400 the claim moves the row
Expected: 409 / Received: 400 the same state from a second attempt
Expected: 403 / Received: 400 a different host reporting anything
51 pass, 22 fail
```
Every 400 is the strict validator refusing `claimToken` — the live
break,
reproduced. After:
```
284 pass, 0 fail, 777 expect() calls
```
against a `dev` baseline of **271 pass, 0 fail, 747 expect()** that I
measured
before starting. 13 new tests.
## What this does not verify
- **No agent has ever sent one of these requests.** Both ends are still
held by
tests written against a document. This PR makes the shapes agree by
reading
both, which is the thing rule 1 exists to avoid needing — it is a
repair, not
evidence that the wire works. Only a live round trip settles it.
- **Two attempts racing now happens in the tests, but only in one
process.**
Two tests claim concurrently: one races whole `transition` calls, the
other
fires the guarded updates directly so nothing but the `where` clause can
refuse the second. Both fail with two winners if the check is moved out
of
the write. What they do not reach is two *processes* against a shared
database, which is the real shape — and with one host it cannot happen
in
the field either.
- **Neither guard in that `where` clause is pinned on its own.** The
state
predicate and the holder predicate each cover the other, so removing
either
one alone leaves every test passing. That redundancy is deliberate, but
it
means these tests hold the pair and not the parts.
- **An agent that restarts loses its token**, and is then locked out of
a run it
is still hosting. The host side persists it to disk, which narrows this
a lot,
but nothing on this side can recover from a lost holder and nothing
reaps the
run that results.
- **`min(22)` is not an entropy check.** A caller can present 22
identical
characters and be believed. Nothing on this side can verify randomness.
- The `notHolder` branch of `publishTicket` is reachable only when a run
is past
`requested`; the invariant guard on the claim path is unreachable by
design and
is marked as such rather than tested.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR binds each session run to the agent attempt that claimed it.
- Accepts `claimToken` in state-report and ticket-publish API payloads.
- Atomically records the token during the `requested → starting`
transition.
- Rejects later state or ticket writes from attempts that do not hold
the claim.
- Keeps the token out of serialized session responses.
- Adds route, core, and concurrent PostgreSQL coverage for claim
ownership and guarded updates.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; the prior concern about concurrent claims
is addressed by tests that execute competing guarded updates against
PostgreSQL.
No actionable new failures or repository-rule violations remain, and the
added concurrent coverage verifies that exactly one attempt can claim a
run while only its token can perform subsequent writes.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| packages/core/src/session/index.ts | Adds atomic claim ownership and
enforces it on subsequent state transitions and ticket publication. |
| apps/api/app/routes/session.ts | Extends strict request schemas with
claim tokens and maps holder conflicts to HTTP 409. |
| packages/core/src/session/session.test.ts | Covers claim persistence,
competing attempts, guarded concurrent updates, ticket ownership, and
response non-disclosure. |
| apps/api/test/session.test.ts | Verifies the claim-token wire contract
and route-level conflict behavior. |
| packages/core/src/examples.ts | Adds a correctly shaped claim-token
example for generated API documentation. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant A as Attempt A
participant B as Attempt B
participant API
participant DB as Session row
par Competing claims
A->>API: report starting + token A
API->>DB: UPDATE WHERE requested AND token IS NULL
and
B->>API: report starting + token B
API->>DB: UPDATE WHERE requested AND token IS NULL
end
DB-->>API: Exactly one guarded update succeeds
API-->>A: moved or conflict
API-->>B: moved or conflict
Note over DB: Winning token remains the holder
A->>API: later state/ticket write + token A
API->>DB: "UPDATE WHERE claimToken = token A"
B->>API: later state/ticket write + token B
API-->>B: 409 Another attempt holds this run
```
<sub>Reviews (2): Last reviewed commit: ["test(core): claim two attempts
at once,
..."](
|
||
|
Wanjohi
|
647e5c5264 |
test(core): claim two attempts at once, not one after the other
The mutual exclusion was asserted only through sequential calls, where the winner had already committed before the rival began. That never reaches the case the design is for: both attempts reading the run as unclaimed before either writes. Two tests, because the first can pass for the wrong reason. The concurrent transitions depend on how the transactions interleave; the paired updates skip the read entirely, so nothing but the predicate in the where clause can refuse the second. Both fail with two winners if the check is moved out of the write and left in the read above it. |
||
|
Wanjohi
|
54d5c81edb |
feat(api): hold a run to the attempt that claimed it
The agent side sends a claim token on every write; this side rejected the field outright, so every state report and every ticket publish answered 400. Both bodies now take it. Underneath that, nothing compared a holder. A run was reachable by any caller on the right machine, and a box names exactly one machine — so two attempts polling the same job presented identical credentials and were told apart only by which one's select landed first. That is timing, not a rule, and no caller could be told which case it was in. The row now remembers which attempt holds it. Taking a claim requires there to be no holder; every write after it requires the caller to be the holder. The same state reported by a different attempt is a lost race and not a retry, and is refused whatever the state is - which is the only thing that separates the two 200s from the 409s. The ticket is held to the claim too, for a worse reason than a double start: the client re-reads the address rather than keeping the first, so a ticket written by a losing attempt produces a client that connects, successfully, to a machine running nothing. The holder is never cleared, including on a terminal state, so a settled claim cannot be replayed and a finished run still records which attempt ran it. It is not in what goes out - holding one permits writing to a run, and the owner reading their own session is not the holder. |
||
|
KAAL1 (Bingus)
|
2faf7d77db |
feat(nesinit): mount what the descriptor names, and relay the layer it cannot read (#320)
Stacked on #319, which has PID 1, the channel and the trait but mounts
nothing.
Review that one first; this PR is the descriptor half.
- **The shares are mounted.** A tag names an export, the descriptor
names where
it lands, and every share goes on `nosuid` and `nodev` whether or not it
is
writable — a share is data handed to the guest, and no descriptor has a
way
to ask for a setuid binary or a device node in one. Mounting needs
privileges a test does not have, so the arguments and flags are derived
by a
function the tests assert; that is where the read-only decision lives.
- **Progress is two messages, not one.** `mounted` / `mount_failed` stay
apart
from `started` / `start_failed`, because a share that did not appear and
a
command that did not run want different things looked at. A failure
carries
the reason the operating system gave, verbatim, and the path it happened
on.
- **The second layer is relayed and never read.** Envelopes cross a unix
socket
to the workload and come back the same way. `body` is a string rather
than
nested JSON on purpose: a document this component can index into is a
document it can grow to depend on, and then the layer is not opaque any
more
and the boundary it exists to draw is gone.
- **An envelope is never logged** — not the body, not truncated, not at
debug
level. The channel name and a byte count are the whole of what may be
said
about one. `Payload`'s `Debug` is written by hand for the same reason.
- **A write to a channel nobody reads now ends the session** the same
way a
closed read does, and stops the workload. A caller that stopped
listening has
also stopped being able to say stop; that was two outcomes and is one.
## The tests, failing first
Progress reporting, with the mount result dropped (the state this branch
started from):
```
running 11 tests
test session::tests::an_unreadable_line_does_not_end_a_session ... ok
test session::tests::a_stop_is_idempotent_and_does_not_end_the_session ... ok
test session::tests::the_guest_speaks_first_and_says_its_version ... ok
test session::tests::a_closed_channel_stops_the_workload ... ok
test session::tests::the_descriptor_mounts_and_starts_what_it_names ... ok
test session::tests::an_envelope_crosses_the_session_in_both_directions_unread ... ok
test session::tests::a_share_that_will_not_mount_is_refused_before_anything_starts ... ok
test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... FAILED
test session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount ... FAILED
test session::tests::a_signalled_workload_is_reported_as_signalled ... FAILED
test session::tests::a_relay_nothing_is_on_does_not_end_a_session ... FAILED
failures:
---- session::tests::an_exit_is_reported_and_the_workload_is_not_started_again stdout ----
thread 'session::tests::an_exit_is_reported_and_the_workload_is_not_started_again' (312969) panicked at apps/nesinit/src/session.rs:248:13:
assertion `left == right` failed
left: Started
right: Mounted
---- session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount stdout ----
thread 'session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount' (312963) panicked at apps/nesinit/src/session.rs:460:9:
assertion `left == right` failed
left: StartFailed { reason: "ENOENT: /usr/bin/workload" }
right: Mounted
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- session::tests::a_signalled_workload_is_reported_as_signalled stdout ----
thread 'session::tests::a_signalled_workload_is_reported_as_signalled' (312966) panicked at apps/nesinit/src/session.rs:248:13:
assertion `left == right` failed
left: Started
right: Mounted
---- session::tests::a_relay_nothing_is_on_does_not_end_a_session stdout ----
thread 'session::tests::a_relay_nothing_is_on_does_not_end_a_session' (312964) panicked at apps/nesinit/src/session.rs:248:13:
assertion `left == right` failed
left: Started
right: Mounted
failures:
session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount
session::tests::a_relay_nothing_is_on_does_not_end_a_session
session::tests::a_signalled_workload_is_reported_as_signalled
session::tests::an_exit_is_reported_and_the_workload_is_not_started_again
```
The read-only flag, ignored:
```
running 9 tests
test shutdown::tests::a_workload_that_leaves_in_time_is_not_killed ... ok
test shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power ... ok
test shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes ... ok
test workload::tests::a_failure_names_the_path_it_happened_on ... ok
test workload::tests::a_writable_share_is_still_mounted_without_devices_or_setuid ... ok
test workload::tests::a_read_only_share_is_mounted_read_only ... FAILED
test session::tests::a_closed_channel_stops_the_workload ... ok
test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... ok
test session::tests::a_signalled_workload_is_reported_as_signalled ... ok
failures:
---- workload::tests::a_read_only_share_is_mounted_read_only stdout ----
thread 'workload::tests::a_read_only_share_is_mounted_read_only' (311963) panicked at apps/nesinit/src/workload.rs:212:9:
assertion `left == right` failed
left: 0
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
workload::tests::a_read_only_share_is_mounted_read_only
```
The relay quoting a line it could not decode — which is the second way a
body
reaches a log line, and the reason the failure path logs a length and
nothing
else:
```
running 2 tests
test payload::tests::an_envelope_crosses_in_both_directions_untouched ... ok
test payload::tests::nothing_the_relay_logs_contains_a_body ... FAILED
failures:
---- payload::tests::nothing_the_relay_logs_contains_a_body stdout ----
thread 'payload::tests::nothing_the_relay_logs_contains_a_body' (312716) panicked at apps/nesinit/src/payload.rs:202:9:
a body reached a log line:
2026-09-04T21:11:56.619025Z WARN nesinit::payload: ignoring an envelope that would not decode bytes=62 line={"channel":"identity","body":"a-credential-nobody-should-read"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
payload::tests::nothing_the_relay_logs_contains_a_body
test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 17 filtered out; finished in 0.06s
```
And the first way: a derived `Debug` instead of the hand-written one.
```
running 6 tests
test lifecycle::tests::a_mount_failure_keeps_its_reason_verbatim ... ok
test lifecycle::tests::a_signalled_exit_is_not_a_zero_exit ... ok
test lifecycle::tests::a_line_round_trips ... ok
test lifecycle::tests::defaults_cover_what_a_caller_may_leave_out ... ok
test lifecycle::tests::an_envelope_does_not_print_its_body ... FAILED
test lifecycle::tests::an_envelope_body_stays_a_string_in_both_directions ... ok
failures:
---- lifecycle::tests::an_envelope_does_not_print_its_body stdout ----
thread 'lifecycle::tests::an_envelope_does_not_print_its_body' (313791) panicked at crates/nesprotocol/src/lifecycle.rs:290:9:
the body reached a log line: Payload { channel: "identity", body: "a-credential-nobody-should-read" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
lifecycle::tests::an_envelope_does_not_print_its_body
test result: FAILED. 5 passed; 1 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesprotocol --lib`
```
All green after: 19 unit tests, 3 against real forked children, 15 in
`nesprotocol`.
## What this does not verify
- **Nothing has been mounted.** No `virtiofs` share has been mounted by
this
code, in a VM or anywhere else. What is tested is the source, target and
flag
word handed to the mount call; that the call succeeds against a real
virtio
transport, that the mount point is where a workload then finds its
files, and
that a `uid` mismatch surfaces as the permission error this is written
to
produce, are all unverified.
- **The relay has never carried a real workload's traffic.** Two
processes, a
real unix socket and bytes that come back unchanged is what the test
shows.
Whether the socket path is the right mechanism is openly a guess, and it
is
meant to be replaceable without anything above it moving.
- **"Never logged" is enforced more narrowly than it reads.** The
hand-written
`Debug` and the failure path's length-only line are both tested. The
capture
test cannot reliably see debug-level lines: callsite interest is cached
process-wide, so a line another test in the same binary reached first
never
arrives in the capture. A future `{:?}` on a whole envelope at debug
level
would not necessarily be caught by these tests, only by the `Debug` impl
keeping its shape.
- **`geometry` is parsed and carried, and nothing consumes it.** This
component
does not start the guest's own services yet. `ticket` exists as a
message
with no producer wired to it.
- **Still no VM, still no vsock, still no `uid` drop**, as in #319, and
no
number in this PR is measured.
- **No third-party workload has gone through any of this.** The claim
that a
descriptor plus a set of shares is enough to run something we did not
write
is untested, and our own workload is the weakest possible witness for
it.
## Since review
`d4d473f` — the relay may not stall the session and may not buffer
without end,
plus a descriptor with a nul byte in it is refused by name. Failing
first, in
order:
The session held still behind a workload that was not reading:
```
running 1 test
test session::tests::a_relay_that_is_not_draining_does_not_stall_the_session ... FAILED
failures:
---- session::tests::a_relay_that_is_not_draining_does_not_stall_the_session stdout ----
thread 'session::tests::a_relay_that_is_not_draining_does_not_stall_the_session' (326881) panicked at apps/nesinit/src/session.rs:670:10:
the session stalled on the relay: Elapsed(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
session::tests::a_relay_that_is_not_draining_does_not_stall_the_session
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 5.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
A frame with no end to it:
```
running 1 test
test payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest ... FAILED
failures:
---- payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest stdout ----
thread 'payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest' (327888) panicked at apps/nesinit/src/payload.rs:375:10:
the relay is still assembling a frame that never ends: Elapsed(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 5.01s
error: test failed, to rerun pass `-p nesinit --lib`
```
And a tag that was quietly emptied instead of refused:
```
running 1 test
test workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name ... FAILED
failures:
---- workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name stdout ----
thread 'workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name' (327380) panicked at apps/nesinit/src/workload.rs:274:40:
an empty source would have been mounted: ("", "/mnt/user", 6)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
One behaviour changed rather than only hardened, and it is worth a
reviewer's
eye: **nothing is queued for a workload that is not on the relay.** An
envelope
that arrives with nobody connected is dropped, as is one that arrives
faster
than the workload reads. That follows the layer's own rule — what
crosses it is
re-sent when it changes, so a held copy is a stale copy — but it does
mean a
sender that assumes delivery is wrong to. Nothing here retries, and
nothing
tells the far end that a particular envelope was dropped.
23 unit tests, 5 against real forked children, 15 in `nesprotocol`.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR mounts descriptor-defined virtiofs shares, reports mount and
process-start progress independently, and relays opaque payload
envelopes between the host channel and workload Unix socket. Changes
since the previous review also bound relay frames, prevent relay
backpressure from stalling lifecycle handling, reject descriptor strings
containing NUL bytes, and track whether a reaped PID remains valid.
- Mounts shares at descriptor-selected targets with `nosuid`, `nodev`,
and optional read-only flags.
- Adds bidirectional opaque payload forwarding with bounded,
non-blocking queues and body-safe logging.
- Adds distinct mounted/start lifecycle responses and failure reporting.
- Adds capped newline-delimited relay frames and drops stale or
backpressured envelopes.
- Reworks workload tracking to avoid signaling a PID after its exit has
been delivered.
<h3>Confidence Score: 5/5</h3>
The reviewed changes appear safe to merge, with no accepted new findings
or outstanding previous root-thread findings.
The resolved relay-stall, unbounded-frame, and invalid-NUL findings are
addressed by non-blocking delivery, capped frame assembly, and explicit
descriptor validation. The protocol-version concern was correctly
withdrawn under the coordinated version-2 deployment model. The
remaining PID check-to-signal race duplicates an existing prior comment
and therefore is not reposted or counted as a new finding.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/nesinit/src/payload.rs | Adds the bounded, bidirectional
Unix-socket payload relay with non-blocking delivery and body-safe
logging. |
| apps/nesinit/src/session.rs | Integrates payload events with lifecycle
handling and separately reports mount and process-start outcomes. |
| apps/nesinit/src/workload.rs | Implements descriptor-driven virtiofs
mounts and switches process signaling to tracked reaper state. |
| apps/nesinit/src/reap.rs | Adds shared reaped-state tracking so
callers stop treating a delivered PID as the workload. |
| crates/nesprotocol/src/lifecycle.rs | Extends lifecycle messages with
mount progress and opaque payload envelopes while redacting payload
bodies from Debug output. |
| apps/nesinit/src/main.rs | Starts the payload relay before the
workload session and wires bounded relay ports into session handling. |
| apps/nesinit/README.md | Documents mount behavior, payload opacity,
delivery semantics, frame limits, and progress reporting. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant H as Host
participant N as nesinit
participant M as virtiofs mounts
participant W as Workload
H->>N: Boot descriptor
N->>M: Mount descriptor shares
M-->>N: Success or failure
N-->>H: mounted / mount_failed
N->>W: Start command
N-->>H: started / start_failed
H->>N: Payload envelope
N-->>W: Non-blocking Unix-socket relay
W->>N: Payload envelope
N-->>H: Payload envelope
W-->>N: Exit
N-->>H: workload_exited
```
<sub>Reviews (3): Last reviewed commit: ["fix(nesinit): the relay may
not stall
th..."](
|
||
|
Wanjohi
|
217e82a9a8 |
feat(auth): email is the root of an account, and Steam is a connection (#318)
## 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..."](
|
||
|
KAAL1
|
a94b323edb |
fix(nesinit): the relay may not stall the session, and may not buffer without end
Three problems in the relay, all of them found in review. Handing an envelope over waited for room. That loop also carries stop, shutdown and the workload's exit, so a workload slow to read its own mail — or one that never connected — could hold the lifecycle layer still behind it. It never waits now: an envelope that will not fit is dropped, which costs nothing, because what crosses this layer is re-sent when it changes. Envelopes were queued for a workload that was not there. The queue filled with copies that would be stale by the time anyone connected, and filling it was what stalled the session. Nothing is held while the socket has nobody on it. A frame had no maximum length. The workload can write for as long as it likes without ever sending a newline, and the process assembling that is the one the kernel has been told not to kill, so the memory it takes comes out of everything else in the guest. Past 64 KiB the connection is dropped and the relay waits for the next one; the failure says how long the frame got and nothing about what was in it. Also, a tag or a mount point with a nul byte in it was quietly turned into an empty string, so an unmountable descriptor arrived later as a mount failure about something else, after the mount point had already been created. It is refused by name now, before anything is created. The relay's tests grew a harness that waits for the connection to be carried before sending anything down it, because dropping what arrives with nobody connected made "connected" something a test has to establish rather than assume. |
||
|
KAAL1
|
7b99f49f62 |
feat(nesinit): mount what the descriptor names, and relay the layer it cannot read
Builds on the previous change, which had PID 1, the channel and the trait but mounted nothing. The shares are mounted now: a tag names an export, the descriptor names where it lands, and every share goes on nosuid and nodev whether or not it is writable — a share is data handed to the guest, and no descriptor has a way to ask for a setuid binary or a device node in one. Mounting needs privileges a test does not have, so the arguments and flags are derived by a function the tests can assert, which is where the read-only decision lives. Progress is reported in two messages rather than one. A share that did not mount and a command that did not run are not the same incident, and each carries the reason the operating system gave and the path it happened on: a permission error on a named directory can be acted on, where "the share did not mount" cannot. The second layer is relayed and never read. Bytes arrive on the channel in an envelope, cross a unix socket to the workload, and come back the same way. The body is a string rather than nested JSON on purpose: a document this component can index into is a document it can grow to depend on, and then the layer is no longer opaque and the boundary it exists to draw is gone. An envelope is never logged — not the body, not truncated, not at debug level — and the channel name with a byte count is the whole of what may be said about one. The type's Debug is written by hand for the same reason, because a derived one puts the body one careless format string away from a log line. A write to a channel nobody is reading now ends the session the same way a closed read does. A caller that has stopped listening has also stopped being able to say stop, which is one situation and was two outcomes. The guest listens on the relay socket and the workload dials in, which is the convention the other guest sockets already use and removes the startup ordering problem: a workload that is not running yet has simply not connected yet. |
||
|
KAAL1 (Bingus)
|
736c0013e9 |
feat(nesinit): PID 1 for a box — reaping, ordered shutdown, and one channel out (#319)
PID 1 inside a box. A microVM has no init unless something is it, and
three of
the jobs belong to nothing else in the guest: reaping whatever the
workload
orphans, turning a signal into an ordered shutdown, and holding the
guest end
of the one channel out.
- `reap.rs` — reaping, plus the subreaper bit and taking init out of the
OOM
killer's reach.
- `shutdown.rs` — the order, behind a trait so it can be asserted
without a VM:
workload first and alone, then everything else, then flush, then power
off.
- `session.rs` — the exchange. The guest speaks first with its protocol
version, is handed one boot descriptor, reports, and stops. Generic over
the
byte stream, so the whole protocol is testable over an in-memory pipe.
- `workload.rs` — one trait the descriptor drops into, a real process
behind
it, and a double.
- `nesprotocol::lifecycle` — the types, behind a feature that is off by
default so the media components keep building without serde.
It reports and does not supervise: when the workload ends, the exit goes
up the
channel and the session is over. Nothing here restarts anything.
**Mounting is not implemented in this PR.** The descriptor's `mounts`
are
refused rather than ignored, and the next PR in the stack implements
them.
### On the OOM killer
`refuse_oom_kill()` writes `-1000` to this process's own
`oom_score_adj`, and
that part is init's job: everything else in the guest exiting is a
message up
the channel, whereas init exiting takes the channel with it, and the far
end
sees a box that stopped answering for no stated reason.
The rest is the image's job and cannot be done from here. Making the
workload
the *preferred* victim means scoring processes this component did not
start, so
the image has to leave the workload's score at or above the default and
must
not lower it for the guest's own services either.
## The tests, failing first
Reaping, with `reap_exited()` returning nothing:
```
running 3 tests
test a_child_that_exits_is_reaped_with_its_code ... FAILED
test a_child_that_is_killed_is_reaped_as_signalled ... FAILED
test an_orphan_is_reaped_by_whoever_inherits_it ... FAILED
failures:
---- a_child_that_exits_is_reaped_with_its_code stdout ----
thread 'a_child_that_exits_is_reaped_with_its_code' (304365) panicked at apps/nesinit/tests/reaping.rs:52:32:
the child was left a zombie
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- a_child_that_is_killed_is_reaped_as_signalled stdout ----
thread 'a_child_that_is_killed_is_reaped_as_signalled' (304366) panicked at apps/nesinit/tests/reaping.rs:66:32:
the child was left a zombie
---- an_orphan_is_reaped_by_whoever_inherits_it stdout ----
thread 'an_orphan_is_reaped_by_whoever_inherits_it' (304367) panicked at apps/nesinit/tests/reaping.rs:98:5:
the child was left a zombie
failures:
a_child_that_exits_is_reaped_with_its_code
a_child_that_is_killed_is_reaped_as_signalled
an_orphan_is_reaped_by_whoever_inherits_it
test result: FAILED. 0 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 15.02s
```
Shutdown, with one signal to everything instead of an order:
```
running 3 tests
test shutdown::tests::a_workload_that_leaves_in_time_is_not_killed ... ok
test shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes ... FAILED
test shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power ... FAILED
failures:
---- shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes stdout ----
thread 'shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes' (304568) panicked at apps/nesinit/src/shutdown.rs:105:79:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power stdout ----
thread 'shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power' (304569) panicked at apps/nesinit/src/shutdown.rs:80:9:
assertion `left == right` failed
left: ["signal_rest", "kill_rest", "flush_disks", "power_off"]
right: ["signal_workload", "await_workload", "signal_rest", "kill_rest", "flush_disks", "power_off"]
failures:
shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes
shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power
test result: FAILED. 1 passed; 2 failed; 0 ignored; 0 measured; 8 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
The handshake, with the version left where it was before the bump:
```
running 1 test
test session::tests::the_guest_speaks_first_and_says_its_version ... FAILED
failures:
---- session::tests::the_guest_speaks_first_and_says_its_version stdout ----
thread 'session::tests::the_guest_speaks_first_and_says_its_version' (304772) panicked at apps/nesinit/src/session.rs:180:9:
assertion `left == right` failed
left: Ready { protocol_version: 1 }
right: Ready { protocol_version: 2 }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
session::tests::the_guest_speaks_first_and_says_its_version
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 10 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
And the property most likely to be quietly regressed later — a
supervisor loop
that restarts what it started, instead of reporting:
```
running 1 test
test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... FAILED
failures:
---- session::tests::an_exit_is_reported_and_the_workload_is_not_started_again stdout ----
thread 'session::tests::an_exit_is_reported_and_the_workload_is_not_started_again' (304968) panicked at apps/nesinit/src/session.rs:237:9:
assertion `left == right` failed: an exit is reported, never restarted
left: 2
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
session::tests::an_exit_is_reported_and_the_workload_is_not_started_again
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 10 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
All green after: 11 unit tests, 3 against real forked children, plus 3
new in
`nesprotocol`.
## What this does not verify
- **Nothing has run in a VM, and nothing has run as PID 1.** Orphan
reparenting is exercised with `PR_SET_CHILD_SUBREAPER` in a test
process,
which is the same kernel mechanism but not the same privilege.
- **No real vsock connection has been made.** The protocol is tested
over an
in-memory pipe; the dial itself — the address, the port, a listener that
is
not there — is unexercised, and the deliberate no-retry behaviour has
never
met a refused connection.
- **The real shutdown is untested.** The order is asserted through a
double;
`kill(-1)`, `sync` and the power-off call themselves need a guest, and a
test
process must not make them.
- **The OOM write is unchecked.** It warns and continues where there is
no
procfs, and nothing here confirms the kernel honoured the score.
- **Dropping to `uid`/`gid` before exec is unexercised** — it needs
privileges
a test does not have, so the `pre_exec` path has run in no test.
- **No number here is measured.** Nothing in this PR claims a timing, a
rate
or a count from hardware.
- **A version mismatch is not refused by this end.** The guest announces
its
version in its first line and the far end compares; as the layer stands
there
is nothing for the guest to compare against, so "both ends refuse on
mismatch" is only half-implementable. Worth settling in the channel's
specification before either end grows a second version, and I would
rather
raise it than invent a message for it here.
## Since review
`cb8f37a` — three fixes, all from the review, described in its message.
The one
worth naming here is that the reaper is now the only thing in the
component
that calls `wait`: it hands each exit to whoever asked for that pid, and
registering interest holds the same lock the delivery takes, so an exit
that
happens before its caller is registered is delivered rather than
dropped.
There is a test for exactly that, and it fails without the lock:
```
running 1 test
test an_exit_that_happens_before_the_caller_is_registered_is_not_lost ... FAILED
failures:
---- an_exit_that_happens_before_the_caller_is_registered_is_not_lost stdout ----
thread 'an_exit_that_happens_before_the_caller_is_registered_is_not_lost' (321463) panicked at apps/nesinit/tests/reaping.rs:171:9:
the exit was dropped on the way through
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
an_exit_that_happens_before_the_caller_is_registered_is_not_lost
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 4 filtered out; finished in 5.20s
error: test failed, to rerun pass `-p nesinit --test reaping`
```
Also, and still not verified: the ordered shutdown's real calls now
include
signalling and waiting for one pid rather than every child, and none of
that
has run in a guest either.
`071241f` — a pid stops being the workload's the moment it is reaped, so
a stop
or a kill during shutdown can no longer land on whatever the kernel gave
that
number to next. One window is left, between the reap and the delivery,
and
closing it needs a handle the kernel keeps rather than a number — noted
below
rather than papered over.
- **A pid is still a number here.** Between a reap and the exit being
handed
on, a freed pid is briefly treated as the workload's. Nothing has hit
that
window, and nothing can until a guest recycles pids under load; a
`pidfd`
would remove the class of bug rather than narrow it.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR adds `nesinit`, a guest PID 1 implementation that reaps orphaned
children, exchanges lifecycle messages over vsock, launches one
workload, and performs ordered shutdown.
- Adds a centralized child reaper and waiter registry.
- Adds workload launch, identity changes, exit reporting, and explicit
rejection of unsupported mounts.
- Adds the lifecycle protocol behind an optional `nesprotocol` feature.
- Adds ordered workload-first shutdown and associated tests.
- Since the previous review, tracks whether the watched workload PID is
still live to reduce stale-PID signaling risk.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge based on the accepted findings and the
resolved state of all previous review threads.
No new actionable finding remains after excluding the stale-PID race as
a duplicate of a manually resolved previous thread and confirming that
the stale state left by the shutdown-only wait path is not subsequently
used to signal a workload PID. Previous thread PRRC_kwDOLnCyk87q1Eew was
manually resolved without explanation.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/nesinit/src/reap.rs | Adds centralized child reaping, waiter
registration, exit delivery, and shared workload-running state. |
| apps/nesinit/src/workload.rs | Implements workload launch and
signaling through a watched PID whose delivered exit marks it inactive.
|
| apps/nesinit/src/main.rs | Wires the reaper, vsock session, workload
handle, and real ordered-shutdown operations together. |
| apps/nesinit/src/session.rs | Implements the versioned one-descriptor
lifecycle exchange and reports one workload exit without restarting it.
|
| apps/nesinit/src/shutdown.rs | Encodes and tests workload-first
shutdown followed by guest services, disk flush, and power-off. |
| crates/nesprotocol/src/lifecycle.rs | Defines the feature-gated
lifecycle protocol types shared by the guest and host. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[nesinit starts] --> B[Become subreaper and adjust OOM score]
B --> C[Connect to host over vsock]
C --> D[Send protocol version]
D --> E[Receive boot descriptor]
E --> F[Start workload and register PID]
F --> G{Session event}
G -->|Workload exits| H[Reaper delivers exit]
H --> I[Report workload exit]
G -->|Stop or channel closes| J[Signal workload]
G -->|Shutdown or session ends| K[Ordered shutdown]
I --> K
J --> K
K --> L[Stop workload]
L --> M[Stop remaining processes]
M --> N[Sync disks]
N --> O[Power off]
```
<sub>Reviews (3): Last reviewed commit: ["fix(nesinit): a pid stops
being the
work..."](
|
||
|
Wanjohi
|
304bb1f2ef |
fix(auth): count sign-in codes against the mailbox, not the browser
The cap on how many codes a sign-in could ask for was held per attempt, keyed by a value in the caller's own cookie. That bounds nothing. The caller decides how many attempts to start, and starting a fresh one costs them a discarded cookie — so either replaying an older cookie or simply beginning again walked straight around it, and the only thing left spacing the mail out was the interval between sends. The count now sits against the claim, over a window. That is the thing being protected: the mailbox belongs to somebody who did not ask to hear from us, and whoever is pointing at it is not the party to trust with the tally. A resend also left the previous code live, with a budget of guesses of its own. Several resends therefore meant several working codes and several times the chances at them, which made asking for a new code the cheapest way to buy more tries at the old one. A new code now retires the one before it. Reported against the replay path. The replay was real and the same hole was wider than that: starting a new attempt needed no replay at all. |
||
|
KAAL1
|
071241f944 |
fix(nesinit): a pid stops being the workload's the moment it is reaped
A pid is only a name for a process until that process is reaped; after that the kernel may hand the same number to something else. The handle kept the number, so a stop or a kill issued during shutdown — which every session outcome reaches — could land on a process nobody meant, and the one aimed at the workload would have been a SIGKILL. The registry now clears the flag as it delivers the exit, in the same call, and nothing signals a pid whose flag is down. Waiting for the workload on the way out takes the same answer: already reaped is already gone. There is a window left, between the reap and the delivery, and closing it entirely needs a handle the kernel keeps rather than a number. Recorded rather than papered over. |
