The Proof That Was Green for the Wrong Reason

The bug was simple enough to state in one sentence: if you open a terminal in sh0 and someone takes away your admin rights, you keep the terminal.

Not for a few seconds. For as long as you leave the tab open.

The fix took an afternoon. Proving the fix took the rest of the day, and the interesting part is not the fix. It is that my first proof came back green on both sides -- green before the fix, green after -- and it looked exactly like a proof that worked.

This post is about that, and about what an adversarial reviewer found in the fix afterwards.

Where the hole came from

Three days earlier we had closed a different issue. sh0 read the caller's role out of the JWT claims, which meant a demotion only took effect when the token expired -- up to 24 hours later. We moved the read into the database, in a single funnel that both the HTTP path and the WebSocket path go through:

rustpub(crate) async fn session_from_claims(
    claims: sh0_auth::jwt::Claims,
    pool: &Arc<sh0_db::DbPool>,
) -> Result<AuthUser, ApiError> {
    // ... interstitial-role guard first, then:
    let me = spawn_blocking(move || sh0_db::User::find_by_id(&pool, &uid)).await??;
    Ok(AuthUser { user_id: me.id, role: me.role, scope: None })
}

That closed the window on every HTTP request and at the opening of a WebSocket. It said nothing about a WebSocket that was already open.

sh0 has five of those:

All five call authenticate_ws exactly once, before ws.on_upgrade, and never again. After the upgrade, the frame loop consults nothing. Two of those five hand out command execution. One of them hands out uid=0.

So the real shape of the bug: demoting someone from owner to viewer does not take their root shell away. It takes away their ability to open a new one.

Three ways to fix it, and the most correct one is a trap

The issue card listed three forms and deliberately refused to pick:

1. Revalidate periodically inside the frame loop.
2. Revalidate on sensitive actions only -- a terminal frame that writes, not a log frame that reads.
3. Close a user's live sessions from the code path that writes the role.

Form 3 is the one that reads best. It is event-driven instead of polling. It has no interval, so no window. It is what you would design on a whiteboard.

It is also the one that does not work, and the reason is worth sitting with: form 3 only sees role changes that go through the HTTP handler.

How does an operator actually revoke someone in a hurry? They are on the box. They open the database. They run an UPDATE. That change never touches the handler, so form 3 fires nothing. It closes a path to the hole, not the hole. And it needs a registry of live connections per user, which does not exist.

Form 2 collapses on inspection too. Its proposed boundary is "a terminal frame that writes versus a log frame that reads" -- but on a terminal, every client-to-server frame is a keystroke, therefore a write. Form 2 degenerates into one database read per character typed, which is more expensive than form 1, or into a debounce, which is form 1 under a different name.

So: form 1, periodic revalidation, 30-second interval. The argument against it is cost -- one read per interval per open connection. Let us put a number on that instead of an intuition. An sh0 instance serves one team on its own server. Open WebSockets are the dashboard tabs actually open: logs, deploy stream, terminal. A busy instance holds maybe ten. A hundred would be implausible. At a hundred connections and 30 seconds, that is 3.3 reads per second against a point index on a local SQLite in an already-warm pool. The dashboard's own polling produces more than that by accident -- we had an issue where a self-invalidating $effect hammered /api/v1/updates/check hard enough to push the whole API into 429s.

The cost argument does not survive its own measurement.

The guard

One module, one background task per connection:

rustpub fn watch<F>(state: &AppState, auth: &AuthUser, authorize: F) -> AuthzGuard
where
    F: Fn(&sh0_db::DbPool, &AuthUser) -> Result<(), ApiError> + Send + Sync + 'static,

The key decision is in the signature. The guard does not re-check "role >= something". Each handler hands it the exact authorization closure it just ran, and the guard replays that, with the role re-read from the database:

rust// handlers/terminal.rs
let authz = crate::ws_authz::watch(&state, &auth_user, {
    let app_id = app_id.clone();
    move |pool, auth| require_app_access(pool, auth, &app_id, "developer").map(|_| ())
});

"developer" for the app terminal. "viewer" for logs. owner|admin for the host terminal. The revalidation is not an approximation of the opening check; it is the opening check.

Then a third branch in the existing select, and a close frame with a reason:

rusttokio::select! {
    _ = d_to_ws => debug!("docker->ws half exited first"),
    _ = ws_to_d => debug!("ws->docker half exited first"),
    reason = authz.revoked() => { revocation = Some(reason); }
}

Two things we deliberately do not revalidate. Token freshness: the access token lives 24 hours and the browser refreshes it without the WebSocket, which holds the old one, ever knowing. Closing on expiry would kill every terminal on every refresh cycle -- a regression, not a fix. And a database outage does not close anything: pool saturation is not a revocation.

