chore: disable SDK transient retry on SCA and UAR clients

CHANGELOG.md

@@ -23,6 +23,7 @@ All notable changes to this project will be documented in this file.
 - SDK v0.8.1 enables automatic retries on HTTP 429 and transient transport errors by default. `grant` disables these on its SCA and UAR service clients (see the retry-policy change) because `Elevate` and `SubmitRequest` are non-idempotent POSTs with no idempotency header.
 - SDK v0.8.1 changes 401 / re-authentication handling: the rejected response is drained, a token refresh is forced, a concurrently-refreshed token is adopted rather than duplicated, and cookie-decode errors that were previously silently ignored are now propagated.
 - No new modules enter the compiled binary: `go list -e -deps` reports 38 → 38 external modules (0 delta) and 309 → 310 packages, the single addition being the SDK's own `pkg/common/pvwa`. `go list -m all` grows 110 → 124 selected modules, but none of the additions are in the linked graph. Binary size 13,529,380 → 13,652,260 bytes (+0.91%).
+- Disabled the SDK v0.8.1 automatic transient-retry on both the SCA and Access Requests (UAR) HTTP clients (`internal/sdkclient.DisableTransientRetry`). The SDK retries HTTP 429 with no method filter and bare `EOF` transport errors for any method including POST, which could replay non-idempotent requests (duplicate elevation or a duplicate access request in an approver's queue). Read paths lose automatic rate-limit retry; a 429 now surfaces as an error
 - Reduced `govulncheck` findings from 34 vulnerabilities across 4 modules to 26 (25 of which are Go standard library issues fixed in go1.25.x patch releases, plus the unmaintained `golang.org/x/crypto/openpgp` pulled in by `rhysd/go-github-selfupdate`)
 
 ## [0.7.0] - 2026-04-21

CLAUDE.md

@@ -60,6 +60,19 @@ Custom `SCAAccessService` follows SDK conventions:
 - **Target category:** `CLOUD_CONSOLE` (hardcoded for v1)
 - **Headers:** `Authorization: Bearer {jwt}`, `Content-Type: application/json`
 
+## SDK Retry Behavior
+- idsec-sdk-golang v0.8.1 added automatic transient retry, enabled by default: 3 retries **on top of** the initial attempt (up to 4 requests), 500ms base wait, 10s cap (`pkg/common/idsec_client.go:53-60`, `:375-377`). Backoff jitter can exceed the cap by up to ~50% (`:1331-1349`)
+- Two retry paths, neither safe for non-idempotent POSTs:
+  - **429** (`:865-885`) — no HTTP method filter at all; honors `Retry-After` (clamped to max wait), else exponential backoff
+  - **Transport errors** (`:835-849` via `isRetryableTransportError` `:1244-1283`) — bare `io.EOF`/`io.ErrUnexpectedEOF`/`"eof"`/`"server closed idle connection"` are retried for **any** method including POST (`:1252-1266`); only connection-reset/broken-pipe/GOAWAY are gated behind `isIdempotentMethod` (`:1269-1281`). An EOF can surface after the server already processed the request, so this path can replay a mutation
+- **grant disables transient retry on BOTH the SCA and UAR/workflows clients** via `internal/sdkclient.DisableTransientRetry` (calls `SetTransientRetry(0, 0, 0)`, `:1187-1197`), invoked right after `isp.FromISPAuth` in `internal/sca/service.go` and `internal/workflows/service.go`
+- Why client-wide rather than per-mutation: the SDK has no per-request opt-out, and toggling client state around individual calls is race-prone (grant fans out concurrently across CSPs). SCA `elevate` / `elevate/groups` are as non-idempotent as UAR `submit`
+- `isp.IdsecISPServiceClient` embeds `*common.IdsecClient`, so the helper takes `client.IdsecClient`
+- POSTs protected, by actual replay risk:
+  - **Mutating** — `internal/sca/service.go` elevate, group elevate (a duplicate creates a second session); `internal/workflows/service.go` submit (a duplicate is visible in an approver's queue), cancel, finalize
+  - **Effectively idempotent / read-shaped, covered incidentally** — `internal/sca/service.go` sessions/revoke (re-revoking is a no-op) and on-demand roles (`POST` used only for role discovery)
+- Tests: `internal/sdkclient/retry_test.go` covers the helper; `internal/sca/retry_policy_test.go` and `internal/workflows/retry_policy_test.go` drive the **real constructors** with a fake-JWT ISP token, then repoint `client.BaseURL` at an `httptest` 429 server and assert exactly one inbound request. Removing a `DisableTransientRetry` call makes them fail with 4
+
 ## Testing
 - TDD: write `_test.go` before `.go` for every package
 - Table-driven tests

internal/sca/retry_policy_test.go

@@ -0,0 +1,96 @@
+package sca
+
+import (
+	"encoding/base64"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+
+	"github.com/cyberark/idsec-sdk-golang/pkg/auth"
+	"github.com/cyberark/idsec-sdk-golang/pkg/common/isp"
+	authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth"
+)
+
+// fakeJWT builds an unsigned JWT carrying just the claims the SDK's
+// resolveServiceURL reads (isp/idsec_isp_service_client.go:148-168). It is
+// parsed with ParseUnverified, so no signature is required.
+func fakeJWT(t *testing.T) string {
+	t.Helper()
+	enc := func(v interface{}) string {
+		b, err := json.Marshal(v)
+		if err != nil {
+			t.Fatalf("marshal jwt segment: %v", err)
+		}
+		return base64.RawURLEncoding.EncodeToString(b)
+	}
+	header := enc(map[string]string{"alg": "none", "typ": "JWT"})
+	payload := enc(map[string]string{
+		"subdomain":       "testtenant",
+		"platform_domain": "example.test",
+	})
+	return header + "." + payload + ".sig"
+}
+
+// ispAuthWithToken returns an ISP authenticator pre-loaded with a fake token so
+// isp.FromISPAuth succeeds without real credentials.
+func ispAuthWithToken(t *testing.T) auth.IdsecAuth {
+	t.Helper()
+	a := auth.NewIdsecISPAuth(false)
+	concrete, ok := a.(*auth.IdsecISPAuth)
+	if !ok {
+		t.Fatalf("NewIdsecISPAuth returned %T, want *auth.IdsecISPAuth", a)
+	}
+	concrete.Token = &authmodels.IdsecToken{
+		Token:     fakeJWT(t),
+		TokenType: authmodels.JWT,
+	}
+	return a
+}
+
+// TestNewSCAAccessServiceDisablesTransientRetry exercises the real constructor
+// and asserts the policy landed on the client it publishes. This is the
+// regression guard for the DisableTransientRetry call in NewSCAAccessService:
+// deleting that call makes this test fail with 4 inbound requests.
+func TestNewSCAAccessServiceDisablesTransientRetry(t *testing.T) {
+	var calls int32
+	var gotAPIVersion string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		atomic.AddInt32(&calls, 1)
+		gotAPIVersion = r.Header.Get("X-API-Version")
+		w.Header().Set("Retry-After", "0") // keeps the test fast if retry is on
+		w.WriteHeader(http.StatusTooManyRequests)
+	}))
+	defer srv.Close()
+
+	svc, err := NewSCAAccessService(ispAuthWithToken(t))
+	if err != nil {
+		t.Fatalf("NewSCAAccessService: %v", err)
+	}
+
+	// Redirect the constructed client at the test server. The client is fully
+	// built by the constructor; only its base URL is swapped.
+	lc, ok := svc.httpClient.(*loggingClient)
+	if !ok {
+		t.Fatalf("httpClient is %T, want *loggingClient", svc.httpClient)
+	}
+	client, ok := lc.inner.(*isp.IdsecISPServiceClient)
+	if !ok {
+		t.Fatalf("inner client is %T, want *isp.IdsecISPServiceClient", lc.inner)
+	}
+	client.BaseURL = srv.URL
+
+	resp, err := svc.httpClient.Post(t.Context(), "/api/access/elevate", map[string]string{"a": "b"})
+	if err != nil {
+		t.Fatalf("Post: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	if got := atomic.LoadInt32(&calls); got != 1 {
+		t.Errorf("inbound requests = %d, want 1 (SDK transient retry must be disabled)", got)
+	}
+	if gotAPIVersion != "2.0" {
+		t.Errorf("X-API-Version = %q, want %q", gotAPIVersion, "2.0")
+	}
+}

