Fjord session refresh has no lock and no crash-safe rotation, so a race or a failed write permanently kills the sign-in #272

New issue

Open

opened 2026-09-19 16:19:24 +00:00 by stephen · 1 comment

stephen commented

2026-09-19 16:19:24 +00:00

Owner

A Fjord Account session can be permanently destroyed by its own refresh path. Two independent holes, both in the rotation handling, both ending in invalid_grant with no recovery short of a full browser sign-in.

Paragon rotates the refresh token on every exchange and detects replay, as exchange_refresh documents (src/fjord/oidc.rs:638):

Paragon rotates on every use and detects replay, so the caller MUST persist the returned refresh_token and discard the one it sent.

fj honours the "persist" half, but has no defence for either failure mode rotation introduces.

1. No cross-process lock around exchange-and-persist

Client::request_with_headers refreshes reactively on a 401 (src/client/mod.rs:353), through refresh_bearer (src/client/mod.rs:484) into refresh_session (src/fjord/oidc.rs:1001). Nothing serialises that across processes.

Two fj invocations whose access token has expired will both 401, both load the same stored refresh token, and both POST it to /oauth/token. One wins. The other is a replay, and replay detection on a rotating-token OP typically revokes the whole token family, so the winner's freshly minted token dies with it. Both sessions are gone, while hosts.toml and the keychain slots still look healthy.

Hitting this needs no unusual usage: a script looping over fj pr list, a shell prompt integration, an editor plugin, or two terminals are each enough.

2. Rotation is not crash-safe

refresh_session (src/fjord/oidc.rs:1001) is exchange, then persist, then return. There is no window in which both tokens are durably held. The moment exchange_refresh returns, the old token is spent server-side; if store_tokens (src/fjord/oidc.rs:960) then fails, the new one is lost and the session is unrecoverable.

store_tokens writes to the OS keychain via store_fjord_bearer / store_fjord_refresh (src/auth/mod.rs:387, src/auth/mod.rs:410), which fails for ordinary reasons: a locked keychain, a denied access prompt, or a code-signature change that invalidates the existing ACL entry. The last one is routine for anyone running a locally built fj.

The error message also understates it. It reads "refreshing the Fjord Account session", not "the sign-in has been destroyed and must be recreated".

Observed

fj 0.5.0 (Homebrew), host https://fjord.sh, OIDC sign-in, immediately after fj auth status reported the session healthy:

$ fj --debug api /version --host https://fjord.sh
→ GET https://fjord.sh/api/v1/forge-gateway/819eadfe-…/api/v1/version
← 401 Unauthorized
error: refreshing the expired Fjord Account session
  caused by: refreshing the Fjord Account session
  caused by: token endpoint returned HTTP 400 Bad Request:
    {"error":"invalid_grant","error_description":"Invalid or expired grant"}

Which of the two paths produced it is not recoverable after the fact, which is part of the problem.

Fix

Lock it. Take an OS-level exclusive lock (flock on the config dir) held across the entire exchange-and-persist inside refresh_session. A process that fails to acquire it waits, re-reads the token store, and retries the original request with whatever the winner persisted, rather than exchanging a token that is already spent.

Two-slot rotation. Persist the new refresh token alongside the old rather than over it: write it as pending, promote it to current only once it has been used successfully, and never drop the surviving token on a failed write. A refresh interrupted between exchange and persist then costs one wasted exchange instead of the account.

Say what happened. A store_tokens failure inside refresh_session should be fatal and named. invalid_grant from the token endpoint should map to a sentence saying the sign-in cannot be renewed and pointing at fj auth login --fjord, rather than printing the raw JSON body.

fj auth status prints ✓ Token: present in token store for a session in this state, because it checks presence and never validity. Tracking expires_in (currently discarded by TokenResponse, src/fjord/oidc.rs:609) would let status tell the truth and would make renewal proactive rather than reactive on a guaranteed-failing request. Worth a separate issue.