The proof that was green for the wrong reason

sh0's engineering rule is blunt: a fix is not "fixed" until there is a dated observation on a real target, with the command and its output. Not a unit test. Not a passing audit. An observation.

So: open a terminal as owner, send one keystroke to show the shell is alive, then go silent. Write viewer into the database. Watch. A proof that reopens the connection proves nothing.

Red, on the shipped binary:

[11:03:41] poignee de main : HTTP/1.1 101 Switching Protocols
[11:03:41] sortie du shell : 'uid=1000 gid=1000 groups=1000'
[11:03:51] *** ROLE ECRIT EN BASE : viewer *** (le client n'a rien envoye et n'enverra rien)
[11:05:21] => ROUGE : 90s apres la retrogradation, la connexion est TOUJOURS OUVERTE
[11:05:21] et elle repond encore : opcode=2 b'whoami\r\n'

Ninety seconds after the demotion, the shell still answers. That is the bug, on video.

Then I built the fix, cut a release candidate, installed it, and ran the identical witness.

It came back red.

The connection stayed open. Still answered whoami. Same as before the fix.

The tempting conclusion is that the guard does not work. The correct move is to stop and measure the thing you have been assuming. So: with the role set to viewer, what does a brand new connection get?

role=owner   app-terminal=101  host-terminal=101
role=viewer  app-terminal=101  host-terminal=403

101 at viewer. The app terminal opens fine for a demoted user. Which means the authorization holds, which means there was nothing to revoke, which means the guard was behaving correctly and my witness was measuring a case whose correct outcome is do nothing.

Why? The account is a member of that app's project with project role admin:

rustpub fn resolve_project_access(pool, user, project_id) -> Result<ProjectAccess, ApiError> {
    if user.role == "owner" || user.role == "admin" {
        return Ok(/* project role: admin */);
    }
    match ProjectMember::find_by_project_and_user(pool, project_id, &user.user_id) { ... }
}

Demoting the global role to viewer drops out of the first branch and into the membership lookup -- which succeeds, with admin. Access is retained. That is not a bug. That is RBAC working: taking away someone's global role does not take away their project membership.

And the consequence for the witness is the part worth remembering: the red run was invalid too. The connection staying open on the old binary was correct behaviour, for the same reason. I had a red/green pair where both halves were green, dressed up as a bug and a fix.

The rerun targeted an app with no project, where the global role decides alone:

=== VERT -- rc54 -- terminal d'application ===
[11:56:57] *** ROLE ECRIT EN BASE : viewer ***
[11:57:17] TRAME DE FERMETURE recue apres 19.9s -- code=1008 raison='App is not assigned to a project'

=== VERT -- rc54 -- terminal HOTE ===
[11:57:47] TRAME DE FERMETURE recue apres 19.9s -- code=1008 raison='Admin or owner role required for host terminal'

Plus the witness that matters just as much and is easy to skip: leave the role unchanged for eighty seconds, across three revalidation beats, and confirm the connection survives. A guard that closes sessions it should not close is a worse product than the bug it replaces.

The lesson is not "measure twice." It is more specific than that. A red/green pair convinces because the two halves differ in exactly one thing: the binary. If the scenario itself does not exercise the defect, both halves come back the same, and red is the one that quietly lies -- it looks like the bug reproducing when it is just the system saying "nothing to do here." Before trusting red, prove the scenario can go green for a reason other than your fix.

The test that caught my own bug

The guard hands its verdict over a oneshot channel. deploy_stream.rs already ticks every 500 ms, so instead of a select branch it just asks:

rustif let Some(reason) = authz.revoked_now() { /* close */ }

The first version of revoked_now was a one-liner: self.rx.try_recv().ok().

A test failed. Not the test that was supposed to fail -- a test I had written to check that a dead watcher never closes a live connection:

thread 'ws_authz::tests::a_dead_watcher_never_closes_the_connection' panicked at
tokio-1.50.0/src/sync/oneshot.rs:1289:13:
called after complete

Tokio's oneshot::Receiver marks itself complete the first time try_recv sees a dropped sender, and panics on any access after that. In deploy_stream.rs, that receiver is polled every 500 ms. The day the revalidation task died, the entire deploy stream would panic on the second tick.

The fix is a done flag consulted by both accessors. The point is the discovery path: this was not caught by review, and it was not caught by the test that was aimed at it. It was caught by a test aimed somewhere else, failing for a reason I had not imagined.

What the adversarial reviewer found

sh0's process runs a separate, read-only session over every non-trivial change before it is pushed. Fresh context, no attachment to the design, briefed as a senior reviewer with a targeted checklist rather than "review this."