internal/sca/service.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 
 	"github.com/aaearon/grant-cli/internal/sca/models"
+	"github.com/aaearon/grant-cli/internal/sdkclient"
 	"github.com/cyberark/idsec-sdk-golang/pkg/auth"
 	"github.com/cyberark/idsec-sdk-golang/pkg/common"
 	"github.com/cyberark/idsec-sdk-golang/pkg/common/isp"
@@ -61,6 +62,14 @@ func NewSCAAccessService(authenticators ...auth.IdsecAuth) (*SCAAccessService, e
 		return nil, fmt.Errorf("failed to create ISP client: %w", err)
 	}
 
+	// Disable the SDK's automatic transient retry. SDK v0.8.1 retries 429s with
+	// no method filter (idsec_client.go:865-885) and bare EOF transport errors
+	// for any method including POST (:1252-1266), which could replay the
+	// non-idempotent elevate / group-elevate POSTs below. The client-wide
+	// switch also covers sessions/revoke and on-demand role discovery, which
+	// are effectively idempotent and safe to retry either way.
+	sdkclient.DisableTransientRetry(client.IdsecClient)
+
 	// Set required API version header
 	client.SetHeader("X-API-Version", "2.0")
 

internal/sdkclient/retry.go

@@ -0,0 +1,32 @@
+// Package sdkclient holds shared policy applied to idsec-sdk-golang HTTP
+// clients before grant's services use them.
+package sdkclient
+
+import "github.com/cyberark/idsec-sdk-golang/pkg/common"
+
+// DisableTransientRetry turns off the SDK's automatic transient-failure retry.
+//
+// idsec-sdk-golang v0.8.1 retries by default — 3 retries on top of the initial
+// attempt, so up to 4 requests; 500ms base, 10s cap (idsec_client.go:53-60,
+// :375-377) — on two paths, neither of which is safe for grant's
+// non-idempotent POSTs:
+//
+//   - HTTP 429 (idsec_client.go:865-885) — no HTTP method filter at all.
+//   - Transport errors (idsec_client.go:835-849 via isRetryableTransportError,
+//     :1244-1283) — a bare io.EOF / io.ErrUnexpectedEOF, or an error containing
+//     "eof" or "server closed idle connection", is retried for ANY method
+//     including POST (:1252-1266). Only connection-reset / broken-pipe / GOAWAY
+//     are gated behind isIdempotentMethod (:1269-1281). An EOF can surface
+//     after the server already processed the request, so that path can replay a
+//     mutation and produce a duplicate.
+//
+// The SDK exposes no per-request opt-out, and toggling client-wide state around
+// individual calls is race-prone (grant fans out concurrently across CSPs), so
+// grant disables transient retry on the whole client for both the SCA and
+// UAR/workflows services. See idsec_client.go:1187-1197 for SetTransientRetry.
+func DisableTransientRetry(c *common.IdsecClient) {
+	if c == nil {
+		return
+	}
+	c.SetTransientRetry(0, 0, 0)
+}

