From 90ce747a61752636e82044770fa21c85fe71a79b Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 19 Sep 2026 16:13:38 +1000 Subject: [PATCH] Name the cause when a watch stops on an auth failure A 401/403 was handled as one thing, so watchpr re-minted on every rejection and reported "token expired" for a permission boundary or an anonymous run that had no token to expire, sending the reader after the wrong problem. Classify a 401/403 as a rejected credential, a permission denial, or a request that carried no token, and re-mint only the first. Reject a re-minted empty token instead of replaying anonymously. Report the classified cause from --once as well as from the watch loop. Document watchpr's exit behaviour per cause. --- AGENTS.md | 19 +++- README.md | 19 ++++ cmd/watchpr/main.go | 28 +++-- cmd/watchpr/main_test.go | 77 ++++++++++++++ internal/agent/client_test.go | 192 ++++++++++++++++++++++++++++++++++ internal/agent/gitea.go | 69 +++++++++++- 6 files changed, 387 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 00f79fe..17c86b7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -175,11 +175,22 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`. ## Gotchas - `watchpr` exits 0 with no output changes on `--once` (just prints state). -- Gitea tokens expire in ~1h, shorter than a watch: the client re-mints once on a - 401/403 and replays the request. If the fresh token is rejected too, `watchpr` - exits non-zero rather than polling blind. +- Gitea tokens expire in ~1h, shorter than a watch: the client re-mints once when + the credential it sent was rejected and replays the request. If the fresh token + is rejected too, `watchpr` exits non-zero rather than polling blind. +- A 401/403 is classified before anything is re-minted, because only one of the + three cases is a stale token: `ErrNoCredential` (the request carried no token — + anonymous access to something not public), `IsPermissionDenied` (a 403 whose + body names no credential, so the identity is simply not allowed) and + `IsCredentialRejected` (any 401, or a 403 mentioning a token/scope/sign-in, + which is what Gitea returns for a token missing a scope). Only the last + re-mints; the others abort immediately, since a fresh token cannot fix them and + blaming one hides the real cause. `IsAuthError` stays "any 401/403" — all three + end a watch. - `watchpr` polls anonymously when no token can be minted (public repos work - fine); only a real 401/403 reaches for Vault. + fine); an anonymous run never reaches for Vault, on any status code. +- A re-mint that hands back an empty token is an error: replaying with it would + drop the Authorization header and silently continue as an anonymous watcher. - The token cache is process-wide (mutex-guarded); `RefreshGiteaToken` replaces it. Tests call the unexported `fetchGiteaToken` to avoid the cache. - `agentvault` never puts a secret in an error string: Vault decode failures and diff --git a/README.md b/README.md index c407371..6a80e59 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,25 @@ watchpr --once --json unkin/argocd-apps#42 On a meaningful change `watchpr` prints the reason and the PR's current state, then exits 0. Use `--json` for machine-readable output. +### Exit behaviour + +A watcher that sees nothing must not look healthy, so every terminal failure +exits non-zero naming its cause: + +| Cause | Message | +|---|---| +| the token was rejected and a fresh one was too | `gitea rejected the token and re-minting did not recover it` | +| authenticated but not allowed (Gitea 403, no token named) | `gitea denied access to ` | +| polling anonymously and the PR is not public | `gitea requires authentication and no token could be minted` | +| the PR lookup 404s (repo deleted, renamed, made private) | `PR no longer visible` | + +Gitea tokens expire in ~1h, far shorter than a watch, so a rejected token is +re-minted once and the request replayed; only a failure that survives that +re-mint ends the watch. Anonymous polling of a public repo is unaffected — with +no token there is nothing to reject and Vault is never reached for one. +Transient failures (5xx, network errors, rate limiting) are warned about and +retried, and give up after 20 consecutive failures of the same PR. + ## agentws `agentws` gives an agent an isolated git worktree per branch without disturbing diff --git a/cmd/watchpr/main.go b/cmd/watchpr/main.go index 037ca74..3efcee5 100644 --- a/cmd/watchpr/main.go +++ b/cmd/watchpr/main.go @@ -105,7 +105,7 @@ func runOnce(c *agent.GiteaClient, refs []agent.PRRef, jsonMode bool) error { for _, ref := range refs { st, err := agent.FetchState(c, ref, login) if err != nil { - return err + return describeFailure(err) } states = append(states, st) } @@ -137,18 +137,30 @@ func runWatch(c *agent.GiteaClient, refs []agent.PRRef, interval time.Duration, res, err := agent.Watch(c, refs, login, ticker.C, onBaseline, onError) if err != nil { - if agent.IsAuthError(err) { - return fmt.Errorf("gitea authentication failed after re-minting the token, watch aborted: %w", err) - } - if agent.IsPRGone(err) { - return fmt.Errorf("PR no longer visible (repo deleted, renamed, or made private), watch aborted: %w", err) - } - return err + return describeFailure(err) } report(res.Ref.String(), res.Reason, res.State, jsonMode) return nil } +// describeFailure names the cause of a terminal failure so a watcher that stops +// says why. An anonymous rejection, a permission boundary and a token that +// outlived its Vault lease are three different problems and only the last is +// fixed by a fresh token. +func describeFailure(err error) error { + switch { + case agent.IsNoCredential(err): + return fmt.Errorf("gitea requires authentication and no token could be minted, aborted: %w", err) + case agent.IsPermissionDenied(err): + return fmt.Errorf("gitea denied access to %s (a fresh token will not help), aborted: %w", agent.AgentLogin(), err) + case agent.IsAuthError(err): + return fmt.Errorf("gitea rejected the token and re-minting did not recover it, aborted: %w", err) + case agent.IsPRGone(err): + return fmt.Errorf("PR no longer visible (repo deleted, renamed, or made private), aborted: %w", err) + } + return err +} + // report emits the change that ended the watch. func report(key, reason string, st agent.PRState, jsonMode bool) { if jsonMode { diff --git a/cmd/watchpr/main_test.go b/cmd/watchpr/main_test.go index 06b784f..4932472 100644 --- a/cmd/watchpr/main_test.go +++ b/cmd/watchpr/main_test.go @@ -1,11 +1,15 @@ package main import ( + "errors" + "fmt" "io" "net/http" "net/http/httptest" "strings" "testing" + + "git.unkin.net/unkin/agent-tools/internal/agent" ) // A bad PR reference must fail the command (so main exits non-zero) rather than @@ -105,3 +109,76 @@ func TestExecuteBadIntervalErrors(t *testing.T) { } } } + +// failingVault serves an AppRole login that never issues a token, so the +// command falls back to anonymous polling exactly as it does when Vault is +// unreachable. +func failingVault(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + t.Cleanup(srv.Close) + return srv +} + +// An anonymous run against a repo that is not public must exit non-zero saying +// no token was available — not claim a token expired, and not keep going. +func TestOnceAnonymousRejectionNamesTheMissingToken(t *testing.T) { + vault := failingVault(t) + + requests := 0 + gitea := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + _, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`) + })) + defer gitea.Close() + + t.Setenv("VAULT_ADDR", vault.URL) + t.Setenv("GITEA_URL", gitea.URL) + + cmd := newRootCmd() + cmd.SetArgs([]string{"--once", "unkin/repo#7"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + if err == nil { + t.Fatal("Execute() = nil, want a non-zero exit when the poll is rejected") + } + if !strings.Contains(err.Error(), "no token could be minted") { + t.Errorf("Execute() error = %q, want it to name the missing token", err) + } + if requests != 1 { + t.Errorf("gitea requests = %d, want 1 (no replay without a credential)", requests) + } +} + +// describeFailure must tell the four terminal causes apart: each one sends the +// reader somewhere different, and a watcher that stops without saying why is +// the failure this names. +func TestDescribeFailureNamesTheCause(t *testing.T) { + rejected := &agent.APIError{Method: "GET", Path: "/p", StatusCode: 401, Body: `{"message":"invalid username, password or token"}`} + forbidden := &agent.APIError{Method: "GET", Path: "/p", StatusCode: 403, Body: `{"message":"Forbidden"}`} + tests := []struct { + name string + err error + want string + }{ + {"anonymous", fmt.Errorf("%w: %w", agent.ErrNoCredential, rejected), "no token could be minted"}, + {"permission boundary", error(forbidden), "denied access"}, + {"rejected token", error(rejected), "re-minting did not recover it"}, + {"other", errors.New("dial tcp: timeout"), "dial tcp: timeout"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := describeFailure(tt.err) + if got == nil || !strings.Contains(got.Error(), tt.want) { + t.Errorf("describeFailure = %v, want it to mention %q", got, tt.want) + } + if !errors.Is(got, tt.err) { + t.Errorf("describeFailure dropped the underlying error %v", tt.err) + } + }) + } +} diff --git a/internal/agent/client_test.go b/internal/agent/client_test.go index c1aacfc..182541f 100644 --- a/internal/agent/client_test.go +++ b/internal/agent/client_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" ) // fakeVault serves the AppRole login and the gitea creds secret at credsPath @@ -618,3 +619,194 @@ func TestAnonymousPollingNeverMints(t *testing.T) { t.Errorf("sent %d Authorization headers, want none", authHeaders) } } + +// Anonymous access to something that is not public is not a stale credential: +// nothing was sent to be rejected, so the client must not burn a Vault mint on +// every poll, and the error must say a token was missing rather than expired. +func TestAnonymousAuthFailureNeverMints(t *testing.T) { + requests := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + _, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + refreshes := 0 + c := &GiteaClient{BaseURL: srv.URL, HTTP: srv.Client(), + Refresh: func() (string, error) { refreshes++; return "fresh", nil }} + + _, err := c.GetPR("unkin/repo", 7) + if err == nil { + t.Fatal("expected an error on an anonymous 401") + } + if !IsNoCredential(err) { + t.Errorf("IsNoCredential(%v) = false, want true", err) + } + if !IsAuthError(err) { + t.Errorf("IsAuthError(%v) = false; an anonymous rejection still ends a watch", err) + } + if refreshes != 0 { + t.Errorf("refreshes = %d, want 0 (nothing was rejected)", refreshes) + } + if requests != 1 { + t.Errorf("requests = %d, want 1 (no replay)", requests) + } +} + +// Gitea's bare "Forbidden" is a permission boundary, not an expired token. A +// re-mint cannot grant a permission the identity lacks, so the client must not +// spend one, and the failure must not be reported as an auth expiry. +func TestPermissionDeniedIsNotRemintedOrRetried(t *testing.T) { + requests := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusForbidden) + _, _ = io.WriteString(w, `{"errors":null,"message":"Forbidden","url":"https://git.unkin.net/api/swagger"}`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + refreshes := 0 + c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client(), + Refresh: func() (string, error) { refreshes++; return "fresh", nil }} + + _, err := c.GetPR("unkin/repo", 7) + if err == nil { + t.Fatal("expected an error on a 403") + } + if !IsPermissionDenied(err) { + t.Errorf("IsPermissionDenied(%v) = false, want true", err) + } + if IsCredentialRejected(err) { + t.Errorf("a bare Forbidden must not read as a rejected credential") + } + if refreshes != 0 || requests != 1 { + t.Errorf("refreshes = %d, requests = %d, want 0 and 1", refreshes, requests) + } +} + +// A 403 that names the token is a credential problem after all (Gitea reports a +// missing scope this way), so it keeps the re-mint-and-replay path. +func TestScopeForbiddenIsRemintedAndRetried(t *testing.T) { + var seen []string + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + tok := strings.TrimPrefix(r.Header.Get("Authorization"), "token ") + seen = append(seen, tok) + if tok != "fresh" { + w.WriteHeader(http.StatusForbidden) + _, _ = io.WriteString(w, `{"message":"token does not have at least one of required scope(s): [read:repository]"}`) + return + } + _, _ = io.WriteString(w, `{"number":7,"state":"open","mergeable":true,"head":{"sha":"cafebabe"}}`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + refreshes := 0 + c := &GiteaClient{BaseURL: srv.URL, Token: "stale", HTTP: srv.Client(), + Refresh: func() (string, error) { refreshes++; return "fresh", nil }} + + pr, err := c.GetPR("unkin/repo", 7) + if err != nil { + t.Fatalf("GetPR after re-mint: %v", err) + } + if pr.Number != 7 || refreshes != 1 { + t.Errorf("PR = %+v, refreshes = %d, want PR 7 and 1 re-mint", pr, refreshes) + } + if len(seen) != 2 || seen[1] != "fresh" { + t.Errorf("tokens seen = %v, want [stale fresh]", seen) + } +} + +// A re-mint that hands back an empty token must fail loudly. Replaying with it +// would drop the Authorization header, and on a public repo that anonymous +// replay succeeds — the watch would carry on having quietly lost its identity. +func TestEmptyRemintedTokenFailsInsteadOfGoingAnonymous(t *testing.T) { + var seen []string + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + tok := strings.TrimPrefix(r.Header.Get("Authorization"), "token ") + seen = append(seen, tok) + if tok == "stale" { + w.WriteHeader(http.StatusUnauthorized) + _, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`) + return + } + _, _ = io.WriteString(w, `{"number":7,"state":"open","mergeable":true,"head":{"sha":"cafebabe"}}`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, Token: "stale", HTTP: srv.Client(), + Refresh: func() (string, error) { return "", nil }} + + _, err := c.GetPR("unkin/repo", 7) + if err == nil { + t.Fatal("an empty re-minted token must be an error, not an anonymous retry") + } + if !IsAuthError(err) { + t.Errorf("IsAuthError(%v) = false, want true", err) + } + if len(seen) != 1 { + t.Errorf("requests = %d, want 1 (no anonymous replay)", len(seen)) + } +} + +// The real-world rotation case end to end: the watch runs happily for several +// polls, then the Vault lease expires and Gitea rejects every token, the fresh +// one included. Watch must end with a named auth failure instead of polling on. +func TestWatchAbortsWhenTokenExpiresMidWatch(t *testing.T) { + polls := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + polls++ + if polls > 3 { + w.WriteHeader(http.StatusUnauthorized) + _, _ = io.WriteString(w, `{"message":"invalid username, password or token"}`) + return + } + _, _ = io.WriteString(w, `{"number":7,"state":"open","mergeable":true,"head":{"sha":"cafebabe"}}`) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/commits/cafebabe/status", func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"state":"success"}`) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/issues/7/comments", func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `[]`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + refreshes := 0 + c := &GiteaClient{BaseURL: srv.URL, Token: "t1", HTTP: srv.Client(), + Refresh: func() (string, error) { refreshes++; return "t2", nil }} + + ref := PRRef{Owner: "unkin", Repo: "repo", Number: 7} + ticks := make(chan time.Time, 5) + for i := 0; i < 5; i++ { + ticks <- time.Now() + } + close(ticks) + + warned := 0 + _, err := Watch(c, []PRRef{ref}, "unkin-agent", ticks, nil, func(PRRef, error) { warned++ }) + if err == nil { + t.Fatal("Watch returned nil: an expired token must end the watch, not be polled past") + } + if !IsAuthError(err) { + t.Errorf("Watch error = %v, want an auth error", err) + } + if IsNoCredential(err) { + t.Errorf("a rejected token must not be reported as a missing one: %v", err) + } + if refreshes != 1 { + t.Errorf("refreshes = %d, want 1", refreshes) + } + if warned != 0 { + t.Errorf("auth failure logged as a warning %d time(s); it must abort", warned) + } +} diff --git a/internal/agent/gitea.go b/internal/agent/gitea.go index 2f7fd74..d075580 100644 --- a/internal/agent/gitea.go +++ b/internal/agent/gitea.go @@ -32,14 +32,61 @@ func IsNotFound(err error) bool { return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound } -// IsAuthError reports whether err is a Gitea 401/403: the token is expired or -// unauthorised, which retrying the same request cannot fix. +// IsAuthError reports whether err is a Gitea 401/403. Both end a watch: neither +// a rejected credential nor a permission boundary clears itself on a retry. func IsAuthError(err error) bool { var apiErr *APIError return errors.As(err, &apiErr) && (apiErr.StatusCode == http.StatusUnauthorized || apiErr.StatusCode == http.StatusForbidden) } +// ErrNoCredential marks a 401/403 on a request that carried no token at all. +// Anonymous polling of a public repo is supported, so this is not a rejected +// credential: the resource simply is not public and no token was available. +var ErrNoCredential = errors.New("gitea requires authentication and no token was available") + +// IsNoCredential reports whether err is an auth failure on an anonymous request. +func IsNoCredential(err error) bool { + return errors.Is(err, ErrNoCredential) +} + +// credentialHints are the fragments Gitea puts in a 403 body when the +// credential itself is at fault ("token does not have at least one of required +// scope(s)", "sign in required") rather than the identity's permissions, whose +// body is a bare "Forbidden". +var credentialHints = []string{"token", "sign in", "credential"} + +// IsCredentialRejected reports whether err means the credential that was sent +// was refused, which a freshly minted token may fix. A 401 always qualifies. +// Gitea 403s both for a token missing a scope and for an identity that may not +// do this at all, so for a 403 the response body decides. +func IsCredentialRejected(err error) bool { + var apiErr *APIError + if !errors.As(err, &apiErr) { + return false + } + switch apiErr.StatusCode { + case http.StatusUnauthorized: + return true + case http.StatusForbidden: + body := strings.ToLower(apiErr.Body) + for _, hint := range credentialHints { + if strings.Contains(body, hint) { + return true + } + } + } + return false +} + +// IsPermissionDenied reports a 403 that names no credential problem: the +// identity is authenticated but not allowed, so re-minting cannot help. +func IsPermissionDenied(err error) bool { + var apiErr *APIError + return errors.As(err, &apiErr) && + apiErr.StatusCode == http.StatusForbidden && !IsCredentialRejected(err) +} + // GiteaClient talks to the Gitea REST API as the agent user. type GiteaClient struct { BaseURL string @@ -56,8 +103,10 @@ func NewGiteaClient(token string) *GiteaClient { return &GiteaClient{BaseURL: GiteaURL(), Token: token, HTTP: httpClient, Refresh: RefreshGiteaToken} } -// do sends the request and, if the token was rejected, re-mints it once and -// replays the request with the fresh token. +// do sends the request and, if the credential it carried was rejected, re-mints +// the token once and replays the request. Anonymous requests and permission +// denials are returned as they are: neither is fixed by a fresh token, and +// re-minting on them would report a stale token as the cause of something else. func (c *GiteaClient) do(method, path string, body any, out any) error { var payload []byte if body != nil { @@ -67,14 +116,24 @@ func (c *GiteaClient) do(method, path string, body any, out any) error { } payload = b } + anonymous := c.Token == "" err := c.attempt(method, path, payload, out) - if !IsAuthError(err) || c.Refresh == nil { + if !IsAuthError(err) { + return err + } + if anonymous { + return fmt.Errorf("%w: %w", ErrNoCredential, err) + } + if !IsCredentialRejected(err) || c.Refresh == nil { return err } token, refreshErr := c.Refresh() if refreshErr != nil { return fmt.Errorf("%w; re-minting token: %v", err, refreshErr) } + if token == "" { + return fmt.Errorf("%w; re-minting token yielded an empty token", err) + } c.Token = token return c.attempt(method, path, payload, out) }