Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions pkg/api/aggregate_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package api
import (
"fmt"
"strings"

"github.com/block/schemabot/pkg/storage"
)

// Aggregate-check roles for a repository. In a multi-tenant aggregate check,
Expand Down Expand Up @@ -65,7 +67,7 @@ func (c *ServerConfig) AggregateRoleForRepo(repo string) string {
if c == nil {
return ""
}
repoConfig, ok := c.Repos[repo]
repoConfig, ok := c.Repos[storage.CanonicalKey(repo)]
if !ok || repoConfig.Aggregate == nil {
return ""
}
Expand Down Expand Up @@ -107,7 +109,7 @@ func (c *ServerConfig) ExpectedParticipantChecksForPR(repo string, changedFiles
if c == nil {
return nil
}
repoConfig, ok := c.Repos[repo]
repoConfig, ok := c.Repos[storage.CanonicalKey(repo)]
if !ok || !repoConfig.Aggregate.isLeader() {
return nil
}
Expand Down
45 changes: 39 additions & 6 deletions pkg/api/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -1204,7 +1204,7 @@ type RepoConfig struct {
// RepoAdmins returns the repository-scoped admin principals configured for
// repo. A repository with no config entry has no repo admins.
func (c *ServerConfig) RepoAdmins(repo string) (teams, users []string) {
repoConfig, ok := c.Repos[repo]
repoConfig, ok := c.Repos[storage.CanonicalKey(repo)]
if !ok {
return nil, nil
}
Expand Down Expand Up @@ -1339,13 +1339,46 @@ func LoadServerConfigFromFile(path string) (*ServerConfig, error) {
return nil, fmt.Errorf("parse config file: %w", err)
}

if err := config.canonicalizeRepositories(); err != nil {
return nil, fmt.Errorf("invalid config: %w", err)
}
if err := config.Validate(); err != nil {
return nil, fmt.Errorf("invalid config: %w", err)
}

return &config, nil
}

func (c *ServerConfig) canonicalizeRepositories() error {
repoNames := make([]string, 0, len(c.Repos))
for repo := range c.Repos {
repoNames = append(repoNames, repo)
}
slices.Sort(repoNames)

canonicalRepos := make(map[string]RepoConfig, len(c.Repos))
originalNames := make(map[string]string, len(c.Repos))
for _, repo := range repoNames {
canonical := storage.CanonicalKey(repo)
if original, ok := originalNames[canonical]; ok {
return fmt.Errorf("repos contains keys %q and %q that canonicalize to %q", original, repo, canonical)
}
canonicalRepos[canonical] = c.Repos[repo]
originalNames[canonical] = repo
}
if c.Repos != nil {
c.Repos = canonicalRepos
}

for database, dbConfig := range c.Databases {
for i, repo := range dbConfig.AllowedRepos {
dbConfig.AllowedRepos[i] = storage.CanonicalKey(repo)
}
c.Databases[database] = dbConfig
}
return nil
}

// Validate checks the configuration for required fields and consistency.
func (c *ServerConfig) Validate() error {
// The database registry is required for the control plane and for a
Expand Down Expand Up @@ -2349,7 +2382,7 @@ func (c *ServerConfig) IsRepoAllowed(repo string) bool {
if c == nil || len(c.Repos) == 0 {
return true
}
_, ok := c.Repos[repo]
_, ok := c.Repos[storage.CanonicalKey(repo)]
return ok
}

Expand All @@ -2360,7 +2393,7 @@ func (c *ServerConfig) AreChecksEnabled(repo string) bool {
if c == nil || len(c.Repos) == 0 {
return true
}
repoConfig, ok := c.Repos[repo]
repoConfig, ok := c.Repos[storage.CanonicalKey(repo)]
if !ok || repoConfig.EnableChecks == nil {
return true
}
Expand Down Expand Up @@ -2411,7 +2444,7 @@ func (c *ServerConfig) ResolveGitHubAppForRepo(repo string) (ResolvedGitHubApp,
return ResolvedGitHubApp{}, fmt.Errorf("server config is nil")
}
if len(c.Apps) > 0 {
repoConfig, ok := c.Repos[repo]
repoConfig, ok := c.Repos[storage.CanonicalKey(repo)]
if !ok {
return ResolvedGitHubApp{}, fmt.Errorf("repository %q: %w", repo, ErrRepoNotConfigured)
}
Expand Down Expand Up @@ -2682,7 +2715,7 @@ func (c *ServerConfig) GitHubCheckNameBaseForRepo(repo string) string {
return DefaultGitHubCheckName
}
if len(c.Apps) > 0 {
repoConfig, ok := c.Repos[repo]
repoConfig, ok := c.Repos[storage.CanonicalKey(repo)]
if !ok {
return DefaultGitHubCheckName
}
Expand All @@ -2704,7 +2737,7 @@ func (c *ServerConfig) PromotionCheckNameBaseForRepo(repo string) string {
return DefaultGitHubCheckName
}
if len(c.Apps) > 0 {
repoConfig, ok := c.Repos[repo]
repoConfig, ok := c.Repos[storage.CanonicalKey(repo)]
if !ok {
return DefaultGitHubCheckName
}
Expand Down
51 changes: 51 additions & 0 deletions pkg/api/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,55 @@ repos:
assert.False(t, cfg.AreChecksEnabled("org/repo"))
}

func TestLoadServerConfigFromFileCanonicalizesRepositories(t *testing.T) {
configPath := filepath.Join(t.TempDir(), "config.yaml")
content := `
databases:
testapp:
type: mysql
allowed_repos:
- MixedCase/Sample-Repo
environments:
staging:
target: testapp-staging
deployment: default
tern_deployments:
default:
staging: tern-staging:9090
repos:
MixedCase/Sample-Repo: {}
`
require.NoError(t, os.WriteFile(configPath, []byte(content), 0644))

cfg, err := LoadServerConfigFromFile(configPath)
require.NoError(t, err)
assert.Contains(t, cfg.Repos, "mixedcase/sample-repo")
assert.NotContains(t, cfg.Repos, "MixedCase/Sample-Repo")
assert.Equal(t, []string{"mixedcase/sample-repo"}, cfg.Databases["testapp"].AllowedRepos)
}

func TestLoadServerConfigFromFileRejectsCanonicalRepositoryCollision(t *testing.T) {
configPath := filepath.Join(t.TempDir(), "config.yaml")
content := `
databases:
testapp:
type: mysql
environments:
staging:
target: testapp-staging
deployment: default
repos:
MixedCase/Sample-Repo: {}
mixedcase/sample-repo: {}
`
require.NoError(t, os.WriteFile(configPath, []byte(content), 0644))

_, err := LoadServerConfigFromFile(configPath)
require.Error(t, err)
assert.ErrorContains(t, err, `"MixedCase/Sample-Repo"`)
assert.ErrorContains(t, err, `"mixedcase/sample-repo"`)
}

func TestLoadServerConfigFromFile_DSNFrom(t *testing.T) {
dir := t.TempDir()
configPath := filepath.Join(dir, "config.yaml")
Expand Down Expand Up @@ -2970,6 +3019,8 @@ func TestServerConfig_IsRepoAllowed(t *testing.T) {
},
}
assert.True(t, cfg.IsRepoAllowed("org/allowed-repo"))
assert.True(t, cfg.IsRepoAllowed("ORG/ALLOWED-REPO"))
assert.True(t, cfg.IsRepoAllowed("Org/Allowed-Repo"))
})

t.Run("populated repos rejects unlisted repo", func(t *testing.T) {
Expand Down
8 changes: 6 additions & 2 deletions pkg/api/source_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"
"path"
"strings"

"github.com/block/schemabot/pkg/storage"
)

const (
Expand Down Expand Up @@ -217,9 +219,11 @@ func validateAllowedRepos(field string, repos []string) error {
}

func repoAllowed(allowedRepos []string, repo string) bool {
repo = strings.TrimSpace(repo)
repo = storage.CanonicalKey(strings.TrimSpace(repo))
for _, allowed := range allowedRepos {
switch strings.TrimSpace(allowed) {
// Entries loaded through config are already canonical; folding here
// too keeps the match correct for allow-lists built any other way.
switch storage.CanonicalKey(strings.TrimSpace(allowed)) {
case "*":
return true
case repo:
Expand Down
6 changes: 6 additions & 0 deletions pkg/api/source_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ func TestRepoAllowed(t *testing.T) {
repository: " octocat/hello-world ",
want: true,
},
{
name: "uppercase allow-list entry matches canonical request",
allowedRepos: []string{"Octocat/Hello-World"},
repository: "octocat/hello-world",
want: true,
},
{
name: "similar repo name is blocked",
allowedRepos: []string{"octocat/hello-world"},
Expand Down
25 changes: 25 additions & 0 deletions pkg/storage/canonical_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package storage

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestCanonicalKey(t *testing.T) {
tests := []struct {
name string
key string
want string
}{
{name: "mixed case", key: "MixedCase/Sample-Repo", want: "mixedcase/sample-repo"},
{name: "lowercase", key: "mixedcase/sample-repo", want: "mixedcase/sample-repo"},
{name: "empty", key: "", want: ""},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, CanonicalKey(tt.key))
})
}
}
2 changes: 2 additions & 0 deletions pkg/webhook/check_run.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/http"

"github.com/block/schemabot/pkg/metrics"
"github.com/block/schemabot/pkg/storage"
)

type checkRunPayload struct {
Expand Down Expand Up @@ -34,6 +35,7 @@ func (h *Handler) handleCheckRun(ctx context.Context, metricApp string, w http.R
h.writeError(w, http.StatusBadRequest, "invalid check_run payload")
return
}
payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName)

switch payload.Action {
case "rerequested":
Expand Down
3 changes: 2 additions & 1 deletion pkg/webhook/check_suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func (h *Handler) handleCheckSuite(ctx context.Context, metricApp string, w http
h.writeError(w, http.StatusBadRequest, "invalid check_suite payload")
return
}
payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName)
repo := payload.Repository.FullName
headSHA := payload.CheckSuite.HeadSHA

Expand Down Expand Up @@ -210,7 +211,7 @@ func (h *Handler) processDurableCheckSuite(ctx context.Context, event *storage.W
"delivery_id", event.DeliveryID, "repo", event.Repository, "head_sha", event.HeadSHA)
return false, nil
}
repo := event.Repository
repo := storage.CanonicalKey(event.Repository)
headSHA := event.HeadSHA
if repo == "" || headSHA == "" {
return false, fmt.Errorf("durable check_suite delivery %s missing repo or head SHA", event.DeliveryID)
Expand Down
42 changes: 42 additions & 0 deletions pkg/webhook/check_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,27 @@ func TestCheckSuiteWebhookQueuesWithGrace(t *testing.T) {
assert.WithinDuration(t, before.Add(defaultCheckSuiteRecoveryGrace), *row.RetryAfter, 10*time.Second)
}

func TestCheckSuiteWebhookCanonicalizesRepository(t *testing.T) {
store := newRecordingWebhookEventStore()
h := newCheckSuiteIngressHandler(t, store, map[string]api.RepoConfig{"mixedcase/sample-repo": {}})

req := buildCheckSuiteWebhookRequest(t, "requested", "MixedCaseSHA", "MixedCaseBranch", 7)
body, err := io.ReadAll(req.Body)
require.NoError(t, err)
req.Body = io.NopCloser(strings.NewReader(strings.ReplaceAll(string(body), "octocat/hello-world", "MixedCase/Sample-Repo")))
req.Header.Set(headerDeliveryID, "mixed-case-check-suite")
rr := httptest.NewRecorder()

h.ServeHTTP(rr, req)

require.Equal(t, http.StatusOK, rr.Code)
row, err := store.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, "mixed-case-check-suite")
require.NoError(t, err)
require.NotNil(t, row)
assert.Equal(t, "mixedcase/sample-repo", row.Repository)
assert.Equal(t, "MixedCaseSHA", row.HeadSHA)
}

// Only "requested" carries recovery work: "rerequested" re-plans through
// check_run.rerequested and "completed" is pure noise, so neither may occupy
// an inbox row.
Expand Down Expand Up @@ -428,6 +449,27 @@ func TestDurableCheckSuiteSynthesizesMissingCoverage(t *testing.T) {
assert.Equal(t, "12345", row.TenantID)
}

// A durable row is normalized before config and GitHub routing so replayed
// rows from any producer use the same repository identity as ingress rows.
func TestDurableCheckSuiteCanonicalizesStoredRepository(t *testing.T) {
store := newRecordingWebhookEventStore()
h, mux := newCheckSuiteProcessHandler(t, store, map[string]api.RepoConfig{"octocat/hello-world": {}})
mux.HandleFunc("/repos/octocat/hello-world/pulls/7", func(w http.ResponseWriter, _ *http.Request) {
writeSinglePR(t, w, 7, "open", "suite-sha")
})
event := durableCheckSuiteEvent(t, "suite-sha", 7)
event.Repository = "OctoCat/Hello-World"

retry, err := h.processDurableCheckSuite(t.Context(), event)

require.NoError(t, err)
require.False(t, retry)
row, err := store.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, synthesizedDeliveryGUID("octocat/hello-world", 7, "suite-sha"))
require.NoError(t, err)
require.NotNil(t, row)
assert.Equal(t, "octocat/hello-world", row.Repository)
}

// Every payload PR still open at the suite head gets its own PR-scoped
// recovery row — multiple PRs can share one head SHA.
func TestDurableCheckSuiteSynthesizesPerPR(t *testing.T) {
Expand Down
12 changes: 6 additions & 6 deletions pkg/webhook/durable_dispatch.go
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,7 @@ func (h *Handler) processDurablePullRequest(ctx context.Context, event *storage.
// delivery under the delivery lease. runPRCloseCleanup is idempotent, so a
// retry after a partial cleanup reconciles rather than double-acts.
func (h *Handler) processDurablePullRequestClosed(ctx context.Context, event *storage.WebhookEvent, payload pullRequestPayload) (retry bool, err error) {
repo := payload.Repository.FullName
repo := storage.CanonicalKey(payload.Repository.FullName)
pr := payload.PullRequest.Number
if repo == "" || pr == 0 {
return false, fmt.Errorf("durable pull_request closed delivery %s missing repo or PR", event.DeliveryID)
Expand All @@ -700,7 +700,7 @@ func (h *Handler) processDurablePullRequestAutoPlan(ctx context.Context, event *
return false, err
}

repo := payload.Repository.FullName
repo := storage.CanonicalKey(payload.Repository.FullName)
pr := payload.PullRequest.Number
headSHA := payload.PullRequest.Head.SHA
if repo == "" || pr == 0 || headSHA == "" {
Expand Down Expand Up @@ -792,7 +792,7 @@ func (h *Handler) processDurableCheckRun(ctx context.Context, event *storage.Web
// participant convergence returns no error and is handed to the in-memory
// re-fold budget through the shared follow-up, exactly as the wrapper does.
func (h *Handler) processDurableCheckRunCompleted(ctx context.Context, event *storage.WebhookEvent, payload checkRunPayload) (retry bool, err error) {
repo := payload.Repository.FullName
repo := storage.CanonicalKey(payload.Repository.FullName)
if h.service == nil || !h.service.Config().IsAggregateLeaderForRepo(repo) {
h.logger.Info("durable check_run completion ignored because deployment is not the aggregate leader",
"delivery_id", event.DeliveryID, "repo", repo, "pr", event.PullRequest,
Expand Down Expand Up @@ -877,7 +877,7 @@ func (h *Handler) processDurableCheckRunCompleted(ctx context.Context, event *st
// delivery. A GitHub failure verifying the head keeps the delivery retryable
// rather than completing it, so a transient outage cannot drop the re-plan.
func (h *Handler) processDurableCheckRunRerequest(ctx context.Context, event *storage.WebhookEvent, payload checkRunPayload) (retry bool, err error) {
repo := payload.Repository.FullName
repo := storage.CanonicalKey(payload.Repository.FullName)
pr, ok := checkRunPullRequestNumber(payload)
if !ok {
h.logger.Info("durable check_run rerequest ignored without pull request",
Expand Down Expand Up @@ -970,7 +970,7 @@ func (h *Handler) enqueueDurablePullRequest(ctx context.Context, payload pullReq
DeliveryID: deliveryID,
Event: "pull_request",
Action: payload.Action,
Repository: payload.Repository.FullName,
Repository: storage.CanonicalKey(payload.Repository.FullName),
PullRequest: payload.PullRequest.Number,
HeadSHA: payload.PullRequest.Head.SHA,
TenantID: strconv.FormatInt(installationID, 10),
Expand All @@ -984,7 +984,7 @@ func (h *Handler) enqueueDurableCheckRun(ctx context.Context, payload checkRunPa
DeliveryID: deliveryID,
Event: "check_run",
Action: payload.Action,
Repository: payload.Repository.FullName,
Repository: storage.CanonicalKey(payload.Repository.FullName),
PullRequest: pr,
HeadSHA: payload.CheckRun.HeadSHA,
TenantID: strconv.FormatInt(installationID, 10),
Expand Down
Loading
Loading