fix: handle revocation 401 gracefully, add retry on 400/404, improve policy error messages, fix docs (#4)

* fix: handle revocation 401 gracefully, add retry on 400/404, improve policy error messages, fix docs

* fix: handle revocation 401 gracefully, add retry on 400/404, improve policy error messages, fix docs

Author: hibare

deployments/terraform/scripts/aws/push-github-apps-to-aws-secrets-manager.sh

@@ -120,15 +120,14 @@ TERRAFORM INTEGRATION:
     (See Terraform module README for details)
 
 GITHUB APPS JSON FORMAT:
-    {
-      "apps": [
+    [
         {
           "client_id": "Iv1...",
-          "private_key_path": "/path/to/key.pem",
+          "private_key": "<PEM-ENCODED-PRIVATE-KEY>",
           "organization": "my-org"
         }
-      ]
-    }
+    ]
+
 
 EOF
 }

internal/clients/github/client.go

@@ -126,8 +126,12 @@ func (c *Client) RevokeToken(ctx context.Context, token string) error {
 	if err != nil {
 		return err
 	}
-	_, err = gh.Apps.RevokeInstallationToken(ctx)
+	resp, err := gh.Apps.RevokeInstallationToken(ctx)
 	if err != nil {
+		// 401 means the token is already invalid (expired or revoked),
+		if resp != nil && resp.StatusCode == http.StatusUnauthorized {
+			return nil
+		}
 		return fmt.Errorf("revoking installation token: %w", err)
 	}
 	return nil

internal/clients/github/client_test.go

@@ -441,7 +441,7 @@ func TestRevokeToken_Success(t *testing.T) {
 	assert.Equal(t, "/installation/token", gotPath)
 }
 
-func TestRevokeToken_Error(t *testing.T) {
+func TestRevokeToken_UnauthorizedTreatedAsSuccess(t *testing.T) {
 	t.Parallel()
 	_, keyPEM := testutil.GenerateRSAKey(t)
 
@@ -453,7 +453,23 @@ func TestRevokeToken_Error(t *testing.T) {
 	c, err := New(Options{ClientID: "client-1", PrivateKey: keyPEM, BaseURL: server.URL})
 	require.NoError(t, err)
 
-	err = c.RevokeToken(t.Context(), "ghs_invalid_token")
+	err = c.RevokeToken(t.Context(), "ghs_expired_token")
+	require.NoError(t, err, "401 means token is already invalid, which achieves revocation")
+}
+
+func TestRevokeToken_ServerError(t *testing.T) {
+	t.Parallel()
+	_, keyPEM := testutil.GenerateRSAKey(t)
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, `{"message":"Internal Server Error"}`, http.StatusInternalServerError)
+	}))
+	t.Cleanup(server.Close)
+
+	c, err := New(Options{ClientID: "client-1", PrivateKey: keyPEM, BaseURL: server.URL})
+	require.NoError(t, err)
+
+	err = c.RevokeToken(t.Context(), "ghs_test_token")
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "revoking installation token")
 }

internal/clients/github/retry.go

@@ -102,16 +102,18 @@ func (t *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	return lastResp, nil
 }
 
-// shouldRetry returns true for 5xx, 429, 408, or nil response (retry on transient failure).
+var retryableStatusCodes = map[int]bool{
+	http.StatusBadRequest:      true,
+	http.StatusNotFound:        true,
+	http.StatusRequestTimeout:  true,
+	http.StatusTooManyRequests: true,
+}
+
 func shouldRetry(resp *http.Response) bool {
 	if resp == nil {
 		return true
 	}
-	if resp.StatusCode >= http.StatusInternalServerError {
-		return true
-	}
-	return resp.StatusCode == http.StatusTooManyRequests ||
-		resp.StatusCode == http.StatusRequestTimeout
+	return resp.StatusCode >= http.StatusInternalServerError || retryableStatusCodes[resp.StatusCode]
 }
 
 // isTransient returns true for timeouts, DNS/connection errors, and connection reset/refused/EOF.

internal/clients/github/retry_test.go

@@ -58,7 +58,9 @@ func TestRetryTransport_StatusBehavior(t *testing.T) {
 		{"success_first_attempt", http.StatusOK, 0, http.StatusOK, 1},
 		{"retries_on_5xx", http.StatusBadGateway, 3, http.StatusOK, 3},
 		{"retries_on_429", http.StatusTooManyRequests, 2, http.StatusOK, 2},
-		{"no_retry_on_4xx", http.StatusNotFound, 999, http.StatusNotFound, 1},
+		{"retries_on_400", http.StatusBadRequest, 2, http.StatusOK, 2},
+		{"retries_on_404", http.StatusNotFound, 2, http.StatusOK, 2},
+		{"no_retry_on_other_4xx", http.StatusForbidden, 999, http.StatusForbidden, 1},
 		{"exhausts_max_attempts", http.StatusServiceUnavailable, 999, http.StatusServiceUnavailable, 3},
 	}
 	for _, tt := range tests {
@@ -155,10 +157,10 @@ func TestShouldRetry(t *testing.T) {
 	}{
 		{"200 OK", http.StatusOK, false},
 		{"201 Created", http.StatusCreated, false},
-		{"400 Bad Request", http.StatusBadRequest, false},
+		{"400 Bad Request", http.StatusBadRequest, true},
 		{"401 Unauthorized", http.StatusUnauthorized, false},
 		{"403 Forbidden", http.StatusForbidden, false},
-		{"404 Not Found", http.StatusNotFound, false},
+		{"404 Not Found", http.StatusNotFound, true},
 		{"408 Request Timeout", http.StatusRequestTimeout, true},
 		{"429 Too Many Requests", http.StatusTooManyRequests, true},
 		{"500 Internal Server Error", http.StatusInternalServerError, true},

internal/sts/authorizer/policy_fetch.go

@@ -90,7 +90,7 @@ func (a *Authorizer) fetchPolicyFromGitHub(ctx context.Context, repository, reso
 		}
 	}
 
-	return nil, fmt.Errorf("%w at %s", ErrPolicyFileNotFound, paths[0])
+	return nil, fmt.Errorf("%w at %s", ErrPolicyFileNotFound, strings.Join(paths, ", "))
 }
 
 // extensionVariants returns the paths to try when fetching a trust policy.