-
Notifications
You must be signed in to change notification settings - Fork 16
Description
MCP Gateway Compliance Review - 2026-01-14
Summary
Found 2 compliance issues during review of commit a53fac8 (PR #245: Add internal/auth package).
Recent Changes Reviewed:
- Full repository initialized (grafted commit history)
- PR Add internal/auth package and update documentation for MCP spec 7.1 authentication #245: Added
internal/authpackage for MCP spec 7.1 authentication - Created comprehensive authentication header parsing with backward compatibility
Commits Reviewed: a53fac8
Critical Issues
Issue 1: Specification Version Constant Out of Date
Specification Section: Version tracking (Header)
Deep Link: https://github.com/githubnext/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md
Current State:
The MCPGatewaySpecVersion constant in internal/server/unified.go:27 is set to "1.5.0", but the official specification is now at version 1.6.0.
Gap:
The implementation version constant does not match the actual specification version being followed.
Severity: Minor (version tracking accuracy)
File References:
internal/server/unified.go:27- MCPGatewaySpecVersion constant
Suggested Fix:
Update the constant to match the specification version:
const MCPGatewaySpecVersion = "1.6.0"Issue 2: Authentication Middleware Not Using internal/auth Package
Specification Section: 7.1 Authorization Header Format, 7.2 API Key Authentication
Deep Link: https://github.com/githubnext/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md#71-authorization-header-format
Requirement:
"Implementations MAY use different formats (e.g., direct value or Bearer scheme)"
"The specific format of the Authorization header is implementation-dependent"
Current State:
There are TWO separate authentication paths in the codebase:
-
Session Extraction (
routed.go:48-69,transport.go:95-115)- Extracts sessionID from Authorization header
- Supports both
"Bearer <token>"and plain"<token>"formats - Used for session/agent identification
- Always active regardless of apiKey configuration
-
API Key Validation (
auth.go:17-42)- Validates Authorization header matches configured apiKey
- Only active when apiKey is configured
- Currently only supports plain format:
if authHeader != apiKey - Does NOT support
"Bearer <key>"format when apiKey is set
The Problem:
- When apiKey IS configured:
authMiddlewarerejects"Bearer <key>"format - When apiKey is NOT configured: session extraction accepts
"Bearer <token>"format - The
internal/authpackage exists withParseAuthHeader()supporting Bearer, but it's not used byauthMiddleware
Gap:
- Documentation (README.md, AGENTS.md, internal/auth comments) claims "Backward compatibility for Bearer tokens is maintained"
- The
internal/authpackage was added in PR Add internal/auth package and update documentation for MCP spec 7.1 authentication #245 to support multiple formats - The actual middleware code in
internal/server/auth.godoesn't import or use it - This means requests with
Authorization: Bearer <key>will be rejected when apiKey is configured
Severity: Important (feature documentation mismatch, breaks backward compatibility claims)
File References:
internal/server/auth.go:1-9- Missing import of internal/auth packageinternal/server/auth.go:31- Direct string comparison instead of using ParseAuthHeader()internal/auth/header.go- Unused authentication parsing logic with Bearer supportREADME.md:167- Claims "Backward compatibility for Bearer tokens is maintained"internal/server/auth.go:14-16- Comment says to use internal/auth package but code doesn't
Suggested Fix:
Update internal/server/auth.go to use the internal/auth package:
package server
import (
"log"
"net/http"
"time"
"github.com/githubnext/gh-aw-mcpg/internal/auth"
"github.com/githubnext/gh-aw-mcpg/internal/logger"
)
// authMiddleware implements API key authentication per spec section 7.1
// Uses internal/auth package for spec-compliant header parsing
func authMiddleware(apiKey string, next http.HandlerFunc) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
authHeader := r.Header.Get("Authorization")
// Use auth.ParseAuthHeader to extract API key (supports Bearer, Agent, and plain formats)
providedKey, _, err := auth.ParseAuthHeader(authHeader)
if err != nil {
// Spec 7.1: Missing token returns 401
logger.LogErrorMd("auth", "Authentication failed: missing Authorization header, remote=%s, path=%s", r.RemoteAddr, r.URL.Path)
logRuntimeError("authentication_failed", "missing_auth_header", r, nil)
http.Error(w, "Unauthorized: missing Authorization header", http.StatusUnauthorized)
return
}
// Use auth.ValidateAPIKey for validation
if !auth.ValidateAPIKey(providedKey, apiKey) {
logger.LogErrorMd("auth", "Authentication failed: invalid API key, remote=%s, path=%s", r.RemoteAddr, r.URL.Path)
logRuntimeError("authentication_failed", "invalid_api_key", r, nil)
http.Error(w, "Unauthorized: invalid API key", http.StatusUnauthorized)
return
}
logger.LogInfo("auth", "Authentication successful, remote=%s, path=%s", r.RemoteAddr, r.URL.Path)
next(w, r)
}
}This change would:
- ✅ Support
Authorization: my-key(plain format, per spec 7.1) - ✅ Support
Authorization: Bearer my-key(backward compatibility) - ✅ Support
Authorization: Agent agent-123(agent format) - ✅ Make documentation claims accurate
- ✅ Use the newly created internal/auth package
- ✅ Maintain existing security validation
Compliance Status
| Specification Section | Status | Notes |
|---|---|---|
| ✅ Configuration Format (4.1) | Compliant | JSON schema validation working |
| ✅ Variable Expansion (4.2) | Compliant | Fail-fast on undefined variables |
| ✅ Containerization (3.2.1) | Compliant | Command field properly rejected |
| Partial | Package exists but not used in middleware | |
| Partial | Constant outdated (1.5.0 vs 1.6.0) | |
| ✅ Protocol Translation (5.2) | Compliant | Stdio and HTTP translation working |
| ✅ Server Isolation (6) | Compliant | Container-based isolation |
| ✅ Health Monitoring (8) | Compliant | /health endpoint implemented |
| ✅ Error Handling (9) | Compliant | JSON-RPC error format correct |
Suggested Remediation Tasks
Task 1: Update Specification Version Constant
Description: Update MCPGatewaySpecVersion to match actual spec version
Files: internal/server/unified.go
Specification Reference: https://github.com/githubnext/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md
Estimated Effort: Trivial (1 line change)
Priority: Low (cosmetic issue)
Task 2: Integrate internal/auth Package into Middleware
Description: Refactor authMiddleware to use auth.ParseAuthHeader and auth.ValidateAPIKey
Files: internal/server/auth.go
Specification Reference: https://github.com/githubnext/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md#71-authorization-header-format
Estimated Effort: Small (2-3 hours including tests)
Priority: Medium (feature parity with documentation)
Test Requirements:
- Add test case: Bearer format with apiKey configured should succeed
- Add test case: Agent format with apiKey configured should succeed
- Add test case: Plain format with apiKey configured should succeed (existing behavior)
- Verify existing auth tests still pass
Task 3: Add Compliance Test IDs to Test Suite
Description: Label existing tests with T-CFG-, T-PTL-, T-ISO-, T-AUTH- IDs from spec section 10
Files: Test files in test/integration/ and internal/*/
Specification Reference: https://github.com/githubnext/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md#10-compliance-testing
Estimated Effort: Medium (review and label all tests)
Priority: Low (organizational improvement)
References
- MCP Gateway Specification v1.6.0
- Last review: 2026-01-13 (commit 6420089)
- Commits reviewed: a53fac8
- PR Add internal/auth package and update documentation for MCP spec 7.1 authentication #245: Add internal/auth package and update documentation for MCP spec 7.1 authentication
Positive Findings
The implementation shows strong compliance in most areas:
- ✅ Configuration validation is comprehensive with helpful error messages
- ✅ Variable expansion properly fails fast on undefined variables
- ✅ Containerization requirement is enforced (command field rejected)
- ✅ Protocol translation handles stdio and HTTP correctly
- ✅ Server isolation uses container-based separation
- ✅ Health monitoring implements the /health endpoint
- ✅ Error handling uses proper JSON-RPC error codes
- ✅ Extensive test coverage across unit and integration tests
- ✅ Authentication package has excellent design (just needs to be integrated)
- ✅ Session extraction already supports multiple formats correctly
The issues found are relatively minor and primarily about consistency between code and documentation. The authentication architecture is well-designed - it just needs the final integration step to use the internal/auth package in the middleware.
AI generated by Daily Compliance Checker