Skip to content

Commit 202cade

Browse files
fix: reject configured ambiguous repo actions
Resolve issue state changes from the query platform_host used by generated clients, while preserving the legacy body fallback. Reject unqualified repo lookups when configured tracked repositories are ambiguous even if SQLite currently has only one matching row, preventing maintainer mutations from guessing a host.\n\nVerified with:\n- go test ./internal/server -run 'TestAPISetIssueStateUsesPlatformHostBody|TestAPISetIssueStateUsesPlatformHostQuery|TestAPIApprovePRRejectsAmbiguousRepoBeforeMutation|TestAPIApprovePRRejectsAmbiguousTrackedRepoBeforeMutation' -shuffle=on\n- go test ./internal/server -shuffle=on\n- git diff --check\n\n🤖 Generated with [OpenAI Codex](https://openai.com/codex)\nCo-authored-by: OpenAI Codex <noreply@openai.com>
1 parent 9a996ba commit 202cade

3 files changed

Lines changed: 190 additions & 1 deletion

File tree

internal/server/api_test.go

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4881,6 +4881,121 @@ func TestAPISetIssueStateUsesPlatformHostBody(t *testing.T) {
48814881
assert.Equal("closed", ghesIssue.State)
48824882
}
48834883

4884+
func TestAPISetIssueStateUsesPlatformHostQuery(t *testing.T) {
4885+
require := require.New(t)
4886+
assert := Assert.New(t)
4887+
ctx := context.Background()
4888+
4889+
database, err := db.Open(
4890+
filepath.Join(t.TempDir(), "test.db"),
4891+
)
4892+
require.NoError(err)
4893+
t.Cleanup(func() { database.Close() })
4894+
4895+
seedIssueOnHost(
4896+
t, database,
4897+
"github.com", "acme", "widget", 7,
4898+
"open", "GitHub issue",
4899+
)
4900+
seedIssueOnHost(
4901+
t, database,
4902+
"ghe.example.com", "acme", "widget", 7,
4903+
"open", "GHES issue",
4904+
)
4905+
4906+
var githubEditCalls int
4907+
var ghesEditCalls int
4908+
githubClient := &mockGH{
4909+
editIssueFn: func(_ context.Context, _, _ string, _ int, _ string) (*gh.Issue, error) {
4910+
githubEditCalls++
4911+
return nil, errors.New("github.com client should not edit issue")
4912+
},
4913+
}
4914+
ghesClient := &mockGH{
4915+
editIssueFn: func(_ context.Context, _, _ string, number int, state string) (*gh.Issue, error) {
4916+
ghesEditCalls++
4917+
url := fmt.Sprintf("https://ghe.example.com/acme/widget/issues/%d", number)
4918+
createdAt := gh.Timestamp{Time: time.Now().UTC()}
4919+
updatedAt := gh.Timestamp{Time: time.Now().UTC()}
4920+
title := "GHES issue"
4921+
return &gh.Issue{
4922+
Number: &number,
4923+
Title: &title,
4924+
State: &state,
4925+
HTMLURL: &url,
4926+
CreatedAt: &createdAt,
4927+
UpdatedAt: &updatedAt,
4928+
User: &gh.User{Login: new("ghes-user")},
4929+
}, nil
4930+
},
4931+
}
4932+
4933+
syncer := ghclient.NewSyncer(
4934+
map[string]ghclient.Client{
4935+
"github.com": githubClient,
4936+
"ghe.example.com": ghesClient,
4937+
},
4938+
database,
4939+
nil,
4940+
[]ghclient.RepoRef{
4941+
{Owner: "acme", Name: "widget", PlatformHost: "github.com"},
4942+
{Owner: "acme", Name: "widget", PlatformHost: "ghe.example.com"},
4943+
},
4944+
time.Minute,
4945+
nil,
4946+
nil,
4947+
)
4948+
t.Cleanup(syncer.Stop)
4949+
4950+
srv := New(
4951+
database, syncer, nil, "/", nil, ServerOptions{},
4952+
)
4953+
t.Cleanup(func() {
4954+
shutdownCtx, cancel := context.WithTimeout(
4955+
context.Background(), 5*time.Second,
4956+
)
4957+
defer cancel()
4958+
_ = srv.Shutdown(shutdownCtx)
4959+
})
4960+
4961+
req := httptest.NewRequest(
4962+
http.MethodPost,
4963+
"/api/v1/repos/acme/widget/issues/7/github-state?platform_host=ghe.example.com",
4964+
strings.NewReader(`{"state":"closed"}`),
4965+
).WithContext(ctx)
4966+
req.Header.Set("Content-Type", "application/json")
4967+
rr := httptest.NewRecorder()
4968+
srv.ServeHTTP(rr, req)
4969+
4970+
require.Equal(http.StatusOK, rr.Code, rr.Body.String())
4971+
assert.Zero(githubEditCalls)
4972+
assert.Equal(1, ghesEditCalls)
4973+
4974+
githubRepo, err := database.GetRepoByHostOwnerName(
4975+
ctx, "github.com", "acme", "widget",
4976+
)
4977+
require.NoError(err)
4978+
require.NotNil(githubRepo)
4979+
githubIssue, err := database.GetIssueByRepoIDAndNumber(
4980+
ctx, githubRepo.ID, 7,
4981+
)
4982+
require.NoError(err)
4983+
require.NotNil(githubIssue)
4984+
assert.Equal("open", githubIssue.State)
4985+
4986+
ghesRepo, err := database.GetRepoByHostOwnerName(
4987+
ctx, "ghe.example.com", "acme", "widget",
4988+
)
4989+
require.NoError(err)
4990+
require.NotNil(ghesRepo)
4991+
ghesIssue, err := database.GetIssueByRepoIDAndNumber(
4992+
ctx, ghesRepo.ID, 7,
4993+
)
4994+
require.NoError(err)
4995+
require.NotNil(ghesIssue)
4996+
assert.Equal("closed", ghesIssue.State)
4997+
}
4998+
48844999
// TestAPIIssueDataFromGraphQLSync verifies the API correctly serves
48855000
// issue data that was persisted by the GraphQL sync path. The sync
48865001
// path itself (GraphQL fetch → normalize → DB upsert) is tested in
@@ -6936,6 +7051,43 @@ func TestAPIApprovePRRejectsAmbiguousRepoBeforeMutation(t *testing.T) {
69367051
assert.Zero(reviewCalls)
69377052
}
69387053

7054+
func TestAPIApprovePRRejectsAmbiguousTrackedRepoBeforeMutation(t *testing.T) {
7055+
assert := Assert.New(t)
7056+
require := require.New(t)
7057+
var reviewCalls int
7058+
mock := &mockGH{
7059+
createReviewFn: func(_ context.Context, _, _ string, _ int, _, body string) (*gh.PullRequestReview, error) {
7060+
reviewCalls++
7061+
id := int64(42)
7062+
state := "APPROVED"
7063+
submittedAt := gh.Timestamp{Time: time.Now().UTC()}
7064+
return &gh.PullRequestReview{
7065+
ID: &id,
7066+
State: &state,
7067+
Body: &body,
7068+
SubmittedAt: &submittedAt,
7069+
User: &gh.User{Login: new("reviewer")},
7070+
}, nil
7071+
},
7072+
}
7073+
srv, database := setupTestServerWithRepos(t, mock, []ghclient.RepoRef{
7074+
{Owner: "acme", Name: "widget", PlatformHost: "github.com"},
7075+
{Owner: "acme", Name: "widget", PlatformHost: "ghe.example.com"},
7076+
})
7077+
seedPROnHost(t, database, "github.com", "acme", "widget", 1)
7078+
7079+
rr := doJSON(
7080+
t,
7081+
srv,
7082+
http.MethodPost,
7083+
"/api/v1/repos/acme/widget/pulls/1/approve",
7084+
map[string]string{"body": "ship it"},
7085+
)
7086+
7087+
require.Equal(http.StatusBadRequest, rr.Code)
7088+
assert.Zero(reviewCalls)
7089+
}
7090+
69397091
func TestAPISetPRGitHubStateRejectsAmbiguousRepoBeforeMutation(t *testing.T) {
69407092
assert := Assert.New(t)
69417093
require := require.New(t)

internal/server/helpers.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,35 @@ func (s *Server) lookupRepo(
6767
ctx context.Context,
6868
owner, name, platformHost string,
6969
) (*db.Repo, error) {
70+
if err := s.requireTrackedRepoUnambiguous(owner, name, platformHost); err != nil {
71+
return nil, err
72+
}
7073
return s.repoIdentity().LookupRepo(ctx, repoIdentityRef{
7174
owner: owner,
7275
name: name,
7376
platformHost: platformHost,
7477
})
7578
}
7679

80+
func (s *Server) requireTrackedRepoUnambiguous(
81+
owner, name, platformHost string,
82+
) error {
83+
if strings.TrimSpace(platformHost) == "" &&
84+
s.syncer.IsTrackedRepoAmbiguous(owner, name) {
85+
return errRepoAmbiguous
86+
}
87+
return nil
88+
}
89+
90+
func firstNonEmpty(values ...string) string {
91+
for _, value := range values {
92+
if trimmed := strings.TrimSpace(value); trimmed != "" {
93+
return trimmed
94+
}
95+
}
96+
return ""
97+
}
98+
7799
func (s *Server) filterConfiguredRepoSummaries(
78100
summaries []db.RepoSummary,
79101
) []db.RepoSummary {
@@ -89,18 +111,33 @@ func (s *Server) filterConfiguredRepoSummaries(
89111

90112
// lookupMRID resolves the internal MR id from the common route tuple.
91113
func (s *Server) lookupMRID(ctx context.Context, ref repoNumberPathRef) (int64, error) {
114+
if err := s.requireTrackedRepoUnambiguous(
115+
ref.owner, ref.name, ref.platformHost,
116+
); err != nil {
117+
return 0, err
118+
}
92119
return s.repoIdentity().LookupMRID(ctx, ref)
93120
}
94121

95122
// lookupIssueID resolves the internal issue id from the common route tuple.
96123
func (s *Server) lookupIssueID(ctx context.Context, ref repoNumberPathRef) (int64, error) {
124+
if err := s.requireTrackedRepoUnambiguous(
125+
ref.owner, ref.name, ref.platformHost,
126+
); err != nil {
127+
return 0, err
128+
}
97129
return s.repoIdentity().LookupIssueID(ctx, ref)
98130
}
99131

100132
func (s *Server) lookupIssue(
101133
ctx context.Context,
102134
ref repoNumberPathRef,
103135
) (*db.Repo, *db.Issue, error) {
136+
if err := s.requireTrackedRepoUnambiguous(
137+
ref.owner, ref.name, ref.platformHost,
138+
); err != nil {
139+
return nil, nil, err
140+
}
104141
return s.repoIdentity().LookupIssue(ctx, ref)
105142
}
106143

internal/server/huma_routes.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1899,7 +1899,7 @@ func (s *Server) setIssueGitHubState(
18991899
owner: input.Owner,
19001900
name: input.Name,
19011901
number: input.Number,
1902-
platformHost: input.Body.PlatformHost,
1902+
platformHost: firstNonEmpty(input.PlatformHost, input.Body.PlatformHost),
19031903
})
19041904
if err != nil {
19051905
if errors.Is(err, errRepoAmbiguous) {

0 commit comments

Comments
 (0)