Remote verification #221
Closed
Remote verification #221
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Work in progress . This commit adds the core registry service implementation with methods to: - Retrieve all registry entries - List entries with cursor-based pagination - Fetch specific server details by ID
Init world. Basic registry API server <WIP>
Update README to reflect binary name change
Add count to server response and fix Makefile paths
…retrieval is validated
Add Swagger API documentation and handlers for v0
Contains about 300+ initial servers that can be used to seed a mongoDB
We need to work in multiple orgs with different private repos, this is a temporary solution until we make the main repo public.
…blishing - Added authentication service and GitHub OAuth integration. - Introduced new authentication methods and structures in the model. - Updated the API to handle authentication during the publishing process. - Created handlers for starting and checking the status of the OAuth flow. - Enhanced the publish handler to validate authentication credentials. - Updated configuration to include GitHub OAuth settings. - Added tests for the OAuth flow and publishing with authentication. - Updated documentation to reflect changes in the API and authentication process.
…e argument descriptions
…cation and update documentation
…b application and improve error handling
…ecks and introducing dynamic auth method determination
Co-authored-by: toby <83556+toby@users.noreply.github.com>
…t timestamps and show flags in --long-flag/-short format Co-authored-by: toby <83556+toby@users.noreply.github.com>
…#164) <!-- Provide a brief summary of your changes --> Add more information about the value field under Input in the API doc. ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> the descrioption for [value](https://github.com/modelcontextprotocol/registry/blob/81e5174a593a7488715b0bf0ae77a4dc466b4d35/docs/openapi.yaml#L219) is not clearly. ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> No need. ## Breaking Changes <!-- Will users need to update their code or configurations? --> No ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ x ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [ x ] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [ x ] My code follows the repository's style guidelines - [ x ] New and existing tests pass locally - [ x ] I have added appropriate error handling - [ x ] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions -->
<!-- Provide a brief summary of your changes --> ## Motivation and Context This is in support of #159, to make it easier, I hope. My team noticed some differences in the JSON schema, most notably the "choices" property was missing. I tried to write a tool to do this but slowed down since I am not experienced with Go. This is a first pass, maybe someone else can finish the tooling. I restructured the JSON schema to break out child objects like the YAML file, and brought the descriptions and example into sync. The only differences are two extra properties in the YAML which are computed by the server: - `VersionDetail.is_latest` is not present in the JSON schema, computed by the registry based on #158 - `Server.id` is not present in the JSON schema, generated by the registry The idea is the a tool could look at ServerDetail in the YAML and then generate the schema.json with minimal fix-ups (add a root title, remove `VersionDetail.is_latest` and `Server.id`. ~Question for @tadasant - should `VersionDetail.release_date` be in the JSON schema? I think _no_ (and let it be computed) but it's there right now.~ - aligned with #167 and removed ## How Has This Been Tested? I have tested the JSON schema against the sample server.json in the repo (files and markdown blocks). ## Breaking Changes None. ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [x] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [x] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions -->
Closes #155 The "generalized" `server.json` is the one that can be used in a variety of situations: - Discoverability on a centralized registry (i.e. our official Registry work) - Discoverability on a decentralized .well-known endpoint - As a response to an initialization call, so the client knows information about the MCP server to which it is connecting - As an input into crafting a [DXT file](https://www.anthropic.com/engineering/desktop-extensions) - Packaged in with the source code of an MCP server, so as to have a structured way context Of these, the official Registry is but one use case. The Registry has a unique set of concerns for which we are optimizing, like whitelisting of specific registries we trust to host MCP server source code for the public community (`npm`, `pypi`, etc). It is not necessary to have such a constraint for the generalized server.json, which may be used in, for example, an internal context where such a validation doesn't make sense. So I've done the following at a high level: - Renamed the prior `schema.json` to be `registry-schema.json` - Introduced a more generic `schema.json` to serve as the schema for the generalized `server.json` - Refactored `registry-schema.json` to be simply a constraint on the base `schema.json` I made a few small changes in this transition: - Removed `gitlab` as an option in `repository.source` for `registry-schema.json`. I'm fine going live with just GitHub support; would be happy to take a contribution from e.g. Gitlab employees or a motivated Gitlab customer to expand anytime. - Removed `homebrew` as an option in `packages.registry_name`; never really made sense, not a place where folks are publishing MCP servers - Added `docker` to `packages.runtime_hint`; was just a missing oversight - Gave both schemas a global `$id` ~- Removed `release_date` from both schemas. Certainly not something that makes sense for the generalized schema. It actually doesn't make sense for the Registry schema either, because this is the server.json that _users will submit_, i.e. the _input_ into the Registry publication API. `release_date` will be a piece of _output_ metadata. The Registry API will be free to append such extra metadata for GET /:id requests of servers, and the data can be present in the OpenAPI spec, but it does not need to be in the registry-server.json JSON schema.~ - Note this is no longer in this PR; rebased on #168 which included it - Removed unnecessary enums from the generalized `schema.json`
## Summary - Adds CLAUDE.md file to provide guidance for Claude Code instances working with this repository - Includes essential development commands and high-level architecture overview - Follows the standard format for Claude Code documentation ## Details The CLAUDE.md file contains: - Common development commands for building, running, testing, and linting - Architecture overview explaining the clean architecture pattern - Request flow documentation - Key interfaces and their implementations - Authentication flow details - Important design patterns used in the codebase This will help future Claude Code instances be more productive when working with the MCP Registry codebase. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Avinash Sridhar <sridharavinash@users.noreply.github.com>
Closes #149 As noted in the FAQ update here, this is a helpful starting point in thinking about this: ``` ### What is the difference between "Official MCP Registry", "MCP Registry", "MCP registry", "MCP Registry API", etc? There are four underlying concepts: - "MCP Server Registry API" (or "MCP Registry API"): The OpenAPI specification defined in [openapi.yaml](./server-registry-api/openapi.yaml). This is a reusable API specification that anyone building any sort of "MCP server registry" should consider adopting / aligning with. - "Official MCP Registry" (or "MCP Registry"): The application that lives at `https://registry.modelcontextprotocol.io`. This registry currently only catalogs MCP servers, but may be extended in the future to also catalog MCP client/host apps and frameworks. - "Official MCP Registry API": The REST API that lives at `https://registry.modelcontextprotocol.io/api`, with an OpenAPI specification defined at [official-registry-openapi.yaml](./server-registry-api/official-registry-openapi.yaml) - "MCP server registry" (or "MCP registry"): A third party, likely commercial, implementation of the MCP Server Registry API or derivative specification. ``` Prior to this PR, we were just treating "MCP Server Registry API" and "Official MCP Registry API" to be the same thing. They are not the same: the latter is inherently a more constrained version of the former. A private deployment of a similar registry need not have constraints like "the only valid source code repositories are `github` and `gitlab`". This PR separates out the definitions to match that path forward. ## Notable Changes ### Base OpenAPI spec (`openapi.yaml`) - Removed official-registry-specific constraints: - No maximum limit on `/servers` endpoint - No enum constraint on `Repository.source` - No enum constraint on `Package.registry_name` - Removed `/v0` prefix from paths (it's now just baked into the `official` one's URL prefix) - Added `$id` for proper schema identification ### Official Registry OpenAPI spec (`official-registry-openapi.yaml`) - Created as a derivative of the base spec using `$ref` - Re-adds official-registry-specific constraints: - `Repository.source` enum: `[github]` - `Package.registry_name` enum: `[npm, docker, pypi, nuget]` ### Documentation updates - Added `server-registry-api/README.md` explaining the relationship between specs - Updated FAQ to clarify terminology - Reorganized docs structure
<!-- Provide a brief summary of your changes --> ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> This is one attempt to resolve #169. Instead of adding a new "constant" argument type, I expanded the existing positional one and just dropped the `value_hint` requirement. I am not sure if this has VS Code impact @sandy081, @connor4312. Also, removed unneeded release_date values from the samples. ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> I tested the current server.json samples in the repo against the new (less strict) schema. Of course they passed (aside from an unrelated bug in my previous NuGet sample 🤦). ## Breaking Changes <!-- Will users need to update their code or configurations? --> ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [x] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [x] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions -->
<!-- Provide a brief summary of your changes --> ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> Resolve #173 ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> Copilot fixed my spelling mistakes. ## Breaking Changes <!-- Will users need to update their code or configurations? --> None. ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [x] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [ ] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [ ] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions --> Ideally the `id` would be populated by the publisher CLI tool "on the way out" and MCP Registry would validate the `id`, perhaps using the token used for auth in the case of private repositories.
## Summary This PR improves the developer experience by adding clear linting documentation and optional pre-commit hooks to catch issues before CI. Helpful to avoid fighting Claude Code / Copilot on "please make sure linting passes CI". ## Changes - Updated `README.md` with: - Go 1.23.x requirement (was "1.18 or later") - golangci-lint v1.61.0 in prerequisites - New Development section with linting instructions - Updated `CLAUDE.md` with: - Development Setup section - Added `.githooks/pre-commit`: - Runs golangci-lint and gofmt checks - References README for installation instructions - Can be bypassed with `--no-verify` when needed ## Test Plan - [x] Verified pre-commit hook blocks commits when golangci-lint is missing - [x] Confirmed hook shows clear error message pointing to README - [x] Tested that linting passes with Go 1.23.0 and golangci-lint v1.61.0 - [x] Verified gofmt correctly identifies formatting issues Co-authored w/ Claude Code --------- Co-authored-by: Charles Lowell <10964656+chlowell@users.noreply.github.com>
This is a first PR for #159 - adding some validation scripts. I want to get to a place where we are confident that making schema changes is in sync with the rest of our code. Step one is making sure the schemas are in sync with our examples. Step two will be making sure the schemas are in sync with our actual Registry implementation code. This is just for the JSON right now; if we're feeling good about this approach I'll do an analogous change for the OpenAPI schemas as well. I'll also follow with adding usage of these scripts to our CI pipeline so we have automated checks on them with every change. ## Summary - Added CLI validation tools for JSON schemas and examples in `docs/server-json/` - Fixed some minor validation errors found in `examples.md` - Ensures schema consistency and example validity in CI/CD pipelines ## Changes ### New Validation Tools 1. **`tools/validate-schemas`** - Validates that `schema.json` and `registry-schema.json` are valid JSON Schema documents 2. **`tools/validate-examples`** - Validates all JSON examples in `examples.md` against both schemas ### Key Features - ✅ Validates JSON schema syntax and structure - ✅ Validates all examples against both base and registry schemas - ✅ Tracks validation counts to prevent silent failures - ✅ Proper exit codes for CI integration - ✅ Clear error messages for debugging ### Fixes to examples.md - Added missing `repository.id` field in NuGet example - Changed `repository.source` from "gitlab" to "github" in Docker example ### Usage ```bash # From repository root ./tools/validate-schemas.sh ./tools/validate-examples.sh ``` ## Testing I ran these scripts locally with minor (breaking) changes, which they each correctly reported. As submitted, they are both passing. 🤖 Co-authored with [Claude Code](https://claude.ai/code) --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Charles Lowell <10964656+chlowell@users.noreply.github.com>
The `publisher create` command now automatically sets the `runtime_hint` field in the generated `server.json` based on the `registry_name` when no explicit `--runtime-hint` is provided: - When `registry_name` is `docker`, sets `runtime_hint` to `docker` - When `registry_name` is `npm`, sets `runtime_hint` to `npx` - Other registries remain unchanged (no `runtime_hint` field) ## Before ```bash # NPM registry - no runtime_hint field generated $ mcp-publisher create --name "test-server" --description "Test" --repo-url "https://github.com/test/repo" --registry npm ``` ```json { "packages": [ { "registry_name": "npm", "name": "test-server", "version": "1.0.0" } ] } ``` ## After ```bash # NPM registry - automatically sets runtime_hint to "npx" $ mcp-publisher create --name "test-server" --description "Test" --repo-url "https://github.com/test/repo" --registry npm ``` ```json { "packages": [ { "registry_name": "npm", "name": "test-server", "version": "1.0.0", "runtime_hint": "npx" } ] } ``` The manual `--runtime-hint` flag continues to work and takes precedence over the automatic setting, ensuring backward compatibility. Fixes #204. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click [here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to start the survey. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: toby <83556+toby@users.noreply.github.com>
The following PR adds a new server `status` field to support the status of servers, like deprecated or active Details: * Add status field to server.json schema with 'active' and 'deprecated' values * Update OpenAPI spec to include status field in Server responses * Add ServerStatus enum and field to Go model structs * Update examples to demonstrate status field usage (`active` and `deprecated`) Since this is my first PR there's a good chance I might have missed something so I'll appreciate any feedback to make this and any PRs in the future easier for you to review 😃 Fixes: #181 ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> The main goal is to have a way for server authors to notify registry consumers that a server may no longer be maintained and shouldn't be considered for using. ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> ## Breaking Changes <!-- Will users need to update their code or configurations? --> ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [ ] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions --> --------- Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com> Co-authored-by: Tadas Antanavicius <tadas@tadasant.com>
#206) Closes [#22191](github/copilot#22191) This PR implements cryptographically secure 128-bit token generation for domain ownership verification in the MCP Registry. The package provides a single, focused function for generating base64url-encoded tokens suitable for both DNS TXT record and HTTP-01 challenge verification methods. ## Motivation and Context The MCP Registry requires a robust domain verification system to ensure that server publishers actually own the domains they claim. This package provides the foundational token generation capability needed for: 1. **DNS TXT Record Verification**: Publishers add `mcp-verify=<token>` to their domain's DNS 2. **HTTP-01 Web Challenge**: Publishers serve tokens at `https://domain/.well-known/mcp-challenge/<token>` The implementation follows security best practices with 128-bit entropy and base64url encoding for maximum compatibility with both DNS and HTTP standards. ## How Has This Been Tested? - **Unit tests**: Comprehensive test suite covering token generation, uniqueness, entropy validation, and error handling - **Security tests**: Verified 128-bit entropy, cryptographically secure randomness, and proper base64url encoding - **Compatibility tests**: Ensured tokens are safe for both DNS TXT records and HTTP URLs - **Performance tests**: Benchmarked token generation (~196ns/op on Apple M4 Max) - **Integration tests**: Verified the package builds and integrates correctly with the larger registry codebase - **Linting**: Passed all Go linting tools (go vet, gofmt, golangci-lint) ## Breaking Changes No breaking changes. This is a new package that doesn't affect existing functionality. ## Types of changes - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [x] I have added appropriate error handling - [x] I have added or updated documentation as needed ## Additional context **Design Decisions:** - **Simplified API**: Based on PR feedback, removed `ValidateTokenFormat` and `TokenInfo` struct to keep the API minimal and focused on core functionality - **String comparison**: Token validation is handled through simple string comparison rather than format validation, as suggested in code review - **128-bit entropy**: Chosen to provide cryptographically secure randomness while maintaining reasonable token length (22 characters) - **Base64url encoding**: Ensures tokens are safe for both DNS TXT records and HTTP URLs without escaping **Security Considerations:** - Uses `crypto/rand` for cryptographically secure random number generation - Tokens should be treated as single-use and time-limited - Always use HTTPS when transmitting tokens - 2^128 possible values make collision probability negligible **Performance:** - Token generation is extremely fast (~196ns/op) and suitable for real-time web applications - No external dependencies --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Trent Jones <57182095+trent-j@users.noreply.github.com> Co-authored-by: Thomas Sickert <80130182+thomas-sickert@users.noreply.github.com>
Closes [#22190](github/copilot#22190) <!-- Provide a brief summary of your changes --> ## Motivation and Context This PR implements DNS record verification functionality for the MCP Registry's server name verification system. This feature enables domain owners to prove ownership of their domains by adding specific TXT records to their DNS, which is required for publishing MCP servers under domain-scoped namespaces (e.g., com.example/my-server). This prevents domain impersonation and ensures only legitimate domain owners can publish under their domains. The implementation follows industry best practices used by certificate authorities and cloud services for domain ownership verification, providing a secure and reliable method for continuous domain validation. ## How Has This Been Tested? - Comprehensive unit tests: Added extensive test coverage for all DNS verification functions including success cases, failure scenarios, timeout handling, and retry logic - Mock DNS resolver: Implemented a complete mock DNS resolver system for reliable testing without external dependencies - Token generation and validation: Added tests for cryptographically secure token generation and format validation Error handling: Tested various error conditions including DNS failures, timeouts, and invalid inputs - Integration scenarios: Tested with both mock and real DNS configurations - Performance benchmarks: Added benchmark tests for token generation and validation functions - Example CLI tool: Created a dns-verify example application for manual testing and demonstration ## Breaking Changes <!-- Will users need to update their code or configurations? --> ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [X] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [X] My code follows the repository's style guidelines - [X] New and existing tests pass locally - [X] I have added appropriate error handling - [X] I have added or updated documentation as needed ## Additional context - Cryptographically secure tokens: 128-bit random tokens using crypto/rand with base64url encoding - Robust DNS resolution: Configurable DNS resolvers with support for secure resolvers (8.8.8.8, 1.1.1.1) - Retry logic: Exponential backoff for transient DNS failures with configurable timeouts and retry limits - Comprehensive error handling: Detailed error types with proper error wrapping and retry logic - Security considerations: Uses secure DNS resolvers, validates token formats, and implements proper timeout handling - Extensive documentation: Added detailed README with usage examples, security considerations, and integration guides --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Trent Jones <57182095+trent-j@users.noreply.github.com>
<!-- Provide a brief summary of your changes --> ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> ## Breaking Changes <!-- Will users need to update their code or configurations? --> ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [x] I have added appropriate error handling - [ x I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions --> --------- Co-authored-by: Ameya Phansalkar <aphansal123@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
internal/database/mongo.go
Outdated
| @@ -117,7 +138,7 @@ | |||
|
|
|||
| // Fetch the document at the cursor to get its sort values | |||
| var cursorDoc model.Server | |||
| err := db.collection.FindOne(ctx, bson.M{"id": cursor}).Decode(&cursorDoc) | |||
| err := db.serverCollection.FindOne(ctx, bson.M{"id": cursor}).Decode(&cursorDoc) | |||
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
To fix the problem, we should ensure that the user-provided cursor value is strictly validated and type-checked before being used in the MongoDB query. Since the code already checks that the cursor is a valid UUID, the main improvement is to ensure that the query uses the correct type for the id field and that no other types can be injected. In MongoDB, if the id field is always a string (UUID), we should ensure that the query uses a string type and not allow any other BSON types. Additionally, we can explicitly cast the cursor to a string after validation, and avoid using any user input directly in the query without validation.
Changes to make:
- After validating the cursor as a UUID, use the parsed UUID's string representation for the query, rather than the raw user input.
- This ensures that even if the user input is a valid UUID, only its canonical string form is used in the query.
- No changes to imports or method definitions are needed, just a small change in the way the cursor is handled in the query.
Suggested changeset
1
internal/database/mongo.go
| @@ -143,3 +143,4 @@ | ||
| // Validate that the cursor is a valid UUID | ||
| if _, err := uuid.Parse(cursor); err != nil { | ||
| parsedUUID, err := uuid.Parse(cursor) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("invalid cursor format: %w", err) | ||
| @@ -147,5 +148,8 @@ | ||
|
|
||
| // Use the canonical string representation of the UUID for the query | ||
| cursorStr := parsedUUID.String() | ||
|
|
||
| // Fetch the document at the cursor to get its sort values | ||
| var cursorDoc model.Server | ||
| err := db.serverCollection.FindOne(ctx, bson.M{"id": cursor}).Decode(&cursorDoc) | ||
| err = db.serverCollection.FindOne(ctx, bson.M{"id": cursorStr}).Decode(&cursorDoc) | ||
| if err != nil { | ||
| @@ -157,3 +161,3 @@ | ||
| // Use the cursor document's ID to paginate (records with ID > cursor's ID) | ||
| mongoFilter["id"] = bson.M{"$gt": cursor} | ||
| mongoFilter["id"] = bson.M{"$gt": cursorStr} | ||
| } |
Copilot is powered by AI and may make mistakes. Always verify output.
internal/database/mongo.go
Outdated
| @@ -138,7 +159,7 @@ | |||
| } | |||
|
|
|||
| // Execute find operation with options | |||
| mongoCursor, err := db.collection.Find(ctx, mongoFilter, findOptions) | |||
| mongoCursor, err := db.serverCollection.Find(ctx, mongoFilter, findOptions) | |||
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Copilot Autofix
AI 15 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
internal/database/mongo.go
Outdated
| @@ -195,7 +216,7 @@ | |||
| } | |||
|
|
|||
| var existingEntry model.ServerDetail | |||
| err := db.collection.FindOne(ctx, filter).Decode(&existingEntry) | |||
| err := db.serverCollection.FindOne(ctx, filter).Decode(&existingEntry) | |||
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 18 days ago
To fix the problem, we should sanitize and validate the serverDetail.Name field before using it in a MongoDB query filter. The best approach is to ensure that serverDetail.Name is a string that matches a safe pattern (e.g., only allows alphanumeric characters, dashes, underscores, and periods), and to reject or sanitize any input that does not conform. This should be done as early as possible, ideally in the API handler before the value is passed to the database layer.
Specifically, in internal/api/handlers/v0/publish.go, before calling registry.Publish(&serverDetail), we should validate serverDetail.Name using a regular expression. If the name does not match the allowed pattern, return a 400 Bad Request error. This prevents any malicious or malformed input from reaching the database layer.
We will:
- Add a regular expression to validate
serverDetail.Namein the handler. - Reject requests with invalid names.
- (No changes are needed in the database layer, as the input will be validated before reaching it.)
Suggested changeset
1
internal/api/handlers/v0/publish.go
Outside changed files
| @@ -8,2 +8,3 @@ | ||
| "net/http" | ||
| "regexp" | ||
| "strings" | ||
| @@ -17,2 +18,4 @@ | ||
|
|
||
| var nameRegexp = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) | ||
|
|
||
| // PublishHandler handles requests to publish new server details to the registry | ||
| @@ -54,2 +57,8 @@ | ||
| return | ||
| } | ||
| // Validate that Name contains only allowed characters (alphanumeric, dash, underscore, period) | ||
| validName := nameRegexp.MatchString(serverDetail.Name) | ||
| if !validName { | ||
| http.Error(w, "Name contains invalid characters", http.StatusBadRequest) | ||
| return | ||
| } |
Copilot is powered by AI and may make mistakes. Always verify output.
Co-authored-by: Ameya Phansalkar <aphansal123@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: adam jones <domdomegg@users.noreply.github.com>
| } | ||
|
|
||
| opts := options.Update().SetUpsert(true) | ||
| _, err := db.verificationCollection.UpdateOne(ctx, domainFilter, update, opts) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
To fix the problem, we need to ensure that the domain value used in MongoDB queries is strictly validated and cannot contain any characters or patterns that could be interpreted as MongoDB operators or otherwise manipulate the query. The best way to do this is to add a validation function that checks the normalized domain string for allowed characters (e.g., only alphanumeric characters, hyphens, and dots), and rejects any input that does not conform. This validation should be performed immediately after normalization, before the value is passed to the database layer.
Steps:
- Implement a
validateDomainfunction that uses a regular expression to ensure the domain contains only allowed characters. - Call this function in the handler after
normalizeDomainand before passing the domain to the service layer. - If validation fails, return an error to the user and do not proceed with the database operation.
Files/regions to change:
internal/api/handlers/v0/verification.go: Add thevalidateDomainfunction and call it in the handler after normalization.- No changes are needed in the database or service layers, as the handler will now guarantee that only safe domain strings reach those layers.
Suggested changeset
1
internal/api/handlers/v0/verification.go
Outside changed files
| @@ -37,2 +37,14 @@ | ||
|
|
||
| // validateDomain ensures the domain contains only allowed characters (alphanumeric, hyphens, dots) | ||
| func validateDomain(domain string) error { | ||
| // Only allow a-z, 0-9, hyphens, and dots | ||
| for _, c := range domain { | ||
| if (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '-' || c == '.' { | ||
| continue | ||
| } | ||
| return errors.New("domain contains invalid characters") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // DomainClaimRequest represents the request body for domain claiming | ||
| @@ -89,2 +101,8 @@ | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| // Validate the normalized domain for allowed characters | ||
| if err := validateDomain(normalizedDomain); err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| return |
Copilot is powered by AI and may make mistakes. Always verify output.
internal/database/mongo.go
Outdated
| } | ||
|
|
||
| var domainVerification model.DomainVerification | ||
| err := db.verificationCollection.FindOne(ctx, filter).Decode(&domainVerification) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
To fix the problem, we should strictly validate the domain after normalization to ensure it is a valid DNS hostname and does not contain any special characters, whitespace, or structures that could be interpreted as BSON/JSON objects or arrays. This can be done by adding a validation step in the normalizeDomain function to check that the result matches a regular expression for valid hostnames. If the domain does not match, return an error. This ensures that only safe, simple strings are used in the MongoDB query filter, preventing NoSQL injection.
Files/regions to change:
internal/api/handlers/v0/verification.go: Update thenormalizeDomainfunction to include strict validation of the extracted domain using a regular expression for valid hostnames.
What is needed:
- Import the
regexppackage. - Add a regular expression for valid hostnames.
- Add a validation step in
normalizeDomainafter extracting the host.
Suggested changeset
1
internal/api/handlers/v0/verification.go
Outside changed files
| @@ -8,2 +8,3 @@ | ||
| "strings" | ||
| "regexp" | ||
|
|
||
| @@ -21,5 +22,12 @@ | ||
|
|
||
| // Hostname validation regex (RFC 1035, simplified) | ||
| var hostnameRegex = regexp.MustCompile(`^(?i)[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?(?:\.[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?)*$`) | ||
|
|
||
| // Try parsing as URL first (handles cases with protocol) | ||
| if u, err := url.Parse(domain); err == nil && u.Host != "" { | ||
| return strings.ToLower(u.Host), nil | ||
| host := strings.ToLower(u.Host) | ||
| if !hostnameRegex.MatchString(host) { | ||
| return "", errors.New("invalid domain format") | ||
| } | ||
| return host, nil | ||
| } | ||
| @@ -29,3 +37,7 @@ | ||
| if u, err := url.Parse("https://" + domain); err == nil && u.Host != "" { | ||
| return strings.ToLower(u.Host), nil | ||
| host := strings.ToLower(u.Host) | ||
| if !hostnameRegex.MatchString(host) { | ||
| return "", errors.New("invalid domain format") | ||
| } | ||
| return host, nil | ||
| } |
Copilot is powered by AI and may make mistakes. Always verify output.
domdomegg
reviewed
Aug 8, 2025
registry
Outdated
|
I sincerely apologize for the disruption. This PR was accidentally closed due to a git history rewrite mistake. The PR has been recreated as #238 with all the original content preserved. Please continue any discussions or reviews on the new PR: #238 Again, I apologize for the inconvenience this has caused. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context