feat: Search, Filtering, Rate Limiting & API Key Management#43
feat: Search, Filtering, Rate Limiting & API Key Management#43bad-antics wants to merge 3 commits intoadhit-r:mainfrom
Conversation
SafeDep Report Summary
No dependency changes detected. Nothing to scan. Installation is not linked with SafeDep Tenant. Click here to optionally link your GitHub App installation with SafeDep Tenant. This report is generated by SafeDep Github App |
|
Hi @bad-antics, Thank you for this comprehensive PR addressing search, filtering, rate limiting, and API key management! These are critical features we need. We've merged a few other PRs which appear to have caused merge conflicts on this one. Could you please rebase this PR against the latest Once rebased, we'll prioritize it for thorough testing and review. Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR adds backend support for MCP server search/filtering, introduces an advanced rate limiter implementation (with a management handler), and adds an API key management module plus related database migrations.
Changes:
- Add search/filter HTTP handlers and repository queries for MCP servers, plus supporting DB indexes.
- Add an “advanced” multi-tier rate limiter implementation with configurable endpoint limits and a config/stats handler.
- Add API key CRUD/validation logic plus a migration to enhance the
api_keystable.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
backend/internal/database/search.go |
Implements server search/filter query building and response shaping (incl. highlighting). |
backend/internal/database/search_handler.go |
Adds HTTP endpoints for search, quick-search, recent, and filter-by-status/type. |
backend/internal/middleware/rate_limiter.go |
Introduces a configurable advanced rate limiter and a management handler for stats/config. |
backend/internal/auth/api_keys.go |
Adds API key generation/validation and CRUD HTTP handlers + API key auth middleware. |
migrations/006_add_search_indexes.sql |
Adds full-text/trigram/tag-related indexes (and pg_trgm extension) for search performance. |
migrations/007_enhance_api_keys.sql |
Enhances api_keys table with prefix/scopes/rate-limit/etc. columns and indexes. |
backend/cmd/server/main.go |
Wires search routes into the protected API group. |
| // Search and filtering endpoints | ||
| searchHandler := database.NewSearchHandler(repo, logger) | ||
| database.RegisterSearchRoutes(protected, searchHandler) |
There was a problem hiding this comment.
The PR description lists new rate-limit management endpoints and API key management endpoints, but main.go only wires search routes (and still uses the legacy middleware.RateLimiter). If these features are intended to be part of this PR, their handlers/middleware need to be registered here as well (e.g., AdvancedRateLimiter middleware + RateLimitHandler routes + APIKeyHandler routes/middleware).
| orgIDStr, exists := c.Get("organization_id") | ||
| if !exists { | ||
| orgIDStr = "00000000-0000-0000-0000-000000000000" | ||
| } | ||
|
|
||
| orgID, err := uuid.Parse(orgIDStr.(string)) |
There was a problem hiding this comment.
orgIDStr is type-asserted to string later, but organization_id can be a uuid.UUID in Gin context depending on auth middleware. This can panic at runtime; use a type switch (string/uuid.UUID) and fail closed when organization_id is missing/invalid.
| orgIDStr, exists := c.Get("organization_id") | |
| if !exists { | |
| orgIDStr = "00000000-0000-0000-0000-000000000000" | |
| } | |
| orgID, err := uuid.Parse(orgIDStr.(string)) | |
| orgVal, exists := c.Get("organization_id") | |
| if !exists { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Missing organization ID"}) | |
| return | |
| } | |
| var ( | |
| orgID uuid.UUID | |
| err error | |
| ) | |
| switch v := orgVal.(type) { | |
| case string: | |
| orgID, err = uuid.Parse(v) | |
| case uuid.UUID: | |
| orgID = v | |
| default: | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID type"}) | |
| return | |
| } |
| // highlightText adds HTML highlight tags around matched text | ||
| func highlightText(text, query string) string { | ||
| if query == "" || text == "" { | ||
| return text | ||
| } |
There was a problem hiding this comment.
highlightText produces HTML ( tags) from unescaped database content + user input. If clients render this as HTML, it becomes an XSS risk when server fields contain markup. Prefer returning structured highlight offsets or ensure the text is HTML-escaped before adding markup.
| userID := c.GetString("user_id") // Set by auth middleware | ||
| apiKey := c.GetString("api_key") | ||
| path := c.Request.URL.Path | ||
| method := c.Request.Method | ||
|
|
There was a problem hiding this comment.
This middleware attempts to derive a per-user key from c.GetString("api_key"), but no middleware sets that context key (APIKeyMiddleware sets api_key_id, not the plaintext key). Per-user limiting for API keys will therefore be skipped; consider keying off api_key_id or key_prefix instead (and ensure auth runs before rate limiting).
| orgIDStr, exists := c.Get("organization_id") | ||
| if !exists { | ||
| // Default org for development | ||
| orgIDStr = "00000000-0000-0000-0000-000000000000" | ||
| } | ||
|
|
||
| orgID, err := uuid.Parse(orgIDStr.(string)) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID"}) |
There was a problem hiding this comment.
Organization ID extraction uses a raw string assertion (orgIDStr.(string)) later in this function, but auth middleware can store organization_id as uuid.UUID (e.g., JWT AuthMiddleware). This will panic at runtime; handle both string and uuid.UUID via a type switch and avoid the silent default-org fallback for protected endpoints.
| orgIDStr, exists := c.Get("organization_id") | |
| if !exists { | |
| // Default org for development | |
| orgIDStr = "00000000-0000-0000-0000-000000000000" | |
| } | |
| orgID, err := uuid.Parse(orgIDStr.(string)) | |
| if err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID"}) | |
| orgValue, exists := c.Get("organization_id") | |
| if !exists { | |
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Missing organization ID"}) | |
| return | |
| } | |
| var orgID uuid.UUID | |
| switch v := orgValue.(type) { | |
| case string: | |
| orgID, err = uuid.Parse(v) | |
| if err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID"}) | |
| return | |
| } | |
| case uuid.UUID: | |
| orgID = v | |
| default: | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID type"}) |
| func (rl *AdvancedRateLimiter) normalizeEndpoint(path string) string { | ||
| // Remove UUID patterns | ||
| path = strings.ReplaceAll(path, "/", "/") | ||
|
|
There was a problem hiding this comment.
normalizeEndpoint(): strings.ReplaceAll(path, "/", "/") is a no-op and should be removed (or replaced with actual normalization like trimming trailing slashes). Leaving it in makes the intent unclear.
| -- Add trigram extension for fuzzy matching (if not exists) | ||
| -- This enables LIKE queries to use indexes | ||
| CREATE EXTENSION IF NOT EXISTS pg_trgm; |
There was a problem hiding this comment.
This migration unconditionally attempts CREATE EXTENSION IF NOT EXISTS pg_trgm;. In some managed Postgres environments, extension creation requires elevated privileges and will fail the migration. If this repo targets such environments, consider documenting the requirement or moving extension creation to an environment bootstrap step.
| -- Add trigram extension for fuzzy matching (if not exists) | |
| -- This enables LIKE queries to use indexes | |
| CREATE EXTENSION IF NOT EXISTS pg_trgm; | |
| -- Trigram support for fuzzy matching | |
| -- NOTE: This migration assumes that the pg_trgm extension has already been installed | |
| -- by a database administrator or environment bootstrap step. Creating extensions | |
| -- often requires elevated privileges that application migration roles do not have. | |
| -- | |
| -- To install pg_trgm (run once per database, with appropriate privileges): | |
| -- CREATE EXTENSION IF NOT EXISTS pg_trgm; | |
| -- | |
| -- The following block verifies that the extension is available before creating | |
| -- trigram-based indexes, and fails fast with a clear message if it is missing. | |
| DO $$ | |
| BEGIN | |
| IF NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'pg_trgm') THEN | |
| RAISE EXCEPTION 'pg_trgm extension must be installed by a database administrator before running migration 006_add_search_indexes.sql'; | |
| END IF; | |
| END | |
| $$; |
| -- Ensure key_prefix is unique | ||
| IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'api_keys_key_prefix_unique') THEN | ||
| ALTER TABLE api_keys ADD CONSTRAINT api_keys_key_prefix_unique UNIQUE (key_prefix); | ||
| END IF; |
There was a problem hiding this comment.
key_prefix is constrained UNIQUE but is derived from only 8 characters of base64 output. At higher key volumes this increases the chance of collision, which would make API key creation fail unexpectedly. Consider increasing the stored prefix length (and the unique constraint) or using a longer random identifier for lookup.
| orgIDStr := c.GetString("organization_id") | ||
| if orgIDStr == "" { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Organization ID required"}) | ||
| return | ||
| } | ||
|
|
||
| orgID, err := uuid.Parse(orgIDStr) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID"}) | ||
| return | ||
| } | ||
|
|
||
| userIDStr := c.GetString("user_id") | ||
| var userID *uuid.UUID | ||
| if userIDStr != "" { | ||
| if id, err := uuid.Parse(userIDStr); err == nil { | ||
| userID = &id |
There was a problem hiding this comment.
Using c.GetString("organization_id") assumes organization_id is stored as a string, but JWT auth stores uuid.UUID in context (so GetString returns ""). With the current auth middleware selection in main.go, organization_id also may not be set at all. Consider reading organization_id via c.Get + type switch (string/uuid.UUID) and returning 401 when missing.
| orgIDStr := c.GetString("organization_id") | |
| if orgIDStr == "" { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Organization ID required"}) | |
| return | |
| } | |
| orgID, err := uuid.Parse(orgIDStr) | |
| if err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID"}) | |
| return | |
| } | |
| userIDStr := c.GetString("user_id") | |
| var userID *uuid.UUID | |
| if userIDStr != "" { | |
| if id, err := uuid.Parse(userIDStr); err == nil { | |
| userID = &id | |
| // Retrieve organization ID from context; it may be stored as uuid.UUID or string. | |
| orgVal, ok := c.Get("organization_id") | |
| if !ok { | |
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Organization ID required"}) | |
| return | |
| } | |
| var orgID uuid.UUID | |
| switch v := orgVal.(type) { | |
| case uuid.UUID: | |
| orgID = v | |
| case string: | |
| parsed, err := uuid.Parse(v) | |
| if err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID"}) | |
| return | |
| } | |
| orgID = parsed | |
| default: | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid organization ID type"}) | |
| return | |
| } | |
| // Retrieve user ID from context; it may be stored as uuid.UUID or string. | |
| var userID *uuid.UUID | |
| if userVal, ok := c.Get("user_id"); ok { | |
| switch v := userVal.(type) { | |
| case uuid.UUID: | |
| u := v | |
| userID = &u | |
| case string: | |
| if v != "" { | |
| if parsed, err := uuid.Parse(v); err == nil { | |
| userID = &parsed | |
| } | |
| } |
| "DELETE:/api/v1/mcp/servers": { | ||
| Path: "/api/v1/mcp/servers", |
There was a problem hiding this comment.
The configured delete endpoint limit is keyed as "DELETE:/api/v1/mcp/servers", but the actual route is DELETE /api/v1/mcp/servers/:id. Since normalizeEndpoint replaces IDs with :id, this config entry won’t match and deletes won’t be endpoint-rate-limited as intended.
| "DELETE:/api/v1/mcp/servers": { | |
| Path: "/api/v1/mcp/servers", | |
| "DELETE:/api/v1/mcp/servers/:id": { | |
| Path: "/api/v1/mcp/servers/:id", |
|
Hey @adhit-r! Thanks for the feedback. Will rebase this against the latest main to resolve the merge conflicts and push the updated branch shortly! |
Implements issue adhit-r#36 - Search & Filtering Enhancement Features: - Full-text search across server name, description, and URL - Filter by status (online, offline, error, unknown) - Filter by type (filesystem, database, api, custom) - Filter by tags (stored in metadata JSONB) - Date range filtering (created_at) - Sorting by multiple fields (name, created_at, updated_at, status) - Pagination with page/offset support - Search result highlighting New endpoints: - GET/POST /api/v1/servers/search - Advanced search with filters - GET /api/v1/servers/quick-search - Simple text search - GET /api/v1/servers/recent - Recently modified servers - GET /api/v1/servers/filter/status/:status - Filter by status - GET /api/v1/servers/filter/type/:type - Filter by type Database changes: - Migration 006: Adds GIN indexes for full-text search - Adds pg_trgm extension for fuzzy matching - Adds composite indexes for common query patterns Closes adhit-r#36
Implements issue adhit-r#15 - API Rate Limiting Features: - Multi-tier rate limiting: - Global rate limiting across all clients - Per-IP rate limiting - Per-user/API key rate limiting - Per-endpoint rate limiting with custom limits - Configurable limits per endpoint: - Health check: 600/min (permissive) - Search: 60/min (moderate) - Create server: 30/min (strict) - Delete server: 10/min (very strict) - Security tests: 10/min (resource intensive) - Auth login: 20/min (brute force prevention) - Auth register: 10/min (spam prevention) - Rate limit response headers: - X-RateLimit-Limit - X-RateLimit-Remaining - X-RateLimit-Reset - Retry-After - Memory management: - Automatic cleanup of stale entries - Configurable TTL for entries - Stats endpoint for monitoring - Management API: - GET /api/v1/rate-limit/stats - GET /api/v1/rate-limit/config - PUT /api/v1/rate-limit/config Closes adhit-r#15
Implements issue adhit-r#35 - API Key Management Features: - Secure API key generation (mcp_ prefix + 32 random bytes) - Key prefix for identification without exposing full key - SHA-256 hashing for secure storage - Constant-time comparison for validation Key capabilities: - Per-key rate limits - Permission-based access control - Scope-based authorization (servers:read, etc.) - Key expiration with configurable TTL - Key revocation - Key regeneration without changing metadata - Usage tracking (last used time/IP) API Endpoints: - POST /api/v1/api-keys - Create new key - GET /api/v1/api-keys - List all keys - GET /api/v1/api-keys/:id - Get key details - PUT /api/v1/api-keys/:id - Update key settings - DELETE /api/v1/api-keys/:id - Revoke key - POST /api/v1/api-keys/:id/regenerate - Regenerate secret Middleware: - APIKeyMiddleware for validating requests - Automatic last-used tracking - Context injection for downstream handlers Database: - Migration 007: Enhances api_keys table - Indexes for prefix lookup and org queries Closes adhit-r#35
bad-antics
force-pushed
the
feature/search-filtering
branch
from
5cee491 to
efe4843
Compare
|
Hey @adhit-r! Rebased against latest |
Summary
This PR implements multiple requested features in a cohesive manner:
1. Search & Filtering (Closes #36)
Full-text search and advanced filtering for MCP servers:
Endpoints:
GET/POST /api/v1/servers/searchGET /api/v1/servers/quick-searchGET /api/v1/servers/recentGET /api/v1/servers/filter/status/:statusGET /api/v1/servers/filter/type/:type2. Enhanced Rate Limiting (Closes #15)
Multi-tier rate limiting system:
Endpoints:
GET /api/v1/rate-limit/statsGET /api/v1/rate-limit/configPUT /api/v1/rate-limit/config3. API Key Management (Closes #35)
Comprehensive API key system:
Endpoints:
POST /api/v1/api-keysGET /api/v1/api-keysGET /api/v1/api-keys/:idPUT /api/v1/api-keys/:idDELETE /api/v1/api-keys/:idPOST /api/v1/api-keys/:id/regenerateDatabase Migrations
006_add_search_indexes.sql- Full-text search indexes, GIN indexes007_enhance_api_keys.sql- API key table enhancementsTesting
Breaking Changes
None - all additions are backward compatible.
Files Changed
backend/internal/database/search.go- Search repositorybackend/internal/database/search_handler.go- Search HTTP handlersbackend/internal/middleware/rate_limiter.go- Advanced rate limiterbackend/internal/auth/api_keys.go- API key managementmigrations/006_add_search_indexes.sqlmigrations/007_enhance_api_keys.sql