A Fjord Account session can be permanently destroyed by its own refresh path. Two independent holes, both in the rotation handling, both ending in `invalid_grant` with no recovery short of a full browser sign-in. Paragon rotates the refresh token on every exchange and detects replay, as `exchange_refresh` documents (`src/fjord/oidc.rs:638`): > Paragon rotates on every use and detects replay, so the caller MUST persist the returned `refresh_token` and discard the one it sent. `fj` honours the "persist" half, but has no defence for either failure mode rotation introduces. ### 1. No cross-process lock around exchange-and-persist `Client::request_with_headers` refreshes reactively on a 401 (`src/client/mod.rs:353`), through `refresh_bearer` (`src/client/mod.rs:484`) into `refresh_session` (`src/fjord/oidc.rs:1001`). Nothing serialises that across processes. Two `fj` invocations whose access token has expired will both 401, both load the same stored refresh token, and both POST it to `/oauth/token`. One wins. The other is a replay, and replay detection on a rotating-token OP typically revokes the whole token family, so the winner's freshly minted token dies with it. Both sessions are gone, while `hosts.toml` and the keychain slots still look healthy. Hitting this needs no unusual usage: a script looping over `fj pr list`, a shell prompt integration, an editor plugin, or two terminals are each enough. ### 2. Rotation is not crash-safe `refresh_session` (`src/fjord/oidc.rs:1001`) is exchange, then persist, then return. There is no window in which both tokens are durably held. The moment `exchange_refresh` returns, the old token is spent server-side; if `store_tokens` (`src/fjord/oidc.rs:960`) then fails, the new one is lost and the session is unrecoverable. `store_tokens` writes to the OS keychain via `store_fjord_bearer` / `store_fjord_refresh` (`src/auth/mod.rs:387`, `src/auth/mod.rs:410`), which fails for ordinary reasons: a locked keychain, a denied access prompt, or a code-signature change that invalidates the existing ACL entry. The last one is routine for anyone running a locally built `fj`. The error message also understates it. It reads "refreshing the Fjord Account session", not "the sign-in has been destroyed and must be recreated". ### Observed fj 0.5.0 (Homebrew), host `https://fjord.sh`, OIDC sign-in, immediately after `fj auth status` reported the session healthy: ``` $ fj --debug api /version --host https://fjord.sh → GET https://fjord.sh/api/v1/forge-gateway/819eadfe-…/api/v1/version ← 401 Unauthorized error: refreshing the expired Fjord Account session caused by: refreshing the Fjord Account session caused by: token endpoint returned HTTP 400 Bad Request: {"error":"invalid_grant","error_description":"Invalid or expired grant"} ``` Which of the two paths produced it is not recoverable after the fact, which is part of the problem. ### Fix **Lock it.** Take an OS-level exclusive lock (flock on the config dir) held across the entire exchange-and-persist inside `refresh_session`. A process that fails to acquire it waits, re-reads the token store, and retries the original request with whatever the winner persisted, rather than exchanging a token that is already spent. **Two-slot rotation.** Persist the new refresh token alongside the old rather than over it: write it as `pending`, promote it to `current` only once it has been used successfully, and never drop the surviving token on a failed write. A refresh interrupted between exchange and persist then costs one wasted exchange instead of the account. **Say what happened.** A `store_tokens` failure inside `refresh_session` should be fatal and named. `invalid_grant` from the token endpoint should map to a sentence saying the sign-in cannot be renewed and pointing at `fj auth login --fjord`, rather than printing the raw JSON body. ### Related `fj auth status` prints `✓ Token: present in token store` for a session in this state, because it checks presence and never validity. Tracking `expires_in` (currently discarded by `TokenResponse`, `src/fjord/oidc.rs:609`) would let status tell the truth and would make renewal proactive rather than reactive on a guaranteed-failing request. Worth a separate issue.

stephen referenced this issue from a commit

2026-09-19 16:47:29 +00:00

Single-flight and order the Fjord Account token refresh

stephen commented

2026-09-19 16:47:37 +00:00

Author

Owner

Two corrections to the above, found while fixing it.

Hole 2 is narrower than described. The claim that a keychain failure loses the token is wrong: store_token_with_entry_and_file_path (src/auth/mod.rs) already falls back to a 0600 file store on any keychain error, so a locked keychain, a denied prompt, or an invalidated code-signature ACL all still persist the token. The real window is a crash or SIGINT between the exchange response and the write, or the keychain and the file store both failing. Smaller, but still unrecoverable, and still worth closing.

Two-slot rotation is the wrong shape for this OP. Keeping the old refresh token alongside the new one has no value when the OP rotates on every use: the old one is spent the instant the exchange returns, and a later attempt to use it is precisely the replay that revokes the family. There is no "promote once used successfully" state to reach. What actually closes the window is write ordering: persist the refresh record first, abort before touching the access bearer if it fails. A crash between the two then leaves the previous, still-valid pair intact and costs one wasted exchange.

Both holes reproduce against a mock OP. Two concurrent fj processes on an expired access token produce 2 token-endpoint exchanges on 0.5.0 and 1 on the fix.

Two corrections to the above, found while fixing it. **Hole 2 is narrower than described.** The claim that a keychain failure loses the token is wrong: `store_token_with_entry_and_file_path` (`src/auth/mod.rs`) already falls back to a 0600 file store on any keychain error, so a locked keychain, a denied prompt, or an invalidated code-signature ACL all still persist the token. The real window is a crash or SIGINT between the exchange response and the write, or the keychain and the file store both failing. Smaller, but still unrecoverable, and still worth closing. **Two-slot rotation is the wrong shape for this OP.** Keeping the old refresh token alongside the new one has no value when the OP rotates on every use: the old one is spent the instant the exchange returns, and a later attempt to use it is precisely the replay that revokes the family. There is no "promote once used successfully" state to reach. What actually closes the window is write ordering: persist the refresh record first, abort before touching the access bearer if it fails. A crash between the two then leaves the previous, still-valid pair intact and costs one wasted exchange. Both holes reproduce against a mock OP. Two concurrent `fj` processes on an expired access token produce 2 token-endpoint exchanges on 0.5.0 and 1 on the fix.

stephen referenced this issue

2026-09-19 16:47:56 +00:00

Single-flight and order the Fjord Account token refresh #273

stephen referenced this issue from a commit

2026-09-19 19:39:03 +00:00

Single-flight and order the Fjord Account token refresh (#273)