[WIP] Jira MCP integration #225
Conversation
Code Review: Move .mcp.json into runnersSummaryThis PR simplifies MCP server configuration by removing the dynamic discovery mechanism and using a single, centrally-controlled configuration file. The changes improve security by preventing user workspaces from loading arbitrary MCP servers. ✅ StrengthsSecurity Improvements
Code Quality
Container Integration
🔍 Issues & Recommendations1. Unused Parameter (Minor)Issue: The Recommendation: Consider one of these options: # Option A: Keep parameter for backward compatibility (current approach is fine)
def _load_mcp_config(self, cwd_path: str) -> dict | None:
"""..."""
# cwd_path intentionally unused - MCP config only loaded from runner directory
# Option B: Remove parameter if nothing calls this with explicit arguments
def _load_mcp_config(self) -> dict | None:Impact: Low - the current approach maintains API compatibility 2. Missing Test Coverage (Medium)Issue: No tests found for MCP configuration loading in Recommendation: Add unit tests covering:
Example test structure: def test_load_mcp_config_success():
# Test successful load from runner directory
def test_load_mcp_config_not_found():
# Test graceful handling when file missing
def test_load_mcp_config_invalid_json():
# Test error handling for malformed JSON
def test_filter_mcp_servers_only_allows_http_sse():
# Test security filtering3. Documentation Gap (Low)Issue: No documentation found explaining MCP configuration to operators/admins. Recommendation: Add documentation explaining:
Suggested location: 4. Container Build Consideration (Low)Issue: Adding MCP servers requires rebuilding the container image. Observation: This is more restrictive than the previous approach but more secure. Consider if this workflow is acceptable for your use case, or if you need a ConfigMap-based approach for dynamic configuration. Alternative approach (if dynamic config needed): # Load from ConfigMap-mounted file instead
runner_mcp_file = Path("/etc/mcp-config/.mcp.json") # ConfigMap mount point🎯 Security Review✅ Security Improvements
|
This comment has been minimized.
This comment has been minimized.
Claude Code ReviewSummaryThis PR refactors MCP (Model Context Protocol) configuration loading to centralize it in the runner's own directory ( Overall Assessment: Issues by Severity🚫 Blocker Issues1. Function signature mismatch causing runtime error Location: Issue: The function signature was changed to remove the # New signature (line 1815)
def _load_mcp_config(self) -> dict | None:
# But the call site still passes the parameter (line 328)
mcp_servers = self._load_mcp_config(cwd_path) # ❌ BREAKSImpact: This will cause a Fix Required: # Line 328 should be changed to:
mcp_servers = self._load_mcp_config()🟡 Major Issues2. Hardcoded container path reduces flexibility Location: Issue: The MCP config path is hardcoded to runner_mcp_file = Path("/app/claude-runner/.mcp.json")Concerns:
Recommendation: Use a configurable path with a sensible default: # At class level or module level
DEFAULT_MCP_CONFIG_PATH = Path("/app/claude-runner/.mcp.json")
# In method
mcp_config_path = os.getenv('RUNNER_MCP_CONFIG_PATH', str(DEFAULT_MCP_CONFIG_PATH))
runner_mcp_file = Path(mcp_config_path)This maintains security (no user-controlled paths) while improving testability. 3. Missing documentation updates Issue: The PR removes significant functionality (environment variable controls, multi-location search) but doesn't update:
Impact: Operators and developers won't understand:
Recommendation: Add documentation covering: ### MCP Server Configuration (v2.0+)
**Security Change:** MCP servers are now only loaded from the runner's
centrally-controlled configuration file, not from user repositories.
**Location:** `/app/claude-runner/.mcp.json` (inside container)
**Removed Features:**
- ❌ `MCP_CONFIG_PATH` environment variable (security risk)
- ❌ `MCP_CONFIG_SEARCH` toggle (no longer needed)
- ❌ Workspace/repo .mcp.json discovery (security risk)
**Rationale:** Prevents users from loading arbitrary MCP servers that could
bypass security controls or access unauthorized resources.🔵 Minor Issues4. Logging could be more informative Location: Issue: When no MCP config is found, the log just says: logging.info("No .mcp.json file found in runner directory")Suggestion: Add the expected path for easier debugging: logging.info(f"No .mcp.json file found at {runner_mcp_file}")5. Empty .mcp.json file lacks metadata Location: Issue: The file is valid but minimal: {
"mcpServers": {}
}Suggestion: Add a comment header (JSON doesn't support comments, but consider adding a {
"_comment": "MCP server configuration for vTeam runner. Only 'http' and 'sse' types allowed. See docs/mcp-servers.md for examples.",
"mcpServers": {}
}6. Missing test coverage Issue: No tests were added to verify:
Recommendation: Add unit tests: def test_load_mcp_config_from_runner_dir():
# Test successful load from /app/claude-runner/.mcp.json
pass
def test_load_mcp_config_ignores_workspace():
# Test that workspace .mcp.json is NOT loaded
pass
def test_load_mcp_config_handles_missing_file():
# Test graceful handling when file doesn't exist
passPositive Highlights✅ Excellent security improvement - Centralizing MCP config prevents users from potentially injecting malicious or unintended MCP servers via their repositories. ✅ Simpler mental model - Reducing from 3 search locations to 1 makes the system more predictable and easier to reason about. ✅ Good error handling preservation - The try/except blocks for JSON parsing and file errors were retained properly. ✅ Consistent filtering - The ✅ Clear intent in docstring - The updated docstring explicitly states the security reasoning. RecommendationsPriority Order:
Before merging:
Security Impact: ✅ Positive - Reduces attack surface by preventing user-controlled MCP server injection. Breaking Change: Backward Compatibility: ❌ Not maintained for environment variables, but most deployments likely weren't using them. Estimated Review Time: 15 minutes cc @MichaelClifford - Great security-focused refactoring! Just need to fix that one function call and this will be ready to merge. |
Signed-off-by: Michael Clifford <mcliffor@redhat.com>
Claude Code ReviewSummaryThis PR centralizes MCP (Model Context Protocol) server configuration to a single, platform-controlled location within the runner container. The change improves security significantly by removing the ability for user repositories to inject arbitrary MCP server configurations. The refactoring reduces code complexity (-51 lines, -70%) while maintaining backward compatibility and proper error handling. Overall Assessment: ✅ APPROVE - Well-executed security improvement with minor documentation gaps. Issues by Severity🟡 Major Issues1. Missing Test Coverage for Security-Critical Code Location: Issue: No unit tests found for the MCP configuration loading and filtering logic. This is security-critical code that:
Impact:
Recommendation: Add comprehensive test coverage in def test_load_mcp_config_success():
"""Test successful load from runner directory"""
# Mock file exists with valid config
# Verify returns filtered servers
def test_load_mcp_config_missing_file():
"""Test graceful handling when .mcp.json doesn't exist""
# Verify returns None, logs info message
def test_load_mcp_config_invalid_json():
"""Test error handling for malformed JSON""
# Verify returns None, logs error
def test_filter_mcp_servers_allows_http_sse():
"""Test only http and sse server types are allowed""
# Verify stdio, other types are filtered out
def test_filter_mcp_servers_requires_url():
"""Test servers without URL are rejected""
def test_workspace_mcp_ignored():
"""Security: Verify user workspace .mcp.json files are NOT loaded""
# This is the key security boundary testPriority: HIGH - Should be added before or immediately after merge. 2. No Documentation for MCP Configuration Location: Issue: No documentation explaining:
Impact:
Recommendation: Add documentation to ## MCP Server Configuration
### Overview
MCP (Model Context Protocol) servers are configured via a centrally-controlled
configuration file in the runner container. User repositories cannot load MCP
servers for security reasons.
### Configuration File Location
- **Container path:** `/app/claude-runner/.mcp.json`
- **Source path:** `components/runners/claude-code-runner/.mcp.json`
### Security Model
- ✅ MCP servers loaded ONLY from runner's .mcp.json
- ❌ User workspace .mcp.json files are IGNORED
- ✅ Only 'http' and 'sse' server types allowed
- ❌ 'stdio' and other types are filtered out
### Adding MCP Servers
To add MCP servers:
1. Edit `components/runners/claude-code-runner/.mcp.json`
2. Rebuild the container image: `make build-runner`
3. Redeploy with new image
Example configuration:
```json
{
"mcpServers": {
"weather": {
"type": "sse",
"url": "https://weather-mcp.example.com/sse"
}
}
}Breaking Changes (v2.0+)
Rationale: Prevents users from loading arbitrary MCP servers that could Current approach is acceptable (maintains backward compatibility), but could be improved with a docstring note: def _load_mcp_config(self, cwd_path: str) -> dict | None:
"""Load MCP server configuration from the vTeam runner's .mcp.json file.
Args:
cwd_path: Unused. Kept for backward compatibility with calling code.
...
"""Priority: LOW - Current approach is fine. 4. Hardcoded Path Reduces Testability Location: Issue: The path Suggestion: Consider making the path configurable via environment variable (with secure default): DEFAULT_MCP_CONFIG = "/app/claude-runner/.mcp.json"
runner_mcp_file = Path(os.getenv('RUNNER_MCP_CONFIG_PATH', DEFAULT_MCP_CONFIG))Tradeoff:
Current approach is acceptable for production use. Only change if testing becomes painful. Priority: LOW - Nice-to-have. 5. Logging Could Include Full Path Location: Current: logging.info("No .mcp.json file found in runner directory")Suggested: logging.info(f"No .mcp.json file found at {runner_mcp_file}")Benefit: Easier debugging, especially if path becomes configurable in future. Priority: LOW Positive Highlights🎯 Security Excellence
💎 Code Quality
🏗️ Architecture Alignment
RecommendationsBefore Merge
After Merge (Follow-up PRs)
Manual Testing ChecklistBefore deploying to production:
Security Analysis✅ Security Improvements
🔍 Remaining Security Considerations
Breaking Changes
Assessment: Breaking changes are acceptable and intentional for security reasons. Impact is likely minimal as these features were probably rarely used. Python Code Quality Assessment✅ Follows CLAUDE.md Standards
✅ Best Practices
Overall Assessment✅ APPROVE This is a well-executed security improvement that significantly reduces the attack surface of the MCP server loading mechanism. The code quality is high, follows project standards, and maintains backward compatibility. Strengths:
Gaps (non-blocking):
Recommendation: Merge this PR and address test coverage + documentation in follow-up PRs. The core security improvement is valuable and should not be delayed. Review Methodology:
Reviewed by: Claude Code |
MichaelClifford
changed the title
Claude Code ReviewSummaryThis PR centralizes MCP (Model Context Protocol) configuration by moving Overall Assessment: Strong architectural improvement with good security considerations. Several critical issues need addressing before merge, primarily around error handling, environment variable substitution validation, and testing coverage. Issues by SeverityCritical Issues1. Missing Environment Variable Substitution Implementation The 2. No Validation of VTEAM_FRONTEND_URL The operator blindly passes the result of 3. Insecure postMessage targetOrigin Using Major Issues4. No Error Handling for DiscoverFrontendURL 5. Missing Type Safety: NextRequest Param Validation 6. Race Condition in Auto-Close Logic 7. Operator Package Standards Violation: Missing Error Context Minor Issues8. Missing Loading/Error States - New route missing 9. Hardcoded Route Names - Should be configurable via environment variable 10. Inconsistent Error Message Casing - Mix of sentence case and OAuth standard snake_case 11. Missing JSDoc for Public Component - Component has comment but not proper JSDoc format 12. No Telemetry/Metrics - No tracking of OAuth flow success/failure rates Positive Highlights✅ Excellent Security Improvement: Moving MCP config from user-controlled workspace to runner directory prevents arbitrary MCP server injection ✅ Proper Type Safety in React: Using proper TypeScript union types for status ✅ Good Separation of Concerns: OAuth callback endpoint just redirects to completion page ✅ Follows Shadcn UI Patterns: Uses proper Shadcn components ✅ Type-Safe Unstructured Access: Operator code correctly uses ✅ Graceful Degradation: Warning logs instead of failing when frontend URL cant be discovered ✅ Simplified MCP Loading: Refactored RecommendationsBefore Merge (Priority Order):
Testing Requirements:Unit Tests Needed:
Integration Tests Needed:
Code Quality SummaryGo Code: ⭐⭐⭐⭐ (4/5) - Follows operator patterns, minor error handling improvements needed TypeScript/React: ⭐⭐⭐ (3/5) - Good Shadcn usage, needs input validation and security fixes Python: ⭐⭐⭐⭐⭐ (5/5) - Excellent refactoring, clear documentation Overall: ⭐⭐⭐⭐ (4/5) - Good work, address critical issues before merge. Summary: This PR makes meaningful security and architectural improvements. Main concerns are around environment variable substitution validation, error handling visibility, and OAuth security best practices. Great refactoring of the MCP config loading logic! |
|
Tracked in Jira: https://issues.redhat.com/browse/RHOAIENG-39130 |
Signed-off-by: Michael Clifford <mcliffor@redhat.com>
Claude Code ReviewSummaryThis PR implements MCP (Model Context Protocol) OAuth integration for Jira and other MCP servers. The implementation adds OAuth callback handling in the frontend and automatic frontend URL discovery in the operator. The changes follow a secure, centralized configuration approach for MCP servers. Overall Assessment: The implementation is well-designed with good security practices. However, there are several critical security and error handling issues that must be addressed before merging, particularly around XSS vulnerabilities and missing input validation. Issues by Severity🚫 Blocker Issues1. XSS Vulnerability in OAuth Callback Route (components/frontend/src/app/api/oauth/mcp/callback/route.ts)Location: The OAuth callback route directly passes user-controlled query parameters ( // VULNERABLE CODE
const error = request.nextUrl.searchParams.get('error')
const errorDescription = request.nextUrl.searchParams.get('error_description')
completionUrl.searchParams.set('error', error)
completionUrl.searchParams.set('error_description', errorDescription)Fix Required:
2. XSS Vulnerability in Complete Page (components/frontend/src/app/oauth/mcp/complete/page.tsx)Location: User-controlled error parameters are directly rendered in the DOM without sanitization: // VULNERABLE CODE
setErrorMessage(errorDescription || error)
// ...later rendered as:
<p className="text-sm text-red-600 font-medium">{errorMessage}</p>Fix Required:
3. Missing postMessage Origin Validation (components/frontend/src/app/oauth/mcp/complete/page.tsx)Location: The code uses // INSECURE
window.opener.postMessage(messageData, window.location.origin)Fix Required:
🔴 Critical Issues4. Missing Error Handling in DiscoverFrontendURL (components/operator/internal/config/config.go)Location: The function silently ignores errors from Get() calls. If there's a permissions issue or API problem, the operator won't know why MCP OAuth isn't working. // Missing error logging
route, err := DynamicClient.Resource(routeGVR).Namespace(namespace).Get(ctx, name, metav1.GetOptions{})
if err == nil { // Only handles success case
// ...
}Fix Required:
5. No Timeout on Context (components/operator/internal/config/config.go)Location: Using ctx := context.TODO() // No timeoutFix Required: ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()6. Hardcoded Resource Names (components/operator/internal/config/config.go)Location: The function assumes the Route/Ingress is named "frontend" with no way to configure it. This breaks if deployments use different naming conventions. route, err := DynamicClient.Resource(routeGVR).Namespace(namespace).Get(ctx, "frontend", metav1.GetOptions{})Fix Required:
7. Type Safety Issues with Unstructured Access (components/operator/internal/config/config.go)Location: Multiple type assertions without checking the // VIOLATES CLAUDE.md Rule 4
if spec, found, _ := unstructured.NestedMap(route.Object, "spec"); found {
if host, ok := spec["host"].(string); ok && host != "" { // ok check, but ignoring 'found'Fix Required: spec, found, err := unstructured.NestedMap(route.Object, "spec")
if !found || err != nil {
log.Printf("Failed to get spec from Route: err=%v", err)
return ""
}
host, found, err := unstructured.NestedString(spec, "host")
if !found || err != nil || host == "" {
log.Printf("No host found in Route spec")
return ""
}🟡 Major Issues8. Missing React Query Integration (components/frontend/src/app/oauth/mcp/complete/page.tsx)Violation: Frontend Design Guidelines - "React Query for ALL Data Operations" The component uses direct Recommendation: // Better pattern
const useMcpOAuthComplete = () => {
const searchParams = useSearchParams()
const [status, setStatus] = useState<'processing' | 'success' | 'error'>('processing')
useEffect(() => {
// OAuth completion logic
}, [searchParams])
return { status, errorMessage }
}While not strictly data fetching, consider extracting the OAuth completion logic into a custom hook for better testability. 9. No Loading/Error States in Route Handler (components/frontend/src/app/api/oauth/mcp/callback/route.ts)Location: The route handler doesn't log errors or track metrics. If OAuth fails, debugging will be difficult. Recommendation:
10. Empty .mcp.json File (components/runners/claude-code-runner/.mcp.json)Location: The file is empty with no documentation or examples. The PR description mentions "Jira MCP integration" but there's no Jira configuration. Recommendation:
11. No TestsMissing Coverage:
Recommendation:
🔵 Minor Issues12. Inconsistent Error Messages (components/frontend/src/app/oauth/mcp/complete/page.tsx)Location: Error messages have different formats and levels of detail. Standardize for better UX. Recommendation: const ERROR_MESSAGES = {
NO_CODE: 'Authentication failed: No authorization code received',
NO_PARENT: 'Authentication failed: Unable to communicate with session',
POST_FAILED: 'Authentication failed: Communication error',
} as const13. Missing JSDoc/Comments (components/frontend/src/app/api/oauth/mcp/callback/route.ts)The route handler has good high-level comments but missing details on security considerations and expected query parameters. Recommendation:
14. Auto-close Timeout is Magic Number (components/frontend/src/app/oauth/mcp/complete/page.tsx)Location: setTimeout(() => { window.close() }, 2000) // Why 2000?Recommendation: const AUTO_CLOSE_DELAY_MS = 2000 // 2 seconds - enough time for user to see success message
setTimeout(() => { window.close() }, AUTO_CLOSE_DELAY_MS)15. Python Logging Could Be More Structured (components/runners/claude-code-runner/wrapper.py)Location: The logging is good but could include more structured context for debugging. Recommendation: logging.info(f"Loading MCP config", extra={
'config_path': str(runner_mcp_file),
'exists': runner_mcp_file.exists()
})16. Missing Route Pattern Validation (components/frontend/src/app/api/oauth/mcp/callback/route.ts)The PR doesn't show the route configuration. Ensure the callback URL pattern is restricted to expected OAuth providers. Recommendation: Positive Highlights✅ Excellent Security Improvement in MCP Config Loading (wrapper.py:1815-1853)
✅ Good Separation of Concerns
✅ Proper Use of Shadcn Components (page.tsx)
✅ Good Error UX (page.tsx)
✅ Graceful Degradation (config.go:122-123)
✅ Environment Variable Integration (sessions.go:483)
RecommendationsHigh Priority (Before Merge)
Medium Priority
Low Priority
Testing ChecklistBefore merging, verify:
Architecture ConcernsQuestion for Review:
Estimated Fix Time:
Recommendation: Do not merge until all Blocker and Critical issues are resolved and tests are added. |
|
"Yes, we can close 225 - that was an attempt at getting the jira cloud MCP server working, but got blocked by its need for webflow Ouath. So that will need to be adressed first before we can proceed with this one." mc |
5 participants
No description provided.