internal/sdkclient/retry_test.go

@@ -0,0 +1,72 @@
+package sdkclient
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+
+	"github.com/cyberark/idsec-sdk-golang/pkg/common"
+)
+
+// TestDisableTransientRetry verifies that DisableTransientRetry stops the SDK
+// from replaying a POST. The SDK's 429 branch (idsec_client.go:865-885) has no
+// HTTP method filter, so without the opt-out a non-idempotent POST is sent
+// defaultTransientRetryCount+1 times (idsec_client.go:53-60).
+func TestDisableTransientRetry(t *testing.T) {
+	tests := []struct {
+		name         string
+		disable      bool
+		retryAfter   string
+		wantRequests int32
+	}{
+		{
+			name:         "default client replays a rate-limited POST",
+			disable:      false,
+			retryAfter:   "0", // keeps the test fast; delay parsing is SDK-tested
+			wantRequests: 4,   // initial attempt + 3 transient retries
+		},
+		{
+			name:         "disabled client issues exactly one POST",
+			disable:      true,
+			retryAfter:   "1",
+			wantRequests: 1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var calls int32
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				atomic.AddInt32(&calls, 1)
+				w.Header().Set("Retry-After", tt.retryAfter)
+				w.WriteHeader(http.StatusTooManyRequests)
+			}))
+			defer srv.Close()
+
+			client := common.NewSimpleIdsecClient(srv.URL)
+			if tt.disable {
+				DisableTransientRetry(client)
+			}
+
+			resp, err := client.Post(t.Context(), "/api/thing", map[string]string{"a": "b"})
+			if err != nil {
+				t.Fatalf("Post returned error: %v", err)
+			}
+			defer func() { _ = resp.Body.Close() }()
+
+			if resp.StatusCode != http.StatusTooManyRequests {
+				t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusTooManyRequests)
+			}
+			if got := atomic.LoadInt32(&calls); got != tt.wantRequests {
+				t.Errorf("inbound requests = %d, want %d", got, tt.wantRequests)
+			}
+		})
+	}
+}
+
+// TestDisableTransientRetryNilClient guards the call sites, which pass an
+// embedded *common.IdsecClient that could in principle be nil.
+func TestDisableTransientRetryNilClient(t *testing.T) {
+	DisableTransientRetry(nil) // must not panic
+}

