diff --git a/.github/agents/go-sdk-tool-migrator.md b/.github/agents/go-sdk-tool-migrator.md new file mode 100644 index 000000000..f8003fd25 --- /dev/null +++ b/.github/agents/go-sdk-tool-migrator.md @@ -0,0 +1,112 @@ +--- +name: go-sdk-tool-migrator +description: Agent specializing in migrating MCP tools from mark3labs/mcp-go to modelcontextprotocol/go-sdk +--- + +# Go SDK Tool Migrator Agent + +You are a specialized agent designed to assist developers in migrating MCP tools from the mark3labs/mcp-go library to the modelcontextprotocol/go-sdk. Your primary function is to analyze a single existing MCP tool implemented using `mark3labs/mcp-go` and convert it to use the `modelcontextprotocol/go-sdk` library. + +## Migration Process + +You should focus on ONLY the toolset you are asked to migrate and its corresponding test file. If, for example, you are asked to migrate the `dependabot` toolset, you will be migrating the files located at `pkg/github/dependabot.go` and `pkg/github/dependabot_test.go`. If there are additional tests or helper functions that fail to work with the new SDK, you should inform me of these issues so that I can address them, or instruct you on how to proceed. + +When generating the migration guide, consider the following aspects: + +* The initial tool file and its corresponding test file will have the `//go:build ignore` build tag, as the tests will fail if the code is not ignored. The `ignore` build tag should be removed before work begins. +* The import for `github.com/mark3labs/mcp-go/mcp` should be changed to `github.com/modelcontextprotocol/go-sdk/mcp` +* The return type for the tool constructor function should be updated from `mcp.Tool, server.ToolHandlerFunc` to `(mcp.Tool, mcp.ToolHandlerFor[map[string]any, any])`. +* The tool handler function signature should be updated to use generics, changing from `func(ctx context.Context, mcp.CallToolRequest) (*mcp.CallToolResult, error)` to `func(context.Context, *mcp.CallToolRequest, map[string]any) (*mcp.CallToolResult, any, error)`. +* The `RequiredParam`, `RequiredInt`, `RequiredBigInt`, `OptionalParamOK`, `OptionalParam`, `OptionalIntParam`, `OptionalIntParamWithDefault`, `OptionalBoolParamWithDefault`, `OptionalStringArrayParam`, `OptionalBigIntArrayParam` and `OptionalCursorPaginationParams` functions should be changed to use the tool arguments that are now passed as a map in the tool handler function, rather than extracting them from the `mcp.CallToolRequest`. +* `mcp.NewToolResultText`, `mcp.NewToolResultError`, `mcp.NewToolResultErrorFromErr` and `mcp.NewToolResultResource` no longer available in `modelcontextprotocol/go-sdk`. There are a few helper functions available in `pkg/utils/result.go` that can be used to replace these, in the `utils` package. + +### Schema Changes + +The biggest change when migrating MCP tools from mark3labs/mcp-go to modelcontextprotocol/go-sdk is the way input and output schemas are defined and handled. In `mark3labs/mcp-go`, input and output schemas were often defined using a DSL provided by the library. In `modelcontextprotocol/go-sdk`, schemas are defined using `jsonschema.Schema` structures using `github.com/google/jsonschema-go`, which are more verbose. + +When migrating a tool, you will need to convert the existing schema definitions to JSON Schema format. This involves defining the properties, types, and any validation rules using the JSON Schema specification. + +#### Example Schema Guide + +If we take an example of a tool that has the following input schema in mark3labs/mcp-go: + +```go +... +return mcp.NewTool( + "list_dependabot_alerts", + mcp.WithDescription(t("TOOL_LIST_DEPENDABOT_ALERTS_DESCRIPTION", "List dependabot alerts in a GitHub repository.")), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: t("TOOL_LIST_DEPENDABOT_ALERTS_USER_TITLE", "List dependabot alerts"), + ReadOnlyHint: ToBoolPtr(true), + }), + mcp.WithString("owner", + mcp.Required(), + mcp.Description("The owner of the repository."), + ), + mcp.WithString("repo", + mcp.Required(), + mcp.Description("The name of the repository."), + ), + mcp.WithString("state", + mcp.Description("Filter dependabot alerts by state. Defaults to open"), + mcp.DefaultString("open"), + mcp.Enum("open", "fixed", "dismissed", "auto_dismissed"), + ), + mcp.WithString("severity", + mcp.Description("Filter dependabot alerts by severity"), + mcp.Enum("low", "medium", "high", "critical"), + ), + ), +... +``` + +The corresponding input schema in modelcontextprotocol/go-sdk would look like this: + +```go +... +return mcp.Tool{ + Name: "list_dependabot_alerts", + Description: t("TOOL_LIST_DEPENDABOT_ALERTS_DESCRIPTION", "List dependabot alerts in a GitHub repository."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_LIST_DEPENDABOT_ALERTS_USER_TITLE", "List dependabot alerts"), + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "The owner of the repository.", + }, + "repo": { + Type: "string", + Description: "The name of the repository.", + }, + "state": { + Type: "string", + Description: "Filter dependabot alerts by state. Defaults to open", + Enum: []any{"open", "fixed", "dismissed", "auto_dismissed"}, + Default: "open", + }, + "severity": { + Type: "string", + Description: "Filter dependabot alerts by severity", + Enum: []any{"low", "medium", "high", "critical"}, + }, + }, + Required: []string{"owner", "repo"}, + }, +} +``` + +### Tests + +After migrating the tool code and test file, ensure that all tests pass successfully. If any tests fail, review the error messages and adjust the migrated code as necessary to resolve any issues. If you encounter any challenges or need further assistance during the migration process, please let me know. + +At the end of your changes, you will continue to have an issue with the `toolsnaps` tests, these validate that the schema has not changed unexpectedly. You can update the snapshots by setting `UPDATE_TOOLSNAPS=true` before running the tests, e.g.: + +```bash +UPDATE_TOOLSNAPS=true go test ./... +``` + +You should however, only update the toolsnaps after confirming that the schema changes are intentional and correct. Some schema changes are unavoidable, such as argument ordering, however the schemas themselves should remain logically equivalent. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 000000000..bc7a647a6 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,292 @@ +# GitHub MCP Server - Copilot Instructions + +## Project Overview + +This is the **GitHub MCP Server**, a Model Context Protocol (MCP) server that connects AI tools to GitHub's platform. It enables AI agents to manage repositories, issues, pull requests, workflows, and more through natural language. + +**Key Details:** +- **Language:** Go 1.24+ (~38k lines of code) +- **Type:** MCP server application with CLI interface +- **Primary Package:** github-mcp-server (stdio MCP server - **this is the main focus**) +- **Secondary Package:** mcpcurl (testing utility - don't break it, but not the priority) +- **Framework:** Uses mark3labs/mcp-go for MCP protocol, google/go-github for GitHub API +- **Size:** ~60MB repository, 70 Go files +- **Library Usage:** This repository is also used as a library by the remote server. Functions that could be called by other repositories should be exported (capitalized), even if not required internally. Preserve existing export patterns. + +**Code Quality Standards:** +- **Popular Open Source Repository** - High bar for code quality and clarity +- **Comprehension First** - Code must be clear to a wide audience +- **Clean Commits** - Atomic, focused changes with clear messages +- **Structure** - Always maintain or improve, never degrade +- **Code over Comments** - Prefer self-documenting code; comment only when necessary + +## Critical Build & Validation Steps + +### Required Commands (Run Before Committing) + +**ALWAYS run these commands in this exact order before using report_progress or finishing work:** + +1. **Format Code:** `script/lint` (runs `gofmt -s -w .` then `golangci-lint`) +2. **Run Tests:** `script/test` (runs `go test -race ./...`) +3. **Update Documentation:** `script/generate-docs` (if you modified MCP tools/toolsets) + +**These commands are FAST:** Lint ~1s, Tests ~1s (cached), Build ~1s + +### When Modifying MCP Tools/Endpoints + +If you change any MCP tool definitions or schemas: +1. Run tests with `UPDATE_TOOLSNAPS=true go test ./...` to update toolsnaps +2. Commit the updated `.snap` files in `pkg/github/__toolsnaps__/` +3. Run `script/generate-docs` to update README.md +4. Toolsnaps document API surface and ensure changes are intentional + +### Common Build Commands + +```bash +# Download dependencies (rarely needed - usually cached) +go mod download + +# Build the server binary +go build -v ./cmd/github-mcp-server + +# Run the server +./github-mcp-server stdio + +# Run specific package tests +go test ./pkg/github -v + +# Run specific test +go test ./pkg/github -run TestGetMe +``` + +## Project Structure + +### Directory Layout + +``` +. +├── cmd/ +│ ├── github-mcp-server/ # Main MCP server entry point (PRIMARY FOCUS) +│ └── mcpcurl/ # MCP testing utility (secondary - don't break it) +├── pkg/ # Public API packages +│ ├── github/ # GitHub API MCP tools implementation +│ │ └── __toolsnaps__/ # Tool schema snapshots (*.snap files) +│ ├── toolsets/ # Toolset configuration & management +│ ├── errors/ # Error handling utilities +│ ├── sanitize/ # HTML/content sanitization +│ ├── log/ # Logging utilities +│ ├── raw/ # Raw data handling +│ ├── buffer/ # Buffer utilities +│ └── translations/ # i18n translation support +├── internal/ # Internal implementation packages +│ ├── ghmcp/ # GitHub MCP server core logic +│ ├── githubv4mock/ # GraphQL API mocking for tests +│ ├── toolsnaps/ # Toolsnap validation system +│ └── profiler/ # Performance profiling +├── e2e/ # End-to-end tests (require GitHub PAT) +├── script/ # Build and maintenance scripts +├── docs/ # Documentation +├── .github/workflows/ # CI/CD workflows +└── [config files] # See below +``` + +### Key Configuration Files + +- **go.mod / go.sum:** Go module dependencies (Go 1.24.0+) +- **.golangci.yml:** Linter configuration (v2 format, ~15 linters enabled) +- **Dockerfile:** Multi-stage build (golang:1.25.3-alpine → distroless) +- **server.json:** MCP server metadata for registry +- **.goreleaser.yaml:** Release automation config +- **.gitignore:** Excludes bin/, dist/, vendor/, *.DS_Store, github-mcp-server binary + +### Important Scripts (script/ directory) + +- **script/lint** - Runs `gofmt` + `golangci-lint`. **MUST RUN** before committing +- **script/test** - Runs `go test -race ./...` (full test suite) +- **script/generate-docs** - Updates README.md tool documentation. Run after tool changes +- **script/licenses** - Updates third-party license files when dependencies change +- **script/licenses-check** - Validates license compliance (runs in CI) +- **script/get-me** - Quick test script for get_me tool +- **script/get-discussions** - Quick test for discussions +- **script/tag-release** - **NEVER USE THIS** - releases are managed separately + +## GitHub Workflows (CI/CD) + +All workflows run on push/PR unless noted. Located in `.github/workflows/`: + +1. **go.yml** - Build and test on ubuntu/windows/macos. Runs `script/test` and builds binary +2. **lint.yml** - Runs golangci-lint-action v2.5 (GitHub Action) with actions/setup-go stable +3. **docs-check.yml** - Verifies README.md is up-to-date by running generate-docs and checking git diff +4. **code-scanning.yml** - CodeQL security analysis for Go and GitHub Actions +5. **license-check.yml** - Runs `script/licenses-check` to validate compliance +6. **docker-publish.yml** - Publishes container image to ghcr.io +7. **goreleaser.yml** - Creates releases (main branch only) +8. **registry-releaser.yml** - Updates MCP registry + +**All of these must pass for PR merge.** If docs-check fails, run `script/generate-docs` and commit changes. + +## Testing Guidelines + +### Unit Tests + +- Use `testify` for assertions (`require` for critical checks, `assert` for non-blocking) +- Tests are in `*_test.go` files alongside implementation (internal tests, not `_test` package) +- Mock GitHub API with `go-github-mock` (REST) or `githubv4mock` (GraphQL) +- Test structure for tools: + 1. Test tool snapshot + 2. Verify critical schema properties (e.g., ReadOnly annotation) + 3. Table-driven behavioral tests + +### Toolsnaps (Tool Schema Snapshots) + +- Every MCP tool has a JSON schema snapshot in `pkg/github/__toolsnaps__/*.snap` +- Tests fail if current schema differs from snapshot (shows diff) +- To update after intentional changes: `UPDATE_TOOLSNAPS=true go test ./...` +- **MUST commit updated .snap files** - they document API changes +- Missing snapshots cause CI failure + +### End-to-End Tests + +- Located in `e2e/` directory with `e2e_test.go` +- **Require GitHub PAT token** - you usually cannot run these yourself +- Run with: `GITHUB_MCP_SERVER_E2E_TOKEN= go test -v --tags e2e ./e2e` +- Tests interact with live GitHub API via Docker container +- **Keep e2e tests updated when changing MCP tools** +- **Use only the e2e test style** when modifying tests in this directory +- For debugging: `GITHUB_MCP_SERVER_E2E_DEBUG=true` runs in-process (no Docker) + +## Code Style & Linting + +### Go Code Requirements + +- **gofmt with simplify flag (-s)** - Automatically run by `script/lint` +- **golangci-lint** with these linters enabled: + - bodyclose, gocritic, gosec, makezero, misspell, nakedret, revive + - errcheck, staticcheck, govet, ineffassign, unused +- Exclusions for: third_party/, builtin/, examples/, generated code + +### Go Naming Conventions + +- **Acronyms in identifiers:** Use `ID` not `Id`, `API` not `Api`, `URL` not `Url`, `HTTP` not `Http` +- Examples: `userID`, `getAPI`, `parseURL`, `HTTPClient` +- This applies to variable names, function names, struct fields, etc. + +### Code Patterns + +- **Keep changes minimal and focused** on the specific issue being addressed +- **Prefer clarity over cleverness** - code must be understandable by a wide audience +- **Atomic commits** - each commit should be a complete, logical change +- **Maintain or improve structure** - never degrade code organization +- Use table-driven tests for behavioral testing +- Comment sparingly - code should be self-documenting +- Follow standard Go conventions (Effective Go, Go proverbs) +- **Test changes thoroughly** before committing +- Export functions (capitalize) if they could be used by other repos as a library + +## Common Development Workflows + +### Adding a New MCP Tool + +1. Add tool implementation in `pkg/github/` (e.g., `foo_tools.go`) +2. Register tool in appropriate toolset in `pkg/github/` or `pkg/toolsets/` +3. Write unit tests following the tool test pattern +4. Run `UPDATE_TOOLSNAPS=true go test ./...` to create snapshot +5. Run `script/generate-docs` to update README +6. Run `script/lint` and `script/test` before committing +7. If e2e tests are relevant, update `e2e/e2e_test.go` using existing test style +8. Commit code + snapshots + README changes together + +### Fixing a Bug + +1. Write a failing test that reproduces the bug +2. Fix the bug with minimal changes +3. Verify test passes and existing tests still pass +4. Run `script/lint` and `script/test` +5. If tool schema changed, update toolsnaps (see above) + +### Updating Dependencies + +1. Update `go.mod` (e.g., `go get -u ./...` or manually) +2. Run `go mod tidy` +3. Run `script/licenses` to update license files +4. Run `script/test` to verify nothing broke +5. Commit go.mod, go.sum, and third-party-licenses* files + +## Common Errors & Solutions + +### "Documentation is out of date" in CI + +**Fix:** Run `script/generate-docs` and commit README.md changes + +### Toolsnap mismatch failures + +**Fix:** Run `UPDATE_TOOLSNAPS=true go test ./...` and commit updated .snap files + +### Lint failures + +**Fix:** Run `script/lint` locally - it will auto-format and show issues. Fix manually reported issues. + +### License check failures + +**Fix:** Run `script/licenses` to regenerate license files after dependency changes + +### Test failures after changing a tool + +**Likely causes:** +1. Forgot to update toolsnaps - run with `UPDATE_TOOLSNAPS=true` +2. Changed behavior broke existing tests - verify intent and fix tests +3. Schema change not reflected in test - update test expectations + +## Environment Variables + +- **GITHUB_PERSONAL_ACCESS_TOKEN** - Required for server operation and e2e tests +- **GITHUB_HOST** - For GitHub Enterprise Server (prefix with `https://`) +- **GITHUB_TOOLSETS** - Comma-separated toolset list (overrides --toolsets flag) +- **GITHUB_READ_ONLY** - Set to "1" for read-only mode +- **GITHUB_DYNAMIC_TOOLSETS** - Set to "1" for dynamic toolset discovery +- **UPDATE_TOOLSNAPS** - Set to "true" when running tests to update snapshots +- **GITHUB_MCP_SERVER_E2E_TOKEN** - Token for e2e tests +- **GITHUB_MCP_SERVER_E2E_DEBUG** - Set to "true" for in-process e2e debugging + +## Key Files Reference + +### Root Directory Files +``` +.dockerignore - Docker build exclusions +.gitignore - Git exclusions (includes bin/, dist/, vendor/, binaries) +.golangci.yml - Linter configuration +.goreleaser.yaml - Release automation +CODE_OF_CONDUCT.md - Community guidelines +CONTRIBUTING.md - Contribution guide (fork, clone, test, lint workflow) +Dockerfile - Multi-stage Go build +LICENSE - MIT license +README.md - Main documentation (auto-generated sections) +SECURITY.md - Security policy +SUPPORT.md - Support resources +gemini-extension.json - Gemini CLI configuration +go.mod / go.sum - Go dependencies +server.json - MCP server registry metadata +``` + +### Main Entry Point + +`cmd/github-mcp-server/main.go` - Uses cobra for CLI, viper for config, supports: +- `stdio` command (default) - MCP stdio transport +- `generate-docs` command - Documentation generation +- Flags: --toolsets, --read-only, --dynamic-toolsets, --gh-host, --log-file + +## Important Reminders + +1. **PRIMARY FOCUS:** The local stdio MCP server (github-mcp-server) - this is what you should work on and test with +2. **REMOTE SERVER:** Ignore remote server instructions when making code changes (unless specifically asked). This repo is used as a library by the remote server, so keep functions exported (capitalized) if they could be called by other repos, even if not needed internally. +3. **ALWAYS** trust these instructions first - only search if information is incomplete or incorrect +4. **NEVER** use `script/tag-release` or push tags +5. **NEVER** skip `script/lint` before committing Go code changes +6. **ALWAYS** update toolsnaps when changing MCP tool schemas +7. **ALWAYS** run `script/generate-docs` after modifying tools +8. For specific test files, use `go test ./path -run TestName` not full suite +9. E2E tests require PAT token - you likely cannot run them +10. Toolsnaps are API documentation - treat changes seriously +11. Build/test/lint are very fast (~1s each) - run frequently +12. CI failures for docs-check or license-check have simple fixes (run the script) +13. mcpcurl is secondary - don't break it, but it's not the priority \ No newline at end of file diff --git a/README.md b/README.md index 7c4884074..c9a1fd70b 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,5 @@ +[![Go Report Card](https://goreportcard.com/badge/github.com/github/github-mcp-server)](https://goreportcard.com/report/github.com/github/github-mcp-server) + # GitHub MCP Server The GitHub MCP Server connects AI tools directly to GitHub's platform. This gives AI agents, assistants, and chatbots the ability to read repositories and code files, manage issues and PRs, analyze code, and automate workflows. All through natural language interactions. @@ -1264,7 +1266,7 @@ docker run -i --rm \ ## Lockdown Mode -Lockdown mode limits the content that the server will surface from public repositories. When enabled, requests that fetch issue details will return an error if the issue was created by someone who does not have push access to the repository. Private repositories are unaffected, and collaborators can still access their own issues. +Lockdown mode limits the content that the server will surface from public repositories. When enabled, the server checks whether the author of each item has push access to the repository. Private repositories are unaffected, and collaborators keep full access to their own content. ```bash ./github-mcp-server --lockdown-mode @@ -1279,7 +1281,20 @@ docker run -i --rm \ ghcr.io/github/github-mcp-server ``` -At the moment lockdown mode applies to the issue read toolset, but it is designed to extend to additional data surfaces over time. +The behavior of lockdown mode depends on the tool invoked. + +Following tools will return an error when the author lacks the push access: + +- `issue_read:get` +- `pull_request_read:get` + +Following tools will filter out content from users lacking the push access: + +- `issue_read:get_comments` +- `issue_read:get_sub_issues` +- `pull_request_read:get_comments` +- `pull_request_read:get_review_comments` +- `pull_request_read:get_reviews` ## i18n / Overriding Descriptions diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 359370760..2fa81d45a 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/toolsets" "github.com/github/github-mcp-server/pkg/translations" @@ -64,7 +65,8 @@ func generateReadmeDocs(readmePath string) error { t, _ := translations.TranslationHelper() // Create toolset group with mock clients - tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}) + repoAccessCache := lockdown.GetInstance(nil) + tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}, repoAccessCache) // Generate toolsets documentation toolsetsDoc := generateToolsetsDoc(tsg) @@ -302,7 +304,8 @@ func generateRemoteToolsetsDoc() string { t, _ := translations.TranslationHelper() // Create toolset group with mock clients - tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}) + repoAccessCache := lockdown.GetInstance(nil) + tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000, github.FeatureFlags{}, repoAccessCache) // Generate table header buf.WriteString("| Name | Description | API URL | 1-Click Install (VS Code) | Read-only Link | 1-Click Read-only Install (VS Code) |\n") diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 125cd5a8d..3d4113644 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "strings" + "time" "github.com/github/github-mcp-server/internal/ghmcp" "github.com/github/github-mcp-server/pkg/github" @@ -50,6 +51,7 @@ var ( enabledToolsets = []string{github.ToolsetMetadataDefault.ID} } + ttl := viper.GetDuration("repo-access-cache-ttl") stdioServerConfig := ghmcp.StdioServerConfig{ Version: version, Host: viper.GetString("host"), @@ -62,6 +64,7 @@ var ( LogFilePath: viper.GetString("log-file"), ContentWindowSize: viper.GetInt("content-window-size"), LockdownMode: viper.GetBool("lockdown-mode"), + RepoAccessCacheTTL: &ttl, } return ghmcp.RunStdioServer(stdioServerConfig) }, @@ -84,6 +87,7 @@ func init() { rootCmd.PersistentFlags().String("gh-host", "", "Specify the GitHub hostname (for GitHub Enterprise etc.)") rootCmd.PersistentFlags().Int("content-window-size", 5000, "Specify the content window size") rootCmd.PersistentFlags().Bool("lockdown-mode", false, "Enable lockdown mode") + rootCmd.PersistentFlags().Duration("repo-access-cache-ttl", 5*time.Minute, "Override the repo access cache TTL (e.g. 1m, 0s to disable)") // Bind flag to viper _ = viper.BindPFlag("toolsets", rootCmd.PersistentFlags().Lookup("toolsets")) @@ -95,6 +99,7 @@ func init() { _ = viper.BindPFlag("host", rootCmd.PersistentFlags().Lookup("gh-host")) _ = viper.BindPFlag("content-window-size", rootCmd.PersistentFlags().Lookup("content-window-size")) _ = viper.BindPFlag("lockdown-mode", rootCmd.PersistentFlags().Lookup("lockdown-mode")) + _ = viper.BindPFlag("repo-access-cache-ttl", rootCmd.PersistentFlags().Lookup("repo-access-cache-ttl")) // Add subcommands rootCmd.AddCommand(stdioCmd) diff --git a/docs/installation-guides/README.md b/docs/installation-guides/README.md index 4406f5b98..097d97b02 100644 --- a/docs/installation-guides/README.md +++ b/docs/installation-guides/README.md @@ -7,6 +7,7 @@ This directory contains detailed installation instructions for the GitHub MCP Se - **[Claude Applications](install-claude.md)** - Installation guide for Claude Web, Claude Desktop and Claude Code CLI - **[Cursor](install-cursor.md)** - Installation guide for Cursor IDE - **[Google Gemini CLI](install-gemini-cli.md)** - Installation guide for Google Gemini CLI +- **[OpenAI Codex](install-codex.md)** - Installation guide for OpenAI Codex - **[Windsurf](install-windsurf.md)** - Installation guide for Windsurf IDE ## Support by Host Application diff --git a/docs/installation-guides/install-codex.md b/docs/installation-guides/install-codex.md new file mode 100644 index 000000000..5f92996bc --- /dev/null +++ b/docs/installation-guides/install-codex.md @@ -0,0 +1,112 @@ +# Install GitHub MCP Server in OpenAI Codex + +## Prerequisites + +1. OpenAI Codex (MCP-enabled) installed / available +2. A [GitHub Personal Access Token](https://github.com/settings/personal-access-tokens/new) + +> The remote GitHub MCP server is hosted by GitHub at `https://api.githubcopilot.com/mcp/` and supports Streamable HTTP. + +## Remote Configuration + +Edit `~/.codex/config.toml` (shared by CLI and IDE extension) and add: + +```toml +[mcp_servers.github] +url = "https://api.githubcopilot.com/mcp/" +# Replace with your real PAT (least-privilege scopes). Do NOT commit this. +bearer_token_env_var = "GITHUB_PAT_TOKEN" +``` + +You can also add it via the Codex CLI: + +```cli +codex mcp add github --url https://api.githubcopilot.com/mcp/ +``` + +
+Storing Your PAT Securely +
+ +For security, avoid hardcoding your token. One common approach: + +1. Store your token in `.env` file +``` +GITHUB_PAT_TOKEN=ghp_your_token_here +``` + +2. Add to .gitignore +```bash +echo -e ".env" >> .gitignore +``` +
+ +## Local Docker Configuration + +Use this if you prefer a local, self-hosted instance instead of the remote HTTP server, please refer to the [OpenAI documentation for configuration](https://developers.openai.com/codex/mcp). + +## Verification + +After starting Codex (CLI or IDE): +1. Run `/mcp` in the TUI or use the IDE MCP panel; confirm `github` shows tools. +2. Ask: "List my GitHub repositories". +3. If tools are missing: + - Check token validity & scopes. + - Confirm correct table name: `[mcp_servers.github]`. + +## Usage + +After setup, Codex can interact with GitHub directly. It will use the default tool set automatically but can be [configured](../../README.md#default-toolset). Try these example prompts: + +**Repository Operations:** +- "List my GitHub repositories" +- "Show me recent issues in [owner/repo]" +- "Create a new issue in [owner/repo] titled 'Bug: fix login'" + +**Pull Requests:** +- "List open pull requests in [owner/repo]" +- "Show me the diff for PR #123" +- "Add a comment to PR #123: 'LGTM, approved'" + +**Actions & Workflows:** +- "Show me recent workflow runs in [owner/repo]" +- "Trigger the 'deploy' workflow in [owner/repo]" + +**Gists:** +- "Create a gist with this code snippet" +- "List my gists" + +> **Tip**: Use `/mcp` in the Codex UI to see all available GitHub tools and their descriptions. + +## Choosing Scopes for Your PAT + +Minimal useful scopes (adjust as needed): +- `repo` (general repository operations) +- `workflow` (if you want Actions workflow access) +- `read:org` (if accessing org-level resources) +- `project` (for classic project boards) +- `gist` (if using gist tools) + +Use the principle of least privilege: add scopes only when a tool request fails due to permission. + +## Troubleshooting + +| Issue | Possible Cause | Fix | +|-------|----------------|-----| +| Authentication failed | Missing/incorrect PAT scope | Regenerate PAT; ensure `repo` scope present | +| 401 Unauthorized (remote) | Token expired/revoked | Create new PAT; update `bearer_token_env_var` | +| Server not listed | Wrong table name or syntax error | Use `[mcp_servers.github]`; validate TOML | +| Tools missing / zero tools | Insufficient PAT scopes | Add needed scopes (workflow, gist, etc.) | +| Token in file risks leakage | Committed accidentally | Rotate token; add file to `.gitignore` | + +## Security Best Practices +1. Never commit tokens into version control +3. Rotate tokens periodically +4. Restrict scopes up front; expand only when required +5. Remove unused PATs from your GitHub account + +## References +- Remote server URL: `https://api.githubcopilot.com/mcp/` +- Release binaries: [GitHub Releases](https://github.com/github/github-mcp-server/releases) +- OpenAI Codex MCP docs: https://developers.openai.com/codex/mcp +- Main project README: [Advanced configuration options](../../README.md) diff --git a/docs/remote-server.md b/docs/remote-server.md index b263d70aa..5ee6aea64 100644 --- a/docs/remote-server.md +++ b/docs/remote-server.md @@ -61,6 +61,9 @@ The Remote GitHub MCP server has optional headers equivalent to the Local server - `X-MCP-Readonly`: Enables only "read" tools. - Equivalent to `GITHUB_READ_ONLY` env var for Local server. - If this header is empty, "false", "f", "no", "n", "0", or "off" (ignoring whitespace and case), it will be interpreted as false. All other values are interpreted as true. +- `X-MCP-Lockdown`: Enables lockdown mode, hiding public issue details created by users without push access. + - Equivalent to `GITHUB_LOCKDOWN_MODE` env var for Local server. + - If this header is empty, "false", "f", "no", "n", "0", or "off" (ignoring whitespace and case), it will be interpreted as false. All other values are interpreted as true. Example: @@ -70,7 +73,8 @@ Example: "url": "https://api.githubcopilot.com/mcp/", "headers": { "X-MCP-Toolsets": "repos,issues", - "X-MCP-Readonly": "true" + "X-MCP-Readonly": "true", + "X-MCP-Lockdown": "false" } } ``` diff --git a/go.mod b/go.mod index 02b9ad252..8d5b1b274 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/mark3labs/mcp-go v0.36.0 github.com/microcosm-cc/bluemonday v1.0.27 github.com/migueleliasweb/go-github-mock v1.3.0 + github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021 github.com/spf13/cobra v1.10.1 github.com/spf13/viper v1.21.0 github.com/stretchr/testify v1.11.1 @@ -37,7 +38,7 @@ require ( github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/fsnotify/fsnotify v1.9.0 // indirect github.com/go-viper/mapstructure/v2 v2.4.0 - github.com/google/go-querystring v1.1.0 + github.com/google/go-querystring v1.1.0 // indirect github.com/google/uuid v1.6.0 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/pelletier/go-toml/v2 v2.2.4 // indirect diff --git a/go.sum b/go.sum index 1ac8b7606..0ff7b51fa 100644 --- a/go.sum +++ b/go.sum @@ -63,6 +63,8 @@ github.com/microcosm-cc/bluemonday v1.0.27 h1:MpEUotklkwCSLeH+Qdx1VJgNqLlpY2KXwX github.com/microcosm-cc/bluemonday v1.0.27/go.mod h1:jFi9vgW+H7c3V0lb6nR74Ib/DIB5OBs92Dimizgw2cA= github.com/migueleliasweb/go-github-mock v1.3.0 h1:2sVP9JEMB2ubQw1IKto3/fzF51oFC6eVWOOFDgQoq88= github.com/migueleliasweb/go-github-mock v1.3.0/go.mod h1:ipQhV8fTcj/G6m7BKzin08GaJ/3B5/SonRAkgrk0zCY= +github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021 h1:31Y+Yu373ymebRdJN1cWLLooHH8xAr0MhKTEJGV/87g= +github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021/go.mod h1:WERUkUryfUWlrHnFSO/BEUZ+7Ns8aZy7iVOGewxKzcc= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= github.com/pelletier/go-toml/v2 v2.2.4 h1:mye9XuhQ6gvn5h28+VilKrrPoQVanw5PMw/TB0t5Ec4= github.com/pelletier/go-toml/v2 v2.2.4/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 1067a222f..15b1efc10 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -16,6 +16,7 @@ import ( "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/lockdown" mcplog "github.com/github/github-mcp-server/pkg/log" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/translations" @@ -54,6 +55,9 @@ type MCPServerConfig struct { // LockdownMode indicates if we should enable lockdown mode LockdownMode bool + + // RepoAccessTTL overrides the default TTL for repository access cache entries. + RepoAccessTTL *time.Duration } const stdioServerLogPrefix = "stdioserver" @@ -80,6 +84,14 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { }, } // We're going to wrap the Transport later in beforeInit gqlClient := githubv4.NewEnterpriseClient(apiHost.graphqlURL.String(), gqlHTTPClient) + repoAccessOpts := []lockdown.RepoAccessOption{} + if cfg.RepoAccessTTL != nil { + repoAccessOpts = append(repoAccessOpts, lockdown.WithTTL(*cfg.RepoAccessTTL)) + } + var repoAccessCache *lockdown.RepoAccessCache + if cfg.LockdownMode { + repoAccessCache = lockdown.GetInstance(gqlClient, repoAccessOpts...) + } // When a client send an initialize request, update the user agent to include the client info. beforeInit := func(_ context.Context, _ any, message *mcp.InitializeRequest) { @@ -165,6 +177,7 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { cfg.Translator, cfg.ContentWindowSize, github.FeatureFlags{LockdownMode: cfg.LockdownMode}, + repoAccessCache, ) err = tsg.EnableToolsets(enabledToolsets, nil) @@ -219,6 +232,9 @@ type StdioServerConfig struct { // LockdownMode indicates if we should enable lockdown mode LockdownMode bool + + // RepoAccessCacheTTL overrides the default TTL for repository access cache entries. + RepoAccessCacheTTL *time.Duration } // RunStdioServer is not concurrent safe. @@ -229,23 +245,6 @@ func RunStdioServer(cfg StdioServerConfig) error { t, dumpTranslations := translations.TranslationHelper() - ghServer, err := NewMCPServer(MCPServerConfig{ - Version: cfg.Version, - Host: cfg.Host, - Token: cfg.Token, - EnabledToolsets: cfg.EnabledToolsets, - DynamicToolsets: cfg.DynamicToolsets, - ReadOnly: cfg.ReadOnly, - Translator: t, - ContentWindowSize: cfg.ContentWindowSize, - LockdownMode: cfg.LockdownMode, - }) - if err != nil { - return fmt.Errorf("failed to create MCP server: %w", err) - } - - stdioServer := server.NewStdioServer(ghServer) - var slogHandler slog.Handler var logOutput io.Writer if cfg.LogFilePath != "" { @@ -262,6 +261,24 @@ func RunStdioServer(cfg StdioServerConfig) error { logger := slog.New(slogHandler) logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "dynamicToolsets", cfg.DynamicToolsets, "readOnly", cfg.ReadOnly, "lockdownEnabled", cfg.LockdownMode) stdLogger := log.New(logOutput, stdioServerLogPrefix, 0) + + ghServer, err := NewMCPServer(MCPServerConfig{ + Version: cfg.Version, + Host: cfg.Host, + Token: cfg.Token, + EnabledToolsets: cfg.EnabledToolsets, + DynamicToolsets: cfg.DynamicToolsets, + ReadOnly: cfg.ReadOnly, + Translator: t, + ContentWindowSize: cfg.ContentWindowSize, + LockdownMode: cfg.LockdownMode, + RepoAccessTTL: cfg.RepoAccessCacheTTL, + }) + if err != nil { + return fmt.Errorf("failed to create MCP server: %w", err) + } + + stdioServer := server.NewStdioServer(ghServer) stdioServer.SetErrorLogger(stdLogger) if cfg.ExportTranslations { diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index 338b8b987..3a5fb54bb 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -22,7 +22,7 @@ func GenerateInstructions(enabledToolsets []string) string { // Individual toolset instructions for _, toolset := range enabledToolsets { - if inst := getToolsetInstructions(toolset); inst != "" { + if inst := getToolsetInstructions(toolset, enabledToolsets); inst != "" { instructions = append(instructions, inst) } } @@ -48,12 +48,18 @@ Tool usage guidance: } // getToolsetInstructions returns specific instructions for individual toolsets -func getToolsetInstructions(toolset string) string { +func getToolsetInstructions(toolset string, enabledToolsets []string) string { switch toolset { case "pull_requests": - return `## Pull Requests + pullRequestInstructions := `## Pull Requests PR review workflow: Always use 'pull_request_review_write' with method 'create' to create a pending review, then 'add_comment_to_pending_review' to add comments, and finally 'pull_request_review_write' with method 'submit_pending' to submit the review for complex reviews with line-specific comments.` + if slices.Contains(enabledToolsets, "repos") { + pullRequestInstructions += ` + +Before creating a pull request, search for pull request templates in the repository. Template files are called pull_request_template.md or they're located in '.github/PULL_REQUEST_TEMPLATE' directory. Use the template content to structure the PR description and then call create_pull_request tool.` + } + return pullRequestInstructions case "issues": return `## Issues diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index f00e0ac74..b8ad2ba8c 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -2,6 +2,7 @@ package github import ( "os" + "strings" "testing" ) @@ -128,12 +129,23 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) { func TestGetToolsetInstructions(t *testing.T) { tests := []struct { - toolset string - expectedEmpty bool + toolset string + expectedEmpty bool + enabledToolsets []string + expectedToContain string + notExpectedToContain string }{ { - toolset: "pull_requests", - expectedEmpty: false, + toolset: "pull_requests", + expectedEmpty: false, + enabledToolsets: []string{"pull_requests", "repos"}, + expectedToContain: "pull_request_template.md", + }, + { + toolset: "pull_requests", + expectedEmpty: false, + enabledToolsets: []string{"pull_requests"}, + notExpectedToContain: "pull_request_template.md", }, { toolset: "issues", @@ -151,7 +163,7 @@ func TestGetToolsetInstructions(t *testing.T) { for _, tt := range tests { t.Run(tt.toolset, func(t *testing.T) { - result := getToolsetInstructions(tt.toolset) + result := getToolsetInstructions(tt.toolset, tt.enabledToolsets) if tt.expectedEmpty { if result != "" { t.Errorf("Expected empty result for toolset '%s', but got: %s", tt.toolset, result) @@ -161,6 +173,14 @@ func TestGetToolsetInstructions(t *testing.T) { t.Errorf("Expected non-empty result for toolset '%s', but got empty", tt.toolset) } } + + if tt.expectedToContain != "" && !strings.Contains(result, tt.expectedToContain) { + t.Errorf("Expected result to contain '%s' for toolset '%s', but it did not. Result: %s", tt.expectedToContain, tt.toolset, result) + } + + if tt.notExpectedToContain != "" && strings.Contains(result, tt.notExpectedToContain) { + t.Errorf("Did not expect result to contain '%s' for toolset '%s', but it did. Result: %s", tt.notExpectedToContain, tt.toolset, result) + } }) } } diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 1c4f9514c..f35168705 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -228,7 +228,7 @@ func fragmentToIssue(fragment IssueFragment) *github.Issue { } // GetIssue creates a tool to get details of a specific issue in a GitHub repository. -func IssueRead(getClient GetClientFn, getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc, flags FeatureFlags) (tool mcp.Tool, handler server.ToolHandlerFunc) { +func IssueRead(getClient GetClientFn, getGQLClient GetGQLClientFn, cache *lockdown.RepoAccessCache, t translations.TranslationHelperFunc, flags FeatureFlags) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("issue_read", mcp.WithDescription(t("TOOL_ISSUE_READ_DESCRIPTION", "Get information about a specific issue in a GitHub repository.")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ @@ -297,20 +297,20 @@ Options are: switch method { case "get": - return GetIssue(ctx, client, gqlClient, owner, repo, issueNumber, flags) + return GetIssue(ctx, client, cache, owner, repo, issueNumber, flags) case "get_comments": - return GetIssueComments(ctx, client, owner, repo, issueNumber, pagination, flags) + return GetIssueComments(ctx, client, cache, owner, repo, issueNumber, pagination, flags) case "get_sub_issues": - return GetSubIssues(ctx, client, owner, repo, issueNumber, pagination, flags) + return GetSubIssues(ctx, client, cache, owner, repo, issueNumber, pagination, flags) case "get_labels": - return GetIssueLabels(ctx, gqlClient, owner, repo, issueNumber, flags) + return GetIssueLabels(ctx, gqlClient, owner, repo, issueNumber) default: return mcp.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil } } } -func GetIssue(ctx context.Context, client *github.Client, gqlClient *githubv4.Client, owner string, repo string, issueNumber int, flags FeatureFlags) (*mcp.CallToolResult, error) { +func GetIssue(ctx context.Context, client *github.Client, cache *lockdown.RepoAccessCache, owner string, repo string, issueNumber int, flags FeatureFlags) (*mcp.CallToolResult, error) { issue, resp, err := client.Issues.Get(ctx, owner, repo, issueNumber) if err != nil { return nil, fmt.Errorf("failed to get issue: %w", err) @@ -326,12 +326,16 @@ func GetIssue(ctx context.Context, client *github.Client, gqlClient *githubv4.Cl } if flags.LockdownMode { - if issue.User != nil { - shouldRemoveContent, err := lockdown.ShouldRemoveContent(ctx, gqlClient, *issue.User.Login, owner, repo) + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + login := issue.GetUser().GetLogin() + if login != "" { + isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to check lockdown mode: %v", err)), nil } - if shouldRemoveContent { + if !isSafeContent { return mcp.NewToolResultError("access to issue details is restricted by lockdown mode"), nil } } @@ -355,7 +359,7 @@ func GetIssue(ctx context.Context, client *github.Client, gqlClient *githubv4.Cl return mcp.NewToolResultText(string(r)), nil } -func GetIssueComments(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, pagination PaginationParams, _ FeatureFlags) (*mcp.CallToolResult, error) { +func GetIssueComments(ctx context.Context, client *github.Client, cache *lockdown.RepoAccessCache, owner string, repo string, issueNumber int, pagination PaginationParams, flags FeatureFlags) (*mcp.CallToolResult, error) { opts := &github.IssueListCommentsOptions{ ListOptions: github.ListOptions{ Page: pagination.Page, @@ -376,6 +380,30 @@ func GetIssueComments(ctx context.Context, client *github.Client, owner string, } return mcp.NewToolResultError(fmt.Sprintf("failed to get issue comments: %s", string(body))), nil } + if flags.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + filteredComments := make([]*github.IssueComment, 0, len(comments)) + for _, comment := range comments { + user := comment.User + if user == nil { + continue + } + login := user.GetLogin() + if login == "" { + continue + } + isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to check lockdown mode: %v", err)), nil + } + if isSafeContent { + filteredComments = append(filteredComments, comment) + } + } + comments = filteredComments + } r, err := json.Marshal(comments) if err != nil { @@ -385,7 +413,7 @@ func GetIssueComments(ctx context.Context, client *github.Client, owner string, return mcp.NewToolResultText(string(r)), nil } -func GetSubIssues(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, pagination PaginationParams, _ FeatureFlags) (*mcp.CallToolResult, error) { +func GetSubIssues(ctx context.Context, client *github.Client, cache *lockdown.RepoAccessCache, owner string, repo string, issueNumber int, pagination PaginationParams, featureFlags FeatureFlags) (*mcp.CallToolResult, error) { opts := &github.IssueListOptions{ ListOptions: github.ListOptions{ Page: pagination.Page, @@ -412,6 +440,31 @@ func GetSubIssues(ctx context.Context, client *github.Client, owner string, repo return mcp.NewToolResultError(fmt.Sprintf("failed to list sub-issues: %s", string(body))), nil } + if featureFlags.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + filteredSubIssues := make([]*github.SubIssue, 0, len(subIssues)) + for _, subIssue := range subIssues { + user := subIssue.User + if user == nil { + continue + } + login := user.GetLogin() + if login == "" { + continue + } + isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to check lockdown mode: %v", err)), nil + } + if isSafeContent { + filteredSubIssues = append(filteredSubIssues, subIssue) + } + } + subIssues = filteredSubIssues + } + r, err := json.Marshal(subIssues) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -420,7 +473,7 @@ func GetSubIssues(ctx context.Context, client *github.Client, owner string, repo return mcp.NewToolResultText(string(r)), nil } -func GetIssueLabels(ctx context.Context, client *githubv4.Client, owner string, repo string, issueNumber int, _ FeatureFlags) (*mcp.CallToolResult, error) { +func GetIssueLabels(ctx context.Context, client *githubv4.Client, owner string, repo string, issueNumber int) (*mcp.CallToolResult, error) { // Get current labels on the issue using GraphQL var query struct { Repository struct { diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 4cc3a1302..a05312b91 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1,9 +1,11 @@ package github import ( + "bytes" "context" "encoding/json" "fmt" + "io" "net/http" "strings" "testing" @@ -11,6 +13,7 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v79/github" "github.com/migueleliasweb/go-github-mock/src/mock" @@ -19,11 +22,108 @@ import ( "github.com/stretchr/testify/require" ) +var defaultGQLClient *githubv4.Client = githubv4.NewClient(newRepoAccessHTTPClient()) +var repoAccessCache *lockdown.RepoAccessCache = stubRepoAccessCache(defaultGQLClient, 15*time.Minute) + +type repoAccessKey struct { + owner string + repo string + username string +} + +type repoAccessValue struct { + isPrivate bool + permission string +} + +type repoAccessMockTransport struct { + responses map[repoAccessKey]repoAccessValue +} + +func newRepoAccessHTTPClient() *http.Client { + responses := map[repoAccessKey]repoAccessValue{ + {owner: "owner2", repo: "repo2", username: "testuser2"}: {isPrivate: true}, + {owner: "owner", repo: "repo", username: "testuser"}: {isPrivate: false, permission: "READ"}, + } + + return &http.Client{Transport: &repoAccessMockTransport{responses: responses}} +} + +func (rt *repoAccessMockTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if req.Body == nil { + return nil, fmt.Errorf("missing request body") + } + + var payload struct { + Query string `json:"query"` + Variables map[string]any `json:"variables"` + } + + if err := json.NewDecoder(req.Body).Decode(&payload); err != nil { + return nil, err + } + _ = req.Body.Close() + + owner := toString(payload.Variables["owner"]) + repo := toString(payload.Variables["name"]) + username := toString(payload.Variables["username"]) + + value, ok := rt.responses[repoAccessKey{owner: owner, repo: repo, username: username}] + if !ok { + value = repoAccessValue{isPrivate: false, permission: "WRITE"} + } + + edges := []any{} + if value.permission != "" { + edges = append(edges, map[string]any{ + "permission": value.permission, + "node": map[string]any{ + "login": username, + }, + }) + } + + responseBody, err := json.Marshal(map[string]any{ + "data": map[string]any{ + "repository": map[string]any{ + "isPrivate": value.isPrivate, + "collaborators": map[string]any{ + "edges": edges, + }, + }, + }, + }) + if err != nil { + return nil, err + } + + resp := &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewReader(responseBody)), + } + resp.Header.Set("Content-Type", "application/json") + return resp, nil +} + +func toString(v any) string { + switch value := v.(type) { + case string: + return value + case fmt.Stringer: + return value.String() + case nil: + return "" + default: + return fmt.Sprintf("%v", value) + } +} + func Test_GetIssue(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) defaultGQLClient := githubv4.NewClient(nil) - tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(defaultGQLClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(defaultGQLClient), repoAccessCache, translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "issue_read", tool.Name) @@ -51,6 +151,22 @@ func Test_GetIssue(t *testing.T) { }, }, } + mockIssue2 := &github.Issue{ + Number: github.Ptr(422), + Title: github.Ptr("Test Issue 2"), + Body: github.Ptr("This is a test issue 2"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42"), + User: &github.User{ + Login: github.Ptr("testuser2"), + }, + Repository: &github.Repository{ + Name: github.Ptr("repo2"), + Owner: &github.User{ + Login: github.Ptr("owner2"), + }, + }, + } tests := []struct { name string @@ -73,8 +189,8 @@ func Test_GetIssue(t *testing.T) { ), requestArgs: map[string]interface{}{ "method": "get", - "owner": "owner", - "repo": "repo", + "owner": "owner2", + "repo": "repo2", "issue_number": float64(42), }, expectedIssue: mockIssue, @@ -101,7 +217,7 @@ func Test_GetIssue(t *testing.T) { mockedClient: mock.NewMockedHTTPClient( mock.WithRequestMatch( mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockIssue, + mockIssue2, ), ), gqlHTTPClient: githubv4mock.NewMockedHTTPClient( @@ -120,9 +236,9 @@ func Test_GetIssue(t *testing.T) { } `graphql:"repository(owner: $owner, name: $name)"` }{}, map[string]any{ - "owner": githubv4.String("owner"), - "name": githubv4.String("repo"), - "username": githubv4.String("testuser"), + "owner": githubv4.String("owner2"), + "name": githubv4.String("repo2"), + "username": githubv4.String("testuser2"), }, githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ @@ -136,11 +252,11 @@ func Test_GetIssue(t *testing.T) { ), requestArgs: map[string]interface{}{ "method": "get", - "owner": "owner", - "repo": "repo", - "issue_number": float64(42), + "owner": "owner2", + "repo": "repo2", + "issue_number": float64(422), }, - expectedIssue: mockIssue, + expectedIssue: mockIssue2, lockdownEnabled: true, }, { @@ -205,14 +321,16 @@ func Test_GetIssue(t *testing.T) { client := github.NewClient(tc.mockedClient) var gqlClient *githubv4.Client + cache := repoAccessCache if tc.gqlHTTPClient != nil { gqlClient = githubv4.NewClient(tc.gqlHTTPClient) + cache = stubRepoAccessCache(gqlClient, 15*time.Minute) } else { gqlClient = defaultGQLClient } flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) - _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper, flags) + _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), cache, translations.NullTranslationHelper, flags) request := createMCPRequest(tc.requestArgs) result, err := handler(context.Background(), request) @@ -1710,7 +1828,7 @@ func Test_GetIssueComments(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) gqlClient := githubv4.NewClient(nil) - tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "issue_read", tool.Name) @@ -1746,10 +1864,12 @@ func Test_GetIssueComments(t *testing.T) { tests := []struct { name string mockedClient *http.Client + gqlHTTPClient *http.Client requestArgs map[string]interface{} expectError bool expectedComments []*github.IssueComment expectedErrMsg string + lockdownEnabled bool }{ { name: "successful comments retrieval", @@ -1809,14 +1929,57 @@ func Test_GetIssueComments(t *testing.T) { expectError: true, expectedErrMsg: "failed to get issue comments", }, + { + name: "lockdown enabled filters comments without push access", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposIssuesCommentsByOwnerByRepoByIssueNumber, + []*github.IssueComment{ + { + ID: github.Ptr(int64(789)), + Body: github.Ptr("Maintainer comment"), + User: &github.User{Login: github.Ptr("maintainer")}, + }, + { + ID: github.Ptr(int64(790)), + Body: github.Ptr("External user comment"), + User: &github.User{Login: github.Ptr("testuser")}, + }, + }, + ), + ), + gqlHTTPClient: newRepoAccessHTTPClient(), + requestArgs: map[string]interface{}{ + "method": "get_comments", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + }, + expectError: false, + expectedComments: []*github.IssueComment{ + { + ID: github.Ptr(int64(789)), + Body: github.Ptr("Maintainer comment"), + User: &github.User{Login: github.Ptr("maintainer")}, + }, + }, + lockdownEnabled: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - gqlClient := githubv4.NewClient(nil) - _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + var gqlClient *githubv4.Client + if tc.gqlHTTPClient != nil { + gqlClient = githubv4.NewClient(tc.gqlHTTPClient) + } else { + gqlClient = githubv4.NewClient(nil) + } + cache := stubRepoAccessCache(gqlClient, 15*time.Minute) + flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) + _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), cache, translations.NullTranslationHelper, flags) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1839,9 +2002,12 @@ func Test_GetIssueComments(t *testing.T) { err = json.Unmarshal([]byte(textContent.Text), &returnedComments) require.NoError(t, err) assert.Equal(t, len(tc.expectedComments), len(returnedComments)) - if len(returnedComments) > 0 { - assert.Equal(t, *tc.expectedComments[0].Body, *returnedComments[0].Body) - assert.Equal(t, *tc.expectedComments[0].User.Login, *returnedComments[0].User.Login) + for i := range tc.expectedComments { + require.NotNil(t, tc.expectedComments[i].User) + require.NotNil(t, returnedComments[i].User) + assert.Equal(t, tc.expectedComments[i].GetID(), returnedComments[i].GetID()) + assert.Equal(t, tc.expectedComments[i].GetBody(), returnedComments[i].GetBody()) + assert.Equal(t, tc.expectedComments[i].GetUser().GetLogin(), returnedComments[i].GetUser().GetLogin()) } }) } @@ -1853,7 +2019,7 @@ func Test_GetIssueLabels(t *testing.T) { // Verify tool definition mockGQClient := githubv4.NewClient(nil) mockClient := github.NewClient(nil) - tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(mockGQClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(mockGQClient), stubRepoAccessCache(mockGQClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "issue_read", tool.Name) @@ -1928,7 +2094,7 @@ func Test_GetIssueLabels(t *testing.T) { t.Run(tc.name, func(t *testing.T) { gqlClient := githubv4.NewClient(tc.mockedClient) client := github.NewClient(nil) - _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) request := createMCPRequest(tc.requestArgs) result, err := handler(context.Background(), request) @@ -2619,7 +2785,7 @@ func Test_GetSubIssues(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) gqlClient := githubv4.NewClient(nil) - tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := IssueRead(stubGetClientFn(mockClient), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "issue_read", tool.Name) @@ -2816,7 +2982,7 @@ func Test_GetSubIssues(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) gqlClient := githubv4.NewClient(nil) - _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + _, handler := IssueRead(stubGetClientFn(client), stubGetGQLClientFn(gqlClient), stubRepoAccessCache(gqlClient, 15*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) // Create call request request := createMCPRequest(tc.requestArgs) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index e64ae03e4..6fb5ed30b 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -14,12 +14,13 @@ import ( "github.com/shurcooL/githubv4" ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/sanitize" "github.com/github/github-mcp-server/pkg/translations" ) // GetPullRequest creates a tool to get details of a specific pull request. -func PullRequestRead(getClient GetClientFn, t translations.TranslationHelperFunc, flags FeatureFlags) (mcp.Tool, server.ToolHandlerFunc) { +func PullRequestRead(getClient GetClientFn, cache *lockdown.RepoAccessCache, t translations.TranslationHelperFunc, flags FeatureFlags) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("pull_request_read", mcp.WithDescription(t("TOOL_PULL_REQUEST_READ_DESCRIPTION", "Get information on a specific pull request in GitHub repository.")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ @@ -86,7 +87,7 @@ Possible options: switch method { case "get": - return GetPullRequest(ctx, client, owner, repo, pullNumber) + return GetPullRequest(ctx, client, cache, owner, repo, pullNumber, flags) case "get_diff": return GetPullRequestDiff(ctx, client, owner, repo, pullNumber) case "get_status": @@ -94,18 +95,18 @@ Possible options: case "get_files": return GetPullRequestFiles(ctx, client, owner, repo, pullNumber, pagination) case "get_review_comments": - return GetPullRequestReviewComments(ctx, client, owner, repo, pullNumber, pagination) + return GetPullRequestReviewComments(ctx, client, cache, owner, repo, pullNumber, pagination, flags) case "get_reviews": - return GetPullRequestReviews(ctx, client, owner, repo, pullNumber) + return GetPullRequestReviews(ctx, client, cache, owner, repo, pullNumber, flags) case "get_comments": - return GetIssueComments(ctx, client, owner, repo, pullNumber, pagination, flags) + return GetIssueComments(ctx, client, cache, owner, repo, pullNumber, pagination, flags) default: return nil, fmt.Errorf("unknown method: %s", method) } } } -func GetPullRequest(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) { +func GetPullRequest(ctx context.Context, client *github.Client, cache *lockdown.RepoAccessCache, owner, repo string, pullNumber int, ff FeatureFlags) (*mcp.CallToolResult, error) { pr, resp, err := client.PullRequests.Get(ctx, owner, repo, pullNumber) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, @@ -134,6 +135,23 @@ func GetPullRequest(ctx context.Context, client *github.Client, owner, repo stri } } + if ff.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + login := pr.GetUser().GetLogin() + if login != "" { + isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo) + if err != nil { + return nil, fmt.Errorf("failed to check content removal: %w", err) + } + + if !isSafeContent { + return mcp.NewToolResultError("access to pull request is restricted by lockdown mode"), nil + } + } + } + r, err := json.Marshal(pr) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -249,7 +267,7 @@ func GetPullRequestFiles(ctx context.Context, client *github.Client, owner, repo return mcp.NewToolResultText(string(r)), nil } -func GetPullRequestReviewComments(ctx context.Context, client *github.Client, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { +func GetPullRequestReviewComments(ctx context.Context, client *github.Client, cache *lockdown.RepoAccessCache, owner, repo string, pullNumber int, pagination PaginationParams, ff FeatureFlags) (*mcp.CallToolResult, error) { opts := &github.PullRequestListCommentsOptions{ ListOptions: github.ListOptions{ PerPage: pagination.PerPage, @@ -275,6 +293,27 @@ func GetPullRequestReviewComments(ctx context.Context, client *github.Client, ow return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request review comments: %s", string(body))), nil } + if ff.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + filteredComments := make([]*github.PullRequestComment, 0, len(comments)) + for _, comment := range comments { + user := comment.GetUser() + if user == nil { + continue + } + isSafeContent, err := cache.IsSafeContent(ctx, user.GetLogin(), owner, repo) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to check lockdown mode: %v", err)), nil + } + if isSafeContent { + filteredComments = append(filteredComments, comment) + } + } + comments = filteredComments + } + r, err := json.Marshal(comments) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -283,7 +322,7 @@ func GetPullRequestReviewComments(ctx context.Context, client *github.Client, ow return mcp.NewToolResultText(string(r)), nil } -func GetPullRequestReviews(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) { +func GetPullRequestReviews(ctx context.Context, client *github.Client, cache *lockdown.RepoAccessCache, owner, repo string, pullNumber int, ff FeatureFlags) (*mcp.CallToolResult, error) { reviews, resp, err := client.PullRequests.ListReviews(ctx, owner, repo, pullNumber, nil) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, @@ -302,6 +341,26 @@ func GetPullRequestReviews(ctx context.Context, client *github.Client, owner, re return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request reviews: %s", string(body))), nil } + if ff.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + filteredReviews := make([]*github.PullRequestReview, 0, len(reviews)) + for _, review := range reviews { + login := review.GetUser().GetLogin() + if login != "" { + isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo) + if err != nil { + return nil, fmt.Errorf("failed to check lockdown mode: %w", err) + } + if isSafeContent { + filteredReviews = append(filteredReviews, review) + } + reviews = filteredReviews + } + } + } + r, err := json.Marshal(reviews) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) @@ -1518,6 +1577,14 @@ func AddCommentToPendingReview(getGQLClient GetGQLClientFn, t translations.Trans return mcp.NewToolResultError(err.Error()), nil } + if addPullRequestReviewThreadMutation.AddPullRequestReviewThread.Thread.ID == nil { + return mcp.NewToolResultError(`Failed to add comment to pending review. Possible reasons: + - The line number doesn't exist in the pull request diff + - The file path is incorrect + - The side (LEFT/RIGHT) is invalid for the specified line +`), nil + } + // Return nothing interesting, just indicate success for the time being. // In future, we may want to return the review ID, but for the moment, we're not leaking // API implementation details to the LLM. diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 347bce672..6eac5ce83 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -21,7 +21,7 @@ import ( func Test_GetPullRequest(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := PullRequestRead(stubGetClientFn(mockClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "pull_request_read", tool.Name) @@ -102,7 +102,7 @@ func Test_GetPullRequest(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := PullRequestRead(stubGetClientFn(client), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + _, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1133,7 +1133,7 @@ func Test_SearchPullRequests(t *testing.T) { func Test_GetPullRequestFiles(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := PullRequestRead(stubGetClientFn(mockClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "pull_request_read", tool.Name) @@ -1236,7 +1236,7 @@ func Test_GetPullRequestFiles(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := PullRequestRead(stubGetClientFn(client), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + _, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1277,7 +1277,7 @@ func Test_GetPullRequestFiles(t *testing.T) { func Test_GetPullRequestStatus(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := PullRequestRead(stubGetClientFn(mockClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "pull_request_read", tool.Name) @@ -1404,7 +1404,7 @@ func Test_GetPullRequestStatus(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := PullRequestRead(stubGetClientFn(client), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + _, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1566,7 +1566,7 @@ func Test_UpdatePullRequestBranch(t *testing.T) { func Test_GetPullRequestComments(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := PullRequestRead(stubGetClientFn(mockClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "pull_request_read", tool.Name) @@ -1610,10 +1610,12 @@ func Test_GetPullRequestComments(t *testing.T) { tests := []struct { name string mockedClient *http.Client + gqlHTTPClient *http.Client requestArgs map[string]interface{} expectError bool expectedComments []*github.PullRequestComment expectedErrMsg string + lockdownEnabled bool }{ { name: "successful comments fetch", @@ -1652,13 +1654,57 @@ func Test_GetPullRequestComments(t *testing.T) { expectError: true, expectedErrMsg: "failed to get pull request review comments", }, + { + name: "lockdown enabled filters review comments without push access", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposPullsCommentsByOwnerByRepoByPullNumber, + []*github.PullRequestComment{ + { + ID: github.Ptr(int64(2010)), + Body: github.Ptr("Maintainer review comment"), + User: &github.User{Login: github.Ptr("maintainer")}, + }, + { + ID: github.Ptr(int64(2011)), + Body: github.Ptr("External review comment"), + User: &github.User{Login: github.Ptr("testuser")}, + }, + }, + ), + ), + gqlHTTPClient: newRepoAccessHTTPClient(), + requestArgs: map[string]interface{}{ + "method": "get_review_comments", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectError: false, + expectedComments: []*github.PullRequestComment{ + { + ID: github.Ptr(int64(2010)), + Body: github.Ptr("Maintainer review comment"), + User: &github.User{Login: github.Ptr("maintainer")}, + }, + }, + lockdownEnabled: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := PullRequestRead(stubGetClientFn(client), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + var gqlClient *githubv4.Client + if tc.gqlHTTPClient != nil { + gqlClient = githubv4.NewClient(tc.gqlHTTPClient) + } else { + gqlClient = githubv4.NewClient(nil) + } + cache := stubRepoAccessCache(gqlClient, 5*time.Minute) + flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) + _, handler := PullRequestRead(stubGetClientFn(client), cache, translations.NullTranslationHelper, flags) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1687,11 +1733,13 @@ func Test_GetPullRequestComments(t *testing.T) { require.NoError(t, err) assert.Len(t, returnedComments, len(tc.expectedComments)) for i, comment := range returnedComments { - assert.Equal(t, *tc.expectedComments[i].ID, *comment.ID) - assert.Equal(t, *tc.expectedComments[i].Body, *comment.Body) - assert.Equal(t, *tc.expectedComments[i].User.Login, *comment.User.Login) - assert.Equal(t, *tc.expectedComments[i].Path, *comment.Path) - assert.Equal(t, *tc.expectedComments[i].HTMLURL, *comment.HTMLURL) + require.NotNil(t, tc.expectedComments[i].User) + require.NotNil(t, comment.User) + assert.Equal(t, tc.expectedComments[i].GetID(), comment.GetID()) + assert.Equal(t, tc.expectedComments[i].GetBody(), comment.GetBody()) + assert.Equal(t, tc.expectedComments[i].GetUser().GetLogin(), comment.GetUser().GetLogin()) + assert.Equal(t, tc.expectedComments[i].GetPath(), comment.GetPath()) + assert.Equal(t, tc.expectedComments[i].GetHTMLURL(), comment.GetHTMLURL()) } }) } @@ -1700,7 +1748,7 @@ func Test_GetPullRequestComments(t *testing.T) { func Test_GetPullRequestReviews(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := PullRequestRead(stubGetClientFn(mockClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "pull_request_read", tool.Name) @@ -1740,10 +1788,12 @@ func Test_GetPullRequestReviews(t *testing.T) { tests := []struct { name string mockedClient *http.Client + gqlHTTPClient *http.Client requestArgs map[string]interface{} expectError bool expectedReviews []*github.PullRequestReview expectedErrMsg string + lockdownEnabled bool }{ { name: "successful reviews fetch", @@ -1782,13 +1832,60 @@ func Test_GetPullRequestReviews(t *testing.T) { expectError: true, expectedErrMsg: "failed to get pull request reviews", }, + { + name: "lockdown enabled filters reviews without push access", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposPullsReviewsByOwnerByRepoByPullNumber, + []*github.PullRequestReview{ + { + ID: github.Ptr(int64(2030)), + State: github.Ptr("APPROVED"), + Body: github.Ptr("Maintainer review"), + User: &github.User{Login: github.Ptr("maintainer")}, + }, + { + ID: github.Ptr(int64(2031)), + State: github.Ptr("COMMENTED"), + Body: github.Ptr("External reviewer"), + User: &github.User{Login: github.Ptr("testuser")}, + }, + }, + ), + ), + gqlHTTPClient: newRepoAccessHTTPClient(), + requestArgs: map[string]interface{}{ + "method": "get_reviews", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectError: false, + expectedReviews: []*github.PullRequestReview{ + { + ID: github.Ptr(int64(2030)), + State: github.Ptr("APPROVED"), + Body: github.Ptr("Maintainer review"), + User: &github.User{Login: github.Ptr("maintainer")}, + }, + }, + lockdownEnabled: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := PullRequestRead(stubGetClientFn(client), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + var gqlClient *githubv4.Client + if tc.gqlHTTPClient != nil { + gqlClient = githubv4.NewClient(tc.gqlHTTPClient) + } else { + gqlClient = githubv4.NewClient(nil) + } + cache := stubRepoAccessCache(gqlClient, 5*time.Minute) + flags := stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}) + _, handler := PullRequestRead(stubGetClientFn(client), cache, translations.NullTranslationHelper, flags) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1817,11 +1914,13 @@ func Test_GetPullRequestReviews(t *testing.T) { require.NoError(t, err) assert.Len(t, returnedReviews, len(tc.expectedReviews)) for i, review := range returnedReviews { - assert.Equal(t, *tc.expectedReviews[i].ID, *review.ID) - assert.Equal(t, *tc.expectedReviews[i].State, *review.State) - assert.Equal(t, *tc.expectedReviews[i].Body, *review.Body) - assert.Equal(t, *tc.expectedReviews[i].User.Login, *review.User.Login) - assert.Equal(t, *tc.expectedReviews[i].HTMLURL, *review.HTMLURL) + require.NotNil(t, tc.expectedReviews[i].User) + require.NotNil(t, review.User) + assert.Equal(t, tc.expectedReviews[i].GetID(), review.GetID()) + assert.Equal(t, tc.expectedReviews[i].GetState(), review.GetState()) + assert.Equal(t, tc.expectedReviews[i].GetBody(), review.GetBody()) + assert.Equal(t, tc.expectedReviews[i].GetUser().GetLogin(), review.GetUser().GetLogin()) + assert.Equal(t, tc.expectedReviews[i].GetHTMLURL(), review.GetHTMLURL()) } }) } @@ -2555,10 +2654,75 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) { PullRequestReviewID: githubv4.NewID("PR_kwDODKw3uc6WYN1T"), }, nil, - githubv4mock.DataResponse(map[string]any{}), + githubv4mock.DataResponse(map[string]any{ + "addPullRequestReviewThread": map[string]any{ + "thread": map[string]any{ + "id": "MDEyOlB1bGxSZXF1ZXN0UmV2aWV3VGhyZWFkMTIzNDU2", + }, + }, + }), ), ), }, + { + name: "thread ID is nil - invalid line number", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "path": "file.go", + "body": "Comment on non-existent line", + "subjectType": "LINE", + "line": float64(999), + "side": "RIGHT", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + viewerQuery("williammartin"), + getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ + author: "williammartin", + owner: "owner", + repo: "repo", + prNum: 42, + + reviews: []getLatestPendingReviewQueryReview{ + { + id: "PR_kwDODKw3uc6WYN1T", + state: "PENDING", + url: "https://github.com/owner/repo/pull/42", + }, + }, + }), + githubv4mock.NewMutationMatcher( + struct { + AddPullRequestReviewThread struct { + Thread struct { + ID githubv4.ID + } + } `graphql:"addPullRequestReviewThread(input: $input)"` + }{}, + githubv4.AddPullRequestReviewThreadInput{ + Path: githubv4.String("file.go"), + Body: githubv4.String("Comment on non-existent line"), + SubjectType: githubv4mock.Ptr(githubv4.PullRequestReviewThreadSubjectTypeLine), + Line: githubv4.NewInt(999), + Side: githubv4mock.Ptr(githubv4.DiffSideRight), + StartLine: nil, + StartSide: nil, + PullRequestReviewID: githubv4.NewID("PR_kwDODKw3uc6WYN1T"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "addPullRequestReviewThread": map[string]any{ + "thread": map[string]any{ + "id": nil, + }, + }, + }), + ), + ), + expectToolError: true, + expectedToolErrMsg: "Failed to add comment to pending review", + }, } for _, tc := range tests { @@ -2789,7 +2953,7 @@ func TestGetPullRequestDiff(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := PullRequestRead(stubGetClientFn(mockClient), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + tool, _ := PullRequestRead(stubGetClientFn(mockClient), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "pull_request_read", tool.Name) @@ -2847,7 +3011,7 @@ index 5d6e7b2..8a4f5c3 100644 // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := PullRequestRead(stubGetClientFn(client), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) + _, handler := PullRequestRead(stubGetClientFn(client), stubRepoAccessCache(githubv4.NewClient(nil), 5*time.Minute), translations.NullTranslationHelper, stubFeatureFlags(map[string]bool{"lockdown-mode": false})) // Create call request request := createMCPRequest(tc.requestArgs) diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 3bfc1ef94..2e1c42580 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -7,7 +7,9 @@ import ( "fmt" "net/http" "testing" + "time" + "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/raw" "github.com/google/go-github/v79/github" "github.com/shurcooL/githubv4" @@ -38,6 +40,11 @@ func stubGetGQLClientFn(client *githubv4.Client) GetGQLClientFn { } } +func stubRepoAccessCache(client *githubv4.Client, ttl time.Duration) *lockdown.RepoAccessCache { + cacheName := fmt.Sprintf("repo-access-cache-test-%d", time.Now().UnixNano()) + return lockdown.GetInstance(client, lockdown.WithTTL(ttl), lockdown.WithCacheName(cacheName)) +} + func stubFeatureFlags(enabledFlags map[string]bool) FeatureFlags { return FeatureFlags{ LockdownMode: enabledFlags["lockdown-mode"], diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 0594f2f94..74f3d52f2 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/toolsets" "github.com/github/github-mcp-server/pkg/translations" @@ -159,7 +160,7 @@ func GetDefaultToolsetIDs() []string { } } -func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc, contentWindowSize int, flags FeatureFlags) *toolsets.ToolsetGroup { +func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc, contentWindowSize int, flags FeatureFlags, cache *lockdown.RepoAccessCache) *toolsets.ToolsetGroup { tsg := toolsets.NewToolsetGroup(readOnly) // Define all available features with their default state (disabled) @@ -199,7 +200,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG ) issues := toolsets.NewToolset(ToolsetMetadataIssues.ID, ToolsetMetadataIssues.Description). AddReadTools( - toolsets.NewServerTool(IssueRead(getClient, getGQLClient, t, flags)), + toolsets.NewServerTool(IssueRead(getClient, getGQLClient, cache, t, flags)), toolsets.NewServerTool(SearchIssues(getClient, t)), toolsets.NewServerTool(ListIssues(getGQLClient, t)), toolsets.NewServerTool(ListIssueTypes(getClient, t)), @@ -224,7 +225,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG ) pullRequests := toolsets.NewToolset(ToolsetMetadataPullRequests.ID, ToolsetMetadataPullRequests.Description). AddReadTools( - toolsets.NewServerTool(PullRequestRead(getClient, t, flags)), + toolsets.NewServerTool(PullRequestRead(getClient, cache, t, flags)), toolsets.NewServerTool(ListPullRequests(getClient, t)), toolsets.NewServerTool(SearchPullRequests(getClient, t)), ). diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index 5a474f73c..4c3500440 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -3,34 +3,193 @@ package lockdown import ( "context" "fmt" + "log/slog" "strings" + "sync" + "time" + "github.com/muesli/cache2go" "github.com/shurcooL/githubv4" ) -// ShouldRemoveContent determines if content should be removed based on -// lockdown mode rules. It checks if the repository is private and if the user -// has push access to the repository. -func ShouldRemoveContent(ctx context.Context, client *githubv4.Client, username, owner, repo string) (bool, error) { - isPrivate, hasPushAccess, err := repoAccessInfo(ctx, client, username, owner, repo) +// RepoAccessCache caches repository metadata related to lockdown checks so that +// multiple tools can reuse the same access information safely across goroutines. +type RepoAccessCache struct { + client *githubv4.Client + mu sync.Mutex + cache *cache2go.CacheTable + ttl time.Duration + logger *slog.Logger +} + +type repoAccessCacheEntry struct { + isPrivate bool + knownUsers map[string]bool // normalized login -> has push access + viewerLogin string +} + +// RepoAccessInfo captures repository metadata needed for lockdown decisions. +type RepoAccessInfo struct { + IsPrivate bool + HasPushAccess bool + ViewerLogin string +} + +const ( + defaultRepoAccessTTL = 20 * time.Minute + defaultRepoAccessCacheKey = "repo-access-cache" +) + +var ( + instance *RepoAccessCache + instanceMu sync.Mutex +) + +// RepoAccessOption configures RepoAccessCache at construction time. +type RepoAccessOption func(*RepoAccessCache) + +// WithTTL overrides the default TTL applied to cache entries. A non-positive +// duration disables expiration. +func WithTTL(ttl time.Duration) RepoAccessOption { + return func(c *RepoAccessCache) { + c.ttl = ttl + } +} + +// WithLogger sets the logger used for cache diagnostics. +func WithLogger(logger *slog.Logger) RepoAccessOption { + return func(c *RepoAccessCache) { + c.logger = logger + } +} + +// WithCacheName overrides the cache table name used for storing entries. This option is intended for tests +// that need isolated cache instances. +func WithCacheName(name string) RepoAccessOption { + return func(c *RepoAccessCache) { + if name != "" { + c.cache = cache2go.Cache(name) + } + } +} + +// GetInstance returns the singleton instance of RepoAccessCache. +// It initializes the instance on first call with the provided client and options. +// Subsequent calls ignore the client and options parameters and return the existing instance. +// This is the preferred way to access the cache in production code. +func GetInstance(client *githubv4.Client, opts ...RepoAccessOption) *RepoAccessCache { + instanceMu.Lock() + defer instanceMu.Unlock() + if instance == nil { + instance = &RepoAccessCache{ + client: client, + cache: cache2go.Cache(defaultRepoAccessCacheKey), + ttl: defaultRepoAccessTTL, + } + for _, opt := range opts { + if opt != nil { + opt(instance) + } + } + } + return instance +} + +// SetLogger updates the logger used for cache diagnostics. +func (c *RepoAccessCache) SetLogger(logger *slog.Logger) { + c.mu.Lock() + c.logger = logger + c.mu.Unlock() +} + +// CacheStats summarizes cache activity counters. +type CacheStats struct { + Hits int64 + Misses int64 + Evictions int64 +} + +func (c *RepoAccessCache) IsSafeContent(ctx context.Context, username, owner, repo string) (bool, error) { + repoInfo, err := c.getRepoAccessInfo(ctx, username, owner, repo) if err != nil { + c.logDebug("error checking repo access info for content filtering", "owner", owner, "repo", repo, "user", username, "error", err) return false, err } + if repoInfo.IsPrivate || repoInfo.ViewerLogin == username { + return true, nil + } + return repoInfo.HasPushAccess, nil +} + +func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner, repo string) (RepoAccessInfo, error) { + if c == nil { + return RepoAccessInfo{}, fmt.Errorf("nil repo access cache") + } + + key := cacheKey(owner, repo) + userKey := strings.ToLower(username) + c.mu.Lock() + defer c.mu.Unlock() + + // Try to get entry from cache - this will keep the item alive if it exists + cacheItem, err := c.cache.Value(key) + if err == nil { + entry := cacheItem.Data().(*repoAccessCacheEntry) + if cachedHasPush, known := entry.knownUsers[userKey]; known { + c.logDebug("repo access cache hit", "owner", owner, "repo", repo, "user", username) + return RepoAccessInfo{ + IsPrivate: entry.isPrivate, + HasPushAccess: cachedHasPush, + ViewerLogin: entry.viewerLogin, + }, nil + } + c.logDebug("known users cache miss", "owner", owner, "repo", repo, "user", username) + info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo) + if queryErr != nil { + return RepoAccessInfo{}, queryErr + } + entry.knownUsers[userKey] = info.HasPushAccess + entry.viewerLogin = info.ViewerLogin + entry.isPrivate = info.IsPrivate + c.cache.Add(key, c.ttl, entry) + return RepoAccessInfo{ + IsPrivate: entry.isPrivate, + HasPushAccess: entry.knownUsers[userKey], + ViewerLogin: entry.viewerLogin, + }, nil + } + + c.logDebug("repo access cache miss", "owner", owner, "repo", repo, "user", username) + + info, queryErr := c.queryRepoAccessInfo(ctx, username, owner, repo) + if queryErr != nil { + return RepoAccessInfo{}, queryErr + } - // Do not filter content for private repositories - if isPrivate { - return false, nil + // Create new entry + entry := &repoAccessCacheEntry{ + knownUsers: map[string]bool{userKey: info.HasPushAccess}, + isPrivate: info.IsPrivate, + viewerLogin: info.ViewerLogin, } + c.cache.Add(key, c.ttl, entry) - return !hasPushAccess, nil + return RepoAccessInfo{ + IsPrivate: entry.isPrivate, + HasPushAccess: entry.knownUsers[userKey], + ViewerLogin: entry.viewerLogin, + }, nil } -func repoAccessInfo(ctx context.Context, client *githubv4.Client, username, owner, repo string) (bool, bool, error) { - if client == nil { - return false, false, fmt.Errorf("nil GraphQL client") +func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, username, owner, repo string) (RepoAccessInfo, error) { + if c.client == nil { + return RepoAccessInfo{}, fmt.Errorf("nil GraphQL client") } var query struct { + Viewer struct { + Login githubv4.String + } Repository struct { IsPrivate githubv4.Boolean Collaborators struct { @@ -50,22 +209,33 @@ func repoAccessInfo(ctx context.Context, client *githubv4.Client, username, owne "username": githubv4.String(username), } - err := client.Query(ctx, &query, variables) - if err != nil { - return false, false, fmt.Errorf("failed to query repository access info: %w", err) + if err := c.client.Query(ctx, &query, variables); err != nil { + return RepoAccessInfo{}, fmt.Errorf("failed to query repository access info: %w", err) } - // Check if the user has push access hasPush := false for _, edge := range query.Repository.Collaborators.Edges { login := string(edge.Node.Login) if strings.EqualFold(login, username) { permission := string(edge.Permission) - // WRITE, ADMIN, and MAINTAIN permissions have push access hasPush = permission == "WRITE" || permission == "ADMIN" || permission == "MAINTAIN" break } } - return bool(query.Repository.IsPrivate), hasPush, nil + return RepoAccessInfo{ + IsPrivate: bool(query.Repository.IsPrivate), + HasPushAccess: hasPush, + ViewerLogin: string(query.Viewer.Login), + }, nil +} + +func cacheKey(owner, repo string) string { + return fmt.Sprintf("%s/%s", strings.ToLower(owner), strings.ToLower(repo)) +} + +func (c *RepoAccessCache) logDebug(msg string, args ...any) { + if c != nil && c.logger != nil { + c.logger.Debug(msg, args...) + } } diff --git a/pkg/lockdown/lockdown_test.go b/pkg/lockdown/lockdown_test.go new file mode 100644 index 000000000..c1cf5e86b --- /dev/null +++ b/pkg/lockdown/lockdown_test.go @@ -0,0 +1,112 @@ +package lockdown + +import ( + "net/http" + "sync" + "testing" + "time" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/require" +) + +const ( + testOwner = "octo-org" + testRepo = "octo-repo" + testUser = "octocat" +) + +type repoAccessQuery struct { + Viewer struct { + Login githubv4.String + } + Repository struct { + IsPrivate githubv4.Boolean + Collaborators struct { + Edges []struct { + Permission githubv4.String + Node struct { + Login githubv4.String + } + } + } `graphql:"collaborators(query: $username, first: 1)"` + } `graphql:"repository(owner: $owner, name: $name)"` +} + +type countingTransport struct { + mu sync.Mutex + next http.RoundTripper + calls int +} + +func (c *countingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + c.mu.Lock() + c.calls++ + c.mu.Unlock() + return c.next.RoundTrip(req) +} + +func (c *countingTransport) CallCount() int { + c.mu.Lock() + defer c.mu.Unlock() + return c.calls +} + +func newMockRepoAccessCache(t *testing.T, ttl time.Duration) (*RepoAccessCache, *countingTransport) { + t.Helper() + + var query repoAccessQuery + + variables := map[string]any{ + "owner": githubv4.String(testOwner), + "name": githubv4.String(testRepo), + "username": githubv4.String(testUser), + } + + response := githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "login": testUser, + }, + "repository": map[string]any{ + "isPrivate": false, + "collaborators": map[string]any{ + "edges": []any{ + map[string]any{ + "permission": "WRITE", + "node": map[string]any{ + "login": testUser, + }, + }, + }, + }, + }, + }) + + httpClient := githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(query, variables, response)) + counting := &countingTransport{next: httpClient.Transport} + httpClient.Transport = counting + + gqlClient := githubv4.NewClient(httpClient) + + return GetInstance(gqlClient, WithTTL(ttl)), counting +} + +func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { + ctx := t.Context() + + cache, transport := newMockRepoAccessCache(t, 5*time.Millisecond) + info, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) + require.NoError(t, err) + require.Equal(t, testUser, info.ViewerLogin) + require.True(t, info.HasPushAccess) + require.EqualValues(t, 1, transport.CallCount()) + + time.Sleep(20 * time.Millisecond) + + info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) + require.NoError(t, err) + require.Equal(t, testUser, info.ViewerLogin) + require.True(t, info.HasPushAccess) + require.EqualValues(t, 2, transport.CallCount()) +} diff --git a/server.json b/server.json index 544f1fbc4..1e05b71e0 100644 --- a/server.json +++ b/server.json @@ -9,59 +9,6 @@ "source": "github" }, "version": "${VERSION}", - "packages": [ - { - "registryType": "oci", - "identifier": "ghcr.io/github/github-mcp-server:${VERSION}", - "transport": { - "type": "stdio" - }, - "runtimeArguments": [ - { - "type": "positional", - "value": "run", - "description": "The runtime command to execute", - "isRequired": true - }, - { - "type": "named", - "name": "-i", - "value": "true", - "description": "Run container in interactive mode", - "format": "boolean", - "isRequired": true - }, - { - "type": "named", - "name": "--rm", - "value": "true", - "description": "Automatically remove the container when it exits", - "format": "boolean" - }, - { - "type": "named", - "name": "-e", - "description": "Set an environment variable in the runtime", - "value": "GITHUB_PERSONAL_ACCESS_TOKEN={token}", - "isRequired": true, - "variables": { - "token": { - "isRequired": true, - "isSecret": true, - "format": "string" - } - } - }, - { - "type": "positional", - "valueHint": "image_name", - "value": "ghcr.io/github/github-mcp-server", - "description": "The container image to run", - "isRequired": true - } - ] - } - ], "remotes": [ { "type": "streamable-http", diff --git a/third-party-licenses.darwin.md b/third-party-licenses.darwin.md index eecc6faa8..68a45fa7a 100644 --- a/third-party-licenses.darwin.md +++ b/third-party-licenses.darwin.md @@ -28,6 +28,7 @@ Some packages may only be included on certain architectures or operating systems - [github.com/mark3labs/mcp-go](https://pkg.go.dev/github.com/mark3labs/mcp-go) ([MIT](https://github.com/mark3labs/mcp-go/blob/v0.36.0/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - [github.com/migueleliasweb/go-github-mock/src/mock](https://pkg.go.dev/github.com/migueleliasweb/go-github-mock/src/mock) ([MIT](https://github.com/migueleliasweb/go-github-mock/blob/v1.3.0/LICENSE)) + - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) - [github.com/shurcooL/githubv4](https://pkg.go.dev/github.com/shurcooL/githubv4) ([MIT](https://github.com/shurcooL/githubv4/blob/48295856cce7/LICENSE)) diff --git a/third-party-licenses.linux.md b/third-party-licenses.linux.md index eecc6faa8..68a45fa7a 100644 --- a/third-party-licenses.linux.md +++ b/third-party-licenses.linux.md @@ -28,6 +28,7 @@ Some packages may only be included on certain architectures or operating systems - [github.com/mark3labs/mcp-go](https://pkg.go.dev/github.com/mark3labs/mcp-go) ([MIT](https://github.com/mark3labs/mcp-go/blob/v0.36.0/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - [github.com/migueleliasweb/go-github-mock/src/mock](https://pkg.go.dev/github.com/migueleliasweb/go-github-mock/src/mock) ([MIT](https://github.com/migueleliasweb/go-github-mock/blob/v1.3.0/LICENSE)) + - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) - [github.com/shurcooL/githubv4](https://pkg.go.dev/github.com/shurcooL/githubv4) ([MIT](https://github.com/shurcooL/githubv4/blob/48295856cce7/LICENSE)) diff --git a/third-party-licenses.windows.md b/third-party-licenses.windows.md index 75fe8172a..2d8ef9111 100644 --- a/third-party-licenses.windows.md +++ b/third-party-licenses.windows.md @@ -29,6 +29,7 @@ Some packages may only be included on certain architectures or operating systems - [github.com/mark3labs/mcp-go](https://pkg.go.dev/github.com/mark3labs/mcp-go) ([MIT](https://github.com/mark3labs/mcp-go/blob/v0.36.0/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - [github.com/migueleliasweb/go-github-mock/src/mock](https://pkg.go.dev/github.com/migueleliasweb/go-github-mock/src/mock) ([MIT](https://github.com/migueleliasweb/go-github-mock/blob/v1.3.0/LICENSE)) + - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) - [github.com/shurcooL/githubv4](https://pkg.go.dev/github.com/shurcooL/githubv4) ([MIT](https://github.com/shurcooL/githubv4/blob/48295856cce7/LICENSE)) diff --git a/third-party/github.com/muesli/cache2go/LICENSE.txt b/third-party/github.com/muesli/cache2go/LICENSE.txt new file mode 100644 index 000000000..3dbf3d932 --- /dev/null +++ b/third-party/github.com/muesli/cache2go/LICENSE.txt @@ -0,0 +1,28 @@ +Copyright (c) 2012, Radu Ioan Fericean + 2013-2017, Christian Muehlhaeuser +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are met: + +Redistributions of source code must retain the above copyright notice, this +list of conditions and the following disclaimer. + +Redistributions in binary form must reproduce the above copyright notice, this +list of conditions and the following disclaimer in the documentation and/or +other materials provided with the distribution. + +Neither the name of Radu Ioan Fericean nor the names of its contributors may be +used to endorse or promote products derived from this software without specific +prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.