Verdict: GO-WITH-FIXES. The failure I was most worried about -- a guard dropped early, silently cancelling its own revalidation -- was not there; all five handlers carry it by value to the end. It found something better.

database_servers/logs.rs captured a clone of the server row.

rust// what I wrote
let authz = watch(&state, &auth_user, {
    let server = server.clone();
    move |pool, auth| check_server_access(pool, auth, &server, "viewer")
});

check_server_access decides from server.project_id. So this replays the check against a snapshot frozen at connection time. Reattach the server to another project, detach it, delete it -- the guard keeps evaluating the old row and concludes "still authorized," forever.

The other three handlers re-read their resource every tick, because App::find_by_id lives inside require_app_access. I had reproduced their shape without reproducing their property. That is a specific and recurring failure mode when you copy a working pattern: the thing that made it correct was not visible at the call site.

rust// what it is now
let authz = watch(&state, &auth_user, {
    let server_id = server.id.clone();
    move |pool, auth| {
        let fresh = DatabaseServer::find_by_id(pool, &server_id)?;
        check_server_access(pool, auth, &fresh, "viewer")
    }
});

Isolating a witness for that meant paying the morning's trap again from the other side: check_server_access short-circuits on owner|admin before it ever looks at the project, so with an owner account nothing moves. Demote the account to developer -- where its access comes from project membership -- and move only the resource:

=== VERT -- rc55 -- role developer, acces par APPARTENANCE au projet, puis SERVEUR DEPLACE ===
[12:54:44] *** SERVEUR RATTACHE AU PROJET : projet-inexistant-p229 *** (le compte n'a pas bouge)
[12:55:04] TRAME DE FERMETURE recue apres 20.0s -- code=1008 raison='You do not have access to this project'

The account did not change. Only the resource moved. Under the frozen snapshot, this stays open.

Three quieter findings from the same report:

The revoke branch was right by accident. I had Err(ApiError::Database(_)) => Revoked -- reasoning that a Database error here means the resource is gone. True today, because the three authorization closures only ever produce Database for NotFound. But ApiError carries #[from] sh0_db::Sh0DbError, so the first ? anyone puts on a database read inside a closure would turn pool saturation into an immediate disconnect, bypassing the outage tolerance in the dangerous direction. Now restricted to Database(NotFound), in a pure function with its own tests -- the reviewer also pointed out that the code deciding whether to cut a connection had no test coverage at all.

A safety number in a comment was wrong. "Three consecutive failures, so ninety seconds of tolerance." The pool is 20 connections with no connection_timeout, so r2d2's default of 30 s applies: under saturation each tick costs the interval plus up to thirty seconds of waiting. Three ticks was up to 180 seconds, not 90. An attacker who can cause saturation doubles the window, and my comment hid it. Tolerance is now a measured duration, which does not depend on how fast ticks go.

Close reasons were unbounded. RFC 6455 caps a control frame payload at 125 bytes. The reasons come from format! on database errors. Truncated at the emission point, on a character boundary.

The hole we named instead of hiding

The same report found something the fix does not cover: authenticate_ws accepts an API key as readily as a JWT, and the guard only ever re-reads the users row. It never replays verify_api_key. So a key that is deleted, revoked, or past its expires_at during a live connection closes nothing, as long as its owner's role holds -- which, in the normal case, is forever. Same class of defect as the one we just fixed, on the other authentication method.

Worse, my own comment claimed the opposite: "an API key's scope does not change during a connection." True of the scope string. False of the key's validity, in the direction that matters.

Fixing it means carrying the key's id in AuthUser, a struct read by 55 handlers. That is a different change from this one. So it became its own tracked issue, the module now carries an explicit paragraph naming what it does not revalidate, and the session's ledger went from 10 open issues down to 9 and back up to 10.

Net: zero. That is the honest number, and writing it as "10 → 9" would have been a small lie of the kind that compounds. One issue closed, one of the same family found.

What this costs and what it buys

The window is now bounded by the interval. Thirty seconds, with a unit test that fails if someone stretches it past sixty "to save reads." Before, it was bounded by how long someone felt like leaving a tab open.

There is one residual we did not prove live -- the deploy log stream -- and it is written in the issue card rather than glossed over. Both terminals and the log stream are proven.

The engineering lesson I will carry forward is not about WebSockets. It is that producing an observation is the easy half of proving something. The hard half is establishing that the observation is capable of coming out the other way. My red witness looked exactly like a bug reproducing. It was a system correctly doing nothing, in a scenario I had chosen badly, and I nearly shipped a fix on the strength of it.

A test that cannot fail is not a test. A red that could not have been green is not a red.

This is Part 72 of the sh0 engineering series. The full series documents how sh0 was built from zero to production by a CEO in Abidjan and an AI CTO, with no human engineering team.