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

Merged

stephen merged 2 commits from fix/272-fjord-refresh-lock into main

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

stephen commented

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

Owner

Paragon rotates the Fjord Account refresh token on every exchange and treats a replay as theft, so it revokes the whole token family. exchange_refresh documents that contract but nothing defended either failure mode it introduces, and both end with the user signed out while hosts.toml and the token store still look healthy.

Concurrent processes raced. Every invocation refreshes reactively on a 401, so two commands running at once both loaded the same refresh token and both POSTed it; the second was a replay that killed the first one's freshly minted token. A shell prompt hook plus an interactive command is enough to hit it. Refreshes now take an advisory file lock (src/fjord/lock.rs, std::fs::File::try_lock) keyed per platform URL and held across the exchange and the persist. A process that waited re-reads the bearer and returns whatever the winner stored rather than spending a token that is already gone. The lock file name collapses every non-alphanumeric byte of the platform URL to -, so it can contribute neither a separator nor a .. hop.

Persistence was unordered and did not abort. The new access bearer could land while the rotated refresh token did not, leaving a session that works until it expires and then cannot be renewed at all. store_tokens now writes the refresh record first and returns before touching the bearer if that write fails, so an interruption between the two leaves the previous, still-valid pair intact and costs one wasted exchange. The decision of what to write (store the rotation, keep a record that still belongs to this account, or drop one left by another) is split into a pure plan_refresh_write, and the two writes are injected into store_tokens_with, which is how the ordering is tested without a keychain.

The issue proposed two-slot rotation for this. It does not fit a rotating OP: the old token is spent the moment the exchange returns, so there is no "promote once used successfully" state to reach, and holding it is exactly the replay risk. Ordering is what closes the window. The issue also said a keychain write failure loses the token, which is wrong, store_token_with_entry_and_file_path already falls back to a 0600 file. Both corrections are on #272.

Last, a spent grant reported itself by dumping the raw JSON body. invalid_grant is now a typed terminal error, fjord::UnrenewableGrant, and since main prints only the outermost error unless --debug is set, error_headline lifts its message out of the chain. Callers keep their own .context() for the debug trace. Before, the sentence the user needed was the third caused by: line and invisible by default.

Verified against a mock OP that rotates and detects replay: two concurrent processes on an expired access token produce 2 token-endpoint exchanges on 0.5.0 and 1 here, and a dead grant now prints the recovery as the first line with no flags.

Refs #272.

Verified on macOS: cargo fmt --all clean, cargo clippy --all-targets --all-features -- -D warnings clean, cargo test --all green (817 + 5 + 1), cargo audit clean after the rustls bump. make coverage puts the touched files at 89% (lock.rs) and 93% (oidc.rs) lines.

Two notes outside the fix. The second commit bumps rustls 0.23.40 to 0.23.45 for RUSTSEC-2026-0285, lockfile only; the pre-push audit gate fails without it. And make coverage-strict reports 2.5% on my machine under nightly-2026-08-07 while the tests all run and stable make coverage reports 66%, so the nightly tier looks broken independently of this branch and I could not use it as a gate here.

