From 12405b5c7ad6f7569171550b11cfc828af5f49d5 Mon Sep 17 00:00:00 2001 From: Derek Gabriel Date: Mon, 4 May 2026 20:44:12 -1000 Subject: [PATCH] Add refresh token auth diagnostics --- CHANGELOG.md | 3 +- .../Auth/DcrFacade/TokenHandler.cs | 25 +++++-- .../Auth/Services/CosmosOAuthStateStore.cs | 59 ++++++++++----- .../Auth/Services/IOAuthStateStore.cs | 2 +- .../Auth/Services/RefreshRotationResult.cs | 54 +++++++++++++- .../Auth/Services/FakeOAuthStateStore.cs | 29 +++++--- .../Auth/Services/FakeOAuthStateStoreTests.cs | 74 +++++++++++++++++-- 7 files changed, 197 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 430d852..62de30e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to DevBrain are tracked in this file. Versions follow [Seman - Hardened the OAuth `refresh_token` grant for MCP clients that retry or restart while their local credential cache is catching up to token rotation. A successful refresh now leaves a five-minute replay marker for the old refresh token, so an immediate retry returns the same replacement refresh token instead of forcing a reconnect. Wrong-client refresh attempts are rejected without consuming the legitimate client's token, and successful refresh/replay calls slide the upstream token vault TTL forward with the local refresh window. ### Changed +- Added reason-specific server-side diagnostics for OAuth refresh failures. `TokenHandler/refresh` now logs a stable rejection reason (`missing`, `expired`, `replay_window_expired`, `wrong_client`, `upstream_missing_or_expired`, etc.) plus short SHA-256 refresh-token fingerprints so stale per-session Codex credential generations can be correlated without logging token material. - Synced the release notes with merged Dependabot PR #19, which already moved `Microsoft.AspNetCore.DataProtection` and `System.Security.Cryptography.Xml` to 10.0.7. - Patched the remaining Azure Data Protection helper packages to `Azure.Extensions.AspNetCore.DataProtection.Blobs` 1.5.2 and `Azure.Extensions.AspNetCore.DataProtection.Keys` 1.6.2, then replaced the stale 10.0.6 workaround comment in the project file. - Added `.serena/` to `.gitignore` so local Serena workspace metadata stays out of the public repository. @@ -17,7 +18,7 @@ All notable changes to DevBrain are tracked in this file. Versions follow [Seman - `dotnet list devbrain.slnx package --outdated --highest-patch` reports no patch-level updates for direct package references. - `dotnet list devbrain.slnx package --outdated --include-transitive` was checked; it still reports broader direct/transitive updates in Azure Functions, Application Insights, IdentityModel, Cosmos, and test tooling that are left for a separate dependency refresh. - `dotnet list devbrain.slnx package --deprecated` still reports two known migration items left outside this auth fix: `Microsoft.ApplicationInsights.WorkerService` 2.22.0 and `xunit` 2.9.3. -- `dotnet test devbrain.slnx` passes with 141 tests. +- `dotnet test devbrain.slnx` passes with 142 tests. ## [1.9.0] — 2026-04-15 diff --git a/src/DevBrain.Functions/Auth/DcrFacade/TokenHandler.cs b/src/DevBrain.Functions/Auth/DcrFacade/TokenHandler.cs index e97eb12..cbcf1ad 100644 --- a/src/DevBrain.Functions/Auth/DcrFacade/TokenHandler.cs +++ b/src/DevBrain.Functions/Auth/DcrFacade/TokenHandler.cs @@ -164,24 +164,30 @@ private async Task HandleRefreshAsync(TokenRequest request) RefreshReplayLifetime, UpstreamVaultTtl); - if (rotation is null) + if (!rotation.Succeeded) { - _logger?.LogWarning("TokenHandler/refresh: rejected — refresh_token invalid, expired, wrong client, or upstream session expired"); + _logger?.LogWarning( + "TokenHandler/refresh: rejected reason={Reason} clientId={ClientId} refreshTokenFingerprint={RefreshTokenFingerprint}", + rotation.LogCode, request.ClientId, FingerprintToken(request.RefreshToken)); return TokenResult.Error("invalid_grant", "refresh_token is invalid, expired, already rotated outside the replay window, or bound to a different client."); } - var upstreamJti = rotation.UpstreamJti; + var upstreamJti = rotation.UpstreamJti!; var (jwt, _) = IssueJwtForUpstream(upstreamJti); _logger?.LogInformation( - "TokenHandler/refresh: {RotationKind} refresh clientId={ClientId} upstreamJti={Jti}", - rotation.IsReplay ? "replayed" : "rotated", request.ClientId, upstreamJti); + "TokenHandler/refresh: {RotationKind} refresh clientId={ClientId} upstreamJti={Jti} refreshTokenFingerprint={RefreshTokenFingerprint} returnedRefreshTokenFingerprint={ReturnedRefreshTokenFingerprint}", + rotation.IsReplay ? "replayed" : "rotated", + request.ClientId, + upstreamJti, + FingerprintToken(request.RefreshToken), + FingerprintToken(rotation.RefreshToken!)); return TokenResult.Success(new TokenResponse( AccessToken: jwt, TokenType: "Bearer", ExpiresIn: (int)AccessTokenLifetime.TotalSeconds, - RefreshToken: rotation.RefreshToken, + RefreshToken: rotation.RefreshToken!, Scope: "documents.readwrite")); } @@ -225,6 +231,13 @@ private static string GenerateOpaqueToken() RandomNumberGenerator.Fill(bytes); return Convert.ToBase64String(bytes).TrimEnd('=').Replace('+', '-').Replace('/', '_'); } + + private static string FingerprintToken(string token) + { + Span hash = stackalloc byte[32]; + SHA256.HashData(System.Text.Encoding.UTF8.GetBytes(token), hash); + return Convert.ToHexString(hash[..6]).ToLowerInvariant(); + } } public sealed record TokenRequest( diff --git a/src/DevBrain.Functions/Auth/Services/CosmosOAuthStateStore.cs b/src/DevBrain.Functions/Auth/Services/CosmosOAuthStateStore.cs index 24066c0..86ea968 100644 --- a/src/DevBrain.Functions/Auth/Services/CosmosOAuthStateStore.cs +++ b/src/DevBrain.Functions/Auth/Services/CosmosOAuthStateStore.cs @@ -194,7 +194,7 @@ public Task SaveRefreshAsync(DevBrainRefreshRecord refresh) return UpsertAsync(refresh, key); } - public async Task RotateRefreshAsync( + public async Task RotateRefreshAsync( string refreshToken, string clientId, string replacementRefreshToken, @@ -216,38 +216,41 @@ record = response.Resource; } catch (CosmosException ex) when (ex.StatusCode == HttpStatusCode.NotFound) { - return null; + return RefreshRotationResult.Rejected(RefreshRotationOutcome.Missing); } if (record.ExpiresAt <= now) { await TryDeleteAsync(key, partition, etag); - return null; + return RefreshRotationResult.Rejected( + record.IsReplayMarker + ? RefreshRotationOutcome.ReplayWindowExpired + : RefreshRotationOutcome.Expired); } if (!string.Equals(record.ClientId, clientId, StringComparison.Ordinal)) { - return null; + return RefreshRotationResult.Rejected(RefreshRotationOutcome.WrongClient); } if (record.IsReplayMarker) { if (string.IsNullOrEmpty(record.RotatedToRefreshToken)) { - return null; + return RefreshRotationResult.Rejected(RefreshRotationOutcome.ReplayMarkerMissingReplacement); } if (!await TouchUpstreamTokenAsync(record.UpstreamJti, upstreamVaultLifetime)) { - return null; + return RefreshRotationResult.Rejected(RefreshRotationOutcome.UpstreamMissingOrExpired); } - return new RefreshRotationResult(record.UpstreamJti, record.RotatedToRefreshToken, IsReplay: true); + return RefreshRotationResult.Replayed(record.UpstreamJti, record.RotatedToRefreshToken); } if (!await TouchUpstreamTokenAsync(record.UpstreamJti, upstreamVaultLifetime)) { - return null; + return RefreshRotationResult.Rejected(RefreshRotationOutcome.UpstreamMissingOrExpired); } var replacement = new DevBrainRefreshRecord @@ -283,7 +286,7 @@ await _container.ReplaceItemAsync( partition, new ItemRequestOptions { IfMatchEtag = etag }); - return new RefreshRotationResult(record.UpstreamJti, replacementRefreshToken, IsReplay: false); + return RefreshRotationResult.Rotated(record.UpstreamJti, replacementRefreshToken); } catch (CosmosException ex) when (ex.StatusCode == HttpStatusCode.PreconditionFailed @@ -292,7 +295,10 @@ await _container.ReplaceItemAsync( // Another request won the rotation. Re-read once and, if it left a replay marker, // return that winning replacement instead of surfacing a spurious invalid_grant. await DeleteAsync(RefreshKey(replacementRefreshToken)); - return await ReadRefreshReplayAsync(refreshToken, clientId, upstreamVaultLifetime); + var replay = await ReadRefreshReplayAsync(refreshToken, clientId, upstreamVaultLifetime); + return replay.Succeeded + ? replay + : RefreshRotationResult.Rejected(RefreshRotationOutcome.ConcurrentReplayUnavailable); } } @@ -338,7 +344,7 @@ await _container.DeleteItemAsync( // ---------------- Internal helpers ---------------- - private async Task ReadRefreshReplayAsync( + private async Task ReadRefreshReplayAsync( string refreshToken, string clientId, TimeSpan upstreamVaultLifetime) @@ -348,24 +354,39 @@ await _container.DeleteItemAsync( { var response = await _container.ReadItemAsync(key, new PartitionKey(key)); var record = response.Resource; - if (record.ExpiresAt <= _timeProvider.GetUtcNow() - || !record.IsReplayMarker - || string.IsNullOrEmpty(record.RotatedToRefreshToken) - || !string.Equals(record.ClientId, clientId, StringComparison.Ordinal)) + if (record.ExpiresAt <= _timeProvider.GetUtcNow()) { - return null; + return RefreshRotationResult.Rejected( + record.IsReplayMarker + ? RefreshRotationOutcome.ReplayWindowExpired + : RefreshRotationOutcome.Expired); + } + + if (!string.Equals(record.ClientId, clientId, StringComparison.Ordinal)) + { + return RefreshRotationResult.Rejected(RefreshRotationOutcome.WrongClient); + } + + if (!record.IsReplayMarker) + { + return RefreshRotationResult.Rejected(RefreshRotationOutcome.ConcurrentReplayUnavailable); + } + + if (string.IsNullOrEmpty(record.RotatedToRefreshToken)) + { + return RefreshRotationResult.Rejected(RefreshRotationOutcome.ReplayMarkerMissingReplacement); } if (!await TouchUpstreamTokenAsync(record.UpstreamJti, upstreamVaultLifetime)) { - return null; + return RefreshRotationResult.Rejected(RefreshRotationOutcome.UpstreamMissingOrExpired); } - return new RefreshRotationResult(record.UpstreamJti, record.RotatedToRefreshToken, IsReplay: true); + return RefreshRotationResult.Replayed(record.UpstreamJti, record.RotatedToRefreshToken); } catch (CosmosException ex) when (ex.StatusCode == HttpStatusCode.NotFound) { - return null; + return RefreshRotationResult.Rejected(RefreshRotationOutcome.Missing); } } diff --git a/src/DevBrain.Functions/Auth/Services/IOAuthStateStore.cs b/src/DevBrain.Functions/Auth/Services/IOAuthStateStore.cs index 22060e9..e301b0b 100644 --- a/src/DevBrain.Functions/Auth/Services/IOAuthStateStore.cs +++ b/src/DevBrain.Functions/Auth/Services/IOAuthStateStore.cs @@ -65,7 +65,7 @@ public interface IOAuthStateStore /// replacement token. The referenced upstream vault record is defensively extended as part of /// the rotation/replay path. /// - Task RotateRefreshAsync( + Task RotateRefreshAsync( string refreshToken, string clientId, string replacementRefreshToken, diff --git a/src/DevBrain.Functions/Auth/Services/RefreshRotationResult.cs b/src/DevBrain.Functions/Auth/Services/RefreshRotationResult.cs index 9b077e5..090d6bc 100644 --- a/src/DevBrain.Functions/Auth/Services/RefreshRotationResult.cs +++ b/src/DevBrain.Functions/Auth/Services/RefreshRotationResult.cs @@ -1,10 +1,56 @@ namespace DevBrain.Functions.Auth.Services; +/// +/// Outcome of rotating a DevBrain refresh token. +/// +public enum RefreshRotationOutcome +{ + Rotated, + Replayed, + Missing, + Expired, + ReplayWindowExpired, + WrongClient, + ReplayMarkerMissingReplacement, + UpstreamMissingOrExpired, + ConcurrentReplayUnavailable, +} + /// /// Result of rotating a DevBrain refresh token. Immediate replays of the old token return the -/// same replacement refresh token so client retries remain idempotent. +/// same replacement refresh token so client retries remain idempotent. Rejections carry a +/// reason code for server-side diagnostics; callers still return a generic OAuth +/// invalid_grant to clients. /// public sealed record RefreshRotationResult( - string UpstreamJti, - string RefreshToken, - bool IsReplay); + string? UpstreamJti, + string? RefreshToken, + RefreshRotationOutcome Outcome) +{ + public bool Succeeded => Outcome is RefreshRotationOutcome.Rotated or RefreshRotationOutcome.Replayed; + + public bool IsReplay => Outcome is RefreshRotationOutcome.Replayed; + + public string LogCode => Outcome switch + { + RefreshRotationOutcome.Rotated => "rotated", + RefreshRotationOutcome.Replayed => "replayed", + RefreshRotationOutcome.Missing => "missing", + RefreshRotationOutcome.Expired => "expired", + RefreshRotationOutcome.ReplayWindowExpired => "replay_window_expired", + RefreshRotationOutcome.WrongClient => "wrong_client", + RefreshRotationOutcome.ReplayMarkerMissingReplacement => "replay_marker_missing_replacement", + RefreshRotationOutcome.UpstreamMissingOrExpired => "upstream_missing_or_expired", + RefreshRotationOutcome.ConcurrentReplayUnavailable => "concurrent_replay_unavailable", + _ => "unknown", + }; + + public static RefreshRotationResult Rotated(string upstreamJti, string refreshToken) => + new(upstreamJti, refreshToken, RefreshRotationOutcome.Rotated); + + public static RefreshRotationResult Replayed(string upstreamJti, string refreshToken) => + new(upstreamJti, refreshToken, RefreshRotationOutcome.Replayed); + + public static RefreshRotationResult Rejected(RefreshRotationOutcome outcome) => + new(null, null, outcome); +} diff --git a/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStore.cs b/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStore.cs index 5a7de8a..48b43fd 100644 --- a/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStore.cs +++ b/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStore.cs @@ -242,7 +242,7 @@ public Task SaveRefreshAsync(DevBrainRefreshRecord refresh) } } - public Task RotateRefreshAsync( + public Task RotateRefreshAsync( string refreshToken, string clientId, string replacementRefreshToken, @@ -255,35 +255,41 @@ public Task SaveRefreshAsync(DevBrainRefreshRecord refresh) ReadCallCount++; if (!_refreshes.TryGetValue(refreshToken, out var record)) { - return Task.FromResult(null); + return Task.FromResult(RefreshRotationResult.Rejected(RefreshRotationOutcome.Missing)); } if (IsExpired(record.ExpiresAt)) { _refreshes.Remove(refreshToken); - return Task.FromResult(null); + return Task.FromResult(RefreshRotationResult.Rejected( + record.IsReplayMarker + ? RefreshRotationOutcome.ReplayWindowExpired + : RefreshRotationOutcome.Expired)); } if (!string.Equals(record.ClientId, clientId, StringComparison.Ordinal)) { - return Task.FromResult(null); + return Task.FromResult(RefreshRotationResult.Rejected(RefreshRotationOutcome.WrongClient)); } if (record.IsReplayMarker) { - if (string.IsNullOrEmpty(record.RotatedToRefreshToken) - || !TouchUpstreamTokenCore(record.UpstreamJti, upstreamVaultLifetime)) + if (string.IsNullOrEmpty(record.RotatedToRefreshToken)) { - return Task.FromResult(null); + return Task.FromResult(RefreshRotationResult.Rejected(RefreshRotationOutcome.ReplayMarkerMissingReplacement)); } - return Task.FromResult( - new RefreshRotationResult(record.UpstreamJti, record.RotatedToRefreshToken, IsReplay: true)); + if (!TouchUpstreamTokenCore(record.UpstreamJti, upstreamVaultLifetime)) + { + return Task.FromResult(RefreshRotationResult.Rejected(RefreshRotationOutcome.UpstreamMissingOrExpired)); + } + + return Task.FromResult(RefreshRotationResult.Replayed(record.UpstreamJti, record.RotatedToRefreshToken)); } if (!TouchUpstreamTokenCore(record.UpstreamJti, upstreamVaultLifetime)) { - return Task.FromResult(null); + return Task.FromResult(RefreshRotationResult.Rejected(RefreshRotationOutcome.UpstreamMissingOrExpired)); } var now = _timeProvider.GetUtcNow(); @@ -309,8 +315,7 @@ public Task SaveRefreshAsync(DevBrainRefreshRecord refresh) Ttl = (int)replayLifetime.TotalSeconds, }; - return Task.FromResult( - new RefreshRotationResult(record.UpstreamJti, replacementRefreshToken, IsReplay: false)); + return Task.FromResult(RefreshRotationResult.Rotated(record.UpstreamJti, replacementRefreshToken)); } } diff --git a/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStoreTests.cs b/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStoreTests.cs index 53673aa..fd4d599 100644 --- a/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStoreTests.cs +++ b/tests/DevBrain.Functions.Tests/Auth/Services/FakeOAuthStateStoreTests.cs @@ -180,7 +180,8 @@ await store.SaveRefreshAsync(new DevBrainRefreshRecord TimeSpan.FromMinutes(5), TimeSpan.FromDays(30)); - Assert.NotNull(rotated); + Assert.True(rotated.Succeeded); + Assert.Equal(RefreshRotationOutcome.Rotated, rotated.Outcome); Assert.False(rotated.IsReplay); Assert.Equal("refresh-new", rotated.RefreshToken); @@ -192,7 +193,8 @@ await store.SaveRefreshAsync(new DevBrainRefreshRecord TimeSpan.FromMinutes(5), TimeSpan.FromDays(30)); - Assert.NotNull(replay); + Assert.True(replay.Succeeded); + Assert.Equal(RefreshRotationOutcome.Replayed, replay.Outcome); Assert.True(replay.IsReplay); Assert.Equal("refresh-new", replay.RefreshToken); @@ -206,7 +208,8 @@ await store.SaveRefreshAsync(new DevBrainRefreshRecord TimeSpan.FromMinutes(5), TimeSpan.FromDays(30)); - Assert.Null(expiredReplay); + Assert.False(expiredReplay.Succeeded); + Assert.Equal(RefreshRotationOutcome.ReplayWindowExpired, expiredReplay.Outcome); } [Fact] @@ -235,7 +238,8 @@ await store.SaveRefreshAsync(new DevBrainRefreshRecord TimeSpan.FromMinutes(5), TimeSpan.FromDays(30)); - Assert.Null(attacker); + Assert.False(attacker.Succeeded); + Assert.Equal(RefreshRotationOutcome.WrongClient, attacker.Outcome); var legitimate = await store.RotateRefreshAsync( "refresh-old", @@ -245,7 +249,7 @@ await store.SaveRefreshAsync(new DevBrainRefreshRecord TimeSpan.FromMinutes(5), TimeSpan.FromDays(30)); - Assert.NotNull(legitimate); + Assert.True(legitimate.Succeeded); Assert.Equal("refresh-new", legitimate.RefreshToken); } @@ -275,13 +279,71 @@ await store.SaveRefreshAsync(new DevBrainRefreshRecord TimeSpan.FromMinutes(5), TimeSpan.FromDays(30)); - Assert.NotNull(rotated); + Assert.True(rotated.Succeeded); var upstream = await store.GetUpstreamTokenAsync("jti-old"); Assert.NotNull(upstream); Assert.Equal(Epoch.AddDays(30), upstream!.ExpiresAt); } + [Fact] + public async Task RefreshToken_RotateReportsFailureReasons() + { + var (store, clock) = CreateStore(); + + var missing = await store.RotateRefreshAsync( + "missing-refresh", + "abc-123", + "replacement", + TimeSpan.FromDays(30), + TimeSpan.FromMinutes(5), + TimeSpan.FromDays(30)); + + Assert.False(missing.Succeeded); + Assert.Equal(RefreshRotationOutcome.Missing, missing.Outcome); + Assert.Equal("missing", missing.LogCode); + + await store.SaveRefreshAsync(new DevBrainRefreshRecord + { + RefreshToken = "expired-refresh", + ClientId = "abc-123", + UpstreamJti = "jti-expired", + ExpiresAt = Epoch.AddMinutes(1), + }); + + clock.Advance(TimeSpan.FromMinutes(2)); + + var expired = await store.RotateRefreshAsync( + "expired-refresh", + "abc-123", + "replacement", + TimeSpan.FromDays(30), + TimeSpan.FromMinutes(5), + TimeSpan.FromDays(30)); + + Assert.False(expired.Succeeded); + Assert.Equal(RefreshRotationOutcome.Expired, expired.Outcome); + + await store.SaveRefreshAsync(new DevBrainRefreshRecord + { + RefreshToken = "orphan-refresh", + ClientId = "abc-123", + UpstreamJti = "missing-upstream", + ExpiresAt = Epoch.AddDays(1), + }); + + var orphan = await store.RotateRefreshAsync( + "orphan-refresh", + "abc-123", + "replacement", + TimeSpan.FromDays(30), + TimeSpan.FromMinutes(5), + TimeSpan.FromDays(30)); + + Assert.False(orphan.Succeeded); + Assert.Equal(RefreshRotationOutcome.UpstreamMissingOrExpired, orphan.Outcome); + } + [Fact] public async Task UpstreamToken_RoundTripPreservesEnvelopeAndClaims() {