Skip to content

🐛 Fail-closed token revocation on intermediate stage failure #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
savitharaghunathan merged 2 commits into from
Aug 12, 2026
Merged

🐛 Fail-closed token revocation on intermediate stage failure #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da2277a
:bug: Fail-closed token revocation on intermediate workflow stage fai…
savitharaghunathan Aug 10, 2026
a9a175d
:memo: Add changelog fragment for #109
savitharaghunathan Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changes/unreleased/109-fail-closed-token-revocation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
55 changes: 32 additions & 23 deletions harness/cmd/migration-harness/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -436,33 +443,35 @@ 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
}
tokenID, err := strconv.ParseUint(cfg.HubTokenID, 10, 64)
if err != nil {
return 0, false
}
if cfg.WorkflowStage == "" && cfg.WorkflowStageCount == "" {
return uint(tokenID), true
return uint(tokenID), true
Comment on lines 452 to +456

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect declared build targets. A 32-bit target makes the uint64-to-uint
# conversion lossy for token IDs above math.MaxUint32.
rg -n -i \
  --glob 'Dockerfile*' \
  --glob '*.yaml' \
  --glob '*.yml' \
  --glob '*.json' \
  --glob '*.sh' \
  'GOARCH|GOOS|--platform|architecture' .

# Show the parser, its tests, and the downstream uint API contract.
rg -n -C 4 'func parseHubTokenID|ParseUint|func \(c \*Client\) RevokeToken|func TestParseHubTokenID' \
  harness/cmd/migration-harness/main.go \
  harness/cmd/migration-harness/main_test.go \
  harness/internal/hub/client.go

Repository: konveyor/agentic-controller

Length of output: 6771

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== parser and tests =="
sed -n '440,505p' harness/cmd/migration-harness/main.go
echo
sed -n '75,95p' harness/cmd/migration-harness/main_test.go

echo
echo "== Dockerfile multiarch build context =="
sed -n '1,35p' Dockerfile

echo
echo "== runtime semantics probe =="
cat > /tmp/probe.go <<'EOF'
package main

import (
	"fmt"
	"strconv"
)

func main() {
	for _, s := range []string{"281474976710656", "42"} {
		n, err := strconv.ParseUint(s, 10, 64)
		if err != nil {
			panic(err)
		}
		u := uint(n)
		fmt.Printf("%s -> uint64=%d -> uint=%d (overflow=%t)\n", s, n, u, n != uint64(u))
	}
}
EOF
go run /tmp/probe.go
rm /tmp/probe.go

Repository: konveyor/agentic-controller

Length of output: 4171

🌐 Web query:

Go strconv.IntSize documentation

💡 Result:

In the Go programming language, strconv.IntSize is a constant that represents the size in bits of an int or uint value on the machine where the code is currently running [1][2]. Because the Go int and uint types have a size that is platform-dependent (typically 32 bits on 32-bit systems and 64 bits on 64-bit systems), this constant allows developers to programmatically determine the bit size of these types at runtime [2][3]. It is defined as a constant within the standard library's strconv package [1]. You can access it as follows: import "strconv" fmt.Println(strconv.IntSize) // Outputs 32 or 64 depending on the architecture [2][3]

Citations:

Reject Hub token IDs that overflow the target uint.

strconv.ParseUint(..., 10, 64) accepts values larger than uint on 32-bit targets. In a 32-bit build, a parsed ID truncates when converted to uint, so the harness can revoke the wrong token. Parse with strconv.IntSize and add boundary tests around the maximum uint value and one value above it.

📍 Affects 2 files
  • harness/cmd/migration-harness/main.go#L452-L456 (this comment)
  • harness/cmd/migration-harness/main_test.go#L82-L85
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@harness/cmd/migration-harness/main.go` around lines 452 - 456, Update the Hub
token ID parsing in the relevant helper in main.go to use strconv.IntSize
instead of a fixed 64-bit size, rejecting values that exceed the target uint
range before conversion. Add boundary tests in main_test.go covering the maximum
uint value and the value immediately above it, including 32-bit behavior where
applicable.

}

// 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) {
Expand Down
186 changes: 81 additions & 105 deletions harness/cmd/migration-harness/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
})
}
Expand Down
Loading