Paragon rotates the Fjord Account refresh token on every exchange and treats a replay as theft, so it revokes the whole token family. `exchange_refresh` documents that contract but nothing defended either failure mode it introduces, and both end with the user signed out while `hosts.toml` and the token store still look healthy. Concurrent processes raced. Every invocation refreshes reactively on a 401, so two commands running at once both loaded the same refresh token and both POSTed it; the second was a replay that killed the first one's freshly minted token. A shell prompt hook plus an interactive command is enough to hit it. Refreshes now take an advisory file lock (`src/fjord/lock.rs`, `std::fs::File::try_lock`) keyed per platform URL and held across the exchange *and* the persist. A process that waited re-reads the bearer and returns whatever the winner stored rather than spending a token that is already gone. The lock file name collapses every non-alphanumeric byte of the platform URL to `-`, so it can contribute neither a separator nor a `..` hop. Persistence was unordered and did not abort. The new access bearer could land while the rotated refresh token did not, leaving a session that works until it expires and then cannot be renewed at all. `store_tokens` now writes the refresh record first and returns before touching the bearer if that write fails, so an interruption between the two leaves the previous, still-valid pair intact and costs one wasted exchange. The decision of what to write (store the rotation, keep a record that still belongs to this account, or drop one left by another) is split into a pure `plan_refresh_write`, and the two writes are injected into `store_tokens_with`, which is how the ordering is tested without a keychain. The issue proposed two-slot rotation for this. It does not fit a rotating OP: the old token is spent the moment the exchange returns, so there is no "promote once used successfully" state to reach, and holding it is exactly the replay risk. Ordering is what closes the window. The issue also said a keychain write failure loses the token, which is wrong, `store_token_with_entry_and_file_path` already falls back to a 0600 file. Both corrections are on #272. Last, a spent grant reported itself by dumping the raw JSON body. `invalid_grant` is now a typed terminal error, `fjord::UnrenewableGrant`, and since `main` prints only the outermost error unless `--debug` is set, `error_headline` lifts its message out of the chain. Callers keep their own `.context()` for the debug trace. Before, the sentence the user needed was the third `caused by:` line and invisible by default. Verified against a mock OP that rotates and detects replay: two concurrent processes on an expired access token produce 2 token-endpoint exchanges on 0.5.0 and 1 here, and a dead grant now prints the recovery as the first line with no flags. Refs #272. Verified on macOS: `cargo fmt --all` clean, `cargo clippy --all-targets --all-features -- -D warnings` clean, `cargo test --all` green (817 + 5 + 1), `cargo audit` clean after the rustls bump. `make coverage` puts the touched files at 89% (lock.rs) and 93% (oidc.rs) lines. Two notes outside the fix. The second commit bumps rustls 0.23.40 to 0.23.45 for RUSTSEC-2026-0285, lockfile only; the pre-push audit gate fails without it. And `make coverage-strict` reports 2.5% on my machine under `nightly-2026-08-07` while the tests all run and stable `make coverage` reports 66%, so the nightly tier looks broken independently of this branch and I could not use it as a gate here.

stephen added 2 commits

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

Single-flight and order the Fjord Account token refresh ecb86b8c43

Paragon rotates the refresh token on every exchange and treats a replay
as theft: the whole token family is revoked. fj had two ways to trip
that, and both end with the user signed out and no idea why.

Concurrent processes raced. Every fj invocation refreshes reactively on
a 401, so two commands running at once (a shell prompt hook plus an
interactive command, a script's parallel calls) both spent the same
refresh token, and the second exchange was a replay. Refreshes now take
an advisory file lock per platform URL, held across the exchange and the
persist. A process that waited re-reads the bearer first and reuses what
the winner stored rather than spending its own.

Persistence was unordered and non-aborting. The new access bearer could
land while the rotated refresh token did not, leaving a session that
works until it expires and then cannot be renewed. The refresh record is
now written first and a failure aborts before the bearer is touched, so
a crash between the two leaves the old, still-valid pair intact.

A spent grant also reported itself as a JSON dump. `invalid_grant` is
now a typed terminal error, and `main` lifts its message to the headline
so the recovery survives the context wrapping that `--debug` would
otherwise hide.

Refs #272

Bump rustls to 0.23.45

ci / check (pull_request) Successful in 13m50s

Details

ci / coverage (pull_request) Successful in 2m25s

Details

ci / live-e2e (pull_request) Successful in 2m30s

Details

2cab3ef633

RUSTSEC-2026-0285: TLS 1.3 handshake messages were accepted across
encryption level boundaries. Lockfile-only, no API change, and it is
what the pre-push audit gate is currently failing on.

stephen referenced this pull request

2026-09-19 19:36:04 +00:00

Keep the selected instance across a Fjord Account re-login #274

stephen merged commit 7789d2d5f9 into main

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

stephen deleted branch fix/272-fjord-refresh-lock

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

stephen referenced this pull request from a commit

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

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