internal/workflows/retry_policy_test.go

@@ -0,0 +1,92 @@
+package workflows
+
+import (
+	"encoding/base64"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+
+	"github.com/cyberark/idsec-sdk-golang/pkg/auth"
+	"github.com/cyberark/idsec-sdk-golang/pkg/common/isp"
+	authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth"
+)
+
+// fakeJWT builds an unsigned JWT carrying just the claims the SDK's
+// resolveServiceURL reads (isp/idsec_isp_service_client.go:148-168). It is
+// parsed with ParseUnverified, so no signature is required.
+func fakeJWT(t *testing.T) string {
+	t.Helper()
+	enc := func(v interface{}) string {
+		b, err := json.Marshal(v)
+		if err != nil {
+			t.Fatalf("marshal jwt segment: %v", err)
+		}
+		return base64.RawURLEncoding.EncodeToString(b)
+	}
+	header := enc(map[string]string{"alg": "none", "typ": "JWT"})
+	payload := enc(map[string]string{
+		"subdomain":       "testtenant",
+		"platform_domain": "example.test",
+	})
+	return header + "." + payload + ".sig"
+}
+
+// ispAuthWithToken returns an ISP authenticator pre-loaded with a fake token so
+// isp.FromISPAuth succeeds without real credentials.
+func ispAuthWithToken(t *testing.T) auth.IdsecAuth {
+	t.Helper()
+	a := auth.NewIdsecISPAuth(false)
+	concrete, ok := a.(*auth.IdsecISPAuth)
+	if !ok {
+		t.Fatalf("NewIdsecISPAuth returned %T, want *auth.IdsecISPAuth", a)
+	}
+	concrete.Token = &authmodels.IdsecToken{
+		Token:     fakeJWT(t),
+		TokenType: authmodels.JWT,
+	}
+	return a
+}
+
+// TestNewAccessRequestServiceDisablesTransientRetry exercises the real
+// constructor and asserts the policy landed on the client it publishes.
+// Deleting the DisableTransientRetry call in NewAccessRequestService makes this
+// test fail with 4 inbound requests — i.e. a duplicate access request in an
+// approver's queue.
+func TestNewAccessRequestServiceDisablesTransientRetry(t *testing.T) {
+	var calls int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		atomic.AddInt32(&calls, 1)
+		w.Header().Set("Retry-After", "0") // keeps the test fast if retry is on
+		w.WriteHeader(http.StatusTooManyRequests)
+	}))
+	defer srv.Close()
+
+	svc, err := NewAccessRequestService(ispAuthWithToken(t))
+	if err != nil {
+		t.Fatalf("NewAccessRequestService: %v", err)
+	}
+
+	// Redirect the constructed client at the test server. The client is fully
+	// built by the constructor; only its base URL is swapped.
+	lc, ok := svc.httpClient.(*loggingClient)
+	if !ok {
+		t.Fatalf("httpClient is %T, want *loggingClient", svc.httpClient)
+	}
+	client, ok := lc.inner.(*isp.IdsecISPServiceClient)
+	if !ok {
+		t.Fatalf("inner client is %T, want *isp.IdsecISPServiceClient", lc.inner)
+	}
+	client.BaseURL = srv.URL
+
+	resp, err := svc.httpClient.Post(t.Context(), "/api/workflows/requests", map[string]string{"a": "b"})
+	if err != nil {
+		t.Fatalf("Post: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	if got := atomic.LoadInt32(&calls); got != 1 {
+		t.Errorf("inbound requests = %d, want 1 (SDK transient retry must be disabled)", got)
+	}
+}

internal/workflows/service.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"strconv"
 
+	"github.com/aaearon/grant-cli/internal/sdkclient"
 	"github.com/aaearon/grant-cli/internal/workflows/models"
 	"github.com/cyberark/idsec-sdk-golang/pkg/auth"
 	"github.com/cyberark/idsec-sdk-golang/pkg/common"
@@ -57,6 +58,12 @@ func NewAccessRequestService(authenticators ...auth.IdsecAuth) (*AccessRequestSe
 		return nil, fmt.Errorf("failed to create ISP client: %w", err)
 	}
 
+	// Disable the SDK's automatic transient retry. SDK v0.8.1 retries 429s with
+	// no method filter (idsec_client.go:865-885) and bare EOF transport errors
+	// for any method including POST (:1252-1266); a replayed submit would put a
+	// duplicate access request in front of approvers.
+	sdkclient.DisableTransientRetry(client.IdsecClient)
+
 	svc.httpClient = newLoggingClient(client, common.GetLogger("grant", -1))
 
 	return svc, nil