Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
98aef1f
feat(repos): add confirmed repository deletion
SamMorrowDrums Aug 14, 2026
c34055e
refactor(inventory): generalize tool availability guards
SamMorrowDrums Aug 17, 2026
14aea52
feat(http): protect MRTR request state
SamMorrowDrums Aug 17, 2026
676a5b2
fix(repos): expire deletion confirmations
SamMorrowDrums Aug 17, 2026
8eba6fd
Merge remote-tracking branch 'origin/main' into sammorrowdrums-add-de…
SamMorrowDrums Aug 17, 2026
c285c6a
fix(http): preserve tool and scope restrictions
SamMorrowDrums Aug 17, 2026
2dce589
fix(repos): require protected confirmation state
SamMorrowDrums Aug 18, 2026
cceb5cf
fix(oauth): request repository deletion scope
SamMorrowDrums Aug 18, 2026
ec7bd6f
fix(oauth): require deletion scope opt-in
SamMorrowDrums Aug 18, 2026
f3a32d6
refactor(oauth): derive scope sets from catalog
SamMorrowDrums Aug 18, 2026
1dd5f33
refactor(scopes): own OAuth scope catalog
SamMorrowDrums Aug 18, 2026
213d53a
fix(scopes): require workflow scope opt-in
SamMorrowDrums Aug 18, 2026
60f2768
Merge remote-tracking branch 'origin/main' into sammorrowdrums-add-de…
SamMorrowDrums Aug 18, 2026
345459d
Merge remote-tracking branch 'origin/main' into sammorrowdrums-add-de…
SamMorrowDrums Aug 18, 2026
560b0ea
Merge remote-tracking branch 'origin/main' into sammorrowdrums-add-de…
SamMorrowDrums Aug 18, 2026
5eac1f0
Merge remote-tracking branch 'origin/main' into sammorrowdrums-add-de…
SamMorrowDrums Aug 18, 2026
5ea9a0e
Merge remote-tracking branch 'origin/main' into sammorrowdrums-add-de…
SamMorrowDrums Aug 18, 2026
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
Prev Previous commit
Next Next commit
fix(repos): expire deletion confirmations
Bind sealed repository deletion state to the immutable repository ID and a ten-minute expiry. Re-check identity before deletion so replay cannot affect a recreated repository.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
  • Loading branch information
SamMorrowDrums committed Aug 17, 2026
commit 676a5b254b30747207e5a90190a63bb16bb9f051
55 changes: 52 additions & 3 deletions pkg/github/repositories.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"slices"
"strconv"
"strings"
"time"

ghErrors "github.com/github/github-mcp-server/pkg/errors"
"github.com/github/github-mcp-server/pkg/ifc"
Expand Down Expand Up @@ -708,11 +709,14 @@ const (
DeleteRepositoryToolName = "delete_repository"
deleteRepositoryConfirmationID = "delete_repository_confirmation"
deleteRepositoryConfirmationField = "repository_name"
deleteRepositoryConfirmationTTL = 10 * time.Minute
)

type deleteRepositoryState struct {
Owner string `json:"owner"`
Repo string `json:"repo"`
Owner string `json:"owner"`
Repo string `json:"repo"`
RepositoryID int64 `json:"repository_id"`
ExpiresAt int64 `json:"expires_at"`
}

// DeleteRepository creates a tool that deletes a GitHub repository after the
Expand Down Expand Up @@ -756,6 +760,7 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool

fullName := owner + "/" + repo
sealer := requestStateSealerFromDeps(deps)
var deletionState *deleteRepositoryState
var responses mcp.InputResponseMap
if req != nil && req.Params != nil {
responses = req.Params.InputResponses
Expand All @@ -764,7 +769,20 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool
if !ok {
var requestState string
if sealer != nil {
state, err := json.Marshal(deleteRepositoryState{Owner: owner, Repo: repo})
client, err := deps.GetClient(ctx)
if err != nil {
return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err)
}
repositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo)
if result != nil {
return result, nil, nil
}
state, err := json.Marshal(deleteRepositoryState{
Owner: owner,
Repo: repo,
RepositoryID: repositoryID,
ExpiresAt: time.Now().Add(deleteRepositoryConfirmationTTL).Unix(),
})
if err != nil {
return nil, nil, fmt.Errorf("failed to marshal repository deletion state: %w", err)
}
Expand Down Expand Up @@ -810,6 +828,13 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool
if state.Owner != owner || state.Repo != repo {
return utils.NewToolResultError("Repository deletion target changed after confirmation was requested. The repository was not deleted."), nil, nil
}
if state.ExpiresAt <= time.Now().Unix() {
return utils.NewToolResultError("Repository deletion confirmation expired. The repository was not deleted."), nil, nil
}
if state.RepositoryID == 0 {
return utils.NewToolResultError("Repository deletion confirmation state was invalid. The repository was not deleted."), nil, nil
}
deletionState = &state
}

