From 639e6eddd5d5bc48b401bd933b8c2dd1f1fac1a1 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Tue, 29 Sep 2026 11:31:53 +0000 Subject: [PATCH] fix: clear only the credential GitHub rejected, and stop treating a plain 403 as one [patch] MakeGitHubRequestAsync called OnAuthenticationFailure on any AuthorizationException, clearing the provider-level AccountId and Token even when the request had been sent with an owner's override token. One owner with an expired token took every other owner offline, while the token that actually failed stayed in the store and was rejected again every cycle. A rejected owner token now clears that owner's token only, so the owner falls back to the provider token, and the provider credentials are left alone. A rejected provider token still clears the provider credentials as before. A non-rate-limit 403 means the token may not make this one request, such as a workflow dispatch without actions: write. It is now reported as an error on the request and no longer erases any credentials. Fixes #297 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr --- .gitignore | 18 +++++ .../GitHubRequestOutcomeTests.cs | 69 ++++++++++++++++++- BuildMonitor/Providers/GitHub.cs | 35 +++++++++- BuildMonitor/Strings.cs | 1 + 4 files changed, 120 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index dc0470a..e043c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,11 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ +# and except a Unity project's Packages/, which is source: Unity's package manifest and its +# resolved lock file are both meant to be committed, and a NuGet restore folder never contains +# a file by either name. +!**/[Pp]ackages/manifest.json +!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -651,3 +656,16 @@ Temporary Items # ImGui.ini files imgui.ini + +# Game engine projects +# +# Godot: the import cache, and the mono/temp bin+obj a C# build writes. +.godot/ + +# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule +# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs +# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently +# breaks those references - including for a plug-in whose .dll is itself a build output. This +# negation has to come after that rule to win, and is scoped to the asset tree so the Visual +# Studio artifact stays ignored everywhere else. +!**/[Aa]ssets/**/*.meta diff --git a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs index 178ca2a..02c86cf 100644 --- a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs +++ b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs @@ -104,7 +104,7 @@ public async Task ARateLimited403ReportsFailure() } /// - /// The other headline case: a plain 403, which is what a just-revoked token looks like. + /// The other headline case: a plain 403, a request the token is not permitted to make. /// [TestMethod] public async Task APlain403ReportsFailure() @@ -114,9 +114,76 @@ public async Task APlain403ReportsFailure() bool succeeded = await RunFailing(provider, ApiFailure(HttpStatusCode.Forbidden)).ConfigureAwait(false); Assert.IsFalse(succeeded, "A request refused for lack of authorization did not happen, and must not report success."); + Assert.AreEqual(ProviderStatus.Error, provider.Status); + } + + /// + /// A 403 that is not a rate limit refuses one request -- a workflow dispatch on a repository the + /// token lacks actions: write for, say. The token still works for everything else, so the + /// refusal must not erase it along with every owner that depends on it. + /// + [TestMethod] + public async Task APlain403LeavesTheCredentialsInPlace() + { + GitHub provider = new(); + SetProviderToken(provider, "provider-pat"); + Owner owner = OwnerWithToken(provider); + + bool succeeded = await provider.MakeGitHubRequestAsync( + RequestName, () => Task.FromException(ApiFailure(HttpStatusCode.Forbidden)), owner).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.AreEqual("provider-pat", ProviderToken(provider)); + Assert.AreEqual("alpha-pat", owner.Token.ToString()); + } + + /// + /// A request sent with an owner's override token that GitHub rejects condemns that token, not + /// the provider's. Clearing the provider token took every other owner offline, while the owner + /// token that actually failed stayed in place and was rejected again every cycle. + /// + [TestMethod] + public async Task ARejectedOwnerTokenClearsOnlyThatOwnersToken() + { + GitHub provider = new(); + SetProviderToken(provider, "provider-pat"); + Owner owner = OwnerWithToken(provider); + AuthorizationException unauthorized = new(new FakeResponse(HttpStatusCode.Unauthorized, new Dictionary())); + + bool succeeded = await provider.MakeGitHubRequestAsync( + RequestName, () => Task.FromException(unauthorized), owner).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.IsFalse(owner.HasToken, "The owner token GitHub rejected must not be sent again."); + Assert.AreEqual("provider-pat", ProviderToken(provider), "The provider token was not the one rejected."); + Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status); + } + + /// + /// An owner without its own token uses the provider's, so a rejection there still clears the + /// provider credentials as before. + /// + [TestMethod] + public async Task ARejectedProviderTokenStillClearsTheProviderCredentials() + { + GitHub provider = new(); + SetProviderToken(provider, "provider-pat"); + Owner owner = provider.CreateOwner(OwnerName.Create("beta")); + AuthorizationException unauthorized = new(new FakeResponse(HttpStatusCode.Unauthorized, new Dictionary())); + + bool succeeded = await provider.MakeGitHubRequestAsync( + RequestName, () => Task.FromException(unauthorized), owner).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.AreEqual(string.Empty, ProviderToken(provider)); Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status); } + private static void SetProviderToken(GitHub provider, string token) => + Assert.IsTrue(TokenStorage.Write(provider.TokenPersona, BuildProviderToken.Create(token))); + + private static string ProviderToken(GitHub provider) => TokenStorage.Read(provider.TokenPersona).ToString(); + [TestMethod] public async Task A429ReportsFailure() { diff --git a/BuildMonitor/Providers/GitHub.cs b/BuildMonitor/Providers/GitHub.cs index b0dd2ab..1f88bfa 100644 --- a/BuildMonitor/Providers/GitHub.cs +++ b/BuildMonitor/Providers/GitHub.cs @@ -680,6 +680,9 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, // concurrent requests use different owner tokens SetCurrentClient(owner); + // Which credential this request is sent with decides which one a rejection condemns. + Owner? tokenOwner = owner?.HasToken == true ? owner : null; + // Use smart waiting: if rate limited with a known reset time, wait until reset TimeSpan waitTime = GetRateLimitWaitTime(); if (waitTime > BaseRequestDelay) @@ -701,7 +704,15 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, catch (AuthorizationException) { Log.Error($"{Name}: AuthorizationException for request '{name}'"); - OnAuthenticationFailure(); + if (tokenOwner is not null) + { + OnOwnerAuthenticationFailure(tokenOwner); + } + else + { + OnAuthenticationFailure(); + } + return false; } catch (ApiException e) @@ -718,8 +729,11 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, } else { + // A token that is not allowed to make this one request -- a workflow + // dispatch without actions: write, say -- is still a working token. + // The refusal belongs to this request, not to the credentials. Log.Error($"{Name}: 403 Forbidden for request '{name}' - {e.Message}"); - OnAuthenticationFailure(); + SetStatus(ProviderStatus.Error, $"{Strings.Forbidden}: {name}"); } break; case System.Net.HttpStatusCode.TooManyRequests: @@ -746,6 +760,23 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, } } + /// + /// Clears an owner's override token after GitHub rejected it. + /// + /// The owner whose token the rejected request was sent with. + /// + /// The provider credentials were not used for the request, so they are left alone: clearing them + /// took every other owner offline while the token that actually failed stayed in place and kept + /// being sent, and rejected, every cycle. Clearing the owner token instead lets that owner fall + /// back to the provider token until a new one is set. + /// + private void OnOwnerAuthenticationFailure(Owner owner) + { + Log.Error($"{Name}: Authentication failed for owner '{owner.Name}' - owner token cleared"); + owner.Token = new(); + SetStatus(ProviderStatus.AuthFailed, $"{Strings.AuthFailedMessage} ({owner.Name})"); + } + /// /// Updates the rate limit budget from the last API response. /// Uses Octokit's GetLastApiInfo() to retrieve rate limit headers. diff --git a/BuildMonitor/Strings.cs b/BuildMonitor/Strings.cs index 5a161f1..36c64f1 100644 --- a/BuildMonitor/Strings.cs +++ b/BuildMonitor/Strings.cs @@ -44,6 +44,7 @@ internal static class Strings internal static string RateLimitedMessage { get; } = "Rate limited."; internal static string AuthFailed { get; } = nameof(AuthFailed).Titleize(); internal static string AuthFailedMessage { get; } = "Authentication failed. Please update credentials."; + internal static string Forbidden { get; } = "Forbidden"; internal static string ConnectionError { get; } = nameof(ConnectionError).Titleize(); internal static string ConnectionErrorMessage { get; } = "Connection error."; internal static string Delay { get; } = nameof(Delay).Titleize();