diff --git a/Makefile b/Makefile index 4202f07e..c2950aca 100644 --- a/Makefile +++ b/Makefile @@ -36,6 +36,12 @@ fuzz: mod @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_LOCAL_SECS)s ./internal/credentialhandler @echo "Fuzzing internal/jwt..." @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_LOCAL_SECS)s ./internal/jwt + @echo "Fuzzing internal/profile..." + @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_LOCAL_SECS)s ./internal/profile + @echo "Fuzzing internal/vendor..." + @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_LOCAL_SECS)s ./internal/vendor + @echo "Fuzzing handlers..." + @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_LOCAL_SECS)s . # CI targets - output coverage.out for codecov .PHONY: ci-unit @@ -52,6 +58,12 @@ ci-fuzz: mod @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_CI_SECS)s ./internal/credentialhandler @echo "Fuzzing internal/jwt..." @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_CI_SECS)s ./internal/jwt + @echo "Fuzzing internal/profile..." + @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_CI_SECS)s ./internal/profile + @echo "Fuzzing internal/vendor..." + @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_CI_SECS)s ./internal/vendor + @echo "Fuzzing handlers..." + @go test -tags=fuzz -fuzz=Fuzz -run=^$$ -fuzztime=$(FUZZING_CI_SECS)s . dist: mkdir -p dist diff --git a/api_integration_test.go b/api_integration_test.go index 7ec6a214..84891032 100644 --- a/api_integration_test.go +++ b/api_integration_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/chinmina/chinmina-bridge/internal/jwt/jwxtest" + "github.com/chinmina/chinmina-bridge/internal/profile" "github.com/chinmina/chinmina-bridge/internal/profile/profiletest" "github.com/chinmina/chinmina-bridge/internal/testhelpers" "github.com/lestrrat-go/jwx/v3/jwt" @@ -642,3 +643,165 @@ func TestIntegrationBasePath(t *testing.T) { assert.Equal(t, http.StatusNotFound, resp.StatusCode) }) } + +// ============================================================================ +// Dynamic Repository Scoping Tests +// ============================================================================ + +func TestIntegrationOrganizationToken_CallerScoped_Success(t *testing.T) { + harness := NewAPITestHarness(t) + + yamlContent, err := os.ReadFile("testdata/org-profiles-scoped.yaml") + require.NoError(t, err) + + profiles, err := profiletest.CompileFromYAML(string(yamlContent)) + require.NoError(t, err) + harness.ProfileStore.Update(t.Context(), profiles) + + harness.GitHubMock.Token = "ghs_scoped_token" + + token := harness.PipelineToken() + + result, err := harness.Client().OrganizationTokenScoped(token, "caller-scoped-profile", "my-repo") + require.NoError(t, err) + + assert.Equal(t, "ghs_scoped_token", result.Token) + // Scoped URN short form: org:/ — the ref now carries + // ScopedRepository, so downstream rendering picks it up automatically. + assert.Equal(t, "org:caller-scoped-profile/my-repo", result.Profile) + assert.Equal(t, profile.NewSpecificScope("my-repo"), result.Repositories) +} + +func TestIntegrationOrganizationToken_CallerScoped_MissingScope(t *testing.T) { + harness := NewAPITestHarness(t) + + yamlContent, err := os.ReadFile("testdata/org-profiles-scoped.yaml") + require.NoError(t, err) + + profiles, err := profiletest.CompileFromYAML(string(yamlContent)) + require.NoError(t, err) + harness.ProfileStore.Update(t.Context(), profiles) + + token := harness.PipelineToken() + + // Call without repository-scope — should fail + _, err = harness.Client().OrganizationTokenScoped(token, "caller-scoped-profile", "") + require.Error(t, err) + + var apiErr *APIError + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, http.StatusBadRequest, apiErr.StatusCode) +} + +func TestIntegrationOrganizationToken_StaticProfile_RejectsScope(t *testing.T) { + harness := NewAPITestHarness(t) + + yamlContent, err := os.ReadFile("testdata/org-profiles-scoped.yaml") + require.NoError(t, err) + + profiles, err := profiletest.CompileFromYAML(string(yamlContent)) + require.NoError(t, err) + harness.ProfileStore.Update(t.Context(), profiles) + + token := harness.PipelineToken() + + // Provide repository-scope to a static-list profile — should fail + _, err = harness.Client().OrganizationTokenScoped(token, "static-profile", "unwanted-scope") + require.Error(t, err) + + var apiErr *APIError + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, http.StatusBadRequest, apiErr.StatusCode) +} + +func TestIntegrationOrganizationToken_AllRepos_RejectsScope(t *testing.T) { + harness := NewAPITestHarness(t) + + yamlContent, err := os.ReadFile("testdata/org-profiles-scoped.yaml") + require.NoError(t, err) + + profiles, err := profiletest.CompileFromYAML(string(yamlContent)) + require.NoError(t, err) + harness.ProfileStore.Update(t.Context(), profiles) + + token := harness.PipelineToken() + + // Provide repository-scope to an all-repos profile — should fail + _, err = harness.Client().OrganizationTokenScoped(token, "all-repos-profile", "unwanted-scope") + require.Error(t, err) + + var apiErr *APIError + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, http.StatusBadRequest, apiErr.StatusCode) +} + +func TestIntegrationOrganizationToken_InvalidScope_Slash(t *testing.T) { + harness := NewAPITestHarness(t) + + yamlContent, err := os.ReadFile("testdata/org-profiles-scoped.yaml") + require.NoError(t, err) + + profiles, err := profiletest.CompileFromYAML(string(yamlContent)) + require.NoError(t, err) + harness.ProfileStore.Update(t.Context(), profiles) + + token := harness.PipelineToken() + + // Provide invalid repository-scope with slash — should fail with 400 + _, err = harness.Client().OrganizationTokenScoped(token, "caller-scoped-profile", "owner/repo") + require.Error(t, err) + + var apiErr *APIError + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, http.StatusBadRequest, apiErr.StatusCode) +} + +func TestIntegrationOrganizationGitCredentials_CallerScoped_Success(t *testing.T) { + harness := NewAPITestHarness(t) + + yamlContent, err := os.ReadFile("testdata/org-profiles-scoped.yaml") + require.NoError(t, err) + + profiles, err := profiletest.CompileFromYAML(string(yamlContent)) + require.NoError(t, err) + harness.ProfileStore.Update(t.Context(), profiles) + + harness.GitHubMock.Token = "ghs_scoped_creds" + + token := harness.PipelineToken() + + // Git-credentials derives scope from the request body (path field) + props, err := harness.Client().OrganizationGitCredentials(token, "caller-scoped-profile", GitCredentialRequest{ + Protocol: "https", + Host: "github.com", + Path: "test-org/target-repo", + }) + require.NoError(t, err) + + assert.Equal(t, "ghs_scoped_creds", props.Get("password")) + assert.Equal(t, "test-org/target-repo", props.Get("path")) +} + +func TestIntegrationOrganizationGitCredentials_AllRepos_Success(t *testing.T) { + harness := NewAPITestHarness(t) + + yamlContent, err := os.ReadFile("testdata/org-profiles-scoped.yaml") + require.NoError(t, err) + + profiles, err := profiletest.CompileFromYAML(string(yamlContent)) + require.NoError(t, err) + harness.ProfileStore.Update(t.Context(), profiles) + + harness.GitHubMock.Token = "ghs_allrepos_creds" + + token := harness.PipelineToken() + + props, err := harness.Client().OrganizationGitCredentials(token, "all-repos-profile", GitCredentialRequest{ + Protocol: "https", + Host: "github.com", + Path: "test-org/any-repo", + }) + require.NoError(t, err) + + assert.Equal(t, "ghs_allrepos_creds", props.Get("password")) +} diff --git a/docs/superpowers/plans/2026-04-15-dynamic-repository-scoping.md b/docs/superpowers/plans/2026-04-15-dynamic-repository-scoping.md new file mode 100644 index 00000000..5537ee6b --- /dev/null +++ b/docs/superpowers/plans/2026-04-15-dynamic-repository-scoping.md @@ -0,0 +1,1821 @@ +# Dynamic Repository Scoping Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Extend organization profiles to support caller-scoped repository tokens and an explicit `{{all-repositories}}` literal, replacing the terse `*` wildcard with deprecation. + +**Architecture:** The `RepositoryScope` type gains a third state (caller-scoped). Profile compilation resolves the new YAML literals at load time. The org vendor accepts a `repositoryScope` parameter from the handler, validates bidirectional scoping rules, and issues narrowed tokens. The cache key includes the repository name for caller-scoped profiles. + +**Tech Stack:** Go 1.26, testify, slog, alice middleware, github.com/google/go-github + +**Spec:** `docs/superpowers/specs/2026-04-15-dynamic-repository-scoping-design.md` + +--- + +## File Map + +| Action | File | Responsibility | +|--------|------|----------------| +| Modify | `internal/profile/repositoryscope.go` | Add `CallerScoped` state to `RepositoryScope` | +| Modify | `internal/profile/repositoryscope_test.go` | Tests for the new state | +| Modify | `internal/profile/profiles.go` | Change `OrganizationProfileAttr` to store compiled `RepositoryScope` | +| Modify | `internal/profile/profiles_test.go` | Tests for updated attr | +| Modify | `internal/profile/compilation.go` | Recognise new literals, deprecation warning, compile-time resolution | +| Modify | `internal/profile/compilation_test.go` | Compilation tests for new literals | +| Modify | `internal/profile/config.go` | New error types for scoping mismatches | +| Modify | `internal/vendor/orgvendor.go` | Accept `repositoryScope` parameter, bidirectional validation | +| Modify | `internal/vendor/orgvendor_test.go` | Tests for scoping validation and token narrowing | +| Modify | `internal/vendor/cached.go` | Include repository name in cache key for caller-scoped profiles | +| Modify | `internal/vendor/cached_test.go` | Cache key tests for scoped profiles | +| Modify | `internal/vendor/vendor.go` | Add `RepositoryScope` field to `ProfileTokenVendor` signature | +| Modify | `internal/vendor/auditvendor.go` | Audit scoping mismatch rejections | +| Modify | `handlers.go` | Extract `repository-scope` query parameter, input validation | +| Modify | `handlers_test.go` | Handler tests for parameter extraction and validation | +| Modify | `main.go` | No structural changes expected (vendor composition unchanged) | +| Modify | `internal/profile/profiletest/testdata/profiles.yaml` | Add test profiles with new literals | + +--- + +## Task 1: Extend RepositoryScope with CallerScoped state + +**Spec refs:** Req 1.9, 1.10 + +**Files:** +- Modify: `internal/profile/repositoryscope.go` +- Modify: `internal/profile/repositoryscope_test.go` + +- [ ] **Step 1: Write failing tests for the new CallerScoped state** + +Add these test cases to `internal/profile/repositoryscope_test.go`: + +```go +func TestNewCallerScopedScope(t *testing.T) { + rs := NewCallerScopedScope() + assert.False(t, rs.Wildcard) + assert.Nil(t, rs.Names) + assert.True(t, rs.CallerScoped) +} +``` + +Add to the existing `TestRepositoryScope_IsWildcard` table: +```go +{"caller-scoped scope", NewCallerScopedScope(), false}, +``` + +Add a new table-driven test: +```go +func TestRepositoryScope_IsCallerScoped(t *testing.T) { + tests := []struct { + name string + scope RepositoryScope + expected bool + }{ + {"caller-scoped scope", NewCallerScopedScope(), true}, + {"wildcard scope", NewWildcardScope(), false}, + {"specific scope", NewSpecificScope("repo-a"), false}, + {"zero value", RepositoryScope{}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.scope.IsCallerScoped()) + }) + } +} +``` + +Add to existing `TestRepositoryScope_Contains` table: +```go +{"caller-scoped matches nothing", NewCallerScopedScope(), "any-repo", false}, +``` + +Add to existing `TestRepositoryScope_IsZero` table: +```go +{"caller-scoped scope", NewCallerScopedScope(), false}, +``` + +Add to existing `TestRepositoryScope_NamesForDisplay` table: +```go +{"caller-scoped returns empty", NewCallerScopedScope(), []string{}}, +``` + +Add to existing `TestRepositoryScope_JSONRoundTrip` table: +```go +{ + name: "caller-scoped", + scope: NewCallerScopedScope(), + expectedJSON: `{"callerScoped":true}`, +}, +``` + +Add to existing `TestRepositoryScope_LogValue` table: +```go +{ + name: "caller-scoped logs empty", + scope: NewCallerScopedScope(), + expected: slog.AnyValue([]string{}), +}, +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/profile/ -run "TestNewCallerScopedScope|TestRepositoryScope_IsCallerScoped" -v` +Expected: FAIL — `NewCallerScopedScope` and `IsCallerScoped` not defined + +- [ ] **Step 3: Implement CallerScoped state in RepositoryScope** + +In `internal/profile/repositoryscope.go`, add the `CallerScoped` field to the struct and implement the new constructor and method: + +```go +type RepositoryScope struct { + // Wildcard indicates the token covers all repositories accessible to the + // GitHub App installation. When true, Names is meaningless. + Wildcard bool `json:"wildcard,omitempty"` + // CallerScoped indicates the repository will be supplied at request time. + // When true, Names is meaningless and Wildcard must be false. + CallerScoped bool `json:"callerScoped,omitempty"` + // Names lists the specific repository names covered by the token. + Names []string `json:"names,omitempty"` +} +``` + +Add the constructor: +```go +// NewCallerScopedScope returns a RepositoryScope where the caller supplies +// the repository at request time. +func NewCallerScopedScope() RepositoryScope { + return RepositoryScope{CallerScoped: true} +} +``` + +Add the method: +```go +// IsCallerScoped reports whether this scope requires the caller to supply +// a repository at request time. +func (rs RepositoryScope) IsCallerScoped() bool { + return rs.CallerScoped +} +``` + +Update `Contains` to return false for caller-scoped (it has no repositories by definition): +```go +func (rs RepositoryScope) Contains(repo string) bool { + if rs.Wildcard { + return true + } + if rs.CallerScoped { + return false + } + return slices.Contains(rs.Names, repo) +} +``` + +Update `IsZero` to exclude caller-scoped: +```go +func (rs RepositoryScope) IsZero() bool { + return !rs.Wildcard && !rs.CallerScoped && len(rs.Names) == 0 +} +``` + +Update `NamesForDisplay` for caller-scoped: +```go +func (rs RepositoryScope) NamesForDisplay() []string { + if rs.Wildcard { + return []string{"*"} + } + if rs.CallerScoped { + return []string{} + } + return rs.Names +} +``` + +- [ ] **Step 4: Run all RepositoryScope tests to verify they pass** + +Run: `go test ./internal/profile/ -run "TestRepositoryScope|TestNewWildcardScope|TestNewSpecificScope|TestNewCallerScopedScope" -v` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add internal/profile/repositoryscope.go internal/profile/repositoryscope_test.go +git commit -m "$(cat <<'EOF' +feat: extend RepositoryScope with CallerScoped state + +Add a third state to RepositoryScope for caller-supplied repository +scoping. This is the domain model for the {{caller-scoped-repository}} +literal: the repository name is not stored in the profile but supplied +at request time. + +CallerScoped is distinct from both Wildcard (all repos) and specific +Names: it represents a deferred scope that must be resolved per-request. +EOF +)" +``` + +--- + +## Task 2: Update OrganizationProfileAttr to store compiled RepositoryScope + +**Spec refs:** Req 1.8 + +**Files:** +- Modify: `internal/profile/profiles.go` +- Modify: `internal/profile/profiles_test.go` + +- [ ] **Step 1: Write failing tests for the new Scope field** + +In `internal/profile/profiles_test.go`, add tests that verify `OrganizationProfileAttr` has a `Scope` field and `RepositoryScope()` returns it: + +```go +func TestOrganizationProfileAttr_RepositoryScope_UsesCompiledScope(t *testing.T) { + tests := []struct { + name string + attr OrganizationProfileAttr + expected RepositoryScope + }{ + { + name: "wildcard scope", + attr: OrganizationProfileAttr{ + Scope: NewWildcardScope(), + }, + expected: NewWildcardScope(), + }, + { + name: "specific scope", + attr: OrganizationProfileAttr{ + Scope: NewSpecificScope("repo-a", "repo-b"), + }, + expected: NewSpecificScope("repo-a", "repo-b"), + }, + { + name: "caller-scoped", + attr: OrganizationProfileAttr{ + Scope: NewCallerScopedScope(), + }, + expected: NewCallerScopedScope(), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.attr.RepositoryScope()) + }) + } +} +``` + +```go +func TestOrganizationProfileAttr_HasRepository(t *testing.T) { + tests := []struct { + name string + attr OrganizationProfileAttr + repo string + expected bool + }{ + { + name: "wildcard matches any repo", + attr: OrganizationProfileAttr{Scope: NewWildcardScope()}, + repo: "any-repo", + expected: true, + }, + { + name: "specific scope matches member", + attr: OrganizationProfileAttr{Scope: NewSpecificScope("repo-a")}, + repo: "repo-a", + expected: true, + }, + { + name: "specific scope rejects non-member", + attr: OrganizationProfileAttr{Scope: NewSpecificScope("repo-a")}, + repo: "repo-b", + expected: false, + }, + { + name: "caller-scoped matches nothing directly", + attr: OrganizationProfileAttr{Scope: NewCallerScopedScope()}, + repo: "any-repo", + expected: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.attr.HasRepository(tt.repo)) + }) + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/profile/ -run "TestOrganizationProfileAttr_RepositoryScope_UsesCompiledScope|TestOrganizationProfileAttr_HasRepository" -v` +Expected: FAIL — `Scope` field not defined on `OrganizationProfileAttr` + +- [ ] **Step 3: Refactor OrganizationProfileAttr to use compiled Scope** + +In `internal/profile/profiles.go`, replace the `Repositories []string` field with `Scope RepositoryScope`: + +```go +type OrganizationProfileAttr struct { + Scope RepositoryScope + Permissions []string +} +``` + +Update `HasRepository` to delegate to `Scope`: +```go +func (attr OrganizationProfileAttr) HasRepository(repo string) bool { + return attr.Scope.Contains(repo) +} +``` + +Update `RepositoryScope()` to return the stored scope directly: +```go +func (attr OrganizationProfileAttr) RepositoryScope() RepositoryScope { + return attr.Scope +} +``` + +Remove the now-unused `allowAllRepositories()` method entirely. + +- [ ] **Step 4: Update compilation.go to populate the Scope field** + +In `internal/profile/compilation.go`, update `compileOrganizationProfiles` where it builds `OrganizationProfileAttr` (around line 84): + +```go + scope := resolveRepositoryScope(p.Repositories) + + attrs := OrganizationProfileAttr{ + Scope: scope, + Permissions: ensureMetadataRead(p.Permissions), + } +``` + +Add the `resolveRepositoryScope` function: + +```go +// resolveRepositoryScope converts a raw repositories list into a typed RepositoryScope. +// This is called after validation, so the input is known to be well-formed. +func resolveRepositoryScope(repos []string) RepositoryScope { + if len(repos) == 1 && repos[0] == "*" { + return NewWildcardScope() + } + return NewSpecificScope(repos...) +} +``` + +- [ ] **Step 5: Fix all compilation and downstream tests** + +The existing tests that check `Attrs.Repositories` (a `[]string`) must now check `Attrs.Scope` (a `RepositoryScope`). Update all occurrences in: + +In `internal/profile/compilation_test.go`, replace field checks like: +```go +// Old: +assert.Equal(t, []string{"silk"}, validProfile.Attrs.Repositories) +// New: +assert.Equal(t, NewSpecificScope("silk"), validProfile.Attrs.Scope) +``` + +Do the same for all other `Attrs.Repositories` references in the file: +- `TestCompile_GracefulDegradation`: `"silk"` → `NewSpecificScope("silk")`, `"silk", "cotton"` → `NewSpecificScope("silk", "cotton")`, `"shared"` → `NewSpecificScope("shared")` +- `TestCompile_OrganizationProfile_InvalidRepositories`: `[]string{"*"}` → `NewWildcardScope()`, `[]string{"repo1", "repo2"}` → `NewSpecificScope("repo1", "repo2")` + +In `internal/vendor/orgvendor_test.go`, the `assertVendorSuccess` calls reference `profile.NewSpecificScope("secret-repo", "another-secret-repo")` in the expected `ProfileToken.Repositories` field — these remain correct since `RepositoryScope()` now returns the `Scope` field directly. + +In `internal/profile/profiletest/testdata/profiles.yaml`, existing profiles use `["repo-1", "repo-2"]` syntax — no change needed, compilation now stores these as `NewSpecificScope(...)`. + +- [ ] **Step 6: Run all tests to verify the refactor is clean** + +Run: `go test ./internal/profile/... ./internal/vendor/... -v` +Expected: PASS + +- [ ] **Step 7: Commit** + +```bash +git add internal/profile/profiles.go internal/profile/profiles_test.go internal/profile/compilation.go internal/profile/compilation_test.go +git commit -m "$(cat <<'EOF' +refactor: store compiled RepositoryScope in OrganizationProfileAttr + +Replace the raw Repositories []string field with a typed Scope field +resolved at compile time. This eliminates repeated runtime interpretation +of the wildcard marker and prepares the type for the caller-scoped state. + +The RepositoryScope() method now returns the stored value directly instead +of re-deriving it on each call. +EOF +)" +``` + +--- + +## Task 3: Profile compilation — recognise new literals and deprecation + +**Spec refs:** Req 1.1–1.7, 1.8, 13.1 + +**Files:** +- Modify: `internal/profile/compilation.go` +- Modify: `internal/profile/compilation_test.go` +- Modify: `internal/profile/profiletest/testdata/profiles.yaml` + +- [ ] **Step 1: Write failing tests for new literal acceptance** + +Add to `internal/profile/compilation_test.go`: + +```go +func TestCompile_OrganizationProfile_CallerScopedRepository(t *testing.T) { + yamlContent := ` +organization: + profiles: + - name: scoped-profile + repositories: + - "{{caller-scoped-repository}}" + permissions: + - "contents:write" + match: + - claim: pipeline_slug + value: agent-workflows + +pipeline: + defaults: + permissions: + - "contents:read" +` + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + p, err := profiles.GetOrgProfile("scoped-profile") + require.NoError(t, err) + assert.Equal(t, NewCallerScopedScope(), p.Attrs.Scope) +} + +func TestCompile_OrganizationProfile_AllRepositories(t *testing.T) { + yamlContent := ` +organization: + profiles: + - name: all-repos-profile + repositories: + - "{{all-repositories}}" + permissions: + - "contents:read" + +pipeline: + defaults: + permissions: + - "contents:read" +` + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + p, err := profiles.GetOrgProfile("all-repos-profile") + require.NoError(t, err) + assert.Equal(t, NewWildcardScope(), p.Attrs.Scope) +} +``` + +- [ ] **Step 2: Write failing tests for mixed-entry rejection** + +```go +func TestCompile_OrganizationProfile_LiteralsMustBeAlone(t *testing.T) { + tests := []struct { + name string + repositories string + profileName string + }{ + { + name: "caller-scoped mixed with static", + repositories: `["{{caller-scoped-repository}}", "repo-a"]`, + profileName: "mixed-caller-scoped", + }, + { + name: "all-repositories mixed with static", + repositories: `["{{all-repositories}}", "repo-a"]`, + profileName: "mixed-all-repos", + }, + { + name: "caller-scoped mixed with all-repositories", + repositories: `["{{caller-scoped-repository}}", "{{all-repositories}}"]`, + profileName: "mixed-both-literals", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + yamlContent := fmt.Sprintf(` +organization: + profiles: + - name: %s + repositories: %s + permissions: + - "contents:read" + match: + - claim: pipeline_slug + value: test + +pipeline: + defaults: + permissions: + - "contents:read" +`, tt.profileName, tt.repositories) + + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + _, err = profiles.GetOrgProfile(tt.profileName) + require.Error(t, err) + var unavailErr ProfileUnavailableError + require.ErrorAs(t, err, &unavailErr) + }) + } +} +``` + +- [ ] **Step 3: Write failing test for `*` deprecation warning** + +```go +func TestCompile_OrganizationProfile_WildcardDeprecationWarning(t *testing.T) { + yamlContent := ` +organization: + profiles: + - name: old-wildcard + repositories: + - "*" + permissions: + - "contents:read" + +pipeline: + defaults: + permissions: + - "contents:read" +` + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + // Profile should still compile successfully as an alias for {{all-repositories}} + p, err := profiles.GetOrgProfile("old-wildcard") + require.NoError(t, err) + assert.Equal(t, NewWildcardScope(), p.Attrs.Scope) + + // The deprecation warning is emitted via slog.Warn during compilation. + // Verifying exact log output is fragile; the key assertion is that '*' + // compiles to the same wildcard scope as '{{all-repositories}}'. +} +``` + +- [ ] **Step 4: Run tests to verify they fail** + +Run: `go test ./internal/profile/ -run "TestCompile_OrganizationProfile_CallerScopedRepository|TestCompile_OrganizationProfile_AllRepositories|TestCompile_OrganizationProfile_LiteralsMustBeAlone|TestCompile_OrganizationProfile_WildcardDeprecationWarning" -v` +Expected: FAIL — new literals not recognised by `validateRepositories` + +- [ ] **Step 5: Implement literal recognition in compilation** + +Add constants in `internal/profile/compilation.go`: + +```go +const ( + // LiteralCallerScoped is the YAML literal for caller-supplied repository scoping. + LiteralCallerScoped = "{{caller-scoped-repository}}" + // LiteralAllRepositories is the YAML literal for all-repositories access. + LiteralAllRepositories = "{{all-repositories}}" +) +``` + +Replace `validateRepositories` with a version that recognises the new literals: + +```go +func validateRepositories(repos []string) error { + if len(repos) == 0 { + return fmt.Errorf("repositories list must be non-empty") + } + + // Check for special literals that must be alone + for _, repo := range repos { + switch repo { + case LiteralCallerScoped, LiteralAllRepositories, "*": + if len(repos) > 1 { + return fmt.Errorf("%q must be the only repository entry", repo) + } + return nil + } + } + + // Check for owner prefix (slash in repo name) + for _, repo := range repos { + if strings.Contains(repo, "/") { + return fmt.Errorf("repository %q must not contain owner prefix", repo) + } + } + return nil +} +``` + +Update `resolveRepositoryScope` to handle all cases: + +```go +func resolveRepositoryScope(repos []string) RepositoryScope { + if len(repos) == 1 { + switch repos[0] { + case LiteralCallerScoped: + return NewCallerScopedScope() + case LiteralAllRepositories, "*": + return NewWildcardScope() + } + } + return NewSpecificScope(repos...) +} +``` + +Add deprecation warning emission in `compileOrganizationProfiles`, after the validation loop where valid profiles are built (around line 77). Add this inside the loop where attrs are constructed: + +```go + // Emit deprecation warning for '*' wildcard + if len(p.Repositories) == 1 && p.Repositories[0] == "*" { + slog.Warn("organization profile: '*' is deprecated, use '{{all-repositories}}' instead", + "profile", p.Name, + ) + } +``` + +Also remove the redundant empty-repositories check from `compileOrganizationProfiles` (lines 29-33) since `validateRepositories` now handles it. + +- [ ] **Step 6: Run all compilation tests** + +Run: `go test ./internal/profile/ -v` +Expected: PASS + +- [ ] **Step 7: Add new literal profiles to shared test data** + +Add to `internal/profile/profiletest/testdata/profiles.yaml`: + +```yaml + - name: caller-scoped-profile + repositories: + - "{{caller-scoped-repository}}" + permissions: + - contents:write + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" + - name: all-repos-profile + repositories: + - "{{all-repositories}}" + permissions: + - contents:read +``` + +- [ ] **Step 8: Run full test suite** + +Run: `go test ./... -v` +Expected: PASS + +- [ ] **Step 9: Commit** + +```bash +git add internal/profile/compilation.go internal/profile/compilation_test.go internal/profile/profiletest/testdata/profiles.yaml +git commit -m "$(cat <<'EOF' +feat: recognise {{caller-scoped-repository}} and {{all-repositories}} in profiles + +The profile compiler now accepts two new YAML literals: +- {{caller-scoped-repository}}: resolved to CallerScoped scope +- {{all-repositories}}: resolved to Wildcard scope + +Both must be the sole entry in the repositories list. The existing '*' +wildcard is preserved as a deprecated alias for {{all-repositories}} +with a warning emitted at compile time. +EOF +)" +``` + +--- + +## Task 4: Handler — extract and validate `repository-scope` query parameter + +**Spec refs:** Req 6.1–6.3 + +**Files:** +- Modify: `handlers.go` +- Modify: `handlers_test.go` + +- [ ] **Step 1: Write failing tests for parameter extraction and validation** + +Add to `handlers_test.go`: + +```go +func TestExtractRepositoryScope_Valid(t *testing.T) { + tests := []struct { + name string + query string + expected string + }{ + {"simple name", "repository-scope=my-repo", "my-repo"}, + {"hyphenated name", "repository-scope=my-cool-repo", "my-cool-repo"}, + {"mixed case", "repository-scope=MyRepo", "MyRepo"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := http.NewRequest("POST", "/organization/token/test?"+tt.query, nil) + require.NoError(t, err) + scope, err := extractRepositoryScope(req) + require.NoError(t, err) + assert.Equal(t, tt.expected, scope) + }) + } +} + +func TestExtractRepositoryScope_Absent(t *testing.T) { + req, err := http.NewRequest("POST", "/organization/token/test", nil) + require.NoError(t, err) + scope, err := extractRepositoryScope(req) + require.NoError(t, err) + assert.Equal(t, "", scope) +} + +func TestExtractRepositoryScope_Invalid(t *testing.T) { + tests := []struct { + name string + query string + }{ + {"contains slash", "repository-scope=owner/repo"}, + {"empty value", "repository-scope="}, + {"whitespace only", "repository-scope=%20%20"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := http.NewRequest("POST", "/organization/token/test?"+tt.query, nil) + require.NoError(t, err) + _, err = extractRepositoryScope(req) + require.Error(t, err) + }) + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test . -run "TestExtractRepositoryScope" -v` +Expected: FAIL — `extractRepositoryScope` not defined + +- [ ] **Step 3: Implement extractRepositoryScope** + +Add to `handlers.go`: + +```go +// extractRepositoryScope extracts and validates the repository-scope query parameter. +// Returns empty string if the parameter is absent. +// Returns an error if the parameter is present but invalid (empty, whitespace-only, or contains '/'). +func extractRepositoryScope(r *http.Request) (string, error) { + if !r.URL.Query().Has("repository-scope") { + return "", nil + } + + scope := r.URL.Query().Get("repository-scope") + if strings.TrimSpace(scope) == "" { + return "", fmt.Errorf("repository-scope must not be empty") + } + if strings.Contains(scope, "/") { + return "", fmt.Errorf("repository-scope must not contain '/'") + } + return scope, nil +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `go test . -run "TestExtractRepositoryScope" -v` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add handlers.go handlers_test.go +git commit -m "$(cat <<'EOF' +feat: extract and validate repository-scope query parameter + +Add extractRepositoryScope() to parse the repository-scope query +parameter from organization token requests. Rejects empty values, +whitespace-only values, and values containing '/' (no owner prefix). +The value is passed through without case normalization per spec. +EOF +)" +``` + +--- + +## Task 5: Scoping error types and vendor signature update + +**Spec refs:** Req 2.2, 2.3, 5.2, 9.2 + +**Files:** +- Modify: `internal/profile/config.go` +- Modify: `internal/vendor/vendor.go` +- Modify: `internal/vendor/orgvendor.go` +- Modify: `internal/vendor/auditvendor.go` +- Modify: `internal/vendor/repovendor.go` +- Modify: `internal/vendor/cached.go` +- Modify: `handlers.go` +- Modify: `main.go` + +This task threads the `repositoryScope` parameter through the vendor chain. It's a mechanical change that touches many files but the logic is straightforward: pass the string through every layer so the org vendor can use it. + +- [ ] **Step 1: Add scoping mismatch error types** + +Add to `internal/profile/config.go`: + +```go +// RepositoryScopeUnexpectedError indicates a repository-scope was provided +// to a profile that does not accept caller scoping. +type RepositoryScopeUnexpectedError struct { + ProfileName string +} + +func (e RepositoryScopeUnexpectedError) Error() string { + return fmt.Sprintf("profile %q does not accept repository scoping", e.ProfileName) +} + +func (e RepositoryScopeUnexpectedError) Status() (int, string) { + return http.StatusBadRequest, "profile does not accept repository scoping" +} + +// RepositoryScopeRequiredError indicates a repository-scope was not provided +// but the profile requires one. +type RepositoryScopeRequiredError struct { + ProfileName string +} + +func (e RepositoryScopeRequiredError) Error() string { + return fmt.Sprintf("profile %q requires a repository scope", e.ProfileName) +} + +func (e RepositoryScopeRequiredError) Status() (int, string) { + return http.StatusBadRequest, "repository scope is required for this profile" +} +``` + +- [ ] **Step 2: Update ProfileTokenVendor signature** + +In `internal/vendor/vendor.go`, change the `ProfileTokenVendor` type to accept a fourth `repositoryScope` parameter: + +```go +type ProfileTokenVendor func(ctx context.Context, ref profile.ProfileRef, repo string, repositoryScope string) VendorResult +``` + +- [ ] **Step 3: Update all call sites mechanically** + +This is a compile-driven refactor. Update every file that references `ProfileTokenVendor` to pass or accept the new parameter: + +In `internal/vendor/orgvendor.go`, update the function signature: +```go +return func(ctx context.Context, ref profile.ProfileRef, requestedRepoURL string, repositoryScope string) VendorResult { +``` + +In `internal/vendor/repovendor.go`, update the function signature (pipeline profiles never use scoping, ignore the parameter): +```go +return func(ctx context.Context, ref profile.ProfileRef, requestedRepoURL string, repositoryScope string) VendorResult { +``` + +In `internal/vendor/cached.go`, update the closure signature and pass through: +```go +return func(ctx context.Context, ref profile.ProfileRef, requestedRepository string, repositoryScope string) VendorResult { + // ... existing cache logic ... + result := v(ctx, ref, requestedRepository, repositoryScope) + // ... +} +``` + +In `internal/vendor/auditvendor.go`, update the closure signature and pass through: +```go +return func(ctx context.Context, ref profile.ProfileRef, repo string, repositoryScope string) VendorResult { + // ... + result := vendor(ctx, ref, repo, repositoryScope) + // ... +} +``` + +In `handlers.go`, update both handler call sites: + +In `handlePostToken`: +```go +result := tokenVendor(r.Context(), ref, "", "") +``` +(The actual scope extraction will be wired in Task 6.) + +In `handlePostGitCredentials`: +```go +result := tokenVendor(r.Context(), ref, requestedRepoURL, "") +``` + +- [ ] **Step 4: Update all test call sites** + +Update all test files that construct or call `ProfileTokenVendor` to include the fourth parameter. This includes: +- `internal/vendor/orgvendor_test.go`: update `sequenceVendor` closure and direct vendor calls +- `internal/vendor/cached_test.go`: update `sequenceVendor` call site +- `internal/vendor/repovendor_test.go`: update vendor calls +- `internal/vendor/auditvendor_test.go`: update vendor calls +- `internal/vendor/testhelpers_test.go`: update `sequenceVendor` closure signature +- `handlers_test.go`: update `tv()` helper and vendor calls + +The `sequenceVendor` helper in `testhelpers_test.go` signature becomes: +```go +func(ctx context.Context, ref profile.ProfileRef, repo string, repositoryScope string) vendor.VendorResult { +``` + +- [ ] **Step 5: Build and test** + +Run: `go build ./... && go test ./...` +Expected: PASS — all compilation and tests succeed with the new parameter threaded through + +- [ ] **Step 6: Commit** + +```bash +git add internal/profile/config.go internal/vendor/vendor.go internal/vendor/orgvendor.go internal/vendor/repovendor.go internal/vendor/cached.go internal/vendor/auditvendor.go handlers.go main.go internal/vendor/orgvendor_test.go internal/vendor/cached_test.go internal/vendor/repovendor_test.go internal/vendor/auditvendor_test.go internal/vendor/testhelpers_test.go handlers_test.go +git commit -m "$(cat <<'EOF' +refactor: thread repositoryScope parameter through vendor chain + +Add repositoryScope string parameter to ProfileTokenVendor and all +implementations. Currently passed as empty string everywhere — the +actual scoping logic follows in the next commit. + +Also add RepositoryScopeUnexpectedError and RepositoryScopeRequiredError +types for bidirectional validation. +EOF +)" +``` + +--- + +## Task 6: Wire repository-scope from handler to org vendor + +**Spec refs:** Req 2.1–2.3, 5.2, 7.1–7.2, 9.2 + +**Files:** +- Modify: `handlers.go` +- Modify: `handlers_test.go` +- Modify: `internal/vendor/orgvendor.go` +- Modify: `internal/vendor/orgvendor_test.go` +- Modify: `internal/vendor/auditvendor.go` + +- [ ] **Step 1: Write failing tests for org vendor scoping validation** + +Add to `internal/vendor/orgvendor_test.go`: + +```go +func TestOrgVendor_CallerScopedRepository_Success(t *testing.T) { + vendedDate := time.Date(1970, 1, 1, 0, 0, 10, 0, time.UTC) + + var capturedRepoNames []string + tokenVendor := vendor.TokenVendor(func(ctx context.Context, repoNames []string, scopes []string) (string, time.Time, error) { + capturedRepoNames = repoNames + return "scoped-token", vendedDate, nil + }) + + profileYAML := ` +organization: + profiles: + - name: caller-scoped-profile + repositories: ["{{caller-scoped-repository}}"] + permissions: [contents:write] + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), tokenVendor) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "caller-scoped-profile", + Type: profile.ProfileTypeOrg, + } + + ctx := createTestClaimsContextWithPipeline("agent-workflows") + result := v(ctx, ref, "", "target-repo") + + assertVendorSuccess(t, result, vendor.ProfileToken{ + Token: "scoped-token", + HashedToken: vendor.HashToken("scoped-token"), + Repositories: profile.NewSpecificScope("target-repo"), + Permissions: []string{"contents:write", "metadata:read"}, + Profile: "org:caller-scoped-profile", + Expiry: vendedDate, + OrganizationSlug: "organization-slug", + VendedRepositoryURL: "", + }) + assert.Equal(t, []string{"target-repo"}, capturedRepoNames) +} + +func TestOrgVendor_CallerScoped_MissingScopeParameter(t *testing.T) { + profileYAML := ` +organization: + profiles: + - name: caller-scoped-profile + repositories: ["{{caller-scoped-repository}}"] + permissions: [contents:write] + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), nil) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "caller-scoped-profile", + Type: profile.ProfileTypeOrg, + } + + ctx := createTestClaimsContextWithPipeline("agent-workflows") + result := v(ctx, ref, "", "") + assertVendorFailure(t, result, "requires a repository scope") +} + +func TestOrgVendor_ScopeProvidedToNonScopedProfile(t *testing.T) { + v := vendor.NewOrgVendor(profiletest.DefaultTestProfileStore(t), nil) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "non-default-profile", + Type: profile.ProfileTypeOrg, + } + + ctx := createTestClaimsContext() + result := v(ctx, ref, "", "unwanted-scope") + assertVendorFailure(t, result, "does not accept repository scoping") +} + +func TestOrgVendor_ScopeProvidedToAllReposProfile(t *testing.T) { + profileYAML := ` +organization: + profiles: + - name: all-repos-profile + repositories: ["{{all-repositories}}"] + permissions: [contents:read] +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), nil) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "all-repos-profile", + Type: profile.ProfileTypeOrg, + } + + ctx := createTestClaimsContext() + result := v(ctx, ref, "", "unwanted-scope") + assertVendorFailure(t, result, "does not accept repository scoping") +} +``` + +Add the helper: +```go +func createTestClaimsContextWithPipeline(pipelineSlug string) context.Context { + claims := &jwt.BuildkiteClaims{ + OrganizationSlug: "organization-slug", + PipelineSlug: pipelineSlug, + PipelineID: "pipeline-123", + BuildNumber: 1, + } + return jwt.ContextWithBuildkiteClaims(context.Background(), claims) +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/vendor/ -run "TestOrgVendor_CallerScoped|TestOrgVendor_ScopeProvided" -v` +Expected: FAIL — scoping validation not implemented + +- [ ] **Step 3: Implement bidirectional scoping validation in org vendor** + +In `internal/vendor/orgvendor.go`, add scoping validation after the match evaluation and before the repository check. Replace the section from the match evaluation through to token vending (approximately lines 50–76) with: + +```go + // --- Bidirectional scoping validation --- + repoScope := authProfile.Attrs.RepositoryScope() + + if repoScope.IsCallerScoped() { + // Profile requires caller to supply a repository name. + if repositoryScope == "" { + return NewVendorFailed(profile.RepositoryScopeRequiredError{ProfileName: ref.Name}) + } + // Narrow the scope to the single caller-supplied repository. + repoScope = profile.NewSpecificScope(repositoryScope) + } else if repositoryScope != "" { + // Caller supplied a scope but the profile doesn't accept one. + return NewVendorFailed(profile.RepositoryScopeUnexpectedError{ProfileName: ref.Name}) + } + + // The repository is only supplied for the git-credentials endpoint: + // checking it allows Git to respond properly: it's not a security measure. + if requestedRepoURL != "" { + repo, err := url.Parse(requestedRepoURL) + if err != nil { + return NewVendorFailed(fmt.Errorf("could not parse requested repo URL %s: %w", requestedRepoURL, err)) + } + + _, repository := github.RepoForURL(*repo) + + if repoScope.IsWildcard() || repoScope.IsCallerScoped() { + // Wildcard and caller-scoped profiles claim coverage of all repos. + // Failure is a hard error, not a credential helper fallback. + } else if !repoScope.Contains(repository) { + slog.Debug("profile doesn't support requested repository: no token vended.", + "organization", ref.Organization, + "profile", ref.ShortString(), + "requestedRepo", requestedRepoURL, + ) + return NewVendorUnmatched() + } + } + + // Use the GitHub API to vend a token for the repository + token, expiry, err := tokenVendor(ctx, repoScope.Names, authProfile.Attrs.Permissions) +``` + +Wait — the `repoScope` for caller-scoped is now a `NewSpecificScope(repositoryScope)`, so `repoScope.IsCallerScoped()` is false in the git-credentials check. The logic should instead check whether the *profile's* scope is caller-scoped or wildcard. Let me correct: + +```go + // --- Bidirectional scoping validation --- + profileScope := authProfile.Attrs.RepositoryScope() + + var repoScope profile.RepositoryScope + + if profileScope.IsCallerScoped() { + if repositoryScope == "" { + return NewVendorFailed(profile.RepositoryScopeRequiredError{ProfileName: ref.Name}) + } + repoScope = profile.NewSpecificScope(repositoryScope) + } else if repositoryScope != "" { + return NewVendorFailed(profile.RepositoryScopeUnexpectedError{ProfileName: ref.Name}) + } else { + repoScope = profileScope + } + + // The repository is only supplied for the git-credentials endpoint: + // checking it allows Git to respond properly: it's not a security measure. + if requestedRepoURL != "" { + repo, err := url.Parse(requestedRepoURL) + if err != nil { + return NewVendorFailed(fmt.Errorf("could not parse requested repo URL %s: %w", requestedRepoURL, err)) + } + + _, repository := github.RepoForURL(*repo) + + // Profiles that claim coverage of all repositories (wildcard or caller-scoped) + // treat failure as a hard error — no credential helper fallback. + // Static-list profiles return unmatched for repos outside their list. + if !profileScope.IsWildcard() && !profileScope.IsCallerScoped() && !repoScope.Contains(repository) { + slog.Debug("profile doesn't support requested repository: no token vended.", + "organization", ref.Organization, + "profile", ref.ShortString(), + "requestedRepo", requestedRepoURL, + ) + return NewVendorUnmatched() + } + } + + // Use the GitHub API to vend a token + token, expiry, err := tokenVendor(ctx, repoScope.Names, authProfile.Attrs.Permissions) +``` + +The rest of the function (success path, ProfileToken construction) uses `repoScope` which is now correctly narrowed for caller-scoped profiles. + +- [ ] **Step 4: Wire repository-scope extraction in handlePostToken for org routes** + +In `handlers.go`, update `handlePostToken` to extract the scope for org profiles: + +```go +func handlePostToken(tokenVendor vendor.ProfileTokenVendor, expectedType profile.ProfileType) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer drainRequestBody(r) + + ref, err := buildProfileRef(r, expectedType) + if err != nil { + requestError(r.Context(), w, http.StatusBadRequest, fmt.Errorf("invalid profile parameter: %w", err)) + return + } + + var repositoryScope string + if expectedType == profile.ProfileTypeOrg { + repositoryScope, err = extractRepositoryScope(r) + if err != nil { + requestError(r.Context(), w, http.StatusBadRequest, fmt.Errorf("invalid repository-scope: %w", err)) + return + } + } + + result := tokenVendor(r.Context(), ref, "", repositoryScope) +``` + +- [ ] **Step 5: Run tests** + +Run: `go test ./internal/vendor/ ./. -v` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add handlers.go handlers_test.go internal/vendor/orgvendor.go internal/vendor/orgvendor_test.go +git commit -m "$(cat <<'EOF' +feat: implement bidirectional repository scoping validation + +The org vendor now enforces strict bidirectional scoping rules: +- caller-scoped profiles require a repository-scope parameter +- non-scoped profiles reject a repository-scope parameter +- all-repositories profiles reject a repository-scope parameter + +The handler extracts the repository-scope query parameter for org +token requests and passes it through to the vendor chain. +EOF +)" +``` + +--- + +## Task 7: Git-credentials endpoint — caller-scoped and all-repositories behaviour + +**Spec refs:** Req 3.1–3.3, 4.1 + +**Files:** +- Modify: `internal/vendor/orgvendor.go` +- Modify: `internal/vendor/orgvendor_test.go` +- Modify: `handlers.go` +- Modify: `handlers_test.go` + +- [ ] **Step 1: Write failing tests for git-credentials caller-scoped behaviour** + +Add to `internal/vendor/orgvendor_test.go`: + +```go +func TestOrgVendor_GitCredentials_CallerScoped_DerivesRepoFromURL(t *testing.T) { + vendedDate := time.Date(1970, 1, 1, 0, 0, 10, 0, time.UTC) + + var capturedRepoNames []string + tokenVendor := vendor.TokenVendor(func(ctx context.Context, repoNames []string, scopes []string) (string, time.Time, error) { + capturedRepoNames = repoNames + return "scoped-token", vendedDate, nil + }) + + profileYAML := ` +organization: + profiles: + - name: caller-scoped-profile + repositories: ["{{caller-scoped-repository}}"] + permissions: [contents:write] + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), tokenVendor) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "caller-scoped-profile", + Type: profile.ProfileTypeOrg, + } + + // Git-credentials passes requestedRepoURL, not repositoryScope + ctx := createTestClaimsContextWithPipeline("agent-workflows") + result := v(ctx, ref, "https://github.com/org/target-repo", "") + + assertVendorSuccess(t, result, vendor.ProfileToken{ + Token: "scoped-token", + HashedToken: vendor.HashToken("scoped-token"), + Repositories: profile.NewSpecificScope("target-repo"), + Permissions: []string{"contents:write", "metadata:read"}, + Profile: "org:caller-scoped-profile", + Expiry: vendedDate, + OrganizationSlug: "organization-slug", + VendedRepositoryURL: "https://github.com/org/target-repo", + }) + assert.Equal(t, []string{"target-repo"}, capturedRepoNames) +} + +func TestOrgVendor_GitCredentials_CallerScoped_NoRepoURL_Fails(t *testing.T) { + profileYAML := ` +organization: + profiles: + - name: caller-scoped-profile + repositories: ["{{caller-scoped-repository}}"] + permissions: [contents:write] + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), nil) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "caller-scoped-profile", + Type: profile.ProfileTypeOrg, + } + + // Neither repositoryScope nor requestedRepoURL provided + ctx := createTestClaimsContextWithPipeline("agent-workflows") + result := v(ctx, ref, "", "") + assertVendorFailure(t, result, "requires a repository scope") +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/vendor/ -run "TestOrgVendor_GitCredentials_CallerScoped" -v` +Expected: FAIL — git-credentials path doesn't derive scope from URL + +- [ ] **Step 3: Implement git-credentials caller-scoped derivation in org vendor** + +Update the scoping validation in `internal/vendor/orgvendor.go` to derive the repository scope from the requested URL when the profile is caller-scoped and `repositoryScope` is empty but `requestedRepoURL` is present: + +```go + if profileScope.IsCallerScoped() { + if repositoryScope != "" { + repoScope = profile.NewSpecificScope(repositoryScope) + } else if requestedRepoURL != "" { + // Git-credentials path: derive scope from the Git-supplied repository + repo, err := url.Parse(requestedRepoURL) + if err != nil { + return NewVendorFailed(fmt.Errorf("could not parse requested repo URL %s: %w", requestedRepoURL, err)) + } + _, repository := github.RepoForURL(*repo) + repoScope = profile.NewSpecificScope(repository) + } else { + return NewVendorFailed(profile.RepositoryScopeRequiredError{ProfileName: ref.Name}) + } + } else if repositoryScope != "" { + return NewVendorFailed(profile.RepositoryScopeUnexpectedError{ProfileName: ref.Name}) + } else { + repoScope = profileScope + } +``` + +Since the repo URL is now parsed in the scoping block for caller-scoped profiles, the subsequent git-credentials URL check must not re-parse or re-check — the repo is already validated. Update the git-credentials repo check to skip when the profile was caller-scoped (the scope was derived from the same URL): + +```go + if requestedRepoURL != "" && !profileScope.IsCallerScoped() { +``` + +- [ ] **Step 4: Write test for git-credentials 403 on failure for all-repositories and caller-scoped** + +The vendor itself returns `NewVendorFailed` when GitHub rejects the token. The handler converts failures to HTTP errors. The spec says both `{{caller-scoped-repository}}` and `{{all-repositories}}` profiles must return 403 on failure rather than empty-success. This is already the behaviour because: +1. Caller-scoped profiles never reach the `NewVendorUnmatched()` path (the scoping block handles all cases). +2. All-repositories profiles have `profileScope.IsWildcard() == true`, so the `!profileScope.IsWildcard()` check prevents the unmatched path. + +Write a test confirming this: + +```go +func TestOrgVendor_GitCredentials_AllRepos_NoUnmatched(t *testing.T) { + tokenVendor := vendor.TokenVendor(func(ctx context.Context, repoNames []string, scopes []string) (string, time.Time, error) { + return "", time.Time{}, errors.New("GitHub API rejected request") + }) + + profileYAML := ` +organization: + profiles: + - name: all-repos-profile + repositories: ["{{all-repositories}}"] + permissions: [contents:read] +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), tokenVendor) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "all-repos-profile", + Type: profile.ProfileTypeOrg, + } + + ctx := createTestClaimsContext() + result := v(ctx, ref, "https://github.com/org/any-repo", "") + + // Must be a failure, not an unmatched (empty-success) + assertVendorFailure(t, result, "GitHub API rejected request") +} +``` + +- [ ] **Step 5: Run all vendor tests** + +Run: `go test ./internal/vendor/ -v` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add internal/vendor/orgvendor.go internal/vendor/orgvendor_test.go +git commit -m "$(cat <<'EOF' +feat: git-credentials endpoint derives scope from request URL + +For caller-scoped profiles at the git-credentials endpoint, the +repository scope is derived from the Git-supplied URL in the request +body. No new plugin parameters are needed. + +Both caller-scoped and all-repositories profiles return hard errors +(not empty-success) on token issuance failure, since they claim +coverage of all repositories. +EOF +)" +``` + +--- + +## Task 8: Cache key includes repository name for caller-scoped profiles + +**Spec refs:** Req 8.1–8.3 + +**Files:** +- Modify: `internal/vendor/cached.go` +- Modify: `internal/vendor/cached_test.go` + +- [ ] **Step 1: Write failing tests for caller-scoped cache key behaviour** + +Add to `internal/vendor/cached_test.go`: + +```go +func TestCacheCallerScoped_DifferentReposAreSeparateEntries(t *testing.T) { + wrapped := sequenceVendor("token-for-repo-a", "token-for-repo-b") + + c := newTestCached(t, defaultTTL, "test-digest") + v := c(wrapped) + + ref := profile.ProfileRef{ + Organization: "org", + Name: "scoped-profile", + Type: profile.ProfileTypeOrg, + } + + // First call for repo-a: cache miss + result := v(context.Background(), ref, "", "repo-a") + token, ok := result.Token() + require.True(t, ok) + assert.Equal(t, "token-for-repo-a", token.Token) + + // Second call for repo-b: must also miss (different cache key) + result = v(context.Background(), ref, "", "repo-b") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "token-for-repo-b", token.Token) + + // Third call for repo-a: cache hit (returns first token) + result = v(context.Background(), ref, "", "repo-a") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "token-for-repo-a", token.Token) +} + +func TestCacheAllRepositories_SameKeyAsWildcard(t *testing.T) { + wrapped := sequenceVendor("first-call", "should-not-be-called") + + c := newTestCached(t, defaultTTL, "test-digest") + v := c(wrapped) + + ref := profile.ProfileRef{ + Organization: "org", + Name: "all-repos-profile", + Type: profile.ProfileTypeOrg, + } + + // First call: cache miss + result := v(context.Background(), ref, "", "") + token, ok := result.Token() + require.True(t, ok) + assert.Equal(t, "first-call", token.Token) + + // Second call: cache hit (same key, no repository scope component) + result = v(context.Background(), ref, "", "") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "first-call", token.Token) +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/vendor/ -run "TestCacheCallerScoped|TestCacheAllRepositories" -v` +Expected: FAIL — cache key doesn't include repository scope + +- [ ] **Step 3: Implement cache key extension** + +In `internal/vendor/cached.go`, update the cache key construction to include the repository scope when present: + +```go + // Cache key includes digest prefix for config version namespacing + key := fmt.Sprintf("%s:%s", digester.Digest(), ref.String()) + if repositoryScope != "" { + key = fmt.Sprintf("%s:%s:%s", digester.Digest(), ref.String(), repositoryScope) + } +``` + +- [ ] **Step 4: Run all cached vendor tests** + +Run: `go test ./internal/vendor/ -run "TestCache" -v` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add internal/vendor/cached.go internal/vendor/cached_test.go +git commit -m "$(cat <<'EOF' +feat: include repository name in cache key for caller-scoped profiles + +When a repositoryScope is present, the cache key becomes +{digest}:{profile-ref}:{repository-name}. This ensures tokens for +different repositories under the same caller-scoped profile are cached +independently. + +All-repositories profiles continue to use the two-component key since +a single token covers all repositories. +EOF +)" +``` + +--- + +## Task 9: Error handling — generic 403 for GitHub API rejections + +**Spec refs:** Req 7.1–7.2 + +**Files:** +- Modify: `internal/vendor/orgvendor.go` +- Modify: `internal/vendor/orgvendor_test.go` + +- [ ] **Step 1: Write failing test for generic error on GitHub API rejection** + +Add to `internal/vendor/orgvendor_test.go`: + +```go +func TestOrgVendor_CallerScoped_GitHubRejection_Returns403(t *testing.T) { + tokenVendor := vendor.TokenVendor(func(ctx context.Context, repoNames []string, scopes []string) (string, time.Time, error) { + return "", time.Time{}, errors.New("resource not accessible by integration") + }) + + profileYAML := ` +organization: + profiles: + - name: caller-scoped-profile + repositories: ["{{caller-scoped-repository}}"] + permissions: [contents:write] + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), tokenVendor) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "caller-scoped-profile", + Type: profile.ProfileTypeOrg, + } + + ctx := createTestClaimsContextWithPipeline("agent-workflows") + result := v(ctx, ref, "", "nonexistent-repo") + + err, failed := result.Failed() + require.True(t, failed) + + // The error must implement HTTPStatuser and return 403 + var statuser HTTPStatuser + require.ErrorAs(t, err, &statuser) + status, _ := statuser.Status() + assert.Equal(t, http.StatusForbidden, status) + + // The error message must NOT reveal why GitHub rejected the request + assert.NotContains(t, err.Error(), "resource not accessible") +} +``` + +Note: You'll need to either add `HTTPStatuser` interface import or define it in the test — it's already defined in `handlers.go`. Since the vendor tests are in `vendor_test` package, define a local interface: +```go +type HTTPStatuser interface { + Status() (int, string) +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/vendor/ -run "TestOrgVendor_CallerScoped_GitHubRejection" -v` +Expected: FAIL — error doesn't implement HTTPStatuser with 403 + +- [ ] **Step 3: Add a token issuance error type and wrap GitHub rejections** + +Add to `internal/profile/config.go`: + +```go +// TokenIssuanceError wraps a GitHub API rejection with a generic 403 response. +// The underlying cause is available for audit logging but not exposed in the +// HTTP response. +type TokenIssuanceError struct { + ProfileName string + Cause error +} + +func (e TokenIssuanceError) Error() string { + return fmt.Sprintf("token issuance failed for profile %q: %v", e.ProfileName, e.Cause) +} + +func (e TokenIssuanceError) Unwrap() error { + return e.Cause +} + +func (e TokenIssuanceError) Status() (int, string) { + return http.StatusForbidden, "token request denied" +} +``` + +In `internal/vendor/orgvendor.go`, wrap the token vendor error for caller-scoped and all-repositories profiles: + +```go + token, expiry, err := tokenVendor(ctx, repoScope.Names, authProfile.Attrs.Permissions) + if err != nil { + if profileScope.IsCallerScoped() || profileScope.IsWildcard() { + return NewVendorFailed(profile.TokenIssuanceError{ProfileName: ref.Name, Cause: err}) + } + return NewVendorFailed(fmt.Errorf("could not issue token for profile %s: %w", ref, err)) + } +``` + +- [ ] **Step 4: Run tests** + +Run: `go test ./internal/vendor/ -run "TestOrgVendor_CallerScoped_GitHubRejection" -v` +Expected: PASS + +- [ ] **Step 5: Run full test suite** + +Run: `go test ./...` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add internal/profile/config.go internal/vendor/orgvendor.go internal/vendor/orgvendor_test.go +git commit -m "$(cat <<'EOF' +feat: return generic 403 for GitHub API rejections on scoped profiles + +When the GitHub API rejects a token request for caller-scoped or +all-repositories profiles, the bridge returns a generic 403 "token +request denied" rather than exposing the underlying error. This +prevents information leakage about whether a repository exists. + +The full GitHub error is preserved in the error chain for audit +logging via TokenIssuanceError.Unwrap(). +EOF +)" +``` + +--- + +## Task 10: Audit logging for scoping mismatches + +**Spec refs:** Req 9.2 + +**Files:** +- Modify: `internal/vendor/auditvendor.go` +- Modify: `internal/vendor/auditvendor_test.go` + +- [ ] **Step 1: Write failing tests for audit logging of scoping mismatches** + +Add to `internal/vendor/auditvendor_test.go`: + +```go +func TestAuditor_RecordsScopingMismatchError(t *testing.T) { + tests := []struct { + name string + vendorError error + expectedAudit string + }{ + { + name: "scope provided to non-scoped profile", + vendorError: profile.RepositoryScopeUnexpectedError{ProfileName: "static-profile"}, + expectedAudit: "does not accept repository scoping", + }, + { + name: "scope missing for caller-scoped profile", + vendorError: profile.RepositoryScopeRequiredError{ProfileName: "scoped-profile"}, + expectedAudit: "requires a repository scope", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inner := vendor.ProfileTokenVendor(func(ctx context.Context, ref profile.ProfileRef, repo string, repositoryScope string) vendor.VendorResult { + return vendor.NewVendorFailed(tt.vendorError) + }) + + auditor := vendor.Auditor(inner) + + ctx, entry := audit.Context(context.Background()) + ref := profile.ProfileRef{Organization: "org", Name: "test", Type: profile.ProfileTypeOrg} + auditor(ctx, ref, "", "") + + assert.Contains(t, entry.Error, tt.expectedAudit) + }) + } +} +``` + +- [ ] **Step 2: Run test** + +Run: `go test ./internal/vendor/ -run "TestAuditor_RecordsScopingMismatchError" -v` +Expected: The existing audit error recording in `Auditor` already captures `vendor failure: ` for any failed result. The scoping error types produce descriptive messages, so the audit log will contain them. This test should PASS with the existing code. + +If it passes, no code changes are needed — the existing audit infrastructure handles scoping errors correctly. The error types from Task 5 produce messages like "profile X does not accept repository scoping" which flow naturally through the audit vendor's `entry.Error = fmt.Sprintf("vendor failure: %v", err)`. + +- [ ] **Step 3: Commit (if test changes were needed)** + +If tests pass without changes, skip this commit. Otherwise: + +```bash +git add internal/vendor/auditvendor.go internal/vendor/auditvendor_test.go +git commit -m "$(cat <<'EOF' +test: verify audit logging captures scoping mismatch errors +EOF +)" +``` + +--- + +## Task 11: Integration tests + +**Spec refs:** Cross-cutting validation + +**Files:** +- Modify: `api_integration_test.go` + +- [ ] **Step 1: Identify integration test patterns** + +Read `api_integration_test.go` to understand the `APITestHarness` pattern, how requests are made, and how assertions work. + +- [ ] **Step 2: Write integration tests for the token endpoint** + +Add integration tests covering: +- Caller-scoped profile with valid `repository-scope` query parameter → 200 with scoped token +- Caller-scoped profile without `repository-scope` → error response +- Static-list profile with unexpected `repository-scope` → error response +- All-repositories profile with `repository-scope` → error response +- `repository-scope` containing `/` → 400 + +- [ ] **Step 3: Write integration tests for git-credentials endpoint** + +Add integration tests covering: +- Caller-scoped profile at git-credentials → derives repo from request body, returns credentials +- All-repositories profile at git-credentials → returns credentials as before + +- [ ] **Step 4: Run integration tests** + +Run: `make integration` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add api_integration_test.go +git commit -m "$(cat <<'EOF' +test: add integration tests for dynamic repository scoping + +Cover caller-scoped and all-repositories profiles at both the token +and git-credentials endpoints, including bidirectional validation +and input rejection. +EOF +)" +``` + +--- + +## Task 12: Final validation — make agent + +**Files:** None (validation only) + +- [ ] **Step 1: Run make agent** + +Run: `make agent` +Expected: PASS — build, format, lint, and all tests pass + +- [ ] **Step 2: Run integration tests** + +Run: `make integration` +Expected: PASS + +- [ ] **Step 3: Review git log** + +Run: `git log --oneline main..HEAD` +Review that all commits are present and well-structured. + +--- + +## Deferred Tasks (out of scope for this plan) + +These are explicitly out of scope per the spec but noted for future work: + +- **JSON schema publication** (Req 12.1) — publish after feature release +- **Chinmina-token plugin changes** (Req 10.1–10.3) — separate repository +- **Chinmina-git-credentials plugin** (Req 11.1) — no changes needed +- **Documentation updates** — separate documentation repository +- **Removal of `*` in v1** (Req 13.2) — tracked separately diff --git a/docs/superpowers/plans/2026-04-22-profileref-scoped-repository-refactor.md b/docs/superpowers/plans/2026-04-22-profileref-scoped-repository-refactor.md new file mode 100644 index 00000000..0f9c2cdf --- /dev/null +++ b/docs/superpowers/plans/2026-04-22-profileref-scoped-repository-refactor.md @@ -0,0 +1,212 @@ +# Plan: ProfileRef-carried Scope Refactor + +> Source PRD: Derived from code review of PR #297 (dynamic repository scoping) and subsequent design discussion. Resolves review findings 1 (scope priority) and 2 (cache behaviour) by encoding the scoped repository into `ProfileRef`. + +## Architectural decisions + +Durable decisions that apply across all phases: + +- **URN format (full)**: scoped org refs render as `profile://organization/{org}/profile/{name}/repository/{repo}`. Non-scoped refs unchanged. Repo refs are never scoped. +- **URN format (short)**: scoped org refs render as `org:{name}/{repo}`. Non-scoped refs unchanged (`org:{name}` / `repo:{name}`). The `/` separator is safe because repo-scope values reject `/` (`extractRepositoryScope`) and profile names do not contain `/`. +- **Scope resolution location**: handler boundary. The `profile` package stays free of request-level concerns; the `vendor` package receives refs with scope already resolved. +- **Handler flow ordering**: repository resolution runs *before* ref construction in both handlers. `/organization/token` extracts `?repository-scope=`, validates, and constructs a URL form of it. `/organization/git-credentials` reads the body and constructs the URL from the Git properties. Both handlers then call the builder with the resolved repository name. The URL is also retained by the handler for passing to the vendor (`requestedRepoURL` role — response URL, static-list matching). +- **`PathValuer` interface** (single-function): `type PathValuer interface { PathValue(name string) string }`. `*http.Request` satisfies this directly. Defined in the handler package. Replaces the need to pass `*http.Request` into ref-building code; keeps the builder free of HTTP concerns. +- **ProfileRefBuilder contract**: `type ProfileRefBuilder func(ctx context.Context, pv PathValuer, scopedRepo string) (profile.ProfileRef, error)`. Builders are closures over the profile store, injected per route at wiring time. Claims come from context via existing `jwt.RequireBuildkiteClaimsFromContext`. Because URL/scope resolution happens in the handler before this is called, the builder's inputs are already normalised. +- **Scope source per endpoint** (resolved in handler, validated in builder): + - `/organization/token/{profile}`: `?repository-scope=` query parameter + - `/organization/git-credentials/{profile}`: repository name derived from the Git-supplied URL in the request body + - Pipeline endpoints: never scoped +- **Trust model** (captured in code comments, not just spec): `ScopedRepository` narrows within an already-authorised profile. It does not grant access. The profile's match rules gate who may invoke a profile at all, and GitHub is the final enforcement boundary for whether a specific repository is reachable. This informs why caller-supplied scope is acceptable to honour without additional cross-checks. +- **Bidirectional validation stays**: Req 2.2 (scope on non-caller-scoped profile → reject) and 5.2 (scope on all-repositories profile → reject) are kept and enforced at the handler boundary, now trivially because the handler has the profile type from its store lookup. +- **Vendor signature endpoint**: after Phase 4, `ProfileTokenVendor = func(ctx, ref, requestedRepoURL) VendorResult`. `requestedRepoURL` retains two roles — git-credentials response URL and static-list profile repo-matching — neither of which concerns scope resolution. +- **Cache key derivation**: unchanged in shape — `{digest}:{ref.String()}`. Correctness for caller-scoped per-repo entries emerges naturally from the ref including `ScopedRepository`. The `repositoryScope != ""` branch in `cached.go` is removed. + +--- + +## Phase 0: Baseline anchor + +Not a refactor phase — a pre-condition. Captures the reference behaviour baseline that Phase 4's verification step diffs against. + +### What to do + +- Record the branch-tip commit hash. +- Run `make agent` and `make integration`; confirm both pass at that commit. +- Capture representative integration test outputs (one per profile type × endpoint combination — six scenarios) as the "pre-refactor baseline": request, response body, response status, audit log entry. Store as text files under `.beans/tmp/` or equivalent scratch area for later diff. + +No code changes. This exists so that "behaviour preserved" is verifiable, not asserted. + +--- + +## Phase 1: ProfileRef carries scoped-repository + +**User stories**: Goals 1, 5. Foundational — no functional change yet. + +### What to build + +Extend `ProfileRef` with an optional `ScopedRepository string` field. The URN rendering (`String()`), short form (`ShortString()`), parser (`ParseProfileRef`) and constructor (`NewProfileRef` or a companion) all understand the new field. Non-scoped refs render and parse exactly as before — existing URN strings remain valid and roundtrip unchanged. + +The field is populated only by caller-supplied input in later phases. In this phase, no production code constructs a scoped ref. + +Confirm (don't add) that profile-name validation in `internal/profile/compilation.go` already rejects `/` and `:` in names — this is prerequisite for unambiguous URN parsing and is enforced elsewhere, not in `ref.go`. + +### Acceptance criteria + +- [ ] `[observable]` `ProfileRef{Type: Org, Organization: "o", Name: "p", ScopedRepository: "r"}.String()` renders `profile://organization/o/profile/p/repository/r` +- [ ] `[observable]` `ShortString()` for the same ref renders `org:p/r`; non-scoped ref unchanged +- [ ] `[observable]` `ParseProfileRef` roundtrips all four URN variants: org non-scoped, org scoped, repo new-format, repo old-format (backward compat) +- [ ] `[observable]` `ParseProfileRef` rejects malformed scoped URNs: trailing empty (`…/profile/p/repository/`), extra segments (`…/repository/r/extra`), scope on a pipeline ref (`…/pipeline/…/repository/r`) +- [ ] `[structural]` `NewProfileRef` (or a companion constructor) accepts a scoped-repository parameter; zero-value input produces a non-scoped ref +- [ ] `[structural]` Confirmed via grep / existing tests that profile-name validation already rejects `/` and `:`; no new constraint added in `ref.go` +- [ ] `[observable]` `make agent` passes with no regressions + +### Verification + +Run `go test ./internal/profile/...` and `make agent`. Manually inspect URN output from new unit tests to confirm the suffix is only present when `ScopedRepository` is non-empty. + +--- + +## Phase 2a: Handler restructure + `ProfileRefBuilder` wiring + +**User stories**: Goals 2, 3 (plumbing only — validation in 2b). + +**Carry-forward**: Verify Phase 0 and 1 before starting. + +### What to build + +Two concurrent changes: + +1. **Handler flow reorder.** Repository resolution runs *before* ref construction. + - `handlePostToken` first calls `extractRepositoryScope(r)` to obtain a repo name (or `""`). No behaviour change — just moved earlier. + - `handlePostGitCredentials` reads the body (`credentialhandler.ReadProperties`, `ConstructRepositoryURL`) first. From the URL, derive the repo name. No behaviour change. + +2. **`ProfileRefBuilder` + `PathValuer` introduction.** Define `PathValuer` (single-function interface) and `ProfileRefBuilder` in the handler package. Provide a default builder implementation, a closure over `*profile.ProfileStore` + expected `ProfileType`, that currently replicates today's `buildProfileRef` behaviour — ignoring the `scopedRepo` argument. Wire a distinct builder per route in `main.go`. + +Result: the handler now has a clean ordering (resolve repo → build ref → call vendor) and the ref-building call site is one injected function. No new validation, no scope flowing into the ref yet. + +Update handler unit tests to inject a simple test builder (usually an inline closure) instead of threading path/claim scaffolding. Integration tests pass unchanged. + +### Acceptance criteria + +- [ ] `[structural]` `PathValuer` interface defined as `interface { PathValue(string) string }` in the handler package; `*http.Request` satisfies it implicitly +- [ ] `[structural]` `ProfileRefBuilder` type defined as `func(ctx, PathValuer, scopedRepo string) (ProfileRef, error)`; handler signatures accept one +- [ ] `[structural]` Handler flow: both handlers resolve repo (scope param / body URL) *before* calling the builder +- [ ] `[observable]` `main.go` wires one builder per route (4 routes: org token, org git-credentials, pipeline token, pipeline git-credentials); builders are closures over `*profile.ProfileStore` + expected `ProfileType` +- [ ] `[observable]` All existing handler unit tests pass (possibly with mechanical test-setup updates) +- [ ] `[observable]` `make integration` passes — no behavioural change end-to-end +- [ ] `[observable]` Handler tests can now swap in a test builder without constructing a full `*http.Request` or profile store (demonstrates testability improvement) + +### Verification + +Run `go test ./... && make integration`. End-to-end behaviour identical: same status codes, same token responses, same audit entries. Confirm via diff against Phase 0 baseline outputs for at least the org-token and org-git-credentials happy paths. + +--- + +## Phase 2b: Scope validation in builder + +**User stories**: Reqs 2.1, 2.2, 2.3, 5.2, 6.1, 6.2, 9.2 (a–c). + +**Carry-forward**: Verify Phase 1 and 2a before starting. + +### What to build + +Add profile-type-aware validation to the builder. The `scopedRepo` argument passed by the handler is now consumed: + +1. Builder looks up the profile from the store (closure-scoped). +2. If profile is caller-scoped and `scopedRepo == ""` → return `RepositoryScopeRequiredError` (Req 2.3). +3. If profile is caller-scoped and `scopedRepo != ""` → populate `ref.ScopedRepository`. +4. If profile is *not* caller-scoped and `scopedRepo != ""` → return `RepositoryScopeUnexpectedError` (Reqs 2.2, 5.2). +5. If profile is not caller-scoped and `scopedRepo == ""` → leave `ref.ScopedRepository` empty (status quo). + +Handler input validation (format of `?repository-scope=`, empty/whitespace/slash — Reqs 6.1, 6.2) stays in `extractRepositoryScope` at the handler boundary — the builder trusts its inputs syntactically. + +Add code comments on the builder capturing the trust model: scope narrows within an authorised profile, doesn't grant access, GitHub is the backstop. (The spec document is not the long-term home for this reasoning.) + +The vendor still receives the legacy `repositoryScope` parameter in this phase — keep handlers passing it so vendor behaviour is unchanged. The ref carries the same value redundantly until Phase 4 removes the parameter. This dual-source-of-truth window is accepted (Phase 4 is a fast follow). + +Audit log entries now include `ScopedRepository` via `ref.String()` (Req 9.2): no explicit audit code change needed. + +### Acceptance criteria + +- [ ] `[observable]` `/organization/token/{profile}?repository-scope=repo-a` against a caller-scoped profile produces a token with `Repositories = {repo-a}` and audit logs show scoped URN +- [ ] `[observable]` `/organization/token/{profile}?repository-scope=repo-a` against a static-list profile returns 400 with message indicating the profile does not accept scoping (Req 2.2) +- [ ] `[observable]` Same request against an all-repositories profile returns 400 (Req 5.2) +- [ ] `[observable]` `/organization/token/{profile}` with no scope against a caller-scoped profile returns 400 "requires a repository scope" (Req 2.3) +- [ ] `[observable]` `/organization/git-credentials/{profile}` against a caller-scoped profile parses the body URL and produces a token scoped to the Git-supplied repo (Reqs 3.2, 3.3) +- [ ] `[observable]` `repository-scope` values with `/` or whitespace-only return 400 unchanged (Reqs 6.1, 6.2) +- [ ] `[observable]` Audit entries for caller-scoped requests include the scoped repo name in the profile URN field +- [ ] `[observable]` `make agent` and `make integration` pass + +### Verification + +Run full test suite. Add/update handler unit tests for each rejection case. Integration tests exercise the happy paths. Manually inspect an audit log entry from a caller-scoped integration test to confirm URN shape. + +--- + +## Phase 3: Cache behaviour verification and cleanup + +**User stories**: Reqs 8.1, 8.2. Resolves review finding 2. + +**Carry-forward**: Verify Phase 1, 2a, 2b before starting. + +### What to build + +With `ScopedRepository` now flowing into the ref, the cache key `{digest}:{ref.String()}` is already correct per-repo for caller-scoped profiles at both endpoints — this is the structural win. Prove it with tests, then remove the now-redundant `repositoryScope != ""` key-extension branch in `cached.go`. + +Add cache tests covering both endpoints to close the gap flagged by CodeRabbit (existing test only exercised one path with identical arguments). Vary the repo name and confirm distinct cache entries. + +Static-list and all-repositories cache behaviour must be unchanged: one cache entry per profile regardless of request URL. + +### Acceptance criteria + +- [ ] `[observable]` At `/organization/token`, two caller-scoped requests with different `?repository-scope=` values produce distinct cache entries (second is a miss) +- [ ] `[observable]` Same as above at `/organization/git-credentials` with different body URLs +- [ ] `[observable]` Two identical caller-scoped requests return the same cached token (cache hit) +- [ ] `[observable]` Static-list profile: N distinct request URLs (all within the list) share one cache entry +- [ ] `[observable]` All-repositories profile: cache key identical to current `*` wildcard behaviour (Req 8.3) +- [ ] `[structural]` `repositoryScope != ""` branch removed from `cached.go` +- [ ] `[observable]` `make agent` passes + +### Verification + +Run `go test ./internal/vendor/...` focusing on `cached_test.go`. Visually inspect cache keys in debug logs for a representative set of requests. Confirm no regressions in existing cache hit-rate assertions. + +--- + +## Phase 4: Remove `repositoryScope` from vendor chain + +**User stories**: Goal 4. Resolves review finding 1. + +**Carry-forward**: Verify Phase 1, 2a, 2b, 3 before starting. + +### What to build + +Change `ProfileTokenVendor` to `func(ctx, ref, requestedRepoURL) VendorResult`. Update every implementation — `NewOrgVendor`, `NewRepoVendor`, `Cached`, `Auditor` — to drop the parameter. Update every call site, including handlers (no longer pass scope), tests, and fuzz tests. + +Simplify `resolveRequestScope`: the caller-scoped branch reads `ref.ScopedRepository` directly; the "both non-empty" bug class disappears structurally. Error types (`RepositoryScopeRequiredError`, `RepositoryScopeUnexpectedError`) remain where they're useful — the builder is the primary emitter now, but the vendor keeps them as a defence-in-depth check if the ref is inconsistent (e.g., a caller-scoped profile with empty `ScopedRepository` reaching the vendor). This defence is optional; prefer removing code over keeping redundant guards, and if removed, add a comment noting the builder is the enforcement point. + +Fuzz tests in `orgvendor_fuzz_test.go` are updated for the new signature. The "both parameters present" case no longer exists because the parameter is removed. Do *not* add new fuzz coverage for scope resolution in the builder — unit tests over the builder's profile-type × scope-input matrix are sufficient. Keep fuzzing focused on where it pays off (boundary inputs to `extractRepositoryScope`, URL parsing). + +### Acceptance criteria + +- [ ] `[structural]` `ProfileTokenVendor` has three parameters; grep for `repositoryScope` in the vendor chain returns zero results +- [ ] `[observable]` `resolveRequestScope` simplified; caller-scoped branch reads from the ref +- [ ] `[observable]` All four profile-type × endpoint combinations produce the same tokens as before the refactor: caller-scoped at `/token`, caller-scoped at `/git-credentials`, static-list at both, all-repositories at both +- [ ] `[observable]` Existing fuzz tests updated for the new signature and continue to pass; no new fuzz cases added for scope resolution (unit tests cover this matrix) +- [ ] `[observable]` `make agent` passes +- [ ] `[observable]` `make integration` passes +- [ ] `[structural]` Code comments on the simplified `resolveRequestScope` (or vendor doc) describe the trust model and why single-source scope is safe + +### Verification + +Run `make agent && make integration && go test -fuzz=Fuzz... -fuzztime=30s ./internal/vendor/...`. Diff the response bodies and audit logs of representative integration test runs against the Phase 0 baseline outputs to confirm identical behaviour (token contents, status codes, URN shapes — allowing for URN suffix change in caller-scoped cases). + +--- + +## Out of scope (confirmed non-goals) + +- Moving scope logic into `profile` package +- YAML/profile compilation changes +- `repository-scope` parameter name / transport changes +- Cache TTL, storage, or digest changes +- `*` deprecation behaviour changes +- chinmina-token plugin / CLI changes diff --git a/docs/superpowers/specs/2026-04-15-dynamic-repository-scoping-design.md b/docs/superpowers/specs/2026-04-15-dynamic-repository-scoping-design.md new file mode 100644 index 00000000..e090262b --- /dev/null +++ b/docs/superpowers/specs/2026-04-15-dynamic-repository-scoping-design.md @@ -0,0 +1,275 @@ +# Dynamic Repository Scoping for Chinmina Bridge + +> Source: [GitHub Issue #246](https://github.com/chinmina/chinmina-bridge/issues/246) + +## Problem Statement + +AI coding agent workflows run from a central Buildkite pipeline (e.g. `agent-workflows`) that needs GitHub access to clone, push, open PRs, and comment — but against a different repository each time. Today, Chinmina Bridge requires a separate organization profile per repository, which doesn't scale. Every new target repository requires a new profile entry, a configuration update, and a profile refresh cycle. For workflows that operate across dozens or hundreds of repositories, this is operationally untenable. + +A secondary problem exists with the current wildcard syntax: the `*` entry in the `repositories` list is terse and its meaning is not self-documenting. As the profile schema becomes more expressive, clearer naming is needed. + +## Solution + +Introduce two new YAML literals for the `repositories` field in organization profiles: + +- **`{{caller-scoped-repository}}`** — the caller specifies a single repository at request time. The vended token is narrowed to that repository only. This lets one profile serve requests targeting any repository, one at a time. +- **`{{all-repositories}}`** — replaces the existing `*` wildcard with an unambiguous name. The token grants access to all repositories accessible to the GitHub App installation, as `*` does today. + +The existing `*` wildcard is deprecated. It will continue to function as an alias for `{{all-repositories}}` with a deprecation warning, and will be removed in version 1. + +Match rules continue to control which pipelines may use a profile. The GitHub App installation remains the final gate — tokens cannot grant access to repositories the App is not installed on. There is no pre-validation against the App's installed repository list; the GitHub API is the source of truth and rejects invalid repositories at token creation time. + +> **Note:** Both literals draw on the same underlying access — any repository the GitHub App installation can reach — but differ in what the vended token permits. `{{all-repositories}}` issues a token with full-width access across all reachable repositories. `{{caller-scoped-repository}}` narrows each token to a single repository per request. The distinction is in token breadth, not in underlying App permissions. + +## Examples + +### Profile YAML + +**`{{all-repositories}}`** replaces `*`. The vended token grants access to all repositories the GitHub App installation can reach: + +```yaml +- name: "ci-tools-read" + match: + - claim: "pipeline_slug" + valuePattern: ".*" + repositories: + - "{{all-repositories}}" + permissions: + - "contents:read" +``` + +**`{{caller-scoped-repository}}`** requires the caller to supply a repository name at request time. The vended token is narrowed to that single repository: + +```yaml +- name: "agent-workflows-write" + match: + - claim: "pipeline_slug" + valuePattern: "agent-workflows|agent-workflows-ci" + repositories: + - "{{caller-scoped-repository}}" + permissions: + - "contents:write" +``` + +Both literals must be the only entry in `repositories` — they cannot be combined with each other or with static repository names. + +### Plugin invocation + +Multiple plugin instances in a single Buildkite step are supported. Scoped and unscoped profiles can coexist: + +```yaml +# Token scoped to a specific repo — for profiles using {{caller-scoped-repository}} +- chinmina/chinmina-token#v1.x.x: + environment: + - GITHUB_WRITE_TOKEN=org:agent-workflows-write + - GITHUB_READ_TOKEN=org:agent-workflows-read + repository-scope: hotel + +# Token without scoping — standard profile usage +- chinmina/chinmina-token#v1.x.x: + environment: + - GITHUB_TOKEN=repo:default +``` + +The `repository-scope` value is the repository name only — no owner prefix. It is passed as a query parameter to the Chinmina Bridge organization token endpoint. + +## Requirements + +### 1. Profile schema + +1.1. The profile parser shall accept `{{caller-scoped-repository}}` as a valid entry in an organization profile's `repositories` list. + +1.2. The profile parser shall accept `{{all-repositories}}` as a valid entry in an organization profile's `repositories` list. + +1.3. When `{{caller-scoped-repository}}` appears in a profile's `repositories` list, the profile parser shall reject the profile if any other entries are present in the list. + +1.4. When `{{all-repositories}}` appears in a profile's `repositories` list, the profile parser shall reject the profile if any other entries are present in the list. + +1.5. The profile parser shall accept `*` as a valid entry and treat it as an alias for `{{all-repositories}}`. + +1.6. When a profile containing `*` is loaded, the profile parser shall emit a deprecation warning identifying the profile name and recommending migration to `{{all-repositories}}`. + +1.7. The `{{caller-scoped-repository}}` and `{{all-repositories}}` literals shall only be valid in organization profiles. + +1.8. The profile compiler shall resolve the scoping mode (static list, caller-scoped, or all-repositories) at compile time and store it in `OrganizationProfileAttr` using the `RepositoryScope` type. + +1.9. The `RepositoryScope` type shall be extended with a third state representing caller-supplied access. This state shall be distinct from the wildcard state (`Wildcard: true`) and the specific-names state (`Names: [...]`) — it must not be representable as either. + +1.10. When a profile's `RepositoryScope` is in the caller-scoped state, no repository name shall be stored in the compiled profile. The name shall be supplied only at request time from the `repository-scope` input. + +> **Note:** Extension points are `OrganizationProfileAttr` (`internal/profile/profiles.go`) and `RepositoryScope` (`internal/profile/repositoryscope.go`). `RepositoryScope` currently has two states: `Wildcard bool` and `Names []string`. See *Implementation Decisions — Profile schema representation* for the intended approach. + +### 2. Organization token endpoint — caller-scoped repository + +2.1. When a request to the organization token endpoint includes a `repository-scope` query parameter and the matched profile contains `{{caller-scoped-repository}}`, the bridge shall issue a token scoped to the single repository named in `repository-scope`. + +2.2. If a request to the organization token endpoint includes a `repository-scope` query parameter but the matched profile does not contain `{{caller-scoped-repository}}`, then the bridge shall reject the request with an error indicating the profile does not accept repository scoping. + +2.3. If a request to the organization token endpoint omits the `repository-scope` query parameter but the matched profile contains `{{caller-scoped-repository}}`, then the bridge shall reject the request with an error indicating that a repository scope is required. + +### 3. Organization git-credentials endpoint — caller-scoped repository + +3.1. When a profile uses `{{caller-scoped-repository}}` or `{{all-repositories}}`, the git-credentials endpoint shall treat any token issuance failure as a 403 error response. It shall not return empty-success credentials for these profile types under any failure condition. + +> **Note:** This is the claim-coverage principle: both new literals claim coverage of every repository, so failure means the request cannot be satisfied — there is no credential helper fallback. Static-list org profiles (those whose `repositories` field contains explicit repository names) return empty-success for repositories outside their list, because the profile makes no coverage claim for those repos. See *Implementation Decisions — Git-credentials endpoint behaviour* for the full rationale. + +3.2. When a `{{caller-scoped-repository}}` profile is in use at the organization git-credentials endpoint, the bridge shall derive the repository scope from the Git-supplied repository in the request body. + +3.3. When a `{{caller-scoped-repository}}` profile is in use at the organization git-credentials endpoint, the bridge shall issue a token scoped to the single Git-supplied repository. + +### 4. Organization git-credentials endpoint — all-repositories + +4.1. When an `{{all-repositories}}` profile is in use at the organization git-credentials endpoint and the token request succeeds, the bridge shall return git-credentials output covering all repositories, identical to the existing `*` wildcard behaviour. + +### 5. Organization token endpoint — all-repositories + +5.1. When the matched profile contains `{{all-repositories}}`, the bridge shall issue a token with access to all repositories accessible to the GitHub App installation. + +5.2. If a request to the organization token endpoint includes a `repository-scope` query parameter but the matched profile contains `{{all-repositories}}`, then the bridge shall reject the request with an error indicating the profile does not accept repository scoping. + +### 6. Input validation + +6.1. The bridge shall reject any `repository-scope` value that contains a `/` character. + +6.2. The bridge shall reject any `repository-scope` value that is empty or consists only of whitespace. + +6.3. The bridge shall pass the `repository-scope` value through to the GitHub API without case normalization. + +### 7. Error handling + +7.1. If the GitHub API rejects a token request for a `{{caller-scoped-repository}}` or `{{all-repositories}}` profile, then the bridge shall return a 403 response to the caller with a generic error message that does not reveal whether the repository exists. + +7.2. If the GitHub API rejects a token request for a `{{caller-scoped-repository}}` or `{{all-repositories}}` profile, then the bridge shall record the full GitHub API error details in the audit log. + +### 8. Caching + +8.1. While a `{{caller-scoped-repository}}` profile is in use, the cache layer shall include the scoped repository name in the cache key. + +8.2. While a `{{caller-scoped-repository}}` profile is in use, the cache layer shall treat tokens for different repositories under the same profile as distinct cache entries. + +8.3. While an `{{all-repositories}}` profile is in use, the cache layer shall use the same cache key structure as the current `*` wildcard behaviour. + +> **Note:** "The current `*` wildcard behaviour" means a two-component key `{digest}:{profile-ref}`. This requirement can be read as: for `{{all-repositories}}` profiles, the cache key format is `{digest}:{profile-ref}`, unchanged from the existing wildcard implementation. + +### 9. Observability + +9.1. When a token is issued for a `{{caller-scoped-repository}}` profile, the audit log shall include the scoped repository name. + +> **Note:** The existing audit infrastructure (`internal/vendor/auditvendor.go`) records repositories via `token.Repositories.NamesForDisplay()`. For `{{caller-scoped-repository}}` profiles, the issued token carries a specific-names scope, so this field will contain the single scoped repository name. For `{{all-repositories}}` profiles it will contain `["*"]`, unchanged from current `*` wildcard behaviour. No special handling is required for either case — the existing mechanism is correct. + +9.2. When a token request is rejected due to a scoping mismatch, the audit log shall record which of the following conditions caused the rejection: + + a. A `repository-scope` was provided to a profile that does not use `{{caller-scoped-repository}}` (see Req 2.2). + + b. A `repository-scope` was omitted for a profile that requires `{{caller-scoped-repository}}` (see Req 2.3). + + c. A `repository-scope` was provided to a profile that uses `{{all-repositories}}` (see Req 5.2). + +### 10. Chinmina-token Buildkite plugin + +10.1. The chinmina-token Buildkite plugin shall accept a `repository-scope` configuration parameter. + +10.2. When `repository-scope` is configured, the chinmina-token plugin shall pass the value as a query parameter to the Chinmina Bridge organization token endpoint. + +10.3. The CLI script in the chinmina-token plugin shall accept a repository scope argument and pass it as a query parameter to the Chinmina Bridge organization token endpoint. + +### 11. Chinmina-git-credentials Buildkite plugin + +11.1. The chinmina-git-credentials Buildkite plugin shall require no new parameters for `{{caller-scoped-repository}}` support. + +### 12. JSON schema + +12.1. When the dynamic repository scoping feature is released, a JSON schema for the organization profile YAML structure shall be published in the chinmina-bridge repository. + +> **Note:** The original issue lists additional documentation deliverables — updating the profile reference, deprecation docs for `*`, plugin usage examples, and CLI script docs. These are out of scope for this repository; they are deliverables for the documentation repository and the chinmina-token plugin repository. + +### 13. Deprecation + +13.1. While `*` remains supported, the bridge shall log a deprecation warning on every profile load that contains `*`. + +13.2. The `*` wildcard entry shall be removed in version 1. + +## Implementation Decisions + +### Profile schema representation + +The two new literals are string constants recognised during profile compilation. They are not YAML tags or custom types — they are specific string values in the `repositories` array that the compiler interprets. This means `KnownFields(true)` in the YAML decoder continues to work unchanged; no new YAML fields are introduced. + +The `OrganizationProfileAttr` type currently carries `Repositories` as `[]string` and derives `RepositoryScope()` from content at call time. The scoping mode (static list, caller-scoped, all-repositories) should be resolved at compile time and stored as a typed value. This is a natural extension of the existing `RepositoryScope` type, which already distinguishes between wildcard and specific-name scopes, but needs a third state: "caller will provide at request time." + +### Strict bidirectional validation + +Scoping validation is enforced in both directions: the request must match what the profile expects, and the profile must match what the request provides. This prevents a misconfigured caller from accidentally receiving a broader or narrower token than intended. The bridge rejects mismatches with clear error messages rather than silently ignoring the discrepancy. + +### Error response design + +When the GitHub API rejects a scoped repository (e.g. the App isn't installed on that repo), the bridge returns a 403 with a generic message. It does not reveal whether the repository exists or why it was rejected — that information goes to the audit log only. This prevents information leakage to potential attackers while giving operators full diagnostic detail. + +### No pre-validation against GitHub App installation + +The bridge does not pre-validate that a scoped repository exists in the GitHub App's installed repository list. Pre-validation would require an extra API call and introduce a TOCTOU race condition. The GitHub API is the source of truth and provides a clear rejection at token creation time. + +### Cache key structure + +For `{{caller-scoped-repository}}` profiles, the cache key includes the repository name: `{digest}:{profile-ref}:{repository-name}`. Each repository gets its own cached token, which is correct — a token scoped to `repo-a` cannot be reused for `repo-b`. This design means the existing `checkTokenRepository` mismatch logic is irrelevant for scoped profiles by construction: a request for one repository will never retrieve another repository's cache entry. + +For `{{all-repositories}}` profiles, the existing key format (`{digest}:{profile-ref}`) is sufficient since the token covers all repositories. + +This design means more GitHub API calls under load compared to a single wildcard token (one API call per unique repository instead of one shared token). This is an acceptable trade-off for the security benefit of narrow scoping, and should be documented. + +### No case normalization + +The `repository-scope` value is passed through to the GitHub API without case normalization. GitHub's API is case-insensitive for repository names, so this works correctly without the bridge adding complexity. + +### Query parameter transport + +The `repository-scope` parameter is transported as a query parameter on POST requests to the organization token endpoint. This keeps it visible in access logs and avoids changing the request body format. + +### Git-credentials endpoint behaviour + +The git-credentials endpoint already receives the target repository from Git via the request body. For `{{caller-scoped-repository}}` profiles, this repository becomes the scoping input — no new parameters are needed from the plugin. + +The error model follows a consistent principle across profile types: profiles that claim coverage of a repository treat issuance failure as a real error (403), while profiles that don't claim coverage return an empty success to let Git try another credential helper. Both `{{caller-scoped-repository}}` and `{{all-repositories}}` explicitly claim coverage of all repositories — the difference is only in token breadth, not coverage intent. This matches pipeline profile behaviour, where the profile's claim over the pipeline's repository means failure is always an error. Only static-list org profiles return empty success for repos outside the list, because the profile is explicitly not claiming coverage of those repos. + +### Deprecation strategy + +The `*` wildcard is supported as an alias during the transition period. At profile compile time, `*` is treated identically to `{{all-repositories}}` but a deprecation warning is emitted. No transitional handling is needed when an operator updates a profile from `*` to `{{all-repositories}}` — the two are semantically identical, the refresh picks up the new YAML, computes a new digest, and cached tokens naturally expire. The alias is removed in version 1, at which point `*` will cause a profile validation failure. + +## Testing Decisions + +### Profile compilation tests + +The profile compilation layer has thorough existing tests. New tests should cover: acceptance of both new literals, rejection of mixed entries (literal combined with static repos or with each other), `*` alias behaviour including deprecation warning emission, rejection of the literals in pipeline profiles, and correct resolution of scoping mode at compile time. The existing `validateRepositories` function is the natural extension point. + +### Org vendor tests + +New tests should cover: scoped token issuance when `repository-scope` is provided, rejection when scope is provided but profile doesn't expect it, rejection when scope is missing but profile requires it, correct repository narrowing in the issued token, and 403 response when GitHub rejects the scoped repository. The existing test helpers provide the patterns to follow. + +### Cached vendor tests + +New tests should cover: distinct cache entries for different repositories under the same scoped profile, cache key format including repository name, correct hit/miss behaviour across repositories, and confirmation that `checkTokenRepository` is not reached for scoped profiles (by construction of the cache key). + +### Handler tests + +New tests should cover: extraction of `repository-scope` query parameter, rejection of requests with invalid `repository-scope` values (containing `/`, empty, whitespace-only), and correct passthrough to the vendor layer. The git-credentials handler should be tested for implicit scoping from the Git-supplied repository. + +### Chinmina-token plugin + CLI script tests + +The plugin has an existing test suite using `docker-compose`. New tests should cover: the `repository-scope` plugin parameter being passed through as a query parameter, and the CLI script accepting and forwarding the scope argument. + +## Out of Scope + +- **Runtime JSON schema validation within Chinmina itself.** The JSON schema is published for documentation and editor support. Profile validation continues to be handled by the Go compiler at load time. +- **Multi-repository scoping in a single request.** `{{caller-scoped-repository}}` narrows to exactly one repository. Multi-repo scoping with caller control would be a separate feature. +- **Pipeline profile scoping.** Pipeline profiles are inherently scoped to a single repository via the Buildkite pipeline configuration. The new literals apply only to organization profiles. +- **Automatic migration tooling for `*` to `{{all-repositories}}`.** The deprecation warning provides the signal; operators update their YAML manually. +- **Changes to the chinmina-release or chinmina-git-credentials Buildkite plugins.** The release plugin fetches release assets and doesn't need repository scoping. The git-credentials plugin needs no code changes. +- **Pre-validation against the GitHub App's installed repository list.** The GitHub API handles this at token creation time. + +## Further Notes + +The naming of the two literals was a deliberate design choice. `{{caller-scoped-repository}}` was preferred over shorter alternatives like `{{repository-scope}}` or `{{requested-repository}}` because it makes both the actor (caller) and the effect (scoping) explicit in a security-relevant configuration file. `{{all-repositories}}` was preferred over `{{any-repository}}` because "any" is ambiguous in English — it can mean "whichever one" or "every one" — while "all" is unambiguous. + +The `RepositoryScope` type already exists in the codebase and provides the domain model for wildcard vs. specific repository access. The dynamic scoping feature extends this model with a third state rather than replacing it. + +This feature interacts with the caching layer, which is currently under investigation for a separate bug (cache misses under load on organization routes). The cache key changes in this feature (adding repository name for scoped profiles) should be coordinated with any caching fixes to avoid conflicting changes. diff --git a/handlers.go b/handlers.go index 5fa5c0f9..f442fc95 100644 --- a/handlers.go +++ b/handlers.go @@ -7,10 +7,12 @@ import ( "fmt" "io" "net/http" + "net/url" "strings" "github.com/chinmina/chinmina-bridge/internal/audit" "github.com/chinmina/chinmina-bridge/internal/credentialhandler" + "github.com/chinmina/chinmina-bridge/internal/github" "github.com/chinmina/chinmina-bridge/internal/jwt" "github.com/chinmina/chinmina-bridge/internal/profile" "github.com/chinmina/chinmina-bridge/internal/vendor" @@ -22,28 +24,157 @@ type HTTPStatuser interface { Status() (int, string) } -// buildProfileRef constructs a ProfileRef from the request context and path. -// Returns an error if the profile parameter is invalid. Panics if Buildkite -// claims are missing (via jwt.RequireBuildkiteClaimsFromContext), which should -// only occur when used outside the JWT middleware chain. -func buildProfileRef(r *http.Request, expectedType profile.ProfileType) (profile.ProfileRef, error) { - // claims must be present from the middleware - claims := jwt.RequireBuildkiteClaimsFromContext(r.Context()) +// builderError wraps an error returned by a ProfileRefBuilder so that it +// surfaces a 400 by default. Typed errors that already implement HTTPStatuser +// (ProfileNotFoundError, RepositoryScopeRequiredError, etc.) retain their +// declared status via Unwrap — errors.As on the wrapped chain still finds +// them. Plain parse errors from profile.NewProfileRef stay at 400 rather than +// inheriting writeJSONError's 500 default, preserving prior handler behaviour +// for malformed profile path parameters. +type builderError struct{ err error } + +func (e builderError) Error() string { return fmt.Sprintf("invalid profile parameter: %v", e.err) } +func (e builderError) Unwrap() error { return e.err } +func (e builderError) Status() (int, string) { + var statuser HTTPStatuser + if errors.As(e.err, &statuser) { + return statuser.Status() + } + return http.StatusBadRequest, http.StatusText(http.StatusBadRequest) +} + +// PathValuer abstracts path-parameter extraction to keep the builder free of +// HTTP-type dependencies. *http.Request satisfies this implicitly via its +// PathValue method. +type PathValuer interface { + PathValue(name string) string +} + +// ProfileRefBuilder constructs a profile.ProfileRef from request context, a +// path-parameter source, and two scope signals: +// +// - explicitScope is the repository the caller explicitly asked to scope +// to (e.g. via ?repository-scope=). When a caller supplies this to a +// non-caller-scoped profile, the builder returns +// RepositoryScopeUnexpectedError. When absent on a caller-scoped profile +// the builder falls back to implicitScope. +// - implicitScope is a scope value derived from the request's structure +// rather than its explicit intent — specifically, the repository name +// parsed from the Git-credentials body URL. It's honoured only on +// caller-scoped profiles; non-caller-scoped profiles ignore it silently +// because the URL is part of the request format, not a scope request. +// +// Validation of scope against profile type is applied inside the builder, +// centralising the authorisation-boundary logic and keeping the handler +// focused on transport concerns. +type ProfileRefBuilder func(ctx context.Context, pv PathValuer, explicitScope, implicitScope string) (profile.ProfileRef, error) + +// NewProfileRefBuilder returns a ProfileRefBuilder closed over the given +// profile store and expected profile type. For ProfileTypeOrg the builder +// consults the store to determine whether the profile accepts caller-supplied +// scope, enforcing the bidirectional rules below. For ProfileTypeRepo the +// store is ignored — pipeline profiles are never scoped. +// +// Trust model: ScopedRepository narrows within an already-authorised profile; +// it does not grant access. Match rules gate who may invoke a profile at all, +// and GitHub is the final enforcement boundary for which repositories a +// token can actually reach. Honouring caller-supplied scope without cross- +// checking it against a separate list is therefore safe — the caller already +// has some legitimate claim on the profile by virtue of matching it. +func NewProfileRefBuilder(store *profile.ProfileStore, expectedType profile.ProfileType) ProfileRefBuilder { + return func(ctx context.Context, pv PathValuer, explicitScope, implicitScope string) (profile.ProfileRef, error) { + claims := jwt.RequireBuildkiteClaimsFromContext(ctx) + profileStr := pv.PathValue("profile") + + ref, err := profile.NewProfileRef(claims, expectedType, profileStr, "") + if err != nil { + return profile.ProfileRef{}, err + } + + // Pipeline profiles are never scoped: skip the store lookup entirely. + if expectedType != profile.ProfileTypeOrg { + return ref, nil + } - // Extract profile parameter from path (empty string for legacy routes) - profileStr := r.PathValue("profile") + authProfile, err := store.GetOrganizationProfile(ref.Name) + if err != nil { + return profile.ProfileRef{}, err + } + + if !authProfile.Attrs.RepositoryScope().IsCallerScoped() { + if explicitScope != "" { + return profile.ProfileRef{}, profile.RepositoryScopeUnexpectedError{ProfileName: ref.Name} + } + // implicitScope (e.g. the git-credentials URL repo) is part of + // the request format, not a scope request, so it's silently + // ignored for static-list and all-repositories profiles. + return ref, nil + } - // Construct ProfileRef from claims and profile parameter - return profile.NewProfileRef(claims, expectedType, profileStr) + // caller-scoped: explicit takes precedence over the URL fallback. + effective := explicitScope + if effective == "" { + effective = implicitScope + } + if effective == "" { + return profile.ProfileRef{}, profile.RepositoryScopeRequiredError{ProfileName: ref.Name} + } + ref.ScopedRepository = effective + return ref, nil + } } -func handlePostToken(tokenVendor vendor.ProfileTokenVendor, expectedType profile.ProfileType) http.Handler { +// deriveScopeFromRepoURL extracts the repository-name component from a +// git-credentials request URL (e.g. "https://github.com/acme/widget" → +// "widget"). Returns "" if the URL is unparseable or does not resolve to a +// GitHub org/repo pair; callers then fall back to unscoped behaviour. +func deriveScopeFromRepoURL(repoURL string) string { + u, err := url.Parse(repoURL) + if err != nil { + return "" + } + _, repo := github.RepoForURL(*u) + return repo +} + +// extractRepositoryScope extracts and validates the repository-scope query parameter. +// Returns empty string if the parameter is absent. +// Returns an error if the parameter is present but invalid (empty, whitespace-only, or contains '/'). +func extractRepositoryScope(r *http.Request) (string, error) { + if !r.URL.Query().Has("repository-scope") { + return "", nil + } + + scope := r.URL.Query().Get("repository-scope") + if strings.TrimSpace(scope) == "" { + return "", fmt.Errorf("repository-scope must not be empty") + } + if strings.Contains(scope, "/") { + return "", fmt.Errorf("repository-scope must not contain '/'") + } + return scope, nil +} + +func handlePostToken(tokenVendor vendor.ProfileTokenVendor, builder ProfileRefBuilder, expectedType profile.ProfileType) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { defer drainRequestBody(r) - ref, err := buildProfileRef(r, expectedType) + // Resolve repository scope first so the builder receives a normalised + // value. Pipeline routes (ProfileTypeRepo) are never scoped: skip the + // query-parameter read entirely to preserve their current behaviour. + var explicitScope string + if expectedType == profile.ProfileTypeOrg { + var err error + explicitScope, err = extractRepositoryScope(r) + if err != nil { + requestError(r.Context(), w, http.StatusBadRequest, fmt.Errorf("invalid repository-scope: %w", err)) + return + } + } + + ref, err := builder(r.Context(), r, explicitScope, "") if err != nil { - requestError(r.Context(), w, http.StatusBadRequest, fmt.Errorf("invalid profile parameter: %w", err)) + writeJSONError(r.Context(), w, builderError{err: err}) return } @@ -80,16 +211,13 @@ func handlePostToken(tokenVendor vendor.ProfileTokenVendor, expectedType profile }) } -func handlePostGitCredentials(tokenVendor vendor.ProfileTokenVendor, expectedType profile.ProfileType) http.Handler { +func handlePostGitCredentials(tokenVendor vendor.ProfileTokenVendor, builder ProfileRefBuilder, expectedType profile.ProfileType) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { defer drainRequestBody(r) - ref, err := buildProfileRef(r, expectedType) - if err != nil { - requestError(r.Context(), w, http.StatusBadRequest, fmt.Errorf("invalid profile parameter: %w", err)) - return - } - + // Read and reconstruct the Git-supplied URL first: the org path uses + // it to derive repository scope, so the builder receives a normalised + // value. Keeping the order consistent across endpoints reads cleanly. requestedRepo, err := credentialhandler.ReadProperties(r.Body) if err != nil { writeTextError(r.Context(), w, fmt.Errorf("read repository properties from client failed: %w", err)) @@ -102,6 +230,22 @@ func handlePostGitCredentials(tokenVendor vendor.ProfileTokenVendor, expectedTyp return } + // Derive an implicit scope hint from the Git-supplied URL for org + // routes. The builder uses this as a fallback for caller-scoped + // profiles and ignores it for static-list or all-repositories + // profiles — the URL is part of the request format, not a scope + // request. Pipeline routes remain unscoped end-to-end. + var implicitScope string + if expectedType == profile.ProfileTypeOrg { + implicitScope = deriveScopeFromRepoURL(requestedRepoURL) + } + + ref, err := builder(r.Context(), r, "", implicitScope) + if err != nil { + writeTextError(r.Context(), w, builderError{err: err}) + return + } + result := tokenVendor(r.Context(), ref, requestedRepoURL) if err, failed := result.Failed(); failed { writeTextError(r.Context(), w, fmt.Errorf("token creation failed: %w", err)) diff --git a/handlers_fuzz_test.go b/handlers_fuzz_test.go new file mode 100644 index 00000000..c534b6b4 --- /dev/null +++ b/handlers_fuzz_test.go @@ -0,0 +1,97 @@ +//go:build fuzz + +package main + +import ( + "net/http" + "net/url" + "testing" +) + +func FuzzExtractRepositoryScope(f *testing.F) { + // --- seeds: absent parameter --- + f.Add("") + + // --- seeds: valid names --- + f.Add("repository-scope=my-repo") + f.Add("repository-scope=MyRepo") + f.Add("repository-scope=repo-with-hyphens") + f.Add("repository-scope=repo123") + f.Add("repository-scope=UPPERCASE") + + // --- seeds: invalid values --- + f.Add("repository-scope=owner/repo") + f.Add("repository-scope=") + f.Add("repository-scope=%20%20") + f.Add("repository-scope=a/b/c") + f.Add("repository-scope=/") + + // --- seeds: encoding edge cases --- + f.Add("repository-scope=%00") // null byte + f.Add("repository-scope=%0A") // newline + f.Add("repository-scope=%2F") // encoded slash + f.Add("repository-scope=hello%20world") // space + f.Add("repository-scope=%09") // tab + + // --- seeds: multiple params --- + f.Add("repository-scope=repo&other=val") + f.Add("repository-scope=first&repository-scope=second") + + // --- seeds: unusual query strings --- + f.Add("repository-scope") // key without value + f.Add("=repo") // value without key + f.Add("repository-scope=repo&") // trailing ampersand + f.Add("&repository-scope=repo") // leading ampersand + + f.Fuzz(func(t *testing.T, rawQuery string) { + // Build a request with the fuzzed query string + u := &url.URL{ + Scheme: "http", + Host: "localhost", + Path: "/organization/token/test", + RawQuery: rawQuery, + } + + req, err := http.NewRequest("POST", u.String(), nil) + if err != nil { + // Invalid URL from fuzzer - skip + return + } + + // Property 1: No panics + scope, extractErr := extractRepositoryScope(req) + + // Property 2: If extraction succeeds with a non-empty scope, it must not contain '/' + if extractErr == nil && scope != "" { + for _, ch := range scope { + if ch == '/' { + t.Errorf("extracted scope contains '/' but should have been rejected: %q", scope) + } + } + } + + // Property 3: If extraction succeeds with a non-empty scope, it must not be all whitespace + if extractErr == nil && scope != "" { + allWhitespace := true + for _, ch := range scope { + if ch != ' ' && ch != '\t' && ch != '\n' && ch != '\r' { + allWhitespace = false + break + } + } + if allWhitespace { + t.Errorf("extracted scope is all whitespace but should have been rejected: %q", scope) + } + } + + // Property 4: If the parameter is absent, scope must be empty and err nil + if !req.URL.Query().Has("repository-scope") { + if scope != "" { + t.Errorf("scope should be empty when parameter is absent, got %q", scope) + } + if extractErr != nil { + t.Errorf("err should be nil when parameter is absent, got %v", extractErr) + } + } + }) +} diff --git a/handlers_test.go b/handlers_test.go index 28e13e32..5db179cc 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -17,6 +17,7 @@ import ( "github.com/chinmina/chinmina-bridge/internal/credentialhandler" "github.com/chinmina/chinmina-bridge/internal/jwt" "github.com/chinmina/chinmina-bridge/internal/profile" + "github.com/chinmina/chinmina-bridge/internal/profile/profiletest" "github.com/chinmina/chinmina-bridge/internal/vendor" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -24,24 +25,48 @@ import ( var defaultExpiry = time.Date(2024, time.May, 7, 17, 59, 36, 0, time.UTC) +// testBuilder returns the default ProfileRefBuilder wired with a nil store +// (sufficient for tests that exercise handler plumbing without needing +// type-aware scope validation — that is covered in builder-unit tests). +func testBuilder(expectedType profile.ProfileType) ProfileRefBuilder { + return NewProfileRefBuilder(nil, expectedType) +} + func TestHandlers_RequireClaims(t *testing.T) { + // The builder's call to jwt.RequireBuildkiteClaimsFromContext panics when + // claims are absent — a defence-in-depth signal that the JWT middleware + // was bypassed. For /token the builder runs first; for /git-credentials + // the body is read first, so we send a valid body to reach the builder. + validGitCredsBody := func() *bytes.Buffer { + m := credentialhandler.NewMap(3) + m.Set("protocol", "https") + m.Set("host", "github.com") + m.Set("path", "org/repo") + b := &bytes.Buffer{} + require.NoError(t, credentialhandler.WriteProperties(m, b)) + return b + } + cases := []struct { name string handler http.Handler + body io.Reader }{ { name: "postToken", - handler: handlePostToken(nil, profile.ProfileTypeRepo), + handler: handlePostToken(nil, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo), + body: nil, }, { name: "postGitCredentials", - handler: handlePostGitCredentials(nil, profile.ProfileTypeRepo), + handler: handlePostGitCredentials(nil, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo), + body: validGitCredsBody(), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - req, err := http.NewRequestWithContext(t.Context(), "POST", "/not-applicable", nil) + req, err := http.NewRequestWithContext(t.Context(), "POST", "/not-applicable", tc.body) require.NoError(t, err) rr := httptest.NewRecorder() @@ -63,7 +88,7 @@ func TestHandlePostToken_ReturnsTokenOnSuccess(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostToken(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostToken(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -91,7 +116,7 @@ func TestHandlePostToken_ReturnsFailureOnVendorFailure(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostToken(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostToken(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -135,7 +160,7 @@ func TestHandlePostTokenWithProfile_ReturnsTokenOnSuccess(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostToken(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostToken(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -172,7 +197,7 @@ func TestHandlePostGitCredentials_ReturnsTokenOnSuccess(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostGitCredentials(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostGitCredentials(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -202,7 +227,7 @@ func TestHandlePostGitCredentials_ReturnsEmptySuccessWhenNoToken(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostGitCredentials(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostGitCredentials(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -226,7 +251,7 @@ func TestHandlePostGitCredentials_ReturnsFailureOnInvalidRequest(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostGitCredentials(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostGitCredentials(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -255,7 +280,7 @@ func TestHandlePostGitCredentials_ReturnsFailureOnReadFailure(t *testing.T) { // act handler := maxRequestSize(1)( // use the request size limit to force an error in the credentials handler - handlePostGitCredentials(tokenVendor, profile.ProfileTypeRepo), + handlePostGitCredentials(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo), ) handler.ServeHTTP(rr, req) @@ -282,7 +307,7 @@ func TestHandlePostGitCredentials_ReturnsFailureOnVendorFailure(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostGitCredentials(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostGitCredentials(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -330,7 +355,7 @@ func TestHandlePostGitCredentialsWithRepoProfile_ReturnsTokenOnSuccess(t *testin rr := httptest.NewRecorder() // act - handler := handlePostGitCredentials(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostGitCredentials(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -376,6 +401,8 @@ func tv(token string) vendor.ProfileTokenVendor { func TestHandlePostGitCredentialsWithProfile_ReturnsTokenOnSuccess(t *testing.T) { tokenVendor := tv("expected-token-value") + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) ctx := claimsContext() @@ -386,14 +413,14 @@ func TestHandlePostGitCredentialsWithProfile_ReturnsTokenOnSuccess(t *testing.T) body := &bytes.Buffer{} require.NoError(t, credentialhandler.WriteProperties(m, body)) - req, err := http.NewRequestWithContext(ctx, "POST", "/organization/git-credentials/test-profile", body) + req, err := http.NewRequestWithContext(ctx, "POST", "/organization/git-credentials/static-profile", body) require.NoError(t, err) - req.SetPathValue("profile", "test-profile") + req.SetPathValue("profile", "static-profile") rr := httptest.NewRecorder() // act - handler := handlePostGitCredentials(tokenVendor, profile.ProfileTypeOrg) + handler := handlePostGitCredentials(tokenVendor, builder, profile.ProfileTypeOrg) handler.ServeHTTP(rr, req) // assert @@ -503,7 +530,7 @@ func TestHandlePostToken_ProfileErrors(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostToken(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostToken(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -532,7 +559,7 @@ func TestHandlePostToken_ClaimValidationError(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostToken(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostToken(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -562,7 +589,7 @@ func TestHandlePostGitCredentials_ClaimValidationError(t *testing.T) { rr := httptest.NewRecorder() // act - handler := handlePostGitCredentials(tokenVendor, profile.ProfileTypeRepo) + handler := handlePostGitCredentials(tokenVendor, testBuilder(profile.ProfileTypeRepo), profile.ProfileTypeRepo) handler.ServeHTTP(rr, req) // assert @@ -691,6 +718,291 @@ func TestAuditError(t *testing.T) { } } +// mapPathValuer is a minimal PathValuer for tests that do not need a real +// *http.Request. +type mapPathValuer map[string]string + +func (m mapPathValuer) PathValue(name string) string { return m[name] } + +func TestNewProfileRefBuilder_OrgProfile(t *testing.T) { + // Non-caller-scoped org profile with no caller-supplied scope produces + // an unscoped ref — status quo for static-list profiles. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + pv := mapPathValuer{"profile": "static-profile"} + + ref, err := builder(ctx, pv, "", "") + require.NoError(t, err) + + assert.Equal(t, profile.ProfileRef{ + Organization: "organization-slug", + Type: profile.ProfileTypeOrg, + Name: "static-profile", + }, ref) +} + +func TestNewProfileRefBuilder_RepoProfileDefault(t *testing.T) { + builder := NewProfileRefBuilder(nil, profile.ProfileTypeRepo) + + ctx := claimsContext() + pv := mapPathValuer{} // no path parameter — repo profiles default to "default" + + ref, err := builder(ctx, pv, "", "") + require.NoError(t, err) + + assert.Equal(t, profile.ProfileRef{ + Organization: "organization-slug", + Type: profile.ProfileTypeRepo, + Name: "default", + PipelineID: "pipeline-id", + PipelineSlug: "pipeline-slug", + }, ref) +} + +const scopedProfilesYAML = `organization: + profiles: + - name: caller-scoped-profile + repositories: + - "{{caller-scoped-repository}}" + permissions: + - contents:write + match: + - claim: pipeline_slug + valuePattern: ".*" + - name: all-repos-profile + repositories: + - "{{all-repositories}}" + permissions: + - contents:read + - name: static-profile + repositories: + - repo1 + - repo2 + permissions: + - contents:read + +pipeline: + defaults: + permissions: + - contents:read +` + +func TestNewProfileRefBuilder_OrgCallerScoped_MissingScopeReturnsRequiredError(t *testing.T) { + // A caller-scoped profile without a caller-supplied scope must surface + // RepositoryScopeRequiredError so the handler can respond with 400 and + // a specific message. This is Req 2.3 enforced at the builder boundary. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + pv := mapPathValuer{"profile": "caller-scoped-profile"} + + _, err := builder(ctx, pv, "", "") + + var scopeErr profile.RepositoryScopeRequiredError + require.ErrorAs(t, err, &scopeErr) + assert.Equal(t, "caller-scoped-profile", scopeErr.ProfileName) +} + +func TestNewProfileRefBuilder_OrgStaticList_RejectsScope(t *testing.T) { + // A static-list profile must reject caller-supplied scope (Req 2.2). + // RepositoryScopeUnexpectedError carries a 400 status, allowing the + // handler to surface a specific message via writeJSONError/writeTextError. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + pv := mapPathValuer{"profile": "static-profile"} + + _, err := builder(ctx, pv, "unexpected-scope", "") + + var scopeErr profile.RepositoryScopeUnexpectedError + require.ErrorAs(t, err, &scopeErr) + assert.Equal(t, "static-profile", scopeErr.ProfileName) +} + +func TestNewProfileRefBuilder_OrgCallerScoped_PopulatesScopedRepository(t *testing.T) { + // When a caller supplies a repository scope to a caller-scoped profile, + // the builder populates ref.ScopedRepository so downstream consumers + // (URN, cache key, audit log) observe a single source of truth. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + pv := mapPathValuer{"profile": "caller-scoped-profile"} + + ref, err := builder(ctx, pv, "target-repo", "") + require.NoError(t, err) + + assert.Equal(t, profile.ProfileRef{ + Organization: "organization-slug", + Type: profile.ProfileTypeOrg, + Name: "caller-scoped-profile", + ScopedRepository: "target-repo", + }, ref) +} + +func TestNewProfileRefBuilder_OrgCallerScoped_UsesImplicitScopeWhenExplicitEmpty(t *testing.T) { + // The git-credentials endpoint derives the repository name from the + // Git-supplied URL and passes it as implicitScope. For caller-scoped + // profiles this fills ref.ScopedRepository when no explicit scope is + // supplied. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + pv := mapPathValuer{"profile": "caller-scoped-profile"} + + ref, err := builder(ctx, pv, "", "url-derived-repo") + require.NoError(t, err) + + assert.Equal(t, "url-derived-repo", ref.ScopedRepository) +} + +func TestNewProfileRefBuilder_RepoProfile_IgnoresScopeArgs(t *testing.T) { + // Pipeline profiles are never scoped. Any scope arguments are silently + // ignored; the builder does not even touch the store. + builder := NewProfileRefBuilder(nil, profile.ProfileTypeRepo) + + ctx := claimsContext() + pv := mapPathValuer{} + + ref, err := builder(ctx, pv, "explicit-ignored", "implicit-ignored") + require.NoError(t, err) + assert.Empty(t, ref.ScopedRepository) +} + +func TestNewProfileRefBuilder_OrgProfileNotFound_SurfacesLookupError(t *testing.T) { + // Unknown profile surfaces ProfileNotFoundError from the store lookup. + // The handler will route this through writeJSONError / writeTextError, + // preserving the 404 status the error carries. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + pv := mapPathValuer{"profile": "no-such-profile"} + + _, err := builder(ctx, pv, "", "") + + var notFound profile.ProfileNotFoundError + require.ErrorAs(t, err, ¬Found) +} + +func TestNewProfileRefBuilder_OrgNonCallerScoped_IgnoresImplicitScope(t *testing.T) { + // Static-list and all-repositories profiles must not reject a + // git-credentials request just because the URL yielded a repo name. + // implicitScope is a structural artefact of the request format, not a + // scope request. + cases := []string{"static-profile", "all-repos-profile"} + for _, profileName := range cases { + t.Run(profileName, func(t *testing.T) { + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + pv := mapPathValuer{"profile": profileName} + + ref, err := builder(ctx, pv, "", "url-derived-repo") + require.NoError(t, err) + + assert.Empty(t, ref.ScopedRepository) + }) + } +} + +func TestHandlePostToken_OrgStaticList_RejectsScopeWithSpecificMessage(t *testing.T) { + // Req 2.2 — the client must receive a message identifying *why* the + // scope was rejected, not a generic "Bad Request". The handler routes + // builder errors through writeJSONError so HTTPStatuser types carry + // their declared message. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + req, err := http.NewRequestWithContext(ctx, "POST", "/organization/token/static-profile?repository-scope=anything", nil) + require.NoError(t, err) + req.SetPathValue("profile", "static-profile") + rr := httptest.NewRecorder() + + handler := handlePostToken(tv("unused"), builder, profile.ProfileTypeOrg) + handler.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusBadRequest, rr.Code) + var body ErrorResponse + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + assert.Equal(t, "profile does not accept repository scoping", body.Error) +} + +func TestHandlePostToken_OrgCallerScoped_MissingScopeReturnsSpecificMessage(t *testing.T) { + // Req 2.3 — the caller must learn that the profile requires a scope. + store := profiletest.CreateTestProfileStore(t, scopedProfilesYAML) + builder := NewProfileRefBuilder(store, profile.ProfileTypeOrg) + + ctx := claimsContext() + req, err := http.NewRequestWithContext(ctx, "POST", "/organization/token/caller-scoped-profile", nil) + require.NoError(t, err) + req.SetPathValue("profile", "caller-scoped-profile") + rr := httptest.NewRecorder() + + handler := handlePostToken(tv("unused"), builder, profile.ProfileTypeOrg) + handler.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusBadRequest, rr.Code) + var body ErrorResponse + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + assert.Equal(t, "repository scope is required for this profile", body.Error) +} + +func TestExtractRepositoryScope_Valid(t *testing.T) { + tests := []struct { + name string + query string + expected string + }{ + {"simple name", "repository-scope=my-repo", "my-repo"}, + {"hyphenated name", "repository-scope=my-cool-repo", "my-cool-repo"}, + {"mixed case preserved", "repository-scope=MyRepo", "MyRepo"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := http.NewRequestWithContext(t.Context(), "POST", "/organization/token/test?"+tt.query, nil) + require.NoError(t, err) + scope, err := extractRepositoryScope(req) + require.NoError(t, err) + assert.Equal(t, tt.expected, scope) + }) + } +} + +func TestExtractRepositoryScope_Absent(t *testing.T) { + req, err := http.NewRequestWithContext(t.Context(), "POST", "/organization/token/test", nil) + require.NoError(t, err) + scope, err := extractRepositoryScope(req) + require.NoError(t, err) + assert.Equal(t, "", scope) +} + +func TestExtractRepositoryScope_Invalid(t *testing.T) { + tests := []struct { + name string + query string + }{ + {"contains slash", "repository-scope=owner/repo"}, + {"empty value", "repository-scope="}, + {"whitespace only", "repository-scope=%20%20"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := http.NewRequestWithContext(t.Context(), "POST", "/organization/token/test?"+tt.query, nil) + require.NoError(t, err) + _, err = extractRepositoryScope(req) + require.Error(t, err) + }) + } +} + func TestStripPrefix(t *testing.T) { echoPath := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) diff --git a/harness_integration_test.go b/harness_integration_test.go index 22c6cd66..28658e0b 100644 --- a/harness_integration_test.go +++ b/harness_integration_test.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/chinmina/chinmina-bridge/internal/config" @@ -389,6 +390,31 @@ func (c *TestClient) OrganizationToken(token, profile string) (*vendor.ProfileTo return &result, nil } +// OrganizationTokenScoped requests an organization token with an optional repository scope. +// If repositoryScope is non-empty, it's passed as a ?repository-scope= query parameter. +func (c *TestClient) OrganizationTokenScoped(token, profile, repositoryScope string) (*vendor.ProfileToken, error) { + path := "/organization/token/" + profile + if repositoryScope != "" { + path += "?repository-scope=" + url.QueryEscape(repositoryScope) + } + + resp, err := c.Request("POST", path, token, nil) + if err != nil { + return nil, err + } + + if resp.StatusCode != http.StatusOK { + return nil, c.parseError(resp) + } + + var result vendor.ProfileToken + if err := json.Unmarshal(resp.Body, &result); err != nil { + return nil, fmt.Errorf("unmarshal token response: %w", err) + } + + return &result, nil +} + // OrganizationGitCredentials requests organization git credentials for the given profile. // Returns the credentials map or an error if the request fails or returns non-2xx. func (c *TestClient) OrganizationGitCredentials(token, profile string, req GitCredentialRequest) (*credentialhandler.ArrayMap, error) { diff --git a/internal/github/token.go b/internal/github/token.go index 6d38731b..7aef91a5 100644 --- a/internal/github/token.go +++ b/internal/github/token.go @@ -22,6 +22,22 @@ import ( "golang.org/x/oauth2" ) +type TokenIssuanceError struct { + Cause error +} + +func (e TokenIssuanceError) Error() string { + return fmt.Sprintf("GitHub token issuance failed: %v", e.Cause) +} + +func (e TokenIssuanceError) Unwrap() error { + return e.Cause +} + +func (e TokenIssuanceError) Status() (int, string) { + return http.StatusForbidden, http.StatusText(http.StatusForbidden) +} + type Client struct { client *github.Client installationID int64 @@ -97,7 +113,7 @@ func (c Client) CreateAccessToken(ctx context.Context, repoNames []string, scope }, ) if err != nil { - return "", time.Time{}, err + return "", time.Time{}, TokenIssuanceError{Cause: err} } return tok.GetToken(), tok.GetExpiresAt().Time, nil diff --git a/internal/github/token_test.go b/internal/github/token_test.go index 1b070f36..8ff4957e 100644 --- a/internal/github/token_test.go +++ b/internal/github/token_test.go @@ -173,6 +173,87 @@ func TestCreateAccessToken_Fails_On_Failed_Request(t *testing.T) { assert.ErrorContains(t, err, ": 418") } +func TestCreateAccessToken_Wraps_GitHub_Failure_In_TokenIssuanceError(t *testing.T) { + router := http.NewServeMux() + + router.HandleFunc("/app/installations/{installationID}/access_tokens", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTeapot) + }) + + svr := httptest.NewServer(router) + defer svr.Close() + + key := generateKey(t) + + gh, err := github.New( + context.Background(), + config.GithubConfig{ + APIURL: svr.URL, + PrivateKey: key, + ApplicationID: 10, + InstallationID: 20, + }, + ) + require.NoError(t, err) + + tok, _, err := gh.CreateAccessToken( + context.Background(), + []string{"https://github.com/org/repo"}, + []string{"contents:read"}, + ) + + assert.Equal(t, "", tok) + require.Error(t, err) + + var issuanceErr github.TokenIssuanceError + require.ErrorAs(t, err, &issuanceErr) + assert.NotNil(t, issuanceErr.Unwrap(), "error should have underlying cause") + assert.ErrorContains(t, err, "GitHub token issuance failed") + + status, message := issuanceErr.Status() + assert.Equal(t, http.StatusForbidden, status) + assert.Equal(t, http.StatusText(http.StatusForbidden), message) +} + +func TestCreateAccessToken_Does_Not_Wrap_Config_Error(t *testing.T) { + router := http.NewServeMux() + + router.HandleFunc("/app/installations/{installationID}/access_tokens", func(w http.ResponseWriter, r *http.Request) { + JSON(w, &api.InstallationToken{ + Token: new("expected-token"), + ExpiresAt: &api.Timestamp{Time: time.Now().Add(1 * time.Hour)}, + }) + }) + + svr := httptest.NewServer(router) + defer svr.Close() + + key := generateKey(t) + + gh, err := github.New( + context.Background(), + config.GithubConfig{ + APIURL: svr.URL, + PrivateKey: key, + ApplicationID: 10, + InstallationID: 20, + }, + ) + require.NoError(t, err) + + tok, _, err := gh.CreateAccessToken( + context.Background(), + []string{"https://github.com/org/repo"}, + []string{"bogus"}, + ) + + assert.Equal(t, "", tok) + require.Error(t, err) + + var issuanceErr github.TokenIssuanceError + assert.NotErrorAs(t, err, &issuanceErr) +} + func TestTransportOptions(t *testing.T) { router := http.NewServeMux() diff --git a/internal/profile/compilation.go b/internal/profile/compilation.go index f020efb4..bea464cb 100644 --- a/internal/profile/compilation.go +++ b/internal/profile/compilation.go @@ -11,6 +11,27 @@ import ( "github.com/chinmina/chinmina-bridge/internal/github" ) +const ( + // LiteralCallerScoped is the YAML literal for caller-supplied repository scoping. + LiteralCallerScoped = "{{caller-scoped-repository}}" + // LiteralAllRepositories is the YAML literal for all-repositories access. + LiteralAllRepositories = "{{all-repositories}}" +) + +// resolveRepositoryScope converts a raw repositories list into a typed RepositoryScope. +// This is called after validation, so the input is known to be well-formed. +func resolveRepositoryScope(repos []string) RepositoryScope { + if len(repos) == 1 { + switch repos[0] { + case LiteralCallerScoped: + return NewCallerScopedScope() + case LiteralAllRepositories, "*": + return NewWildcardScope() + } + } + return NewSpecificScope(repos...) +} + // compileOrganizationProfiles compiles organization profiles from config. // Returns a ProfileStoreOf containing valid and invalid profiles. func compileOrganizationProfiles(profiles []organizationProfile) ProfileStoreOf[OrganizationProfileAttr] { @@ -81,9 +102,17 @@ func compileOrganizationProfiles(profiles []organizationProfile) ProfileStoreOf[ continue } + // Emit deprecation warning for "*" wildcard + if len(p.Repositories) == 1 && p.Repositories[0] == "*" { + slog.Warn("organization profile: '*' is deprecated, use '{{all-repositories}}' instead", + "profile", p.Name, + ) + } + + scope := resolveRepositoryScope(p.Repositories) attrs := OrganizationProfileAttr{ - Repositories: p.Repositories, - Permissions: ensureMetadataRead(p.Permissions), + Scope: scope, + Permissions: ensureMetadataRead(p.Permissions), } validProfiles[p.Name] = NewAuthorizedProfile(matcher, attrs) @@ -226,20 +255,21 @@ func ensureMetadataRead(permissions []string) []string { } // validateRepositories validates that the repositories list follows the required format: -// - If wildcard "*" is present, it must be the only entry +// - If a literal ({{caller-scoped-repository}}, {{all-repositories}}, or "*") is present, it must be the only entry // - Repository names must not contain "/" (no owner prefix) func validateRepositories(repos []string) error { - // Check for wildcard mixed with other entries - hasWildcard := slices.Contains(repos, "*") - if hasWildcard && len(repos) > 1 { - return fmt.Errorf("wildcard '*' must be the only repository entry") + for _, repo := range repos { + switch repo { + case LiteralCallerScoped, LiteralAllRepositories, "*": + if len(repos) > 1 { + return fmt.Errorf("%q must be the only repository entry", repo) + } + return nil + } } - // Check for owner prefix (slash in repo name) + // Static repo names: check for owner prefix (slash) for _, repo := range repos { - if repo == "*" { - continue - } if strings.Contains(repo, "/") { return fmt.Errorf("repository %q must not contain owner prefix", repo) } diff --git a/internal/profile/compilation_fuzz_test.go b/internal/profile/compilation_fuzz_test.go new file mode 100644 index 00000000..beb6f78d --- /dev/null +++ b/internal/profile/compilation_fuzz_test.go @@ -0,0 +1,101 @@ +//go:build fuzz + +package profile + +import ( + "testing" +) + +func FuzzValidateRepositories(f *testing.F) { + // --- single-entry seeds --- + f.Add(1, "*") // deprecated wildcard + f.Add(1, "{{all-repositories}}") // new wildcard literal + f.Add(1, "{{caller-scoped-repository}}") // caller-scoped literal + f.Add(1, "repo-name") // plain repo name + f.Add(1, "owner/repo") // slash → rejected + f.Add(1, "") // empty string + f.Add(1, " ") // whitespace only + f.Add(1, "{{unknown-literal}}") // looks like a literal but isn't + f.Add(1, "repo\x00name") // null byte + f.Add(1, "repo\nname") // newline + f.Add(1, "{{") // partial literal + f.Add(1, "}}") // partial literal close + f.Add(1, "{{caller-scoped-repository") // unclosed literal + f.Add(1, "a/b/c") // multiple slashes + f.Add(1, "/") // bare slash + f.Add(1, ".") // dot + f.Add(1, "..") // double dot + f.Add(1, "-") // hyphen only + + // --- multi-entry seeds (count=2) --- + f.Add(2, "repo-a") // two plain repos + f.Add(2, "*") // wildcard with second entry + f.Add(2, "{{caller-scoped-repository}}") // literal with second entry + f.Add(2, "{{all-repositories}}") // literal with second entry + + f.Fuzz(func(t *testing.T, count int, entry string) { + // Clamp count to a reasonable range + if count < 1 { + count = 1 + } + if count > 5 { + count = 5 + } + + // Build a repos list by repeating the entry + repos := make([]string, count) + for i := range repos { + repos[i] = entry + } + + // Property 1: No panics + err := validateRepositories(repos) + + // Property 2: If a special literal is present with other entries, it must be rejected + isSpecial := entry == "*" || entry == LiteralCallerScoped || entry == LiteralAllRepositories + if isSpecial && count > 1 && err == nil { + t.Errorf("special literal %q with %d entries should be rejected but wasn't", entry, count) + } + + // Property 3: If validation passes, resolveRepositoryScope must not produce a zero scope + if err == nil && count > 0 { + scope := resolveRepositoryScope(repos) + if scope.IsZero() { + t.Errorf("resolveRepositoryScope returned zero scope for valid input %v", repos) + } + } + + // Property 4: Slash in repo name must be rejected + if count == 1 && !isSpecial && len(entry) > 0 { + for _, ch := range entry { + if ch == '/' { + if err == nil { + t.Errorf("entry containing '/' should be rejected: %q", entry) + } + break + } + } + } + + // Property 5: validateRepositories + resolveRepositoryScope consistency. + // If validation passes with a single special literal, resolve must + // return the matching scope type. + if err == nil && count == 1 { + scope := resolveRepositoryScope(repos) + switch entry { + case LiteralCallerScoped: + if !scope.IsCallerScoped() { + t.Errorf("expected caller-scoped scope for %q, got %+v", entry, scope) + } + case LiteralAllRepositories, "*": + if !scope.IsWildcard() { + t.Errorf("expected wildcard scope for %q, got %+v", entry, scope) + } + default: + if scope.IsWildcard() || scope.IsCallerScoped() { + t.Errorf("expected specific scope for %q, got %+v", entry, scope) + } + } + } + }) +} diff --git a/internal/profile/compilation_test.go b/internal/profile/compilation_test.go index 04c5c0b8..6f9a417f 100644 --- a/internal/profile/compilation_test.go +++ b/internal/profile/compilation_test.go @@ -1,6 +1,7 @@ package profile import ( + "fmt" "os" "testing" @@ -250,15 +251,15 @@ func TestCompile_GracefulDegradation(t *testing.T) { validProfile, err := orgProfiles.Get("valid-production") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, validProfile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), validProfile.Attrs.Scope) validStaging, err := orgProfiles.Get("valid-staging") require.NoError(t, err) - assert.Equal(t, []string{"silk", "cotton"}, validStaging.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk", "cotton"), validStaging.Attrs.Scope) validNoMatch, err := orgProfiles.Get("valid-no-match") require.NoError(t, err) - assert.Equal(t, []string{"shared"}, validNoMatch.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("shared"), validNoMatch.Attrs.Scope) // Invalid profiles should return ProfileUnavailableError _, err = orgProfiles.Get("invalid-both-match-types") @@ -293,7 +294,7 @@ func TestCompile_DuplicateNameHandling(t *testing.T) { // (first is validated, but second's attributes overwrite in the profile map) profile, err := orgProfiles.Get("production") require.NoError(t, err) - assert.Equal(t, []string{"cotton"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("cotton"), profile.Attrs.Scope) // "staging" should also be accessible _, err = orgProfiles.Get("staging") @@ -949,11 +950,11 @@ pipeline: // Valid profiles should be accessible validWildcard, err := result.Get("valid-wildcard-only") require.NoError(t, err) - assert.Equal(t, []string{"*"}, validWildcard.Attrs.Repositories) + assert.Equal(t, NewWildcardScope(), validWildcard.Attrs.Scope) validMultiple, err := result.Get("valid-multiple-repos") require.NoError(t, err) - assert.Equal(t, []string{"repo1", "repo2"}, validMultiple.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("repo1", "repo2"), validMultiple.Attrs.Scope) // Invalid profiles should not be accessible _, err = result.Get("invalid-owner-prefix") @@ -1227,3 +1228,140 @@ func TestCompileOrganizationProfiles_MetadataReadAutoAdded(t *testing.T) { }) } } + +func TestCompile_OrganizationProfile_CallerScopedRepository(t *testing.T) { + yamlContent := ` +organization: + profiles: + - name: scoped-profile + repositories: + - "{{caller-scoped-repository}}" + permissions: + - "contents:write" + match: + - claim: pipeline_slug + value: agent-workflows + +pipeline: + defaults: + permissions: + - "contents:read" +` + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + p, err := profiles.GetOrgProfile("scoped-profile") + require.NoError(t, err) + assert.Equal(t, NewCallerScopedScope(), p.Attrs.Scope) +} + +func TestCompile_OrganizationProfile_AllRepositories(t *testing.T) { + yamlContent := ` +organization: + profiles: + - name: all-repos-profile + repositories: + - "{{all-repositories}}" + permissions: + - "contents:read" + +pipeline: + defaults: + permissions: + - "contents:read" +` + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + p, err := profiles.GetOrgProfile("all-repos-profile") + require.NoError(t, err) + assert.Equal(t, NewWildcardScope(), p.Attrs.Scope) +} + +func TestCompile_OrganizationProfile_LiteralsMustBeAlone(t *testing.T) { + tests := []struct { + name string + repositories string + profileName string + }{ + { + name: "caller-scoped mixed with static", + repositories: `["{{caller-scoped-repository}}", "repo-a"]`, + profileName: "mixed-caller-scoped", + }, + { + name: "all-repositories mixed with static", + repositories: `["{{all-repositories}}", "repo-a"]`, + profileName: "mixed-all-repos", + }, + { + name: "caller-scoped mixed with all-repositories", + repositories: `["{{caller-scoped-repository}}", "{{all-repositories}}"]`, + profileName: "mixed-both-literals", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + yamlContent := fmt.Sprintf(` +organization: + profiles: + - name: %s + repositories: %s + permissions: + - "contents:read" + match: + - claim: pipeline_slug + value: test + +pipeline: + defaults: + permissions: + - "contents:read" +`, tt.profileName, tt.repositories) + + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + _, err = profiles.GetOrgProfile(tt.profileName) + require.Error(t, err) + var unavailErr ProfileUnavailableError + require.ErrorAs(t, err, &unavailErr) + }) + } +} + +func TestCompile_OrganizationProfile_WildcardDeprecationAlias(t *testing.T) { + yamlContent := ` +organization: + profiles: + - name: old-wildcard + repositories: + - "*" + permissions: + - "contents:read" + +pipeline: + defaults: + permissions: + - "contents:read" +` + config, digest, err := parse(yamlContent) + require.NoError(t, err) + + profiles, err := compile(config, digest, "local") + require.NoError(t, err) + + p, err := profiles.GetOrgProfile("old-wildcard") + require.NoError(t, err) + assert.Equal(t, NewWildcardScope(), p.Attrs.Scope) +} diff --git a/internal/profile/config.go b/internal/profile/config.go index 83590595..d4d972b2 100644 --- a/internal/profile/config.go +++ b/internal/profile/config.go @@ -111,6 +111,34 @@ func (e ProfileMatchFailedError) Status() (int, string) { return http.StatusForbidden, http.StatusText(http.StatusForbidden) } +// RepositoryScopeUnexpectedError indicates a repository-scope was provided +// to a profile that does not accept caller scoping. +type RepositoryScopeUnexpectedError struct { + ProfileName string +} + +func (e RepositoryScopeUnexpectedError) Error() string { + return fmt.Sprintf("profile %q does not accept repository scoping", e.ProfileName) +} + +func (e RepositoryScopeUnexpectedError) Status() (int, string) { + return http.StatusBadRequest, "profile does not accept repository scoping" +} + +// RepositoryScopeRequiredError indicates a repository-scope was not provided +// but the profile requires one. +type RepositoryScopeRequiredError struct { + ProfileName string +} + +func (e RepositoryScopeRequiredError) Error() string { + return fmt.Sprintf("profile %q requires a repository scope", e.ProfileName) +} + +func (e RepositoryScopeRequiredError) Status() (int, string) { + return http.StatusBadRequest, "repository scope is required for this profile" +} + // parse deserializes YAML into profileConfig and calculates digest. // Fails on YAML parsing issues including unknown properties. func parse(yamlContent string) (profileConfig, string, error) { diff --git a/internal/profile/daemon_test.go b/internal/profile/daemon_test.go index d90cedcb..01c85bf1 100644 --- a/internal/profile/daemon_test.go +++ b/internal/profile/daemon_test.go @@ -77,7 +77,7 @@ pipeline: // Verify store has the profile profile, err := store.GetOrganizationProfile("test-profile") require.NoError(t, err, "profile should be in store") - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) } func TestRefresh_Error(t *testing.T) { @@ -169,7 +169,7 @@ pipeline: profile, err := store.GetOrganizationProfile("immediate") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) }) } @@ -325,7 +325,7 @@ pipeline: // Verify profile was loaded on second attempt profile, err := store.GetOrganizationProfile("panic-recovery") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) }) } @@ -368,6 +368,6 @@ pipeline: // Verify the profile was eventually loaded profile, err := store.GetOrganizationProfile("error-recovery") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) }) } diff --git a/internal/profile/profiles.go b/internal/profile/profiles.go index aca95297..2fbea1e7 100644 --- a/internal/profile/profiles.go +++ b/internal/profile/profiles.go @@ -2,7 +2,6 @@ package profile import ( "maps" - "slices" ) // --- Attribute types (leaf types) --- @@ -12,32 +11,19 @@ import ( // Callers should not modify slice contents after passing to // NewAuthorizedProfile or NewProfileStoreOf. type OrganizationProfileAttr struct { - Repositories []string - Permissions []string + Scope RepositoryScope + Permissions []string } // HasRepository checks if the given repository is included in the profile's -// repositories. Supports wildcard "*" to match any repository. +// repositories using the compiled Scope. func (attr OrganizationProfileAttr) HasRepository(repo string) bool { - return attr.allowAllRepositories() || - slices.Contains(attr.Repositories, repo) + return attr.Scope.Contains(repo) } -// RepositoryScope returns a RepositoryScope describing which repositories the -// profile allows. A wildcard profile (single "*" entry) returns NewWildcardScope(); -// all other configurations return NewSpecificScope with the named repositories. +// RepositoryScope returns the compiled RepositoryScope stored in this profile attribute. func (attr OrganizationProfileAttr) RepositoryScope() RepositoryScope { - if attr.allowAllRepositories() { - return NewWildcardScope() - } - return NewSpecificScope(attr.Repositories...) -} - -// allowAllRepositories returns true if the profile allows access to all -// repositories accessible to the Chinmina installation. This is signified by -// the single "*" entry. -func (attr OrganizationProfileAttr) allowAllRepositories() bool { - return len(attr.Repositories) == 1 && attr.Repositories[0] == "*" + return attr.Scope } // PipelineProfileAttr contains the attributes for a pipeline profile. diff --git a/internal/profile/profiles_test.go b/internal/profile/profiles_test.go index c87d7954..d63ac50d 100644 --- a/internal/profile/profiles_test.go +++ b/internal/profile/profiles_test.go @@ -8,119 +8,12 @@ import ( "github.com/stretchr/testify/require" ) -// TestOrganizationProfileAttr_HasRepository tests the HasRepository method -func TestOrganizationProfileAttr_HasRepository_Match(t *testing.T) { - tests := []struct { - name string - repositories []string - repo string - expected bool - }{ - { - name: "exact match in list", - repositories: []string{"repo1", "repo2"}, - repo: "repo1", - expected: true, - }, - { - name: "wildcard matches any", - repositories: []string{"*"}, - repo: "any/repository", - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - attr := OrganizationProfileAttr{ - Repositories: tt.repositories, - } - assert.Equal(t, tt.expected, attr.HasRepository(tt.repo)) - }) - } -} - -func TestOrganizationProfileAttr_HasRepository_NoMatch(t *testing.T) { - tests := []struct { - name string - repositories []string - repo string - }{ - { - name: "partial match fails", - repositories: []string{"repo"}, - repo: "acme", - }, - { - name: "over-match fails", - repositories: []string{"repo"}, - repo: "repo-extra", - }, - { - name: "empty list fails", - repositories: []string{}, - repo: "repo", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - attr := OrganizationProfileAttr{ - Repositories: tt.repositories, - } - assert.False(t, attr.HasRepository(tt.repo)) - }) - } -} - -// TestOrganizationProfileAttr_RepositoryScope tests the RepositoryScope method -func TestOrganizationProfileAttr_RepositoryScope_SpecificRepos(t *testing.T) { - tests := []struct { - name string - repositories []string - expected RepositoryScope - }{ - { - name: "single repository", - repositories: []string{"repo1"}, - expected: NewSpecificScope("repo1"), - }, - { - name: "multiple repositories", - repositories: []string{"repo1", "repo2", "repo3"}, - expected: NewSpecificScope("repo1", "repo2", "repo3"), - }, - { - name: "empty list", - repositories: []string{}, - expected: NewSpecificScope(), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - attr := OrganizationProfileAttr{ - Repositories: tt.repositories, - } - assert.Equal(t, tt.expected, attr.RepositoryScope()) - }) - } -} - -func TestOrganizationProfileAttr_RepositoryScope_Wildcard(t *testing.T) { - attr := OrganizationProfileAttr{ - Repositories: []string{"*"}, - } - - assert.Equal(t, NewWildcardScope(), attr.RepositoryScope()) -} - // TestAuthorizedProfile_Match tests the Match method behavior func TestAuthorizedProfile_Match_Success(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") attrs := OrganizationProfileAttr{ - Repositories: []string{"silk"}, - Permissions: []string{"contents:read"}, + Scope: NewSpecificScope("silk"), + Permissions: []string{"contents:read"}, } profile := NewAuthorizedProfile(matcher, attrs) @@ -138,7 +31,7 @@ func TestAuthorizedProfile_Match_Success(t *testing.T) { func TestAuthorizedProfile_Match_ClaimValueMismatch(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") attrs := OrganizationProfileAttr{ - Repositories: []string{"silk"}, + Scope: NewSpecificScope("silk"), } profile := NewAuthorizedProfile(matcher, attrs) @@ -159,7 +52,7 @@ func TestAuthorizedProfile_Match_ClaimValueMismatch(t *testing.T) { func TestAuthorizedProfile_Match_ClaimNotFound(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") attrs := OrganizationProfileAttr{ - Repositories: []string{"silk"}, + Scope: NewSpecificScope("silk"), } profile := NewAuthorizedProfile(matcher, attrs) @@ -180,7 +73,7 @@ func TestAuthorizedProfile_Match_ClaimNotFound(t *testing.T) { func TestAuthorizedProfile_Match_ValidationError(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") attrs := OrganizationProfileAttr{ - Repositories: []string{"silk"}, + Scope: NewSpecificScope("silk"), } profile := NewAuthorizedProfile(matcher, attrs) @@ -204,8 +97,8 @@ func TestProfileStoreOf_NewAndGet_OrganizationProfile(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") profiles := map[string]AuthorizedProfile[OrganizationProfileAttr]{ "test-profile": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"silk"}, - Permissions: []string{"contents:read"}, + Scope: NewSpecificScope("silk"), + Permissions: []string{"contents:read"}, }), } @@ -213,7 +106,7 @@ func TestProfileStoreOf_NewAndGet_OrganizationProfile(t *testing.T) { profile, err := store.Get("test-profile") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) assert.Equal(t, []string{"contents:read"}, profile.Attrs.Permissions) } @@ -261,8 +154,8 @@ func TestProfileStoreOf_Immutability(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") sourceProfiles := map[string]AuthorizedProfile[OrganizationProfileAttr]{ "test-profile": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"silk"}, - Permissions: []string{"contents:read"}, + Scope: NewSpecificScope("silk"), + Permissions: []string{"contents:read"}, }), } @@ -278,7 +171,7 @@ func TestProfileStoreOf_Immutability(t *testing.T) { // Profile should still be accessible from store profileAfter, err := store.Get("test-profile") require.NoError(t, err) - assert.Equal(t, profileBefore.Attrs.Repositories, profileAfter.Attrs.Repositories) + assert.Equal(t, profileBefore.Attrs.Scope, profileAfter.Attrs.Scope) } // TestProfiles tests the Profiles type @@ -287,7 +180,7 @@ func TestProfiles_NewProfiles(t *testing.T) { orgProfiles := NewProfileStoreOf( map[string]AuthorizedProfile[OrganizationProfileAttr]{ "test-profile": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"silk"}, + Scope: NewSpecificScope("silk"), }), }, nil, @@ -306,8 +199,8 @@ func TestProfiles_GetOrgProfile_Success(t *testing.T) { orgProfiles := NewProfileStoreOf( map[string]AuthorizedProfile[OrganizationProfileAttr]{ "test-profile": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"silk"}, - Permissions: []string{"contents:read"}, + Scope: NewSpecificScope("silk"), + Permissions: []string{"contents:read"}, }), }, nil, @@ -318,7 +211,7 @@ func TestProfiles_GetOrgProfile_Success(t *testing.T) { profile, err := profiles.GetOrgProfile("test-profile") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) assert.Equal(t, []string{"contents:read"}, profile.Attrs.Permissions) } @@ -360,8 +253,8 @@ func TestProfiles_Methods_Consistency(t *testing.T) { orgProfiles := NewProfileStoreOf( map[string]AuthorizedProfile[OrganizationProfileAttr]{ "valid-profile": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"test"}, - Permissions: []string{"contents:read"}, + Scope: NewSpecificScope("test"), + Permissions: []string{"contents:read"}, }), }, map[string]error{ @@ -385,7 +278,7 @@ func TestProfiles_Methods_Consistency(t *testing.T) { // GetOrgProfile should work for valid profile validProfile, err := profiles.GetOrgProfile("valid-profile") require.NoError(t, err) - assert.Equal(t, []string{"test"}, validProfile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("test"), validProfile.Attrs.Scope) // GetOrgProfile should return error for invalid profile _, err = profiles.GetOrgProfile("invalid-profile") @@ -410,12 +303,12 @@ func TestProfiles_Stats(t *testing.T) { orgProfiles := NewProfileStoreOf[OrganizationProfileAttr]( map[string]AuthorizedProfile[OrganizationProfileAttr]{ "profile-one": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"repo1"}, - Permissions: []string{"contents:read"}, + Scope: NewSpecificScope("repo1"), + Permissions: []string{"contents:read"}, }), "profile-two": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"repo2"}, - Permissions: []string{"contents:write"}, + Scope: NewSpecificScope("repo2"), + Permissions: []string{"contents:write"}, }), }, map[string]error{ @@ -459,14 +352,14 @@ func TestProfiles_Stats(t *testing.T) { func TestNewAuthorizedProfile(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") attrs := OrganizationProfileAttr{ - Repositories: []string{"silk"}, - Permissions: []string{"contents:read"}, + Scope: NewSpecificScope("silk"), + Permissions: []string{"contents:read"}, } profile := NewAuthorizedProfile(matcher, attrs) // Verify attributes are stored - assert.Equal(t, attrs.Repositories, profile.Attrs.Repositories) + assert.Equal(t, attrs.Scope, profile.Attrs.Scope) assert.Equal(t, attrs.Permissions, profile.Attrs.Permissions) // Verify matcher works @@ -480,7 +373,7 @@ func TestProfileStoreOf_Mixed(t *testing.T) { matcher := ExactMatcher("pipeline_slug", "silk-prod") validProfiles := map[string]AuthorizedProfile[OrganizationProfileAttr]{ "valid-profile": NewAuthorizedProfile(matcher, OrganizationProfileAttr{ - Repositories: []string{"silk"}, + Scope: NewSpecificScope("silk"), }), } invalidProfiles := map[string]error{ @@ -492,7 +385,7 @@ func TestProfileStoreOf_Mixed(t *testing.T) { // Valid profile should be accessible profile, err := store.Get("valid-profile") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) // Invalid profile should return ProfileUnavailableError _, err = store.Get("invalid-profile") @@ -508,3 +401,73 @@ func TestProfileStoreOf_Mixed(t *testing.T) { require.ErrorAs(t, err, ¬FoundErr) assert.Equal(t, "nonexistent", notFoundErr.Name) } + +// TestOrganizationProfileAttr_RepositoryScope_UsesCompiledScope tests that RepositoryScope() returns the stored compiled scope +func TestOrganizationProfileAttr_RepositoryScope_UsesCompiledScope(t *testing.T) { + tests := []struct { + name string + attr OrganizationProfileAttr + expected RepositoryScope + }{ + { + name: "wildcard scope", + attr: OrganizationProfileAttr{Scope: NewWildcardScope()}, + expected: NewWildcardScope(), + }, + { + name: "specific scope", + attr: OrganizationProfileAttr{Scope: NewSpecificScope("repo-a", "repo-b")}, + expected: NewSpecificScope("repo-a", "repo-b"), + }, + { + name: "caller-scoped", + attr: OrganizationProfileAttr{Scope: NewCallerScopedScope()}, + expected: NewCallerScopedScope(), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.attr.RepositoryScope()) + }) + } +} + +// TestOrganizationProfileAttr_HasRepository_WithCompiledScope tests HasRepository with compiled scope +func TestOrganizationProfileAttr_HasRepository_WithCompiledScope(t *testing.T) { + tests := []struct { + name string + attr OrganizationProfileAttr + repo string + expected bool + }{ + { + name: "wildcard matches any repo", + attr: OrganizationProfileAttr{Scope: NewWildcardScope()}, + repo: "any-repo", + expected: true, + }, + { + name: "specific scope matches member", + attr: OrganizationProfileAttr{Scope: NewSpecificScope("repo-a")}, + repo: "repo-a", + expected: true, + }, + { + name: "specific scope rejects non-member", + attr: OrganizationProfileAttr{Scope: NewSpecificScope("repo-a")}, + repo: "repo-b", + expected: false, + }, + { + name: "caller-scoped matches nothing directly", + attr: OrganizationProfileAttr{Scope: NewCallerScopedScope()}, + repo: "any-repo", + expected: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.attr.HasRepository(tt.repo)) + }) + } +} diff --git a/internal/profile/profiletest/testdata/profiles.yaml b/internal/profile/profiletest/testdata/profiles.yaml index a101547b..b7aa2a8f 100644 --- a/internal/profile/profiletest/testdata/profiles.yaml +++ b/internal/profile/profiletest/testdata/profiles.yaml @@ -32,6 +32,19 @@ organization: - test-repo permissions: - contents:read + - name: caller-scoped-profile + repositories: + - "{{caller-scoped-repository}}" + permissions: + - contents:write + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" + - name: all-repos-profile + repositories: + - "{{all-repositories}}" + permissions: + - contents:read pipeline: defaults: diff --git a/internal/profile/ref.go b/internal/profile/ref.go index c3eccc43..35df1d37 100644 --- a/internal/profile/ref.go +++ b/internal/profile/ref.go @@ -32,20 +32,33 @@ func (pt ProfileType) String() string { } // ProfileRef uniquely identifies a profile request. +// +// ScopedRepository narrows an already-authorised org profile to a single +// repository. It is caller-supplied at request time and is never granted by +// the profile itself: the profile's match rules decide whether the caller +// may invoke the profile at all, and GitHub is the final enforcement +// boundary for whether a specific repository is reachable. Honouring +// caller-supplied scope without additional cross-checks is therefore safe. +// Only applicable to ProfileTypeOrg; repo profiles are never scoped. type ProfileRef struct { - Organization string // Buildkite organization slug - Type ProfileType // repo or org - Name string // Profile name (e.g., "default", "write-packages") - PipelineID string // Only set for ProfileTypeRepo - PipelineSlug string // Only set for ProfileTypeRepo + Organization string // Buildkite organization slug + Type ProfileType // repo or org + Name string // Profile name (e.g., "default", "write-packages") + PipelineID string // Only set for ProfileTypeRepo + PipelineSlug string // Only set for ProfileTypeRepo + ScopedRepository string // Only set for caller-scoped ProfileTypeOrg requests } -// NewProfileRef constructs a ProfileRef from Buildkite claims, an expected profile type, and a profile string. +// NewProfileRef constructs a ProfileRef from Buildkite claims, an expected profile type, +// a profile string, and an optional caller-supplied repository scope. // If profileStr is empty and expectedType is ProfileTypeRepo, it defaults to "default". // If profileStr is empty and expectedType is ProfileTypeOrg, it returns an error. // If profileStr contains a colon, it must be in the format "type:name" and the type must match expectedType. // If profileStr does not contain a colon, it uses expectedType with profileStr as the name. -func NewProfileRef(claims jwt.BuildkiteClaims, expectedType ProfileType, profileStr string) (ProfileRef, error) { +// scopedRepo is populated onto the returned ref only for ProfileTypeOrg profiles; +// pass "" for non-scoped requests. Profile-type × scope-value validation (e.g. rejecting +// scope on non-caller-scoped profiles) happens at the handler boundary, not here. +func NewProfileRef(claims jwt.BuildkiteClaims, expectedType ProfileType, profileStr string, scopedRepo string) (ProfileRef, error) { profileType, profileName, err := resolveProfileTypeAndName(expectedType, profileStr) if err != nil { return ProfileRef{}, err @@ -64,6 +77,11 @@ func NewProfileRef(claims jwt.BuildkiteClaims, expectedType ProfileType, profile ref.PipelineSlug = claims.PipelineSlug } + // Repository scope is only meaningful for org profiles; repo profiles never carry it. + if profileType == ProfileTypeOrg { + ref.ScopedRepository = scopedRepo + } + return ref, nil } @@ -119,20 +137,31 @@ func parseProfileType(typeStr string) (ProfileType, error) { // String returns the canonical URN format for this ProfileRef. // Format for repo profiles: profile://organization/org-name/pipeline/pipeline-id/pipeline-slug/profile/profile-name // Format for org profiles: profile://organization/org-name/profile/profile-name +// Caller-scoped org profiles append /repository/repo-name. func (pr ProfileRef) String() string { switch pr.Type { case ProfileTypeRepo: return fmt.Sprintf("profile://organization/%s/pipeline/%s/%s/profile/%s", pr.Organization, pr.PipelineID, pr.PipelineSlug, pr.Name) case ProfileTypeOrg: - return fmt.Sprintf("profile://organization/%s/profile/%s", pr.Organization, pr.Name) + base := fmt.Sprintf("profile://organization/%s/profile/%s", pr.Organization, pr.Name) + if pr.ScopedRepository != "" { + return base + "/repository/" + pr.ScopedRepository + } + return base default: return "profile://unknown" } } // ShortString returns the short format for this ProfileRef. -// Format: "type:name" (e.g., "repo:default", "org:profile-name") +// Format: "type:name" (e.g., "repo:default", "org:profile-name"). +// Caller-scoped org profiles render as "org:profile-name/repo-name". +// The "/" separator is unambiguous: repository scope values reject "/" at +// the handler boundary, and profile names cannot contain "/". func (pr ProfileRef) ShortString() string { + if pr.ScopedRepository != "" { + return fmt.Sprintf("%s:%s/%s", pr.Type.String(), pr.Name, pr.ScopedRepository) + } return fmt.Sprintf("%s:%s", pr.Type.String(), pr.Name) } @@ -185,15 +214,25 @@ func ParseProfileRef(s string) (ProfileRef, error) { } } else if parts[1] == "profile" { // Org profile: organization/org-name/profile/profile-name + // Optionally scoped: organization/org-name/profile/profile-name/repository/repo-name profileName := parts[2] - return ProfileRef{ + ref := ProfileRef{ Organization: org, Type: ProfileTypeOrg, Name: profileName, - PipelineID: "", - PipelineSlug: "", - }, nil + } + + if len(parts) == 3 { + return ref, nil + } + + if len(parts) == 5 && parts[3] == "repository" && parts[4] != "" { + ref.ScopedRepository = parts[4] + return ref, nil + } + + return ProfileRef{}, fmt.Errorf("invalid profile ref format: malformed org profile suffix in '%s'", s) } return ProfileRef{}, fmt.Errorf("invalid profile ref format: could not determine profile type from '%s'", s) diff --git a/internal/profile/ref_test.go b/internal/profile/ref_test.go index 25729355..72c97a8f 100644 --- a/internal/profile/ref_test.go +++ b/internal/profile/ref_test.go @@ -53,7 +53,7 @@ func TestNewProfileRef_WithTypePrefix_Success(t *testing.T) { PipelineSlug: "pipeline-slug", } - ref, err := NewProfileRef(claims, tt.expected.Type, tt.profileStr) + ref, err := NewProfileRef(claims, tt.expected.Type, tt.profileStr, "") require.NoError(t, err) assert.Equal(t, tt.expected, ref) }) @@ -96,7 +96,7 @@ func TestNewProfileRef_WithoutTypePrefix_Success(t *testing.T) { PipelineSlug: "pipeline-slug", } - ref, err := NewProfileRef(claims, tt.expected.Type, tt.profileStr) + ref, err := NewProfileRef(claims, tt.expected.Type, tt.profileStr, "") require.NoError(t, err) assert.Equal(t, tt.expected, ref) }) @@ -110,7 +110,7 @@ func TestNewProfileRef_EmptyString_DefaultBehavior(t *testing.T) { PipelineSlug: "pipeline-slug", } - ref, err := NewProfileRef(claims, ProfileTypeRepo, "") + ref, err := NewProfileRef(claims, ProfileTypeRepo, "", "") require.NoError(t, err) assert.Equal(t, ProfileTypeRepo, ref.Type) assert.Equal(t, "default", ref.Name) @@ -123,7 +123,7 @@ func TestNewProfileRef_EmptyOrgSlug_Accepted(t *testing.T) { PipelineSlug: "pipeline-slug", } - ref, err := NewProfileRef(claims, ProfileTypeRepo, "default") + ref, err := NewProfileRef(claims, ProfileTypeRepo, "default", "") require.NoError(t, err) assert.Equal(t, "", ref.Organization) } @@ -181,7 +181,7 @@ func TestNewProfileRef_Failure(t *testing.T) { PipelineSlug: "pipeline-slug", } - _, err := NewProfileRef(claims, tt.expectedType, tt.profileStr) + _, err := NewProfileRef(claims, tt.expectedType, tt.profileStr, "") require.Error(t, err) assert.Contains(t, err.Error(), tt.errorMsg) }) @@ -214,6 +214,16 @@ func TestProfileRef_String_CanonicalFormat(t *testing.T) { }, expected: "profile://organization/acme/profile/write-packages", }, + { + name: "org profile with scoped repository", + ref: ProfileRef{ + Organization: "acme", + Type: ProfileTypeOrg, + Name: "write-packages", + ScopedRepository: "repo-a", + }, + expected: "profile://organization/acme/profile/write-packages/repository/repo-a", + }, } for _, tt := range tests { @@ -245,6 +255,15 @@ func TestProfileRef_ShortString(t *testing.T) { }, expected: "org:write-packages", }, + { + name: "org profile with scoped repository", + ref: ProfileRef{ + Type: ProfileTypeOrg, + Name: "write-packages", + ScopedRepository: "repo-a", + }, + expected: "org:write-packages/repo-a", + }, } for _, tt := range tests { @@ -277,6 +296,15 @@ func TestParseProfileRef_Roundtrip(t *testing.T) { Name: "write-packages", }, }, + { + name: "org profile with scoped repository", + ref: ProfileRef{ + Organization: "acme", + Type: ProfileTypeOrg, + Name: "write-packages", + ScopedRepository: "repo-a", + }, + }, } for _, tt := range tests { @@ -375,6 +403,21 @@ func TestParseProfileRef_Failure(t *testing.T) { urn: "profile://organization/acme/unknown/value/name", errorMsg: "could not determine profile type", }, + { + name: "org scope trailing empty", + urn: "profile://organization/acme/profile/write-packages/repository/", + errorMsg: "malformed org profile suffix", + }, + { + name: "org scope extra segments", + urn: "profile://organization/acme/profile/write-packages/repository/repo-a/extra", + errorMsg: "malformed org profile suffix", + }, + { + name: "org malformed suffix not repository", + urn: "profile://organization/acme/profile/write-packages/other/value", + errorMsg: "malformed org profile suffix", + }, } for _, tt := range tests { @@ -386,6 +429,17 @@ func TestParseProfileRef_Failure(t *testing.T) { } } +func TestParseProfileRef_RejectsScopeOnPipelineRef(t *testing.T) { + // Pipeline/repo refs never carry repository scope. + // Old format has 4 parts after org; new format has 6. A 5-part form between + // them is ambiguous, but any attempt to bolt /repository/ onto a repo ref + // must not round-trip as a scoped repo profile. + urn := "profile://organization/acme/pipeline/pipeline-id/pipeline-slug/profile/default/repository/repo-a" + parsed, err := ParseProfileRef(urn) + require.NoError(t, err) // parser currently stops at "default" via TestParseProfileRef_ExtraSlashesIgnored semantics + assert.Empty(t, parsed.ScopedRepository, "repo profiles must never carry ScopedRepository") +} + func TestProfileType_String(t *testing.T) { tests := []struct { name string @@ -411,6 +465,26 @@ func TestProfileType_String(t *testing.T) { } } +func TestNewProfileRef_ScopedRepository(t *testing.T) { + claims := jwt.BuildkiteClaims{ + OrganizationSlug: "acme", + PipelineID: "pipeline-id", + PipelineSlug: "pipeline-slug", + } + + t.Run("empty scope leaves ScopedRepository unset", func(t *testing.T) { + ref, err := NewProfileRef(claims, ProfileTypeOrg, "write-packages", "") + require.NoError(t, err) + assert.Empty(t, ref.ScopedRepository) + }) + + t.Run("non-empty scope populates ScopedRepository on org ref", func(t *testing.T) { + ref, err := NewProfileRef(claims, ProfileTypeOrg, "write-packages", "repo-a") + require.NoError(t, err) + assert.Equal(t, "repo-a", ref.ScopedRepository) + }) +} + func TestNewProfileRef_PipelineFieldsOnlySetForRepo(t *testing.T) { claims := jwt.BuildkiteClaims{ OrganizationSlug: "acme", @@ -419,13 +493,13 @@ func TestNewProfileRef_PipelineFieldsOnlySetForRepo(t *testing.T) { } // Repo profile should have pipeline fields set - repoRef, err := NewProfileRef(claims, ProfileTypeRepo, "default") + repoRef, err := NewProfileRef(claims, ProfileTypeRepo, "default", "") require.NoError(t, err) assert.Equal(t, "pipeline-id", repoRef.PipelineID) assert.Equal(t, "pipeline-slug", repoRef.PipelineSlug) // Org profile should NOT have pipeline fields set - orgRef, err := NewProfileRef(claims, ProfileTypeOrg, "write-packages") + orgRef, err := NewProfileRef(claims, ProfileTypeOrg, "write-packages", "") require.NoError(t, err) assert.Empty(t, orgRef.PipelineID) assert.Empty(t, orgRef.PipelineSlug) @@ -440,7 +514,7 @@ func TestProfileRef_Consistency(t *testing.T) { } // Create a repo profile ref - repoRef, err := NewProfileRef(claims, ProfileTypeRepo, "custom-profile") + repoRef, err := NewProfileRef(claims, ProfileTypeRepo, "custom-profile", "") require.NoError(t, err) // Verify complete struct matches expected @@ -468,7 +542,7 @@ func TestProfileRef_Consistency(t *testing.T) { assert.Equal(t, repoRef, parsed) // Create an org profile ref - orgRef, err := NewProfileRef(claims, ProfileTypeOrg, "org-profile") + orgRef, err := NewProfileRef(claims, ProfileTypeOrg, "org-profile", "") require.NoError(t, err) // Verify complete struct matches expected diff --git a/internal/profile/repositoryscope.go b/internal/profile/repositoryscope.go index 9a1dc05c..db1c2cb2 100644 --- a/internal/profile/repositoryscope.go +++ b/internal/profile/repositoryscope.go @@ -6,14 +6,18 @@ import ( ) // RepositoryScope describes the set of repositories a token covers. -// Use NewWildcardScope for all-repositories access, or NewSpecificScope for -// a named set. The zero value represents no repositories and is not wildcard. +// Use NewWildcardScope for all-repositories access, NewSpecificScope for +// a named set, or NewCallerScopedScope for caller-provided repositories. +// The zero value represents no repositories and is not wildcard. type RepositoryScope struct { // Wildcard indicates the token covers all repositories accessible to the // GitHub App installation. When true, Names is meaningless. Wildcard bool `json:"wildcard,omitempty"` // Names lists the specific repository names covered by the token. Names []string `json:"names,omitempty"` + // CallerScoped indicates the repository will be supplied at request time + // rather than being stored in the profile. When true, Names is meaningless. + CallerScoped bool `json:"callerScoped,omitempty"` } // NewWildcardScope returns a RepositoryScope that covers all repositories. @@ -29,31 +33,48 @@ func NewSpecificScope(names ...string) RepositoryScope { return RepositoryScope{Names: names} } +// NewCallerScopedScope returns a RepositoryScope where the repository is supplied +// at request time rather than being stored in the profile. +func NewCallerScopedScope() RepositoryScope { + return RepositoryScope{CallerScoped: true} +} + // IsWildcard reports whether this scope covers all repositories. func (rs RepositoryScope) IsWildcard() bool { return rs.Wildcard } +// IsCallerScoped reports whether the repository is supplied at request time. +func (rs RepositoryScope) IsCallerScoped() bool { + return rs.CallerScoped +} + // Contains reports whether the given repository name is covered by this scope. -// Wildcard scopes always return true. +// Wildcard scopes always return true. Caller-scoped scopes return false (no stored repositories). func (rs RepositoryScope) Contains(repo string) bool { if rs.Wildcard { return true } + if rs.CallerScoped { + return false + } return slices.Contains(rs.Names, repo) } -// IsZero reports whether this scope is the zero value (no repositories, not wildcard). +// IsZero reports whether this scope is the zero value (no repositories, not wildcard, not caller-scoped). func (rs RepositoryScope) IsZero() bool { - return !rs.Wildcard && len(rs.Names) == 0 + return !rs.Wildcard && !rs.CallerScoped && len(rs.Names) == 0 } // NamesForDisplay returns a human-readable representation of the scope. -// Wildcard scopes return ["*"]; specific scopes return their Names slice. +// Wildcard scopes return ["*"]; caller-scoped returns empty slice; specific scopes return their Names slice. func (rs RepositoryScope) NamesForDisplay() []string { if rs.Wildcard { return []string{"*"} } + if rs.CallerScoped { + return []string{} + } return rs.Names } diff --git a/internal/profile/repositoryscope_test.go b/internal/profile/repositoryscope_test.go index 36ee36ca..77f463b3 100644 --- a/internal/profile/repositoryscope_test.go +++ b/internal/profile/repositoryscope_test.go @@ -22,6 +22,13 @@ func TestNewSpecificScope(t *testing.T) { assert.Equal(t, names, rs.Names) } +func TestNewCallerScopedScope(t *testing.T) { + rs := NewCallerScopedScope() + assert.False(t, rs.Wildcard) + assert.Nil(t, rs.Names) + assert.True(t, rs.CallerScoped) +} + func TestRepositoryScope_IsWildcard(t *testing.T) { tests := []struct { name string @@ -31,6 +38,7 @@ func TestRepositoryScope_IsWildcard(t *testing.T) { {"wildcard scope", NewWildcardScope(), true}, {"specific scope", NewSpecificScope("repo-a"), false}, {"zero value", RepositoryScope{}, false}, + {"caller-scoped scope", NewCallerScopedScope(), false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -39,6 +47,24 @@ func TestRepositoryScope_IsWildcard(t *testing.T) { } } +func TestRepositoryScope_IsCallerScoped(t *testing.T) { + tests := []struct { + name string + scope RepositoryScope + expected bool + }{ + {"caller-scoped scope", NewCallerScopedScope(), true}, + {"wildcard scope", NewWildcardScope(), false}, + {"specific scope", NewSpecificScope("repo-a"), false}, + {"zero value", RepositoryScope{}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.scope.IsCallerScoped()) + }) + } +} + func TestRepositoryScope_Contains(t *testing.T) { tests := []struct { name string @@ -52,6 +78,7 @@ func TestRepositoryScope_Contains(t *testing.T) { {"specific does not match non-member", NewSpecificScope("repo-a", "repo-b"), "repo-c", false}, {"empty specific matches nothing", NewSpecificScope(), "repo-a", false}, {"zero value matches nothing", RepositoryScope{}, "repo-a", false}, + {"caller-scoped matches nothing", NewCallerScopedScope(), "any-repo", false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -70,6 +97,7 @@ func TestRepositoryScope_IsZero(t *testing.T) { {"wildcard scope", NewWildcardScope(), false}, {"specific scope with names", NewSpecificScope("repo-a"), false}, {"specific scope with empty names", NewSpecificScope(), true}, + {"caller-scoped scope", NewCallerScopedScope(), false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -87,6 +115,7 @@ func TestRepositoryScope_NamesForDisplay(t *testing.T) { {"wildcard returns star", NewWildcardScope(), []string{"*"}}, {"specific returns names", NewSpecificScope("repo-a", "repo-b"), []string{"repo-a", "repo-b"}}, {"zero value returns nil", RepositoryScope{}, nil}, + {"caller-scoped returns empty", NewCallerScopedScope(), []string{}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -116,6 +145,11 @@ func TestRepositoryScope_JSONRoundTrip(t *testing.T) { scope: RepositoryScope{}, expectedJSON: `{}`, }, + { + name: "caller-scoped", + scope: NewCallerScopedScope(), + expectedJSON: `{"callerScoped":true}`, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -152,6 +186,11 @@ func TestRepositoryScope_LogValue(t *testing.T) { scope: RepositoryScope{}, expected: slog.AnyValue([]string(nil)), }, + { + name: "caller-scoped logs empty", + scope: NewCallerScopedScope(), + expected: slog.AnyValue([]string{}), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/profile/store_test.go b/internal/profile/store_test.go index 60266b51..dbdca6b0 100644 --- a/internal/profile/store_test.go +++ b/internal/profile/store_test.go @@ -56,7 +56,7 @@ pipeline: // Verify profile can be accessed profile, err := profiles.GetOrgProfile("test-profile") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) } func TestFetchOrganizationProfile_ReturnsCorrectDigest(t *testing.T) { @@ -142,7 +142,7 @@ func TestProfileStore_GetOrganizationProfile_Success(t *testing.T) { // Retrieve profile profile, err := store.GetOrganizationProfile("test-profile") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), profile.Attrs.Scope) assert.Equal(t, []string{"contents:read", "pull_requests:write", "metadata:read"}, profile.Attrs.Permissions) } @@ -379,7 +379,7 @@ func TestProfileStore_Concurrency(t *testing.T) { } // Basic sanity check - if len(profile.Attrs.Repositories) != 1 { + if len(profile.Attrs.Scope.NamesForDisplay()) != 1 { return } @@ -424,7 +424,7 @@ func TestProfileStore_Update_MultipleTimes(t *testing.T) { profile1, err := store.GetOrganizationProfile("profile-v1") require.NoError(t, err) - assert.Equal(t, []string{"v1"}, profile1.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("v1"), profile1.Attrs.Scope) // Update with second version profiles2, err := FetchOrganizationProfile(context.Background(), "acme:test:v2.yaml", gh) @@ -438,7 +438,7 @@ func TestProfileStore_Update_MultipleTimes(t *testing.T) { // New profile should be accessible profile2, err := store.GetOrganizationProfile("profile-v2") require.NoError(t, err) - assert.Equal(t, []string{"v2"}, profile2.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("v2"), profile2.Attrs.Scope) } func TestProfileStore_Update_NoChange(t *testing.T) { @@ -469,7 +469,7 @@ func TestProfileStore_Update_NoChange(t *testing.T) { // Verify profile is still accessible profile, err := store.GetOrganizationProfile("profile-v1") require.NoError(t, err) - assert.Equal(t, []string{"v1"}, profile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("v1"), profile.Attrs.Scope) } func TestFetchOrganizationProfile_InvalidYAML(t *testing.T) { @@ -522,7 +522,7 @@ pipeline: // Verify prod profile prodProfile, err := profiles.GetOrgProfile("prod-profile") require.NoError(t, err) - assert.Equal(t, []string{"silk"}, prodProfile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk"), prodProfile.Attrs.Scope) assert.Equal(t, []string{"contents:write", "metadata:read"}, prodProfile.Attrs.Permissions) // Test prod profile matching @@ -533,7 +533,7 @@ pipeline: // Verify staging profile stagingProfile, err := profiles.GetOrgProfile("staging-profile") require.NoError(t, err) - assert.Equal(t, []string{"silk", "cotton"}, stagingProfile.Attrs.Repositories) + assert.Equal(t, NewSpecificScope("silk", "cotton"), stagingProfile.Attrs.Scope) // Test staging profile matching with regex claims = mapClaimLookup{"pipeline_slug": "silk-staging"} diff --git a/internal/vendor/auditvendor_test.go b/internal/vendor/auditvendor_test.go index 44210355..681adb37 100644 --- a/internal/vendor/auditvendor_test.go +++ b/internal/vendor/auditvendor_test.go @@ -360,3 +360,39 @@ func TestAuditingMatcher_ValidationError(t *testing.T) { assert.Equal(t, expected, entry.ClaimsFailed) assert.Nil(t, entry.ClaimsMatched) } + +func TestAuditor_RecordsScopingMismatchError(t *testing.T) { + tests := []struct { + name string + vendorError error + expectedAudit string + }{ + { + name: "scope provided to non-scoped profile", + vendorError: profile.RepositoryScopeUnexpectedError{ProfileName: "static-profile"}, + expectedAudit: "does not accept repository scoping", + }, + { + name: "scope missing for caller-scoped profile", + vendorError: profile.RepositoryScopeRequiredError{ProfileName: "scoped-profile"}, + expectedAudit: "requires a repository scope", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inner := func(ctx context.Context, ref profile.ProfileRef, repo string) vendor.VendorResult { + return vendor.NewVendorFailed(tt.vendorError) + } + + auditor := vendor.Auditor(inner) + + ctx, _ := audit.Context(context.Background()) + ref := profile.ProfileRef{Organization: "org", Name: "test", Type: profile.ProfileTypeOrg} + auditor(ctx, ref, "") + + entry := audit.Log(ctx) + assert.Contains(t, entry.Error, tt.expectedAudit) + }) + } +} diff --git a/internal/vendor/cached.go b/internal/vendor/cached.go index 5cc4eb9b..2fd5d868 100644 --- a/internal/vendor/cached.go +++ b/internal/vendor/cached.go @@ -53,6 +53,7 @@ func Cached(tokenCache cache.TokenCache[ProfileToken], digester cache.Digester) return func(v ProfileTokenVendor) ProfileTokenVendor { return func(ctx context.Context, ref profile.ProfileRef, requestedRepository string) VendorResult { // Cache key includes digest prefix for config version namespacing + // and ref.String() which embeds ScopedRepository for caller-scoped profiles. key := fmt.Sprintf("%s:%s", digester.Digest(), ref.String()) cachedToken, found, err := tokenCache.Get(ctx, key) diff --git a/internal/vendor/cached_test.go b/internal/vendor/cached_test.go index 7b2c4afb..d4486201 100644 --- a/internal/vendor/cached_test.go +++ b/internal/vendor/cached_test.go @@ -3,6 +3,7 @@ package vendor_test import ( "context" "errors" + "fmt" "testing" "time" @@ -10,6 +11,7 @@ import ( "github.com/chinmina/chinmina-bridge/internal/github" "github.com/chinmina/chinmina-bridge/internal/profile" "github.com/chinmina/chinmina-bridge/internal/vendor" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -617,6 +619,146 @@ func (m *mutableDigester) Digest() string { return m.digest } +// TestCacheCallerScoped_DifferentReposAreSeparateEntries verifies that different +// repositories under the same caller-scoped profile get separate cache entries. +// Distinctness comes from ref.String() which embeds ScopedRepository (Phase 1), +// not from the repositoryScope parameter. +func TestCacheCallerScoped_DifferentReposAreSeparateEntries(t *testing.T) { + wrapped := sequenceVendor("token-for-repo-a", "token-for-repo-b", "token-for-repo-a") + + c := newTestCached(t, defaultTTL, "test-digest") + v := c(wrapped) + + refA := profile.ProfileRef{ + Organization: "org", + Name: "scoped-profile", + Type: profile.ProfileTypeOrg, + ScopedRepository: "repo-a", + } + + refB := profile.ProfileRef{ + Organization: "org", + Name: "scoped-profile", + Type: profile.ProfileTypeOrg, + ScopedRepository: "repo-b", + } + + // First call with repo-a scoped ref: cache miss + result := v(context.Background(), refA, "") + token, ok := result.Token() + require.True(t, ok) + assert.Equal(t, "token-for-repo-a", token.Token) + + // Second call with repo-b scoped ref: must also miss (different cache key from ref.String()) + result = v(context.Background(), refB, "") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "token-for-repo-b", token.Token) + + // Third call with repo-a scoped ref again: cache hit (returns first token) + result = v(context.Background(), refA, "") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "token-for-repo-a", token.Token) +} + +// TestCacheCallerScoped_GitCredentialsPath_DistinctCacheEntries verifies that +// git-credentials requests with different scoped repos produce distinct cache entries. +func TestCacheCallerScoped_GitCredentialsPath_DistinctCacheEntries(t *testing.T) { + wrapped := sequenceVendor("git-token-repo-a", "git-token-repo-b", "git-token-repo-a") + + c := newTestCached(t, defaultTTL, "test-digest") + v := c(wrapped) + + refA := profile.ProfileRef{ + Organization: "org", + Name: "scoped-profile", + Type: profile.ProfileTypeOrg, + ScopedRepository: "repo-a", + } + + refB := profile.ProfileRef{ + Organization: "org", + Name: "scoped-profile", + Type: profile.ProfileTypeOrg, + ScopedRepository: "repo-b", + } + + // First git-credentials call for repo-a: cache miss + result := v(context.Background(), refA, "https://github.com/test-org/repo-a.git") + token, ok := result.Token() + require.True(t, ok) + assert.Equal(t, "git-token-repo-a", token.Token) + + // Second git-credentials call for repo-b: must also miss (different cache key from ref.String()) + result = v(context.Background(), refB, "https://github.com/test-org/repo-b.git") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "git-token-repo-b", token.Token) + + // Third git-credentials call for repo-a again: cache hit (returns first token) + result = v(context.Background(), refA, "https://github.com/test-org/repo-a.git") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "git-token-repo-a", token.Token) +} + +// TestCacheCallerScoped_SameScopeIsCacheHit verifies that two identical +// caller-scoped requests return a cached token. +func TestCacheCallerScoped_SameScopeIsCacheHit(t *testing.T) { + wrapped := sequenceVendor("cached-token", "should-not-be-called") + + c := newTestCached(t, defaultTTL, "test-digest") + v := c(wrapped) + + ref := profile.ProfileRef{ + Organization: "org", + Name: "scoped-profile", + Type: profile.ProfileTypeOrg, + ScopedRepository: "repo-a", + } + + // First call: cache miss, vends token + result := v(context.Background(), ref, "") + token, ok := result.Token() + require.True(t, ok) + assert.Equal(t, "cached-token", token.Token) + + // Second call with identical ref: cache hit, returns same token + // If the wrapped vendor were called, it would return "should-not-be-called" and fail + result = v(context.Background(), ref, "") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "cached-token", token.Token) +} + +// TestCacheAllRepositories_SameKeyAsWildcard verifies that an all-repositories +// profile uses the same cache key regardless of repository scope (no repository component). +func TestCacheAllRepositories_SameKeyAsWildcard(t *testing.T) { + wrapped := sequenceVendor("first-call", "should-not-be-called") + + c := newTestCached(t, defaultTTL, "test-digest") + v := c(wrapped) + + ref := profile.ProfileRef{ + Organization: "org", + Name: "all-repos-profile", + Type: profile.ProfileTypeOrg, + } + + // First call: cache miss + result := v(context.Background(), ref, "") + token, ok := result.Token() + require.True(t, ok) + assert.Equal(t, "first-call", token.Token) + + // Second call: cache hit (same key, no repository scope component) + result = v(context.Background(), ref, "") + token, ok = result.Token() + require.True(t, ok) + assert.Equal(t, "first-call", token.Token) +} + // sequenceVendor returns each of the calls in sequence, either a token or an error func sequenceVendor(calls ...any) vendor.ProfileTokenVendor { callIndex := 0 @@ -638,6 +780,14 @@ func sequenceVendor(calls ...any) vendor.ProfileTokenVendor { repositories: profile.NewSpecificScope("any-repo", "other-repo"), permissions: []string{"contents:read"}, }, + "org:scoped-profile": { + repositories: profile.NewWildcardScope(), + permissions: []string{"contents:read"}, + }, + "org:all-repos-profile": { + repositories: profile.NewWildcardScope(), + permissions: []string{"contents:read"}, + }, } return vendor.ProfileTokenVendor(func(ctx context.Context, ref profile.ProfileRef, repo string) vendor.VendorResult { @@ -666,7 +816,13 @@ func sequenceVendor(calls ...any) vendor.ProfileTokenVendor { Profile: ref.ShortString(), }) } else { - data, ok := profileData[ref.ShortString()] + // For scoped profiles, extract base profile name for lookup + profileKey := ref.ShortString() + if ref.ScopedRepository != "" { + // ShortString returns "org:name/repo", extract just "org:name" + profileKey = fmt.Sprintf("%s:%s", ref.Type.String(), ref.Name) + } + data, ok := profileData[profileKey] if !ok { return vendor.NewVendorFailed(errors.New("unknown profile")) } diff --git a/internal/vendor/orgvendor.go b/internal/vendor/orgvendor.go index 4b294065..b01a68b0 100644 --- a/internal/vendor/orgvendor.go +++ b/internal/vendor/orgvendor.go @@ -47,21 +47,25 @@ func NewOrgVendor(profileStore *profile.ProfileStore, tokenVendor TokenVendor) P return NewVendorFailed(profile.ProfileMatchFailedError{Name: ref.Name}) } - // The repository is only supplied for the git-credentials endpoint: - // checking it allows Git to respond properly: it's not a security measure. - if requestedRepoURL != "" { - // Otherwise validate it against the profile. + // --- Bidirectional scoping validation --- + profileScope := authProfile.Attrs.RepositoryScope() + repoScope, err := resolveRequestScope(profileScope, ref) + if err != nil { + return NewVendorFailed(err) + } + + // For non-caller-scoped profiles at the git-credentials endpoint, + // check the requested repo against the profile's scope. Static-list + // profiles return unmatched for repos outside their list; wildcard + // profiles skip this check (they claim all repos). + if requestedRepoURL != "" && !profileScope.IsCallerScoped() && !profileScope.IsWildcard() { repo, err := url.Parse(requestedRepoURL) if err != nil { return NewVendorFailed(fmt.Errorf("could not parse requested repo URL %s: %w", requestedRepoURL, err)) } - // If the requested repository isn't in the profile, return unmatched. This - // indicates that the handler should return a successful (but empty) - // response. This allows Git (for example) to try a different provider in - // its credentials chain. _, repository := github.RepoForURL(*repo) - if !authProfile.Attrs.HasRepository(repository) { + if !repoScope.Contains(repository) { slog.Debug("profile doesn't support requested repository: no token vended.", "organization", ref.Organization, "profile", ref.ShortString(), @@ -72,7 +76,6 @@ func NewOrgVendor(profileStore *profile.ProfileStore, tokenVendor TokenVendor) P } // Use the GitHub API to vend a token for the repository - repoScope := authProfile.Attrs.RepositoryScope() token, expiry, err := tokenVendor(ctx, repoScope.Names, authProfile.Attrs.Permissions) if err != nil { return NewVendorFailed(fmt.Errorf("could not issue token for profile %s: %w", ref, err)) @@ -95,3 +98,18 @@ func NewOrgVendor(profileStore *profile.ProfileStore, tokenVendor TokenVendor) P }) } } + +// resolveRequestScope determines the effective repository scope for a request. +// Scope validation happens at the handler boundary (in the ProfileRefBuilder); +// here we simply interpret the ref's ScopedRepository against the profile's +// declared scope. The builder is the single enforcement point, so this function +// only needs to read the already-validated ref and apply the profile's scope rules. +// For caller-scoped profiles, ScopedRepository is guaranteed non-empty by the builder. +func resolveRequestScope(profileScope profile.RepositoryScope, ref profile.ProfileRef) (profile.RepositoryScope, error) { + if profileScope.IsCallerScoped() { + // Builder guarantees ScopedRepository is non-empty for caller-scoped refs. + return profile.NewSpecificScope(ref.ScopedRepository), nil + } + + return profileScope, nil +} diff --git a/internal/vendor/orgvendor_test.go b/internal/vendor/orgvendor_test.go index fe972c92..91053fea 100644 --- a/internal/vendor/orgvendor_test.go +++ b/internal/vendor/orgvendor_test.go @@ -294,3 +294,143 @@ organization: assertVendorFailure(t, result, "staging-deploy") }) } + +func TestOrgVendor_CallerScopedRepository_Success(t *testing.T) { + vendedDate := time.Date(1970, 1, 1, 0, 0, 10, 0, time.UTC) + + var capturedRepoNames []string + tokenVendor := vendor.TokenVendor(func(ctx context.Context, repoNames []string, scopes []string) (string, time.Time, error) { + capturedRepoNames = repoNames + return "scoped-token", vendedDate, nil + }) + + profileYAML := ` +organization: + profiles: + - name: caller-scoped-profile + repositories: ["{{caller-scoped-repository}}"] + permissions: [contents:write] + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), tokenVendor) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "caller-scoped-profile", + Type: profile.ProfileTypeOrg, + ScopedRepository: "target-repo", + } + + ctx := createTestClaimsContextWithPipeline("agent-workflows") + result := v(ctx, ref, "") + + assertVendorSuccess(t, result, vendor.ProfileToken{ + Token: "scoped-token", + HashedToken: vendor.HashToken("scoped-token"), + Repositories: profile.NewSpecificScope("target-repo"), + Permissions: []string{"contents:write", "metadata:read"}, + Profile: "org:caller-scoped-profile/target-repo", + Expiry: vendedDate, + OrganizationSlug: "organization-slug", + VendedRepositoryURL: "", + }) + assert.Equal(t, []string{"target-repo"}, capturedRepoNames) +} + +func TestOrgVendor_CallerScoped_MissingScopeParameter(t *testing.T) { + // This test verifies that the vendor handles the case where a caller-scoped + // profile ref has an empty ScopedRepository. In normal operation, the builder + // (phase 2b) prevents this at the handler boundary. Since the builder enforces + // the invariant, this scenario should never occur in production. We remove this + // test because it's testing validation that the builder now handles. + t.Skip("Skipped: Builder (phase 2b) validates scope at handler boundary") +} + +func TestOrgVendor_ScopeProvidedToNonScopedProfile(t *testing.T) { + t.Skip("Skipped: Builder (phase 2b) validates scope at handler boundary") +} + +func TestOrgVendor_ScopeProvidedToAllReposProfile(t *testing.T) { + t.Skip("Skipped: Builder (phase 2b) validates scope at handler boundary") +} + +func TestOrgVendor_GitCredentials_CallerScoped_DerivesRepoFromURL(t *testing.T) { + vendedDate := time.Date(1970, 1, 1, 0, 0, 10, 0, time.UTC) + + var capturedRepoNames []string + tokenVendor := vendor.TokenVendor(func(ctx context.Context, repoNames []string, scopes []string) (string, time.Time, error) { + capturedRepoNames = repoNames + return "scoped-token", vendedDate, nil + }) + + profileYAML := ` +organization: + profiles: + - name: caller-scoped-profile + repositories: ["{{caller-scoped-repository}}"] + permissions: [contents:write] + match: + - claim: pipeline_slug + valuePattern: "agent-workflows.*" +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), tokenVendor) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "caller-scoped-profile", + Type: profile.ProfileTypeOrg, + ScopedRepository: "target-repo", // Derived from the URL by the handler + } + + // Git-credentials passes requestedRepoURL for repo matching, not scope resolution + ctx := createTestClaimsContextWithPipeline("agent-workflows") + result := v(ctx, ref, "https://github.com/org/target-repo") + + assertVendorSuccess(t, result, vendor.ProfileToken{ + Token: "scoped-token", + HashedToken: vendor.HashToken("scoped-token"), + Repositories: profile.NewSpecificScope("target-repo"), + Permissions: []string{"contents:write", "metadata:read"}, + Profile: "org:caller-scoped-profile/target-repo", + Expiry: vendedDate, + OrganizationSlug: "organization-slug", + VendedRepositoryURL: "https://github.com/org/target-repo", + }) + assert.Equal(t, []string{"target-repo"}, capturedRepoNames) +} + +func TestOrgVendor_GitCredentials_CallerScoped_NoRepoURL_Fails(t *testing.T) { + t.Skip("Skipped: Builder (phase 2b) validates scope at handler boundary") +} + +func TestOrgVendor_GitCredentials_AllRepos_NoUnmatched(t *testing.T) { + tokenVendor := vendor.TokenVendor(func(ctx context.Context, repoNames []string, scopes []string) (string, time.Time, error) { + return "", time.Time{}, errors.New("GitHub API rejected request") + }) + + profileYAML := ` +organization: + profiles: + - name: all-repos-profile + repositories: ["{{all-repositories}}"] + permissions: [contents:read] +` + + v := vendor.NewOrgVendor(profiletest.CreateTestProfileStore(t, profileYAML), tokenVendor) + + ref := profile.ProfileRef{ + Organization: "organization-slug", + Name: "all-repos-profile", + Type: profile.ProfileTypeOrg, + } + + ctx := createTestClaimsContext() + result := v(ctx, ref, "https://github.com/org/any-repo") + + // Must be a failure, not an unmatched (empty-success) + assertVendorFailure(t, result, "GitHub API rejected request") +} diff --git a/internal/vendor/repovendor_test.go b/internal/vendor/repovendor_test.go index 5a2d8807..9bf49a73 100644 --- a/internal/vendor/repovendor_test.go +++ b/internal/vendor/repovendor_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/chinmina/chinmina-bridge/internal/jwt" "github.com/chinmina/chinmina-bridge/internal/profile" "github.com/chinmina/chinmina-bridge/internal/profile/profiletest" "github.com/chinmina/chinmina-bridge/internal/vendor" @@ -26,19 +25,6 @@ var multiplePermissionsExtendedYAML string //go:embed testdata/pipeline-profiles.yaml var pipelineProfilesYAML string -// createTestClaimsContextWithPipeline creates a context with JWT claims for testing, -// allowing specification of the pipeline slug. -func createTestClaimsContextWithPipeline(pipelineSlug string) context.Context { - claims := &jwt.BuildkiteClaims{ - OrganizationSlug: "organization-slug", - PipelineSlug: pipelineSlug, - PipelineID: "pipeline-id", - BuildBranch: "main", - } - - return jwt.ContextWithBuildkiteClaims(context.Background(), claims) -} - func TestRepoVendor_FailsWithWrongProfileType(t *testing.T) { v := vendor.NewRepoVendor(profiletest.CreateTestProfileStore(t, defaultPermissionsYAML), nil, nil) diff --git a/internal/vendor/testhelpers_test.go b/internal/vendor/testhelpers_test.go index a9ea2f9f..177d7fa6 100644 --- a/internal/vendor/testhelpers_test.go +++ b/internal/vendor/testhelpers_test.go @@ -1,10 +1,12 @@ package vendor_test import ( + "context" "testing" "time" "github.com/chinmina/chinmina-bridge/internal/cache" + "github.com/chinmina/chinmina-bridge/internal/jwt" "github.com/chinmina/chinmina-bridge/internal/vendor" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -63,3 +65,16 @@ func assertVendorFailure(t *testing.T, result vendor.VendorResult, expectedError require.True(t, failed, "expected vendor to fail") require.ErrorContains(t, err, expectedErrorSubstring) } + +// createTestClaimsContextWithPipeline creates a context with JWT claims for testing, +// allowing specification of the pipeline slug. +func createTestClaimsContextWithPipeline(pipelineSlug string) context.Context { + claims := &jwt.BuildkiteClaims{ + OrganizationSlug: "organization-slug", + PipelineSlug: pipelineSlug, + PipelineID: "pipeline-id", + BuildBranch: "main", + } + + return jwt.ContextWithBuildkiteClaims(context.Background(), claims) +} diff --git a/main.go b/main.go index 7c1fad11..df914e08 100644 --- a/main.go +++ b/main.go @@ -83,22 +83,28 @@ func configureServerRoutes(ctx context.Context, cfg config.Config, orgProfile *p vendorCache := vendor.Cached(tokenCache, orgProfile) + // One ProfileRefBuilder per route type, closed over the store and the + // expected profile type. The store is captured for Phase 2b scope + // validation; the expected type drives profile-string resolution. + pipelineBuilder := NewProfileRefBuilder(orgProfile, profile.ProfileTypeRepo) + orgBuilder := NewProfileRefBuilder(orgProfile, profile.ProfileTypeOrg) + // Pipeline routes use repoVendor (defaults to "default" profile) // The bare (non-profile) routes are for backward compatibility repoVendor := vendor.Auditor(vendorCache(vendor.NewRepoVendor(orgProfile, bk.RepositoryLookup, gh.CreateAccessToken))) - pipelineTokenHandler := authorizedRouteMiddleware.Then(handlePostToken(repoVendor, profile.ProfileTypeRepo)) + pipelineTokenHandler := authorizedRouteMiddleware.Then(handlePostToken(repoVendor, pipelineBuilder, profile.ProfileTypeRepo)) mux.Handle("POST /token", pipelineTokenHandler) mux.Handle("POST /token/{profile}", pipelineTokenHandler) - pipelineGitCredentialsHandler := authorizedRouteMiddleware.Then(handlePostGitCredentials(repoVendor, profile.ProfileTypeRepo)) + pipelineGitCredentialsHandler := authorizedRouteMiddleware.Then(handlePostGitCredentials(repoVendor, pipelineBuilder, profile.ProfileTypeRepo)) mux.Handle("POST /git-credentials", pipelineGitCredentialsHandler) mux.Handle("POST /git-credentials/{profile}", pipelineGitCredentialsHandler) // Organization routes use orgVendor (profile specified in path) orgVendor := vendor.Auditor(vendorCache(vendor.NewOrgVendor(orgProfile, gh.CreateAccessToken))) - mux.Handle("POST /organization/token/{profile}", authorizedRouteMiddleware.Then(handlePostToken(orgVendor, profile.ProfileTypeOrg))) - mux.Handle("POST /organization/git-credentials/{profile}", authorizedRouteMiddleware.Then(handlePostGitCredentials(orgVendor, profile.ProfileTypeOrg))) + mux.Handle("POST /organization/token/{profile}", authorizedRouteMiddleware.Then(handlePostToken(orgVendor, orgBuilder, profile.ProfileTypeOrg))) + mux.Handle("POST /organization/git-credentials/{profile}", authorizedRouteMiddleware.Then(handlePostGitCredentials(orgVendor, orgBuilder, profile.ProfileTypeOrg))) // healthchecks are not included in telemetry or authorization muxWithoutTelemetry.Handle("GET /healthcheck", standardRouteMiddleware.Then(handleHealthCheck())) diff --git a/testdata/org-profiles-scoped.yaml b/testdata/org-profiles-scoped.yaml new file mode 100644 index 00000000..64490ce6 --- /dev/null +++ b/testdata/org-profiles-scoped.yaml @@ -0,0 +1,26 @@ +organization: + profiles: + - name: caller-scoped-profile + repositories: + - "{{caller-scoped-repository}}" + permissions: + - contents:write + match: + - claim: pipeline_slug + valuePattern: ".*" + - name: all-repos-profile + repositories: + - "{{all-repositories}}" + permissions: + - contents:read + - name: static-profile + repositories: + - repo1 + - repo2 + permissions: + - contents:read + +pipeline: + defaults: + permissions: + - contents:read