From 3f5e4d22b0e00acf1c277f4a7e8769cbb7c3193e Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 6 Oct 2026 22:49:37 -0400 Subject: [PATCH 1/5] feat(secrets): add private GitHub delivery helpers (Task 1) --- docs/wfctl-secrets-scopes.md | 44 ++ secrets/github_delivery_test.go | 701 ++++++++++++++++++++++++++++++++ secrets/github_provider.go | 433 ++++++++++++++++++-- 3 files changed, 1150 insertions(+), 28 deletions(-) create mode 100644 secrets/github_delivery_test.go diff --git a/docs/wfctl-secrets-scopes.md b/docs/wfctl-secrets-scopes.md index c85b3940..8822b2c4 100644 --- a/docs/wfctl-secrets-scopes.md +++ b/docs/wfctl-secrets-scopes.md @@ -167,6 +167,50 @@ printf 'alice\nhunter2\nJBSWY3DPEHPK3PXP\n' | \ --scope org --org GoCodeAlone ``` +## Private delivery from an owner-run plugin + +Plugins that receive a one-time credential can use the canonical Go provider +without placing the GitHub credential in an environment variable: + +- `NewGitHubSecretsProviderWithToken(repo, token)` accepts an in-memory GitHub + credential; `SetEnvironment` selects an existing environment. +- `NewGitHubOrgSecretsProviderWithToken(org, token, visibility, selectedRepoIDs)` + copies and validates positive, unique repository IDs. IDs are accepted only + with `selected` visibility; empty visibility defaults to `private`. +- `Identity(ctx)` resolves immutable repository and owner IDs, an organization + ID, or repository, owner, and environment IDs. Organization visibility and + selected repository IDs describe the intended write policy. +- `Stat(ctx, key)` fetches one exact secret's metadata. A `404` returns + `secrets.ErrNotFound`; permission failures remain errors. `List` and `StatAll` + follow all pages, rejecting links outside the configured origin and namespace + or repeated pages. +- `SetWithReceipt(ctx, key, value)` reuses GitHub's sealed-box encryption and + returns `GitHubSecretWriteReceipt{Created, MayHaveWritten}`. It does not return + a value, ciphertext, or upstream response body. + +The new metadata and receipt methods bound each request to 30 seconds and refuse +redirects, including on providers built with the legacy constructors. They omit +raw response bodies and transport error details. Direct-token providers also +use safe errors for `List` and `StatAll`. The new exact-key methods accept ASCII +letters, digits, and underscores, require a letter or underscore first, reject +the reserved `GITHUB_` prefix case-insensitively, and apply their own conservative +256-byte name bound. Existing constructors and `Set` remain compatible. + +`201` means GitHub reported a created secret: both receipt fields are true. +`204` means GitHub accepted an update: `Created` is false and `MayHaveWritten` +is true. A transport failure after starting the PUT, a timeout response, or a +server failure returns an error with `MayHaveWritten` true. Failures before the +PUT and explicit client rejection responses have `MayHaveWritten` false. + +GitHub's PUT is an upsert with no atomic create-only or compare-and-swap option. +A caller that requires a fresh secret must approve a versioned name, check its +absence, and control concurrent writers in that namespace. An absence check +followed by PUT cannot close a race with another writer. An unexpected `204` +must be reconciled as a possible collision; the receipt cannot recover a prior +value. Metadata cannot prove which value GitHub currently holds. These helpers +do not retry an ambiguous upload, revoke tokens, or delete secrets; the +owner-run action must retain uncertainty and apply its explicit recovery policy. + ## PAT scope cheat sheet | Token use | Required scopes | diff --git a/secrets/github_delivery_test.go b/secrets/github_delivery_test.go new file mode 100644 index 00000000..c79939f5 --- /dev/null +++ b/secrets/github_delivery_test.go @@ -0,0 +1,701 @@ +package secrets + +import ( + "context" + "crypto/rand" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + "time" + + "golang.org/x/crypto/blake2b" + "golang.org/x/crypto/nacl/box" +) + +const deliveryCredential = "synthetic-github-credential" + +func newDeliveryProvider(t *testing.T, scope GitHubSecretScope) *GitHubSecretsProvider { + t.Helper() + var p *GitHubSecretsProvider + var err error + if scope == GitHubScopeOrg { + p, err = NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, OrgVisibilitySelected, []int64{11, 12}) + } else { + p, err = NewGitHubSecretsProviderWithToken("owner/repo", deliveryCredential) + if err == nil && scope == GitHubScopeEnv { + p.SetEnvironment("preview/blue") + } + } + if err != nil { + t.Fatal(err) + } + return p +} + +func deliveryNamespacePath(scope GitHubSecretScope) string { + switch scope { + case GitHubScopeEnv: + return "/repos/owner/repo/environments/preview%2Fblue/secrets" + case GitHubScopeOrg: + return "/orgs/owner/actions/secrets" + default: + return "/repos/owner/repo/actions/secrets" + } +} + +func newPaginationProvider(t *testing.T, scope GitHubSecretScope, direct bool) *GitHubSecretsProvider { + t.Helper() + if direct { + return newDeliveryProvider(t, scope) + } + t.Setenv("GH_DELIVERY_TEST_TOKEN", deliveryCredential) + var p *GitHubSecretsProvider + var err error + if scope == GitHubScopeOrg { + p, err = NewGitHubOrgSecretsProvider("owner", "GH_DELIVERY_TEST_TOKEN", OrgVisibilitySelected, []int64{11, 12}) + } else { + p, err = NewGitHubSecretsProvider("owner/repo", "GH_DELIVERY_TEST_TOKEN") + if err == nil && scope == GitHubScopeEnv { + p.SetEnvironment("preview/blue") + } + } + if err != nil { + t.Fatal(err) + } + return p +} + +func TestGitHubDeliveryDirectConstructors(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "ambient-credential-must-not-be-used") + p := newDeliveryProvider(t, GitHubScopeRepo) + if p.token != deliveryCredential { + t.Fatal("direct constructor did not retain the supplied credential") + } + if p.client.Timeout <= 0 || p.client.Timeout > 30*time.Second || p.client.CheckRedirect == nil { + t.Fatal("direct constructor must bound requests and disable redirects") + } + if err := p.client.CheckRedirect(&http.Request{}, nil); !errors.Is(err, http.ErrUseLastResponse) { + t.Fatalf("redirect policy = %v, want ErrUseLastResponse", err) + } + ids := []int64{11, 12} + op, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, OrgVisibilitySelected, ids) + if err != nil { + t.Fatal(err) + } + ids[0] = 99 + if op.selectedRepoIDs[0] != 11 { + t.Fatal("constructor retained the caller's mutable repository ID slice") + } + private, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, "", nil) + if err != nil || private.orgVisibility != OrgVisibilityPrivate { + t.Fatalf("default visibility = %v, error = %v", private, err) + } +} + +func TestGitHubDeliveryConstructorsRejectInvalidInput(t *testing.T) { + cases := []struct { + name string + make func() error + }{ + {"repo missing owner", func() error { _, err := NewGitHubSecretsProviderWithToken("/repo", deliveryCredential); return err }}, + {"repo extra segment", func() error { + _, err := NewGitHubSecretsProviderWithToken("owner/repo/extra", deliveryCredential) + return err + }}, + {"repo URL injection", func() error { + _, err := NewGitHubSecretsProviderWithToken("owner/repo?x=1", deliveryCredential) + return err + }}, + {"empty token", func() error { _, err := NewGitHubSecretsProviderWithToken("owner/repo", ""); return err }}, + {"header token", func() error { + _, err := NewGitHubSecretsProviderWithToken("owner/repo", deliveryCredential+"\n") + return err + }}, + {"org slash", func() error { + _, err := NewGitHubOrgSecretsProviderWithToken("owner/other", deliveryCredential, "", nil) + return err + }}, + {"visibility", func() error { + _, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, "unknown", nil) + return err + }}, + {"missing selected IDs", func() error { + _, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, OrgVisibilitySelected, nil) + return err + }}, + {"negative ID", func() error { + _, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, OrgVisibilitySelected, []int64{-1}) + return err + }}, + {"zero ID", func() error { + _, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, OrgVisibilitySelected, []int64{0}) + return err + }}, + {"duplicate ID", func() error { + _, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, OrgVisibilitySelected, []int64{11, 11}) + return err + }}, + {"IDs with private", func() error { + _, err := NewGitHubOrgSecretsProviderWithToken("owner", deliveryCredential, OrgVisibilityPrivate, []int64{11}) + return err + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := tc.make() + if err == nil || strings.Contains(err.Error(), deliveryCredential) { + t.Fatalf("expected a credential-free validation error, got %v", err) + } + }) + } +} + +func TestGitHubDeliveryExactStatAcrossScopes(t *testing.T) { + for _, scope := range []GitHubSecretScope{GitHubScopeRepo, GitHubScopeEnv, GitHubScopeOrg} { + t.Run(string(scope), func(t *testing.T) { + p := newDeliveryProvider(t, scope) + path := deliveryNamespacePath(scope) + updated := time.Date(2026, 10, 7, 1, 2, 3, 0, time.UTC) + var calls []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls = append(calls, r.Method+" "+r.URL.EscapedPath()) + if r.Header.Get("Authorization") != "Bearer "+deliveryCredential { + t.Error("request did not use the direct credential") + } + if r.Method == http.MethodGet && r.URL.EscapedPath() == path+"/MISSING" { + http.NotFound(w, r) + return + } + if r.Method != http.MethodGet || r.URL.EscapedPath() != path+"/NEW_TOKEN" { + t.Errorf("unexpected metadata request: %s %s", r.Method, r.URL.EscapedPath()) + http.NotFound(w, r) + return + } + json.NewEncoder(w).Encode(ghSecretEntry{Name: "NEW_TOKEN", UpdatedAt: updated}) + })) + defer srv.Close() + p.client.Transport = rewriteTransport{base: srv.URL} + meta, err := p.Stat(context.Background(), "NEW_TOKEN") + if err != nil || meta.Name != "NEW_TOKEN" || !meta.Exists || !meta.UpdatedAt.Equal(updated) { + t.Fatalf("Stat = %+v, %v", meta, err) + } + missing, err := p.Stat(context.Background(), "MISSING") + if !errors.Is(err, ErrNotFound) || missing.Exists || missing.Name != "MISSING" { + t.Fatalf("missing Stat = %+v, %v", missing, err) + } + want := []string{"GET " + path + "/NEW_TOKEN", "GET " + path + "/MISSING"} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("requests = %v, want exact metadata requests %v", calls, want) + } + }) + } +} + +func TestGitHubDeliveryIdentityAcrossScopes(t *testing.T) { + for _, scope := range []GitHubSecretScope{GitHubScopeRepo, GitHubScopeEnv, GitHubScopeOrg} { + t.Run(string(scope), func(t *testing.T) { + p := newDeliveryProvider(t, scope) + var calls []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls = append(calls, r.Method+" "+r.URL.EscapedPath()) + if r.Method != http.MethodGet { + t.Errorf("unexpected identity method: %s", r.Method) + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + return + } + switch r.URL.Path { + case "/repos/owner/repo": + io.WriteString(w, `{"id":100,"owner":{"id":200}}`) + case "/orgs/owner": + io.WriteString(w, `{"id":200}`) + case "/repos/owner/repo/environments/preview/blue": + io.WriteString(w, `{"id":300}`) + default: + t.Errorf("unexpected identity request: %s %s", r.Method, r.URL.EscapedPath()) + http.NotFound(w, r) + } + })) + defer srv.Close() + p.client.Transport = rewriteTransport{base: srv.URL} + identity, err := p.Identity(context.Background()) + if err != nil || identity.Scope != scope { + t.Fatalf("Identity = %+v, %v", identity, err) + } + if scope == GitHubScopeOrg { + if identity.OrganizationID != 200 || identity.Visibility != OrgVisibilitySelected || !reflect.DeepEqual(identity.SelectedRepositoryIDs, []int64{11, 12}) { + t.Fatalf("organization identity = %+v", identity) + } + identity.SelectedRepositoryIDs[0] = 99 + if p.selectedRepoIDs[0] != 11 { + t.Fatal("Identity exposed the mutable provider ID slice") + } + if !reflect.DeepEqual(calls, []string{"GET /orgs/owner"}) { + t.Fatalf("org requests = %v", calls) + } + } else { + if identity.RepositoryID != 100 || identity.OwnerID != 200 { + t.Fatalf("repository identity = %+v", identity) + } + want := []string{"GET /repos/owner/repo"} + if scope == GitHubScopeEnv { + if identity.EnvironmentID != 300 { + t.Fatalf("environment identity = %+v", identity) + } + want = append(want, "GET /repos/owner/repo/environments/preview%2Fblue") + } + if !reflect.DeepEqual(calls, want) { + t.Fatalf("identity requests = %v, want %v", calls, want) + } + } + }) + } +} + +type deliveryRoundTripper func(*http.Request) (*http.Response, error) + +func (f deliveryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +func TestGitHubDeliverySafeErrors(t *testing.T) { + for _, method := range []string{"stat", "identity", "write", "list", "stat-all"} { + for _, failure := range []string{"forbidden", "transport", "malformed", "redirect"} { + t.Run(method+"/"+failure, func(t *testing.T) { + p := newDeliveryProvider(t, GitHubScopeRepo) + calls := 0 + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + calls++ + if failure == "transport" { + return nil, fmt.Errorf("transport echoed %s", deliveryCredential) + } + status := http.StatusForbidden + body := deliveryCredential + headers := http.Header{} + if failure == "malformed" { + status = http.StatusOK + body = `{"id": "` + deliveryCredential + `"}` + } + if failure == "redirect" { + status = http.StatusTemporaryRedirect + headers.Set("Location", "https://unapproved.example/"+deliveryCredential) + } + return &http.Response{StatusCode: status, Header: headers, Body: io.NopCloser(strings.NewReader(body)), Request: r}, nil + }) + var err error + switch method { + case "stat": + _, err = p.Stat(context.Background(), "NEW_TOKEN") + case "identity": + _, err = p.Identity(context.Background()) + case "write": + _, err = p.SetWithReceipt(context.Background(), "NEW_TOKEN", "synthetic-secret-value") + case "list": + _, err = p.List(context.Background()) + case "stat-all": + _, err = p.StatAll(context.Background()) + } + if err == nil || strings.Contains(err.Error(), deliveryCredential) { + t.Fatalf("expected a safe error, got %v", err) + } + if errors.Is(err, ErrNotFound) { + t.Fatal("an inaccessible or malformed response was classified as absent") + } + if calls != 1 { + t.Fatalf("made %d requests after failure, want 1", calls) + } + }) + } + } +} + +func TestGitHubDeliverySafeMethodsIgnoreLegacyRedirectPolicy(t *testing.T) { + for _, method := range []string{"stat", "identity", "write"} { + t.Run(method, func(t *testing.T) { + t.Setenv("GH_DELIVERY_TEST_TOKEN", deliveryCredential) + p, err := NewGitHubSecretsProvider("owner/repo", "GH_DELIVERY_TEST_TOKEN") + if err != nil { + t.Fatal(err) + } + calls := 0 + p.client.CheckRedirect = func(*http.Request, []*http.Request) error { return nil } + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + calls++ + return &http.Response{StatusCode: 307, Header: http.Header{"Location": []string{"https://unapproved.example/" + deliveryCredential}}, Body: io.NopCloser(strings.NewReader(deliveryCredential)), Request: r}, nil + }) + switch method { + case "stat": + _, err = p.Stat(context.Background(), "NEW_TOKEN") + case "identity": + _, err = p.Identity(context.Background()) + case "write": + _, err = p.SetWithReceipt(context.Background(), "NEW_TOKEN", "synthetic-secret-value") + } + if err == nil || strings.Contains(err.Error(), deliveryCredential) || calls != 1 { + t.Fatalf("legacy client safe method: error = %v, calls = %d", err, calls) + } + }) + } +} + +func TestGitHubDeliveryRejectsIncompleteMetadata(t *testing.T) { + for _, tc := range []struct { + name, body string + identity bool + }{ + {"wrong secret name", `{"name":"OTHER_TOKEN"}`, false}, + {"missing repository ID", `{"owner":{"id":200}}`, true}, + {"missing owner ID", `{"id":100}`, true}, + {"trailing response", `{"id":100,"owner":{"id":200}} {}`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + p := newDeliveryProvider(t, GitHubScopeRepo) + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: 200, Header: http.Header{}, Body: io.NopCloser(strings.NewReader(tc.body)), Request: r}, nil + }) + var err error + if tc.identity { + _, err = p.Identity(context.Background()) + } else { + _, err = p.Stat(context.Background(), "NEW_TOKEN") + } + if err == nil { + t.Fatal("incomplete/mismatched metadata was accepted") + } + }) + } +} + +func TestGitHubDeliveryCanceledBeforePUTIsDefinite(t *testing.T) { + pub, _, err := box.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + p := newDeliveryProvider(t, GitHubScopeRepo) + puts := 0 + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + if r.Method == http.MethodPut { + puts++ + } + body, _ := json.Marshal(repoPublicKeyResponse{KeyID: "delivery-key", Key: base64.StdEncoding.EncodeToString(pub[:])}) + cancel() + return &http.Response{StatusCode: 200, Header: http.Header{}, Body: io.NopCloser(strings.NewReader(string(body))), Request: r}, nil + }) + receipt, err := p.SetWithReceipt(ctx, "NEW_TOKEN", "synthetic-secret-value") + if !errors.Is(err, context.Canceled) || receipt.MayHaveWritten || receipt.Created || puts != 0 { + t.Fatalf("pre-PUT cancellation: receipt = %+v, error = %v, PUTs = %d", receipt, err, puts) + } +} + +func TestGitHubDeliveryInvalidKeysNeverRequest(t *testing.T) { + for _, key := range []string{"", "123_TOKEN", "GITHUB_TOKEN", "github_test", "TOKEN/OTHER", strings.Repeat("A", 257)} { + t.Run(key, func(t *testing.T) { + p := newDeliveryProvider(t, GitHubScopeRepo) + calls := 0 + p.client.Transport = deliveryRoundTripper(func(*http.Request) (*http.Response, error) { + calls++ + return nil, errors.New("request should not be made") + }) + if _, err := p.Stat(context.Background(), key); !errors.Is(err, ErrInvalidKey) { + t.Fatalf("Stat key error = %v", err) + } + receipt, err := p.SetWithReceipt(context.Background(), key, "synthetic-secret-value") + if !errors.Is(err, ErrInvalidKey) || receipt.Created || receipt.MayHaveWritten || calls != 0 { + t.Fatalf("invalid key: receipt = %+v, error = %v, requests = %d", receipt, err, calls) + } + }) + } +} + +func TestGitHubDeliveryWriteReceiptAndEncryption(t *testing.T) { + for _, scope := range []GitHubSecretScope{GitHubScopeRepo, GitHubScopeEnv, GitHubScopeOrg} { + for _, status := range []int{http.StatusCreated, http.StatusNoContent} { + t.Run(fmt.Sprintf("%s/%d", scope, status), func(t *testing.T) { + pub, priv, err := box.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + p := newDeliveryProvider(t, scope) + path := deliveryNamespacePath(scope) + var payload struct { + EncryptedValue string `json:"encrypted_value"` + KeyID string `json:"key_id"` + Visibility string `json:"visibility"` + SelectedRepositoryIDs []int64 `json:"selected_repository_ids"` + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + if r.URL.EscapedPath() != path+"/public-key" { + t.Errorf("unexpected public key path = %s", r.URL.EscapedPath()) + http.NotFound(w, r) + return + } + json.NewEncoder(w).Encode(repoPublicKeyResponse{KeyID: "delivery-key", Key: base64.StdEncoding.EncodeToString(pub[:])}) + case http.MethodPut: + if r.URL.EscapedPath() != path+"/NEW_TOKEN" { + t.Errorf("unexpected secret path = %s", r.URL.EscapedPath()) + http.NotFound(w, r) + return + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Error(err) + } + w.WriteHeader(status) + default: + t.Errorf("unexpected delivery method: %s", r.Method) + http.NotFound(w, r) + } + })) + defer srv.Close() + p.client.Transport = rewriteTransport{base: srv.URL} + receipt, err := p.SetWithReceipt(context.Background(), "NEW_TOKEN", "synthetic-secret-value") + if err != nil || receipt.Created != (status == http.StatusCreated) || !receipt.MayHaveWritten { + t.Fatalf("receipt = %+v, error = %v", receipt, err) + } + if payload.KeyID != "delivery-key" || payload.EncryptedValue == "" { + t.Fatal("missing encrypted payload/key ID") + } + sealed, err := base64.StdEncoding.DecodeString(payload.EncryptedValue) + if err != nil || len(sealed) < 32+box.Overhead { + t.Fatalf("invalid sealed box: %v", err) + } + var ephemeral [32]byte + copy(ephemeral[:], sealed[:32]) + h, err := blake2b.New(24, nil) + if err != nil { + t.Fatal(err) + } + h.Write(ephemeral[:]) + h.Write(pub[:]) + var nonce [24]byte + copy(nonce[:], h.Sum(nil)) + plaintext, ok := box.Open(nil, sealed[32:], &nonce, &ephemeral, priv) + if !ok || string(plaintext) != "synthetic-secret-value" { + t.Fatal("GitHub-compatible sealed-box decryption did not recover the supplied value") + } + if scope == GitHubScopeOrg { + if payload.Visibility != "selected" || !reflect.DeepEqual(payload.SelectedRepositoryIDs, []int64{11, 12}) { + t.Fatal("organization visibility/repository selection not preserved") + } + } else if payload.Visibility != "" || len(payload.SelectedRepositoryIDs) != 0 { + t.Fatal("repository/environment upload included organization controls") + } + }) + } + } +} + +func TestGitHubDeliveryWriteFailureReceipts(t *testing.T) { + pub, _, err := box.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + name string + publicKeyStatus int + publicKey string + putStatus int + putError bool + mayHaveWritten bool + wantPUT bool + }{ + {"public key forbidden", 403, "", 0, false, false, false}, + {"invalid public key", 200, "not-a-key", 0, false, false, false}, + {"rejected PUT", 200, base64.StdEncoding.EncodeToString(pub[:]), 403, false, false, true}, + {"server PUT failure", 200, base64.StdEncoding.EncodeToString(pub[:]), 500, false, true, true}, + {"gateway PUT timeout", 200, base64.StdEncoding.EncodeToString(pub[:]), 504, false, true, true}, + {"request PUT timeout", 200, base64.StdEncoding.EncodeToString(pub[:]), 408, false, true, true}, + {"transport PUT failure", 200, base64.StdEncoding.EncodeToString(pub[:]), 0, true, true, true}, + } { + t.Run(tc.name, func(t *testing.T) { + p := newDeliveryProvider(t, GitHubScopeRepo) + puts := 0 + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + status, body := tc.publicKeyStatus, deliveryCredential + if r.Method == http.MethodGet && status == 200 { + b, _ := json.Marshal(repoPublicKeyResponse{KeyID: "delivery-key", Key: tc.publicKey}) + body = string(b) + } else if r.Method == http.MethodPut { + puts++ + if tc.putError { + return nil, fmt.Errorf("transport echoed %s", deliveryCredential) + } + status = tc.putStatus + } + return &http.Response{StatusCode: status, Header: http.Header{}, Body: io.NopCloser(strings.NewReader(body)), Request: r}, nil + }) + receipt, err := p.SetWithReceipt(context.Background(), "NEW_TOKEN", "synthetic-secret-value") + if err == nil || strings.Contains(err.Error(), deliveryCredential) || receipt.Created || receipt.MayHaveWritten != tc.mayHaveWritten { + t.Fatalf("receipt = %+v, error = %v", receipt, err) + } + if (puts == 1) != tc.wantPUT || puts > 1 { + t.Fatalf("PUT count = %d, want PUT = %v", puts, tc.wantPUT) + } + }) + } +} + +func TestGitHubDeliveryPUTErrorsHideAllCredentialMaterial(t *testing.T) { + pub, _, err := box.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + const issuedValue = "synthetic-issued-value-distinct-from-github-auth" + for _, tc := range []struct { + name string + status int + transport bool + }{ + {"client rejection", 403, false}, + {"server failure", 500, false}, + {"redirect", 307, false}, + {"transport failure", 0, true}, + } { + t.Run(tc.name, func(t *testing.T) { + p := newDeliveryProvider(t, GitHubScopeRepo) + var calls []string + var ciphertext string + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + calls = append(calls, r.Method+" "+r.URL.EscapedPath()) + if r.URL.Host != "api.github.com" || r.Header.Get("Authorization") != "Bearer "+deliveryCredential { + t.Errorf("unexpected host or authentication") + } + if r.Method == http.MethodGet && r.URL.EscapedPath() == "/repos/owner/repo/actions/secrets/public-key" { + body, _ := json.Marshal(repoPublicKeyResponse{KeyID: "delivery-key", Key: base64.StdEncoding.EncodeToString(pub[:])}) + return &http.Response{StatusCode: 200, Header: http.Header{}, Body: io.NopCloser(strings.NewReader(string(body))), Request: r}, nil + } + if r.Method != http.MethodPut || r.URL.EscapedPath() != "/repos/owner/repo/actions/secrets/NEW_TOKEN" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.EscapedPath()) + return nil, errors.New("unexpected request") + } + var payload struct { + EncryptedValue string `json:"encrypted_value"` + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Fatal(err) + } + ciphertext = payload.EncryptedValue + leak := deliveryCredential + " " + issuedValue + " " + ciphertext + if tc.transport { + return nil, errors.New(leak) + } + return &http.Response{StatusCode: tc.status, Header: http.Header{"Location": []string{"https://unapproved.example/" + issuedValue}}, Body: io.NopCloser(strings.NewReader(leak)), Request: r}, nil + }) + receipt, err := p.SetWithReceipt(context.Background(), "NEW_TOKEN", issuedValue) + if err == nil || ciphertext == "" { + t.Fatalf("missing PUT error or ciphertext: %v", err) + } + for _, secret := range []string{deliveryCredential, issuedValue, ciphertext} { + if strings.Contains(err.Error(), secret) { + t.Fatal("PUT error exposed credential material") + } + } + wantCalls := []string{"GET /repos/owner/repo/actions/secrets/public-key", "PUT /repos/owner/repo/actions/secrets/NEW_TOKEN"} + if !reflect.DeepEqual(calls, wantCalls) || receipt.Created || receipt.MayHaveWritten != (tc.status != 403) { + t.Fatalf("PUT failure receipt = %+v, calls = %v", receipt, calls) + } + }) + } +} + +func TestGitHubDeliverySecretPaginationAcrossScopes(t *testing.T) { + for _, scope := range []GitHubSecretScope{GitHubScopeRepo, GitHubScopeEnv, GitHubScopeOrg} { + for _, metadata := range []bool{false, true} { + for _, direct := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/metadata=%v/direct=%v", scope, metadata, direct), func(t *testing.T) { + p := newPaginationProvider(t, scope, direct) + path := deliveryNamespacePath(scope) + var calls []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls = append(calls, r.URL.RequestURI()) + if r.Method != http.MethodGet || r.URL.EscapedPath() != path || r.URL.Query().Get("per_page") != "100" { + t.Errorf("unexpected list request: %s %s", r.Method, r.URL.RequestURI()) + http.NotFound(w, r) + return + } + name := "FIRST" + switch r.URL.Query().Get("page") { + case "": + w.Header().Set("Link", "; rel="next"`) + case "2": + name = "SECOND" + default: + t.Errorf("unexpected page: %s", r.URL.RequestURI()) + http.NotFound(w, r) + return + } + json.NewEncoder(w).Encode(map[string]any{"secrets": []ghSecretEntry{{Name: name, CreatedAt: time.Date(2026, 10, 7, 1, 0, 0, 0, time.UTC)}}}) + })) + defer srv.Close() + p.client.Transport = rewriteTransport{base: srv.URL} + var names []string + if metadata { + metas, err := p.StatAll(context.Background()) + if err != nil { + t.Fatal(err) + } + for _, meta := range metas { + if !meta.Exists || meta.UpdatedAt.IsZero() { + t.Fatalf("incomplete metadata: %+v", meta) + } + names = append(names, meta.Name) + } + } else { + var err error + names, err = p.List(context.Background()) + if err != nil { + t.Fatal(err) + } + } + wantCalls := []string{path + "?per_page=100", path + "?page=2&per_page=100"} + if !reflect.DeepEqual(names, []string{"FIRST", "SECOND"}) || !reflect.DeepEqual(calls, wantCalls) { + t.Fatalf("names = %v, calls = %v; want both pages via %v", names, calls, wantCalls) + } + }) + } + } + } +} + +func TestGitHubDeliveryRejectsUnsafeSecretPagination(t *testing.T) { + for _, tc := range []struct { + name, link string + wantCalls int + }{ + {"foreign origin", "https://unapproved.example/repos/owner/repo/actions/secrets?page=2", 1}, + {"foreign scope", githubAPIBase + "/repos/owner/other/actions/secrets?page=2", 1}, + {"foreign path", githubAPIBase + "/user?page=2", 1}, + {"URL credentials", "https://credential@api.github.com/repos/owner/repo/actions/secrets?page=2", 1}, + {"cycle", githubAPIBase + "/repos/owner/repo/actions/secrets?per_page=100", 1}, + {"later cycle", githubAPIBase + "/repos/owner/repo/actions/secrets?page=2", 2}, + } { + for _, metadata := range []bool{false, true} { + for _, direct := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/metadata=%v/direct=%v", tc.name, metadata, direct), func(t *testing.T) { + p := newPaginationProvider(t, GitHubScopeRepo, direct) + calls := 0 + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + calls++ + return &http.Response{StatusCode: 200, Header: http.Header{"Link": []string{"<" + tc.link + `>; rel="next"`}}, Body: io.NopCloser(strings.NewReader(`{"secrets":[{"name":"FIRST"}]}`)), Request: r}, nil + }) + var err error + if metadata { + _, err = p.StatAll(context.Background()) + } else { + _, err = p.List(context.Background()) + } + if err == nil || calls != tc.wantCalls { + t.Fatalf("unsafe pagination error = %v, calls = %d, want %d", err, calls, tc.wantCalls) + } + }) + } + } + } +} diff --git a/secrets/github_provider.go b/secrets/github_provider.go index a2409743..de36584d 100644 --- a/secrets/github_provider.go +++ b/secrets/github_provider.go @@ -59,6 +59,7 @@ type GitHubSecretsProvider struct { token string client *http.Client baseURL string // overridden in tests to point at an httptest.Server + safeErrors bool // direct-token providers omit upstream bodies and errors } // base returns the API base URL, using baseURL when set (for tests). @@ -124,6 +125,350 @@ func NewGitHubOrgSecretsProvider(org string, tokenEnvVar string, visibility GitH }, nil } +// NewGitHubSecretsProviderWithToken creates a repository provider from an +// in-memory credential. It does not read environment variables. Requests are +// bounded and redirects are refused. Stat, Identity, SetWithReceipt, List, and +// StatAll omit upstream bodies and causes. Legacy Set retains upsert semantics. +func NewGitHubSecretsProviderWithToken(repo, token string) (*GitHubSecretsProvider, error) { + parts := strings.Split(repo, "/") + if len(parts) != 2 || !validGitHubTargetName(parts[0]) || !validGitHubTargetName(parts[1]) { + return nil, errors.New("secrets: github repo must be owner/repo") + } + if !validGitHubToken(token) { + return nil, errors.New("secrets: github token is empty or invalid") + } + return &GitHubSecretsProvider{ + scope: GitHubScopeRepo, owner: parts[0], repo: parts[1], token: token, + client: githubDeliveryClient(nil), safeErrors: true, + }, nil +} + +// NewGitHubOrgSecretsProviderWithToken creates an organization provider from an +// in-memory credential. Repository IDs are copied and must be unique positive +// IDs used only with selected visibility. Empty visibility defaults to private. +func NewGitHubOrgSecretsProviderWithToken(org, token string, visibility GitHubOrgVisibility, selectedRepoIDs []int64) (*GitHubSecretsProvider, error) { + if !validGitHubTargetName(org) { + return nil, errors.New("secrets: github organization name is invalid") + } + if !validGitHubToken(token) { + return nil, errors.New("secrets: github token is empty or invalid") + } + if visibility == "" { + visibility = OrgVisibilityPrivate + } + if err := validateGitHubSelection(visibility, selectedRepoIDs); err != nil { + return nil, err + } + return &GitHubSecretsProvider{ + scope: GitHubScopeOrg, org: org, token: token, orgVisibility: visibility, + selectedRepoIDs: append([]int64(nil), selectedRepoIDs...), + client: githubDeliveryClient(nil), safeErrors: true, + }, nil +} + +func validGitHubTargetName(name string) bool { + if name == "" || name == "." || name == ".." { + return false + } + for _, c := range name { + if !(c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-' || c == '_' || c == '.') { + return false + } + } + return true +} + +func validGitHubToken(token string) bool { + if token == "" { + return false + } + for _, c := range token { + if c < 33 || c > 126 { + return false + } + } + return true +} + +func validateGitHubSelection(visibility GitHubOrgVisibility, ids []int64) error { + switch visibility { + case OrgVisibilityAll, OrgVisibilityPrivate: + if len(ids) != 0 { + return errors.New("secrets: github selected repository IDs require selected visibility") + } + case OrgVisibilitySelected: + if len(ids) == 0 { + return errors.New("secrets: github selected visibility requires repository IDs") + } + seen := make(map[int64]bool, len(ids)) + for _, id := range ids { + if id <= 0 || seen[id] { + return errors.New("secrets: github selected repository IDs must be unique positive IDs") + } + seen[id] = true + } + default: + return errors.New("secrets: github organization visibility must be all, selected, or private") + } + return nil +} + +// githubDeliveryClient copies the caller's client so safe methods cannot inherit +// an unbounded timeout or a redirect policy that forwards authorization. +func githubDeliveryClient(client *http.Client) *http.Client { + bounded := &http.Client{} + if client != nil { + *bounded = *client + } + if bounded.Timeout <= 0 || bounded.Timeout > 30*time.Second { + bounded.Timeout = 30 * time.Second + } + bounded.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse } + return bounded +} + +// GitHubSecretIdentity describes immutable IDs for the configured namespace. +// Organization selection describes the intended write policy, not a remote +// secret's current visibility. Names alone do not identify a namespace forever. +type GitHubSecretIdentity struct { + Scope GitHubSecretScope `json:"scope"` + RepositoryID int64 `json:"repository_id,omitempty"` + OwnerID int64 `json:"owner_id,omitempty"` + OrganizationID int64 `json:"organization_id,omitempty"` + EnvironmentID int64 `json:"environment_id,omitempty"` + Visibility GitHubOrgVisibility `json:"visibility,omitempty"` + SelectedRepositoryIDs []int64 `json:"selected_repository_ids,omitempty"` +} + +// GitHubSecretWriteReceipt reports what a PUT response establishes. Created +// means GitHub returned 201, not an atomic create-only guarantee. MayHaveWritten +// also covers successful updates and ambiguous transport/server failures. +type GitHubSecretWriteReceipt struct { + Created bool `json:"created"` + MayHaveWritten bool `json:"may_have_written"` +} + +func (p *GitHubSecretsProvider) validateDeliveryTarget() error { + if p.scope == GitHubScopeOrg { + if !validGitHubTargetName(p.org) { + return errors.New("secrets: github organization target is invalid") + } + return validateGitHubSelection(p.orgVisibility, p.selectedRepoIDs) + } + if p.scope != "" && p.scope != GitHubScopeRepo && p.scope != GitHubScopeEnv { + return errors.New("secrets: github secret scope is invalid") + } + if !validGitHubTargetName(p.owner) || !validGitHubTargetName(p.repo) || p.scope == GitHubScopeEnv && p.env == "" { + return errors.New("secrets: github repository or environment target is invalid") + } + return nil +} + +func validGitHubSecretKey(key string) bool { + // The 256-byte limit is this helper's bound, not an asserted GitHub API limit. + if key == "" || len(key) > 256 || strings.HasPrefix(strings.ToUpper(key), "GITHUB_") { + return false + } + for i, c := range key { + if !(c >= 'A' && c <= 'Z' || c >= 'a' && c <= 'z' || c == '_' || i > 0 && c >= '0' && c <= '9') { + return false + } + } + return true +} + +func (p *GitHubSecretsProvider) deliveryRequest(ctx context.Context, method, target string, body io.Reader) (*http.Response, error) { + req, err := http.NewRequestWithContext(ctx, method, target, body) + if err != nil { + return nil, errors.New("secrets: github request is invalid") + } + p.setHeaders(req) + return p.deliveryDo(ctx, req) +} + +func (p *GitHubSecretsProvider) deliveryDo(ctx context.Context, req *http.Request) (*http.Response, error) { + resp, err := githubDeliveryClient(p.client).Do(req) + if err != nil { + // Transport errors can embed request URLs, headers, or body fragments. + // Context sentinel errors are safe and retain cancellation semantics. + if ctx.Err() != nil { + return nil, fmt.Errorf("secrets: github request failed: %w", ctx.Err()) + } + return nil, errors.New("secrets: github request failed") + } + return resp, nil +} + +func decodeGitHubDelivery(body io.Reader, out any) error { + decoder := json.NewDecoder(io.LimitReader(body, 1<<20)) + if err := decoder.Decode(out); err != nil { + return errors.New("secrets: github metadata response is invalid") + } + var trailing any + if err := decoder.Decode(&trailing); !errors.Is(err, io.EOF) { + return errors.New("secrets: github metadata response is invalid") + } + return nil +} + +func (p *GitHubSecretsProvider) deliveryMetadata(ctx context.Context, target string, out any) error { + resp, err := p.deliveryRequest(ctx, http.MethodGet, target, nil) + if err != nil { + return err + } + defer resp.Body.Close() + if resp.StatusCode == http.StatusNotFound { + return ErrNotFound + } + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("secrets: github metadata request: HTTP %d", resp.StatusCode) + } + return decodeGitHubDelivery(resp.Body, out) +} + +// Stat fetches metadata for one exact secret name without reading its value. +// HTTP 404 returns ErrNotFound; authorization and response errors remain errors. +func (p *GitHubSecretsProvider) Stat(ctx context.Context, key string) (SecretMeta, error) { + meta := SecretMeta{Name: key} + if !validGitHubSecretKey(key) { + return meta, ErrInvalidKey + } + if err := p.validateDeliveryTarget(); err != nil { + return meta, err + } + var entry ghSecretEntry + if err := p.deliveryMetadata(ctx, p.secretURL(key), &entry); err != nil { + return meta, err + } + if !strings.EqualFold(entry.Name, key) { + return meta, errors.New("secrets: github secret metadata name does not match") + } + meta.Exists = true + meta.UpdatedAt = entry.UpdatedAt + if meta.UpdatedAt.IsZero() { + meta.UpdatedAt = entry.CreatedAt + } + return meta, nil +} + +// Identity resolves the immutable repository/owner, organization, and optional +// environment IDs. It does not create environments or change permissions. +func (p *GitHubSecretsProvider) Identity(ctx context.Context) (GitHubSecretIdentity, error) { + identity := GitHubSecretIdentity{Scope: p.scope} + if err := p.validateDeliveryTarget(); err != nil { + return identity, err + } + if p.scope == GitHubScopeOrg { + var org struct { + ID int64 `json:"id"` + } + if err := p.deliveryMetadata(ctx, p.base()+"/orgs/"+url.PathEscape(p.org), &org); err != nil { + return identity, err + } + if org.ID <= 0 { + return identity, errors.New("secrets: github organization ID is missing or invalid") + } + identity.OrganizationID = org.ID + identity.Visibility = p.orgVisibility + identity.SelectedRepositoryIDs = append([]int64(nil), p.selectedRepoIDs...) + return identity, nil + } + var repo struct { + ID int64 `json:"id"` + Owner struct { + ID int64 `json:"id"` + } `json:"owner"` + } + target := p.base() + "/repos/" + url.PathEscape(p.owner) + "/" + url.PathEscape(p.repo) + if err := p.deliveryMetadata(ctx, target, &repo); err != nil { + return identity, err + } + if repo.ID <= 0 || repo.Owner.ID <= 0 { + return identity, errors.New("secrets: github repository or owner ID is missing or invalid") + } + identity.Scope = GitHubScopeRepo + identity.RepositoryID, identity.OwnerID = repo.ID, repo.Owner.ID + if p.scope == GitHubScopeEnv { + var env struct { + ID int64 `json:"id"` + } + if err := p.deliveryMetadata(ctx, p.environmentURL(p.env), &env); err != nil { + return identity, err + } + if env.ID <= 0 { + return identity, errors.New("secrets: github environment ID is missing or invalid") + } + identity.Scope, identity.EnvironmentID = GitHubScopeEnv, env.ID + } + return identity, nil +} + +// SetWithReceipt encrypts and upserts a secret, exposing only a metadata receipt +// and safe errors. It never provides an atomic create-only operation. A caller +// requiring a fresh name must check Stat and control concurrent writers itself. +func (p *GitHubSecretsProvider) SetWithReceipt(ctx context.Context, key, value string) (GitHubSecretWriteReceipt, error) { + receipt := GitHubSecretWriteReceipt{} + if !validGitHubSecretKey(key) { + return receipt, ErrInvalidKey + } + if err := p.validateDeliveryTarget(); err != nil { + return receipt, err + } + var publicKey repoPublicKeyResponse + if err := p.deliveryMetadata(ctx, p.publicKeyURL(), &publicKey); err != nil { + return receipt, err + } + if publicKey.KeyID == "" { + return receipt, errors.New("secrets: github public key ID is missing") + } + encrypted, err := encryptSecret(publicKey.Key, value) + if err != nil { + return receipt, errors.New("secrets: github secret encryption failed") + } + body, err := json.Marshal(p.secretPayload(publicKey.KeyID, encrypted)) + if err != nil { + return receipt, errors.New("secrets: github secret payload is invalid") + } + req, err := http.NewRequestWithContext(ctx, http.MethodPut, p.secretURL(key), bytes.NewReader(body)) + if err != nil { + return receipt, errors.New("secrets: github secret write request is invalid") + } + p.setHeaders(req) + if ctx.Err() != nil { + return receipt, fmt.Errorf("secrets: github secret write canceled before upload: %w", ctx.Err()) + } + resp, err := p.deliveryDo(ctx, req) + if err != nil { + receipt.MayHaveWritten = true + return receipt, err + } + defer resp.Body.Close() + switch resp.StatusCode { + case http.StatusCreated: + receipt.Created, receipt.MayHaveWritten = true, true + return receipt, nil + case http.StatusNoContent: + receipt.MayHaveWritten = true + return receipt, nil + default: + // Only explicit client rejections establish that the PUT failed. A + // timeout, server failure, redirect, or unexpected success is uncertain. + receipt.MayHaveWritten = resp.StatusCode < 400 || resp.StatusCode >= 500 || resp.StatusCode == http.StatusRequestTimeout + return receipt, fmt.Errorf("secrets: github secret write: HTTP %d", resp.StatusCode) + } +} + +func (p *GitHubSecretsProvider) secretPayload(keyID, encrypted string) map[string]any { + payload := map[string]any{"encrypted_value": encrypted, "key_id": keyID} + if p.scope == GitHubScopeOrg { + payload["visibility"] = string(p.orgVisibility) + if p.orgVisibility == OrgVisibilitySelected { + payload["selected_repository_ids"] = p.selectedRepoIDs + } + } + return payload +} + // Scope reports the current scope. func (p *GitHubSecretsProvider) Scope() GitHubSecretScope { return p.scope } @@ -195,17 +540,7 @@ func (p *GitHubSecretsProvider) Set(ctx context.Context, key, value string) erro return fmt.Errorf("secrets: github encrypt: %w", err) } - payload := map[string]any{ - "encrypted_value": encrypted, - "key_id": pubKeyID, - } - if p.scope == GitHubScopeOrg { - payload["visibility"] = string(p.orgVisibility) - if p.orgVisibility == OrgVisibilitySelected { - payload["selected_repository_ids"] = p.selectedRepoIDs - } - } - body, _ := json.Marshal(payload) + body, _ := json.Marshal(p.secretPayload(pubKeyID, encrypted)) req, err := http.NewRequestWithContext(ctx, http.MethodPut, p.secretURL(key), bytes.NewReader(body)) if err != nil { return err @@ -255,26 +590,68 @@ type ghSecretEntry struct { // listSecretEntries fetches and decodes all secret entries (name + timestamps). func (p *GitHubSecretsProvider) listSecretEntries(ctx context.Context) ([]ghSecretEntry, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.secretsURL(), nil) - if err != nil { - return nil, err + if p.safeErrors { + if err := p.validateDeliveryTarget(); err != nil { + return nil, err + } } - p.setHeaders(req) - resp, err := p.client.Do(req) + initial, err := url.Parse(p.secretsURL()) if err != nil { - return nil, err - } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("secrets: github list secrets: HTTP %d%s", resp.StatusCode, readErrorBody(resp)) - } - var result struct { - Secrets []ghSecretEntry `json:"secrets"` - } - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return nil, fmt.Errorf("secrets: github list decode: %w", err) + return nil, errors.New("secrets: github list URL is invalid") + } + query := initial.Query() + query.Set("per_page", "100") + initial.RawQuery = query.Encode() + nextURL := initial.String() + seen := map[string]bool{} + var entries []ghSecretEntry + for nextURL != "" { + next, err := url.Parse(nextURL) + if err != nil || next.Scheme != initial.Scheme || next.Host != initial.Host || next.User != nil || next.EscapedPath() != initial.EscapedPath() || next.Fragment != "" { + return nil, errors.New("secrets: github secret pagination escaped the configured namespace") + } + // Canonical query ordering also catches cycles with reordered parameters. + next.RawQuery = next.Query().Encode() + canonical := next.String() + if seen[canonical] || len(seen) >= 10000 { + return nil, errors.New("secrets: github secret pagination repeated or exceeded the page limit") + } + seen[canonical] = true + req, err := http.NewRequestWithContext(ctx, http.MethodGet, canonical, nil) + if err != nil { + return nil, errors.New("secrets: github list request is invalid") + } + p.setHeaders(req) + resp, err := githubDeliveryClient(p.client).Do(req) + if err != nil { + if p.safeErrors { + return nil, errors.New("secrets: github list request failed") + } + return nil, err + } + if resp.StatusCode != http.StatusOK { + detail := "" + if !p.safeErrors { + detail = readErrorBody(resp) + } + resp.Body.Close() + return nil, fmt.Errorf("secrets: github list secrets: HTTP %d%s", resp.StatusCode, detail) + } + var result struct { + Secrets *[]ghSecretEntry `json:"secrets"` + } + err = decodeGitHubDelivery(resp.Body, &result) + resp.Body.Close() + if err != nil { + return nil, err + } + if result.Secrets == nil { + return nil, errors.New("secrets: github secret list is missing") + } + entries = append(entries, (*result.Secrets)...) + nextURL = githubNextLink(resp.Header.Get("Link")) } - return result.Secrets, nil + return entries, nil } // List returns the names of all GitHub Actions secrets for the repo. From 9f55803c305ceaf94bd96cc91243b53edd2cc87f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 6 Oct 2026 22:57:25 -0400 Subject: [PATCH 2/5] feat(secrets): expose private delivery transport seam (Task 1) --- docs/wfctl-secrets-scopes.md | 5 + secrets/github_delivery_external_test.go | 247 +++++++++++++++++++++++ secrets/github_provider.go | 40 +++- 3 files changed, 288 insertions(+), 4 deletions(-) create mode 100644 secrets/github_delivery_external_test.go diff --git a/docs/wfctl-secrets-scopes.md b/docs/wfctl-secrets-scopes.md index 8822b2c4..c7b11af4 100644 --- a/docs/wfctl-secrets-scopes.md +++ b/docs/wfctl-secrets-scopes.md @@ -177,6 +177,11 @@ without placing the GitHub credential in an environment variable: - `NewGitHubOrgSecretsProviderWithToken(org, token, visibility, selectedRepoIDs)` copies and validates positive, unique repository IDs. IDs are accepted only with `selected` visibility; empty visibility defaults to `private`. +- Both direct-token constructors accept optional `GitHubSecretsOption` arguments. + `WithGitHubSecretsHTTPTransport(transport)` substitutes only the HTTP service + boundary, retaining the production API URL and request safety policy. This + lets plugin integration tests exercise the real provider without changing + global clients. An explicit nil transport is rejected. - `Identity(ctx)` resolves immutable repository and owner IDs, an organization ID, or repository, owner, and environment IDs. Organization visibility and selected repository IDs describe the intended write policy. diff --git a/secrets/github_delivery_external_test.go b/secrets/github_delivery_external_test.go new file mode 100644 index 00000000..cbf43ff0 --- /dev/null +++ b/secrets/github_delivery_external_test.go @@ -0,0 +1,247 @@ +package secrets_test + +import ( + "context" + "crypto/rand" + "encoding/base64" + "encoding/json" + "errors" + "io" + "net/http" + "reflect" + "strings" + "testing" + "time" + + "github.com/GoCodeAlone/workflow/secrets" + "golang.org/x/crypto/blake2b" + "golang.org/x/crypto/nacl/box" +) + +type githubFixtureTransport func(*http.Request) (*http.Response, error) + +func (f githubFixtureTransport) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +func githubFixtureResponse(r *http.Request, status int, body string) *http.Response { + return &http.Response{StatusCode: status, Header: http.Header{}, Body: io.NopCloser(strings.NewReader(body)), Request: r} +} + +// This test crosses the exported plugin-to-provider boundary. Only the remote +// transport is replaced: identity, exact metadata, encryption, and receipt logic +// are the production provider, with the production API URL retained. +func TestGitHubDeliveryPublicTransportBoundary(t *testing.T) { + const auth = "external-fixture-github-auth" + const value = "external-fixture-issued-cloudflare-value" + for _, scope := range []secrets.GitHubSecretScope{secrets.GitHubScopeRepo, secrets.GitHubScopeEnv, secrets.GitHubScopeOrg} { + t.Run(string(scope), func(t *testing.T) { + pub, priv, err := box.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + path := "/repos/owner/repo/actions/secrets" + if scope == secrets.GitHubScopeEnv { + path = "/repos/owner/repo/environments/preview%2Fblue/secrets" + } + if scope == secrets.GitHubScopeOrg { + path = "/orgs/owner/actions/secrets" + } + var calls []string + var ciphertext string + transport := githubFixtureTransport(func(r *http.Request) (*http.Response, error) { + calls = append(calls, r.Method+" "+r.URL.EscapedPath()) + if r.URL.Scheme != "https" || r.URL.Host != "api.github.com" || r.URL.User != nil { + t.Fatal("transport option changed the production API destination") + } + if r.Header.Get("Authorization") != "Bearer "+auth { + t.Fatal("incorrect direct-token authentication") + } + deadline, bounded := r.Context().Deadline() + if !bounded || time.Until(deadline) > 30*time.Second { + t.Fatal("transport option bypassed the request timeout") + } + if r.Method == http.MethodGet { + switch r.URL.EscapedPath() { + case "/repos/owner/repo": + return githubFixtureResponse(r, 200, `{"id":100,"owner":{"id":200}}`), nil + case "/repos/owner/repo/environments/preview%2Fblue": + return githubFixtureResponse(r, 200, `{"id":300}`), nil + case "/orgs/owner": + return githubFixtureResponse(r, 200, `{"id":200}`), nil + case path + "/MISSING_TOKEN": + return githubFixtureResponse(r, 404, auth), nil + case path + "/EXISTING_TOKEN": + return githubFixtureResponse(r, 200, `{"name":"EXISTING_TOKEN","updated_at":"2026-10-07T01:02:03Z"}`), nil + case path + "/public-key": + body, _ := json.Marshal(map[string]string{"key_id": "fixture-key", "key": base64.StdEncoding.EncodeToString(pub[:])}) + return githubFixtureResponse(r, 200, string(body)), nil + } + } + if r.Method == http.MethodPut && r.URL.EscapedPath() == path+"/NEW_TOKEN" { + var payload struct { + EncryptedValue string `json:"encrypted_value"` + KeyID string `json:"key_id"` + Visibility string `json:"visibility"` + SelectedRepositoryIDs []int64 `json:"selected_repository_ids"` + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Fatal(err) + } + ciphertext = payload.EncryptedValue + if payload.KeyID != "fixture-key" || ciphertext == "" || strings.Contains(ciphertext, value) { + t.Fatal("invalid encrypted upload") + } + if scope == secrets.GitHubScopeOrg { + if payload.Visibility != "selected" || !reflect.DeepEqual(payload.SelectedRepositoryIDs, []int64{11, 12}) { + t.Fatal("organization destination controls were lost") + } + } else if payload.Visibility != "" || len(payload.SelectedRepositoryIDs) != 0 { + t.Fatal("unexpected organization controls") + } + return githubFixtureResponse(r, 201, auth+" "+value+" "+ciphertext), nil + } + t.Fatalf("unexpected remote request: %s %s", r.Method, r.URL.EscapedPath()) + return nil, errors.New("unexpected fixture request") + }) + option := secrets.WithGitHubSecretsHTTPTransport(transport) + var p *secrets.GitHubSecretsProvider + if scope == secrets.GitHubScopeOrg { + p, err = secrets.NewGitHubOrgSecretsProviderWithToken("owner", auth, secrets.OrgVisibilitySelected, []int64{11, 12}, option) + } else { + p, err = secrets.NewGitHubSecretsProviderWithToken("owner/repo", auth, option) + if err == nil && scope == secrets.GitHubScopeEnv { + p.SetEnvironment("preview/blue") + } + } + if err != nil { + t.Fatal(err) + } + identity, err := p.Identity(context.Background()) + if err != nil || identity.Scope != scope { + t.Fatalf("Identity = %+v, %v", identity, err) + } + wantCalls := []string{"GET /repos/owner/repo"} + if scope == secrets.GitHubScopeOrg { + if identity.OrganizationID != 200 || identity.Visibility != secrets.OrgVisibilitySelected || !reflect.DeepEqual(identity.SelectedRepositoryIDs, []int64{11, 12}) { + t.Fatalf("unexpected organization identity: %+v", identity) + } + wantCalls = []string{"GET /orgs/owner"} + } else { + if identity.RepositoryID != 100 || identity.OwnerID != 200 { + t.Fatalf("unexpected repository identity: %+v", identity) + } + if scope == secrets.GitHubScopeEnv { + if identity.EnvironmentID != 300 { + t.Fatalf("unexpected environment identity: %+v", identity) + } + wantCalls = append(wantCalls, "GET /repos/owner/repo/environments/preview%2Fblue") + } + } + missing, err := p.Stat(context.Background(), "MISSING_TOKEN") + if !errors.Is(err, secrets.ErrNotFound) || missing.Exists || strings.Contains(err.Error(), auth) { + t.Fatalf("missing metadata = %+v, %v", missing, err) + } + meta, err := p.Stat(context.Background(), "EXISTING_TOKEN") + if err != nil || !meta.Exists || !meta.UpdatedAt.Equal(time.Date(2026, 10, 7, 1, 2, 3, 0, time.UTC)) { + t.Fatalf("metadata = %+v, %v", meta, err) + } + receipt, err := p.SetWithReceipt(context.Background(), "NEW_TOKEN", value) + if err != nil || !receipt.Created || !receipt.MayHaveWritten { + t.Fatalf("receipt = %+v, %v", receipt, err) + } + wantCalls = append(wantCalls, "GET "+path+"/MISSING_TOKEN", "GET "+path+"/EXISTING_TOKEN", "GET "+path+"/public-key", "PUT "+path+"/NEW_TOKEN") + if !reflect.DeepEqual(calls, wantCalls) { + t.Fatalf("calls = %v, want %v", calls, wantCalls) + } + sealed, err := base64.StdEncoding.DecodeString(ciphertext) + if err != nil || len(sealed) < 32+box.Overhead { + t.Fatalf("invalid sealed box: %v", err) + } + var ephemeral [32]byte + copy(ephemeral[:], sealed[:32]) + h, err := blake2b.New(24, nil) + if err != nil { + t.Fatal(err) + } + h.Write(ephemeral[:]) + h.Write(pub[:]) + var nonce [24]byte + copy(nonce[:], h.Sum(nil)) + plaintext, ok := box.Open(nil, sealed[32:], &nonce, &ephemeral, priv) + if !ok || string(plaintext) != value { + t.Fatal("public provider boundary did not preserve the issued value through sealed-box encryption") + } + }) + } +} + +func TestGitHubDeliveryPublicTransportSafeFailures(t *testing.T) { + const auth = "external-failure-github-auth" + const value = "external-failure-issued-value" + pub, _, err := box.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + for _, failure := range []string{"transport", "server", "redirect"} { + t.Run(failure, func(t *testing.T) { + var calls []string + var ciphertext string + option := secrets.WithGitHubSecretsHTTPTransport(githubFixtureTransport(func(r *http.Request) (*http.Response, error) { + calls = append(calls, r.Method+" "+r.URL.EscapedPath()) + if r.URL.Scheme != "https" || r.URL.Host != "api.github.com" { + t.Fatal("unexpected API host") + } + if r.Method == http.MethodGet && r.URL.EscapedPath() == "/repos/owner/repo/actions/secrets/public-key" { + body, _ := json.Marshal(map[string]string{"key_id": "fixture-key", "key": base64.StdEncoding.EncodeToString(pub[:])}) + return githubFixtureResponse(r, 200, string(body)), nil + } + if r.Method != http.MethodPut || r.URL.EscapedPath() != "/repos/owner/repo/actions/secrets/NEW_TOKEN" { + t.Fatal("unexpected API request") + } + var payload struct { + EncryptedValue string `json:"encrypted_value"` + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Fatal(err) + } + ciphertext = payload.EncryptedValue + leak := auth + " " + value + " " + ciphertext + if failure == "transport" { + return nil, errors.New(leak) + } + resp := githubFixtureResponse(r, 500, leak) + if failure == "redirect" { + resp.StatusCode = 307 + resp.Header.Set("Location", "https://unapproved.example/"+value) + } + return resp, nil + })) + p, err := secrets.NewGitHubSecretsProviderWithToken("owner/repo", auth, option) + if err != nil { + t.Fatal(err) + } + receipt, err := p.SetWithReceipt(context.Background(), "NEW_TOKEN", value) + if err == nil || ciphertext == "" || receipt.Created || !receipt.MayHaveWritten { + t.Fatalf("failure receipt = %+v, %v", receipt, err) + } + for _, sentinel := range []string{auth, value, ciphertext} { + if strings.Contains(err.Error(), sentinel) { + t.Fatal("public transport boundary exposed credential material") + } + } + want := []string{"GET /repos/owner/repo/actions/secrets/public-key", "PUT /repos/owner/repo/actions/secrets/NEW_TOKEN"} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("calls = %v, want %v", calls, want) + } + }) + } +} + +func TestGitHubDeliveryPublicTransportRejectsNil(t *testing.T) { + option := secrets.WithGitHubSecretsHTTPTransport(nil) + if _, err := secrets.NewGitHubSecretsProviderWithToken("owner/repo", "synthetic-auth", option); err == nil { + t.Fatal("nil explicit transport was accepted") + } + if _, err := secrets.NewGitHubOrgSecretsProviderWithToken("owner", "synthetic-auth", secrets.OrgVisibilityPrivate, nil, option); err == nil { + t.Fatal("nil explicit organization transport was accepted") + } +} diff --git a/secrets/github_provider.go b/secrets/github_provider.go index de36584d..527b425a 100644 --- a/secrets/github_provider.go +++ b/secrets/github_provider.go @@ -62,6 +62,30 @@ type GitHubSecretsProvider struct { safeErrors bool // direct-token providers omit upstream bodies and errors } +// GitHubSecretsOption configures only the transport used by direct-token +// providers. The API destination, timeout bound, and redirect policy remain fixed. +type GitHubSecretsOption struct { + transport http.RoundTripper +} + +// WithGitHubSecretsHTTPTransport substitutes the HTTP service boundary without +// changing global clients or the GitHub API URL. An explicit nil transport is +// rejected by the constructor. If repeated, the last transport is used. +func WithGitHubSecretsHTTPTransport(transport http.RoundTripper) GitHubSecretsOption { + return GitHubSecretsOption{transport: transport} +} + +func githubDeliveryClientWithOptions(options []GitHubSecretsOption) (*http.Client, error) { + client := githubDeliveryClient(nil) + for _, option := range options { + if option.transport == nil { + return nil, errors.New("secrets: github HTTP transport must not be nil") + } + client.Transport = option.transport + } + return client, nil +} + // base returns the API base URL, using baseURL when set (for tests). func (p *GitHubSecretsProvider) base() string { if p.baseURL != "" { @@ -129,7 +153,7 @@ func NewGitHubOrgSecretsProvider(org string, tokenEnvVar string, visibility GitH // in-memory credential. It does not read environment variables. Requests are // bounded and redirects are refused. Stat, Identity, SetWithReceipt, List, and // StatAll omit upstream bodies and causes. Legacy Set retains upsert semantics. -func NewGitHubSecretsProviderWithToken(repo, token string) (*GitHubSecretsProvider, error) { +func NewGitHubSecretsProviderWithToken(repo, token string, options ...GitHubSecretsOption) (*GitHubSecretsProvider, error) { parts := strings.Split(repo, "/") if len(parts) != 2 || !validGitHubTargetName(parts[0]) || !validGitHubTargetName(parts[1]) { return nil, errors.New("secrets: github repo must be owner/repo") @@ -137,16 +161,20 @@ func NewGitHubSecretsProviderWithToken(repo, token string) (*GitHubSecretsProvid if !validGitHubToken(token) { return nil, errors.New("secrets: github token is empty or invalid") } + client, err := githubDeliveryClientWithOptions(options) + if err != nil { + return nil, err + } return &GitHubSecretsProvider{ scope: GitHubScopeRepo, owner: parts[0], repo: parts[1], token: token, - client: githubDeliveryClient(nil), safeErrors: true, + client: client, safeErrors: true, }, nil } // NewGitHubOrgSecretsProviderWithToken creates an organization provider from an // in-memory credential. Repository IDs are copied and must be unique positive // IDs used only with selected visibility. Empty visibility defaults to private. -func NewGitHubOrgSecretsProviderWithToken(org, token string, visibility GitHubOrgVisibility, selectedRepoIDs []int64) (*GitHubSecretsProvider, error) { +func NewGitHubOrgSecretsProviderWithToken(org, token string, visibility GitHubOrgVisibility, selectedRepoIDs []int64, options ...GitHubSecretsOption) (*GitHubSecretsProvider, error) { if !validGitHubTargetName(org) { return nil, errors.New("secrets: github organization name is invalid") } @@ -159,10 +187,14 @@ func NewGitHubOrgSecretsProviderWithToken(org, token string, visibility GitHubOr if err := validateGitHubSelection(visibility, selectedRepoIDs); err != nil { return nil, err } + client, err := githubDeliveryClientWithOptions(options) + if err != nil { + return nil, err + } return &GitHubSecretsProvider{ scope: GitHubScopeOrg, org: org, token: token, orgVisibility: visibility, selectedRepoIDs: append([]int64(nil), selectedRepoIDs...), - client: githubDeliveryClient(nil), safeErrors: true, + client: client, safeErrors: true, }, nil } From 159ac40bd481641cb68e94e2821e08893dafe2f7 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 6 Oct 2026 23:06:31 -0400 Subject: [PATCH 3/5] fix(secrets): close delivery pagination review edges (Task 1) --- docs/wfctl-secrets-scopes.md | 7 +- secrets/github_delivery_test.go | 125 ++++++++++++++++++++++++++++++++ secrets/github_provider.go | 116 ++++++++++++++++++++++++++++- 3 files changed, 244 insertions(+), 4 deletions(-) diff --git a/docs/wfctl-secrets-scopes.md b/docs/wfctl-secrets-scopes.md index c7b11af4..ef3ff26e 100644 --- a/docs/wfctl-secrets-scopes.md +++ b/docs/wfctl-secrets-scopes.md @@ -188,7 +188,8 @@ without placing the GitHub credential in an environment variable: - `Stat(ctx, key)` fetches one exact secret's metadata. A `404` returns `secrets.ErrNotFound`; permission failures remain errors. `List` and `StatAll` follow all pages, rejecting links outside the configured origin and namespace - or repeated pages. + or repeated pages. All Link header fields are parsed; malformed or ambiguous + next relations fail instead of reporting an incomplete inventory. - `SetWithReceipt(ctx, key, value)` reuses GitHub's sealed-box encryption and returns `GitHubSecretWriteReceipt{Created, MayHaveWritten}`. It does not return a value, ciphertext, or upstream response body. @@ -196,7 +197,9 @@ without placing the GitHub credential in an environment variable: The new metadata and receipt methods bound each request to 30 seconds and refuse redirects, including on providers built with the legacy constructors. They omit raw response bodies and transport error details. Direct-token providers also -use safe errors for `List` and `StatAll`. The new exact-key methods accept ASCII +use safe errors for `List` and `StatAll`, retaining context cancellation/deadline +sentinels. Metadata responses exceeding 1 MiB are rejected before decoding. The +new exact-key methods accept ASCII letters, digits, and underscores, require a letter or underscore first, reject the reserved `GITHUB_` prefix case-insensitively, and apply their own conservative 256-byte name bound. Existing constructors and `Set` remain compatible. diff --git a/secrets/github_delivery_test.go b/secrets/github_delivery_test.go index c79939f5..45f0e2ab 100644 --- a/secrets/github_delivery_test.go +++ b/secrets/github_delivery_test.go @@ -699,3 +699,128 @@ func TestGitHubDeliveryRejectsUnsafeSecretPagination(t *testing.T) { } } } + +func TestGitHubDeliverySecretPaginationAllLinkHeaders(t *testing.T) { + for _, direct := range []bool{false, true} { + for _, metadata := range []bool{false, true} { + t.Run(fmt.Sprintf("direct=%v/metadata=%v", direct, metadata), func(t *testing.T) { + p := newPaginationProvider(t, GitHubScopeRepo, direct) + var calls []string + p.client.Transport = deliveryRoundTripper(func(r *http.Request) (*http.Response, error) { + calls = append(calls, r.URL.RequestURI()) + if r.Method != http.MethodGet || r.URL.EscapedPath() != "/repos/owner/repo/actions/secrets" { + t.Fatal("unexpected secret-list destination") + } + name := "FIRST" + headers := http.Header{} + if r.URL.Query().Get("page") == "2" { + name = "SECOND" + } else { + headers.Add("Link", `; rel="last"`) + headers.Add("Link", `; rel="next"; title="page, two"`) + } + body, _ := json.Marshal(map[string]any{"secrets": []ghSecretEntry{{Name: name}}}) + return &http.Response{StatusCode: 200, Header: headers, Body: io.NopCloser(strings.NewReader(string(body))), Request: r}, nil + }) + var names []string + if metadata { + metas, err := p.StatAll(context.Background()) + if err != nil { + t.Fatal(err) + } + for _, meta := range metas { + names = append(names, meta.Name) + } + } else { + var err error + names, err = p.List(context.Background()) + if err != nil { + t.Fatal(err) + } + } + want := []string{"/repos/owner/repo/actions/secrets?per_page=100", "/repos/owner/repo/actions/secrets?page=2&per_page=100"} + if !reflect.DeepEqual(names, []string{"FIRST", "SECOND"}) || !reflect.DeepEqual(calls, want) { + t.Fatalf("split Link inventory = %v, requests = %v", names, calls) + } + }) + } + } +} + +func TestGitHubDeliverySecretPaginationRejectsMalformedNext(t *testing.T) { + for _, headers := range [][]string{ + {`https://api.github.com/repos/owner/repo/actions/secrets?page=2; rel="next"`}, + {`