confirmation, ok := response.(*mcp.ElicitResult)
Expand All @@ -828,6 +853,15 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool
if err != nil {
return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err)
}
if deletionState != nil {
currentRepositoryID, result := repositoryIDForDeletion(ctx, client, owner, repo)
if result != nil {
return result, nil, nil
}
if currentRepositoryID != deletionState.RepositoryID {
return utils.NewToolResultError("Repository identity changed after confirmation was requested. The repository was not deleted."), nil, nil
}
}
resp, err := client.Repositories.Delete(ctx, owner, repo)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx,
Expand All @@ -854,6 +888,21 @@ func DeleteRepository(t translations.TranslationHelperFunc) inventory.ServerTool
return tool
}

func repositoryIDForDeletion(ctx context.Context, client *github.Client, owner, repo string) (int64, *mcp.CallToolResult) {
repository, resp, err := client.Repositories.Get(ctx, owner, repo)
if err != nil {
return 0, ghErrors.NewGitHubAPIErrorResponse(ctx,
fmt.Sprintf("failed to get repository: %s/%s", owner, repo),
resp,
err,
)
}
if resp != nil && resp.Body != nil {
defer func() { _ = resp.Body.Close() }()
}
return repository.GetID(), nil
}

// FetchRepoIsPrivate returns whether a repository is private. It is a thin
// wrapper around the GitHub Repositories.Get endpoint provided as a shared
// helper for IFC label computation across tools.
Expand Down
89 changes: 88 additions & 1 deletion pkg/github/repositories_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3042,6 +3042,10 @@ func Test_DeleteRepository(t *testing.T) {
sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef")))
require.NoError(t, err)
client := NewMockedHTTPClient(
WithRequestMatchHandler(
GetReposByOwnerByRepo,
mockResponse(t, http.StatusOK, map[string]any{"id": 123}),
),
WithRequestMatchHandler(
DeleteReposByOwnerByRepo,
mockResponse(t, http.StatusNoContent, nil),
Expand Down Expand Up @@ -3099,15 +3103,54 @@ func Test_DeleteRepository(t *testing.T) {
assert.Contains(t, getErrorResult(t, result).Text, "state was invalid")
})

t.Run("refuses a changed deletion target", func(t *testing.T) {
t.Run("refuses expired deletion state", func(t *testing.T) {
sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef")))
require.NoError(t, err)
stateJSON, err := json.Marshal(deleteRepositoryState{
Owner: "owner",
Repo: "repo",
RepositoryID: 123,
ExpiresAt: time.Now().Add(-time.Minute).Unix(),
})
require.NoError(t, err)
state, err := sealer.Seal(context.Background(), stateJSON)
require.NoError(t, err)
deps := BaseDeps{
Client: mustNewGHClient(t, NewMockedHTTPClient()),
StateSealer: sealer,
}
handler := serverTool.Handler(deps)

request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"})
request.Params.RequestState = state
request.Params.InputResponses = mcp.InputResponseMap{
deleteRepositoryConfirmationID: &mcp.ElicitResult{
Action: "accept",
Content: map[string]any{
deleteRepositoryConfirmationField: "owner/repo",
},
},
}
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
require.NoError(t, err)
require.True(t, result.IsError)
assert.Contains(t, getErrorResult(t, result).Text, "confirmation expired")
})

t.Run("refuses a changed deletion target", func(t *testing.T) {
sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef")))
require.NoError(t, err)
deps := BaseDeps{
Client: mustNewGHClient(t, NewMockedHTTPClient(
WithRequestMatchHandler(
GetReposByOwnerByRepo,
mockResponse(t, http.StatusOK, map[string]any{"id": 123}),
),
)),
StateSealer: sealer,
}
handler := serverTool.Handler(deps)

firstRequest := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"})
firstResult, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest)
require.NoError(t, err)
Expand All @@ -3128,6 +3171,50 @@ func Test_DeleteRepository(t *testing.T) {
assert.Contains(t, getErrorResult(t, result).Text, "target changed")
})

t.Run("refuses a recreated repository", func(t *testing.T) {
sealer, err := requeststate.New(base64.StdEncoding.EncodeToString([]byte("0123456789abcdef0123456789abcdef")))
require.NoError(t, err)
var repositoryLookups int
client := NewMockedHTTPClient(
WithRequestMatchHandler(
GetReposByOwnerByRepo,
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
repositoryLookups++
id := 123
if repositoryLookups > 1 {
id = 456
}
w.WriteHeader(http.StatusOK)
require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"id": id}))
}),
),
)
deps := BaseDeps{
Client: mustNewGHClient(t, client),
StateSealer: sealer,
}
handler := serverTool.Handler(deps)

firstRequest := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"})
firstResult, err := handler(ContextWithDeps(context.Background(), deps), &firstRequest)
require.NoError(t, err)

retry := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"})
retry.Params.RequestState = firstResult.RequestState
retry.Params.InputResponses = mcp.InputResponseMap{
deleteRepositoryConfirmationID: &mcp.ElicitResult{
Action: "accept",
Content: map[string]any{
deleteRepositoryConfirmationField: "owner/repo",
},
},
}
result, err := handler(ContextWithDeps(context.Background(), deps), &retry)
require.NoError(t, err)
require.True(t, result.IsError)
assert.Contains(t, getErrorResult(t, result).Text, "identity changed")
})

t.Run("completes multi-round-trip elicitation before deleting", func(t *testing.T) {
Comment thread
SamMorrowDrums marked this conversation as resolved.
httpClient := NewMockedHTTPClient(
WithRequestMatchHandler(
Expand Down