From da2277ac19277a333a2ed456bd31d6a8d338aad9 Mon Sep 17 00:00:00 2001 From: Savitha Raghunathan Date: Mon, 10 Aug 2026 13:53:14 -0400 Subject: [PATCH 1/2] :bug: Fail-closed token revocation on intermediate workflow stage failure (#109) Assisted-By: Claude Code Signed-off-by: Savitha Raghunathan --- harness/cmd/migration-harness/main.go | 55 +++--- harness/cmd/migration-harness/main_test.go | 186 +++++++++------------ 2 files changed, 113 insertions(+), 128 deletions(-) diff --git a/harness/cmd/migration-harness/main.go b/harness/cmd/migration-harness/main.go index fec55ffd..ef3d89ed 100644 --- a/harness/cmd/migration-harness/main.go +++ b/harness/cmd/migration-harness/main.go @@ -63,11 +63,23 @@ func runStage(cmd *cobra.Command, args []string) error { cloneDir = "/workspace/repo" } - // Stage-aware token revocation: register cleanup before Hub resolution - // so the token is revoked even if resolveFromHub fails partway. + // Fail-closed token revocation: always register cleanup when a valid + // token ID exists. The defer decides at exit time whether to actually + // revoke — only an intermediate workflow stage that succeeded skips + // revocation (the next stage needs the token). Every other exit path + // (failure, last stage, standalone run) revokes immediately. hubClient := hub.NewClient(cfg.HubBaseURL, cfg.HubToken) - if tokenID, revoke := shouldRevokeToken(cfg); revoke { + var stageSucceeded bool + if tokenID, ok := parseHubTokenID(cfg); ok { + intermediate := isIntermediateWorkflowStage(cfg) defer func() { + if intermediate && stageSucceeded { + logging.Info("intermediate workflow stage succeeded — deferring token revocation to next stage") + return + } + if intermediate { + logging.Warn("intermediate workflow stage failed — revoking token early (no subsequent stage will run)") + } if err := hubClient.RevokeToken(tokenID); err != nil { logging.Warn("hub token revocation (id=%d): %v", tokenID, err) } else { @@ -77,13 +89,7 @@ func runStage(cmd *cobra.Command, args []string) error { } else if cfg.HubTokenID == "" && cfg.HubToken != "" { logging.Warn("HUB_TOKEN_ID not set — skipping token revocation (token will expire via TTL)") } else if cfg.HubTokenID != "" { - stage, sErr := strconv.ParseUint(cfg.WorkflowStage, 10, 64) - count, cErr := strconv.ParseUint(cfg.WorkflowStageCount, 10, 64) - if sErr == nil && cErr == nil && stage > 0 && count > 0 { - logging.Info("workflow stage %d/%d — skipping token revocation", stage, count) - } else { - logging.Warn("invalid workflow metadata (stage=%q, count=%q) — skipping token revocation", cfg.WorkflowStage, cfg.WorkflowStageCount) - } + logging.Warn("HUB_TOKEN_ID %q is not a valid numeric ID — skipping token revocation", cfg.HubTokenID) } creds, err := resolveFromHub(cfg, hubClient) @@ -361,6 +367,7 @@ func runStage(cmd *cobra.Command, args []string) error { logging.Err("stage failed") return fmt.Errorf("stage failed") } + stageSucceeded = true emitNotice("stage succeeded — results pushed to branch %s", creds.Branch) logging.Ok("stage succeeded") return nil @@ -436,11 +443,9 @@ func resolveFromHub(cfg *config.Config, hubClient *hub.Client) (*git.Credentials return creds, nil } -// shouldRevokeToken decides whether the harness should revoke the Hub API -// token on exit and returns the parsed token ID. Standalone AgentRuns -// always revoke. Workflow stages revoke only on the last stage so -// subsequent stages can reuse the token. -func shouldRevokeToken(cfg *config.Config) (uint, bool) { +// parseHubTokenID extracts the Hub API token ID from config. +// Returns (0, false) when no valid token ID is available. +func parseHubTokenID(cfg *config.Config) (uint, bool) { if cfg.HubTokenID == "" { return 0, false } @@ -448,21 +453,25 @@ func shouldRevokeToken(cfg *config.Config) (uint, bool) { if err != nil { return 0, false } - if cfg.WorkflowStage == "" && cfg.WorkflowStageCount == "" { - return uint(tokenID), true + return uint(tokenID), true +} + +// isIntermediateWorkflowStage reports whether the harness is running an +// intermediate (not last) stage of a multi-stage workflow. Returns false +// for standalone runs, last stages, and invalid metadata. +func isIntermediateWorkflowStage(cfg *config.Config) bool { + if cfg.WorkflowStage == "" || cfg.WorkflowStageCount == "" { + return false } stage, err := strconv.ParseUint(cfg.WorkflowStage, 10, 64) if err != nil || stage == 0 { - return 0, false + return false } count, err := strconv.ParseUint(cfg.WorkflowStageCount, 10, 64) if err != nil || count == 0 { - return 0, false - } - if stage == count { - return uint(tokenID), true + return false } - return 0, false + return stage < count } func fetchAndWriteAnalysis(hubClient *hub.Client, appIDStr string, workDir string) (bool, error) { diff --git a/harness/cmd/migration-harness/main_test.go b/harness/cmd/migration-harness/main_test.go index b6ab9c65..6380ade8 100644 --- a/harness/cmd/migration-harness/main_test.go +++ b/harness/cmd/migration-harness/main_test.go @@ -72,114 +72,81 @@ func TestDiscoverSkills_EmptySkillFile(t *testing.T) { } } -func TestShouldRevokeToken(t *testing.T) { +func TestParseHubTokenID(t *testing.T) { + tests := []struct { + name string + hubTokenID string + wantID uint + wantOK bool + }{ + {"empty", "", 0, false}, + {"valid", "42", 42, true}, + {"non-numeric", "abc", 0, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &config.Config{HubTokenID: tt.hubTokenID} + id, ok := parseHubTokenID(cfg) + if id != tt.wantID || ok != tt.wantOK { + t.Errorf("parseHubTokenID() = (%d, %v), want (%d, %v)", id, ok, tt.wantID, tt.wantOK) + } + }) + } +} + +func TestIsIntermediateWorkflowStage(t *testing.T) { tests := []struct { name string - hubTokenID string workflowStage string workflowStageCount string want bool }{ - { - name: "no token ID — skip revocation", - want: false, - }, - { - name: "standalone run — revoke", - hubTokenID: "1", - want: true, - }, - { - name: "last workflow stage — revoke", - hubTokenID: "1", - workflowStage: "3", - workflowStageCount: "3", - want: true, - }, - { - name: "intermediate workflow stage — skip", - hubTokenID: "1", - workflowStage: "1", - workflowStageCount: "3", - want: false, - }, - { - name: "first of two stages — skip", - hubTokenID: "1", - workflowStage: "1", - workflowStageCount: "2", - want: false, - }, - { - name: "single-stage workflow — revoke", - hubTokenID: "1", - workflowStage: "1", - workflowStageCount: "1", - want: true, - }, - { - name: "stage set but count missing — skip", - hubTokenID: "1", - workflowStage: "1", - workflowStageCount: "", - want: false, - }, - { - name: "count set but stage missing — skip", - hubTokenID: "1", - workflowStage: "", - workflowStageCount: "3", - want: false, - }, - { - name: "stage exceeds count — skip", - hubTokenID: "1", - workflowStage: "5", - workflowStageCount: "3", - want: false, - }, - { - name: "non-numeric stage — skip", - hubTokenID: "1", - workflowStage: "abc", - workflowStageCount: "3", - want: false, - }, - { - name: "non-numeric count — skip", - hubTokenID: "1", - workflowStage: "1", - workflowStageCount: "xyz", - want: false, - }, - { - name: "stage zero — skip", - hubTokenID: "1", - workflowStage: "0", - workflowStageCount: "3", - want: false, - }, - { - name: "equal non-numeric values — skip", - hubTokenID: "1", - workflowStage: "abc", - workflowStageCount: "abc", - want: false, - }, - { - name: "equal zero values — skip", - hubTokenID: "1", - workflowStage: "0", - workflowStageCount: "0", - want: false, - }, - { - name: "non-numeric token ID — skip", - hubTokenID: "abc", - workflowStage: "", - workflowStageCount: "", - want: false, - }, + {"standalone run", "", "", false}, + {"last stage 3/3", "3", "3", false}, + {"intermediate 1/3", "1", "3", true}, + {"intermediate 1/2", "1", "2", true}, + {"intermediate 2/3", "2", "3", true}, + {"single stage 1/1", "1", "1", false}, + {"stage only", "1", "", false}, + {"count only", "", "3", false}, + {"stage zero", "0", "3", false}, + {"count zero", "1", "0", false}, + {"non-numeric stage", "abc", "3", false}, + {"non-numeric count", "1", "xyz", false}, + {"stage exceeds count", "5", "3", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &config.Config{ + WorkflowStage: tt.workflowStage, + WorkflowStageCount: tt.workflowStageCount, + } + if got := isIntermediateWorkflowStage(cfg); got != tt.want { + t.Errorf("isIntermediateWorkflowStage() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestTokenRevocationDecision(t *testing.T) { + tests := []struct { + name string + hubTokenID string + workflowStage string + workflowStageCount string + stageSucceeded bool + wantRevoke bool + }{ + {"no token ID", "", "", "", false, false}, + {"standalone success", "1", "", "", true, true}, + {"standalone failure", "1", "", "", false, true}, + {"last stage success", "1", "3", "3", true, true}, + {"last stage failure", "1", "3", "3", false, true}, + {"intermediate success — defer to next stage", "1", "1", "3", true, false}, + {"intermediate failure — revoke (#109)", "1", "1", "3", false, true}, + {"single-stage success", "1", "1", "1", true, true}, + {"single-stage failure", "1", "1", "1", false, true}, + {"non-numeric token ID", "abc", "", "", false, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -188,9 +155,18 @@ func TestShouldRevokeToken(t *testing.T) { WorkflowStage: tt.workflowStage, WorkflowStageCount: tt.workflowStageCount, } - _, got := shouldRevokeToken(cfg) - if got != tt.want { - t.Errorf("shouldRevokeToken() = %v, want %v", got, tt.want) + _, hasToken := parseHubTokenID(cfg) + if !hasToken { + if tt.wantRevoke { + t.Error("expected revocation but no token ID available") + } + return + } + intermediate := isIntermediateWorkflowStage(cfg) + shouldRevoke := !(intermediate && tt.stageSucceeded) + if shouldRevoke != tt.wantRevoke { + t.Errorf("revocation decision = %v, want %v (intermediate=%v, stageSucceeded=%v)", + shouldRevoke, tt.wantRevoke, intermediate, tt.stageSucceeded) } }) } From a9a175d1947ea92f17718ce44752c8700e0a6ed7 Mon Sep 17 00:00:00 2001 From: Savitha Raghunathan Date: Mon, 10 Aug 2026 13:54:05 -0400 Subject: [PATCH 2/2] :memo: Add changelog fragment for #109 Assisted-By: Claude Code Signed-off-by: Savitha Raghunathan --- changes/unreleased/109-fail-closed-token-revocation.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/unreleased/109-fail-closed-token-revocation.yaml diff --git a/changes/unreleased/109-fail-closed-token-revocation.yaml b/changes/unreleased/109-fail-closed-token-revocation.yaml new file mode 100644 index 00000000..3b468371 --- /dev/null +++ b/changes/unreleased/109-fail-closed-token-revocation.yaml @@ -0,0 +1,6 @@ +kind: bugfix +description: > + Fix Hub API token leak when an intermediate workflow stage fails. The + harness now defaults to fail-closed: the token is revoked on every exit + path except when an intermediate stage succeeds and a subsequent stage + needs the token.