feat: Runtime credential fetching for GitHub PAT, GitLab, and Jira #568
Conversation
- Introduced endpoints for managing GitHub Personal Access Tokens (PAT) and GitLab credentials at the cluster level. - Implemented functions to save, retrieve, and delete GitHub PATs, enhancing user authentication options. - Updated routing to include new credential fetch endpoints for long-running sessions. - Deprecated project-scoped GitLab authentication endpoints for future removal, ensuring backward compatibility. - Enhanced GitHub status checks to return both GitHub App and PAT status, improving user experience. These changes aim to streamline integration management and provide users with flexible authentication methods.
…time credential fetching - Operator no longer fetches/sets USER_GOOGLE_EMAIL (runner handles this) - Runner sets USER_GOOGLE_EMAIL from backend API response - Supersedes PR ambient-code#562's volume mounting with runtime API fetching - Simpler architecture: no syncing, mounting, or postStart hooks needed
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
- Operator now sets PROJECT_NAME=sessionNamespace for runtime credential API calls - Runner uses AGENTIC_SESSION_NAMESPACE as fallback if PROJECT_NAME not set - Improved error logging in credential fetch to show which env vars are missing
Claude Code ReviewSummaryThis PR implements runtime credential fetching for GitHub PAT, GitLab, and Jira integration tokens, improving upon PR #562's volume mounting approach. The changes introduce cluster-level credential storage with session-scoped runtime fetching, ensuring tokens are always fresh for long-running sessions. Overall Assessment: The PR demonstrates strong architectural improvements with a well-designed precedence system for GitHub tokens. However, there are several critical security and code quality issues that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. User Token Authentication Missing in Runtime Credential Endpoints (
2. Unsafe Type Assertions Without Checking (
3. Token Logging in Integration Validation (
🔴 Critical Issues4. Missing Token Validation Before Storage (
5. Hardcoded Secret Names Without Namespace Constant (
6. Race Condition in Secret Updates (
7. Operator No Longer Sets PROJECT_NAME (
🟡 Major Issues8. No RBAC Check in Integration Status Endpoint (
9. Inconsistent Error Handling in Token Validation (
10. Test Coverage Missing for New Endpoints
11. Frontend Type Safety Concerns (
🔵 Minor Issues12. Inconsistent Logging Levels
13. Magic Numbers in Timeouts
14. Missing Documentation for Token Precedence
15. Server Test Missing Negative Cases (
Positive Highlights✅ Excellent Architecture: Runtime credential fetching is a significant improvement over volume mounting RecommendationsBefore Merge (Required)
Before Merge (Strongly Recommended)
Post-Merge (Technical Debt)
Security Review Checklist
Testing Recommendations
Final VerdictRequest Changes - This PR has strong architectural design but requires fixes for:
Once these issues are addressed, this PR will be ready to merge. The runtime credential fetching architecture is a significant improvement over volume mounting and aligns well with the project's security standards. 🤖 Review performed by Claude Sonnet 4.5 using project memory system:
🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Claude Code Review - PR #568SummaryThis PR implements runtime credential fetching for GitHub PAT, GitLab PAT, and Jira API tokens, superseding the volume mounting approach from PR #562. The architecture improves credential freshness by fetching on-demand from the backend API rather than pre-mounting credentials. Overall Assessment: Strong architectural improvement with good security patterns, but contains several critical issues that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. Type Assertion Violations in Location: Violation: Direct type assertions without using Pattern repeated in 4 functions (lines 48-55, 108-115, 190-197, 252-259). Required Fix: Use type-safe unstructured helpers: import "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
userID, found, err := unstructured.NestedString(obj.Object, "spec", "userContext", "userId")
if \!found || err \!= nil || userID == "" {
c.JSON(http.StatusInternalServerError, gin.H{"error": "User ID not found"})
return
}2. Missing RBAC Validation in Runtime Credential Endpoints Location: Issue: Endpoints validate K8s token but don't verify user has permission to read session credentials. Required: Add RBAC check before returning credentials in all 4 endpoints (GitHub, Google, Jira, GitLab). 3. Incomplete Token Redaction in Logs Location: Issue: Only redacts query string tokens, not Authorization headers (primary transport in this PR). Required: Extend redaction to include Authorization headers in request logs. 4. UserID Ownership Validation Missing Location: Issue: Security Risk: Potential to query other users' integration status. 🔴 Critical Issues5. Token Logging Risk in Validation Functions Location: Issue: Wrapped errors could leak tokens from HTTP request details. Fix: Don't wrap errors from 6. Google Token Refresh Returns Expired Tokens Location: Issue: Returns expired token on refresh failure instead of error. Fix: Return 401 error and require re-authentication. 7. Missing Input Validation Location: All credential endpoints in Issue: Session/project names not validated before K8s API calls. Fix: Validate with 🟡 Major Issues8. Error Messages Expose Internals
9. Missing Unit Tests
🔵 Minor Issues10. Frontend Routes Missing Error Handling Positive Highlights✅ Excellent architectural improvement over volume mounting RecommendationsBefore Merge (Blockers):
High Priority (Critical):
Medium Priority:
Code Quality Metrics
Final VerdictRecommendation: Request Changes Strong architectural improvement, but 4 blocker security issues must be fixed:
The credential fetching architecture is sound but needs security hardening to meet CLAUDE.md standards. 🤖 Reviewed by Claude Code with memory context from:
🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
- oauth.go: Simplify isValidUserID logic (De Morgan's law) - server.go: Use strings.ReplaceAll instead of strings.Replace with -1
Claude Code ReviewSummaryThis PR implements runtime credential fetching for GitHub PAT, GitLab, and Jira, replacing PR #562's volume mounting approach with an on-demand API-based architecture. The implementation is generally well-structured with good security practices, but there are several critical security issues and architectural concerns that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. CRITICAL: Runtime credential endpoints bypass user token authentication (SECURITY VIOLATION) Location: Problem: All four runtime credential endpoints ( Evidence: // Line 28 - uses GetK8sClientsForRequest (user token)
reqK8s, reqDyn := GetK8sClientsForRequest(c)
if reqK8s == nil {
c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
return
}Why this is a BLOCKER:
Required Fix:
Reference: 2. CRITICAL: Token validation calls expose tokens in API requests without verification Location: Problem: The unified status endpoint validates ALL credentials on EVERY status check by calling external APIs (GitHub, GitLab, Jira) with user tokens. This is:
// Line 67 - validates GitHub PAT on every status check
valid, _ := ValidateGitHubToken(ctx, patCreds.Token)Required Fix:
🔴 Critical Issues3. Unsafe type assertions without checking (panic risk) Location: Multiple files
Problem: Direct type assertions without type checking: // Line 64 - unsafe cast
k8sClientset, ok := K8sClient.(*kubernetes.Clientset)
if !ok {
// Returns error, but doesn't handle nil K8sClient
}
// Lines 49-55 - unsafe nested map access
if spec, ok := obj.Object["spec"].(map[string]interface{}); ok {
if userContext, ok := spec["userContext"].(map[string]interface{}); ok {
if uid, ok := userContext["userId"].(string); ok {
userID = uid
}
}
}Why this is CRITICAL: Pattern violation per CLAUDE.md lines 452-456. Should use Required Fix: // Use type-safe helpers
userID, found, err := unstructured.NestedString(obj.Object, "spec", "userContext", "userId")
if !found || err != nil {
return fmt.Errorf("userID not found in session spec")
}Reference: CLAUDE.md lines 452-456, backend-development.md lines 69-85 4. Credentials exposed in response bodies (Information Disclosure) Location:
Problem: Tokens returned as plain JSON with no encryption. If HTTPS is terminated at proxy, tokens travel in cleartext. // Line 78 - token in response body
c.JSON(http.StatusOK, gin.H{"token": token})Required Fix:
5. Missing token redaction in new log statements Location: Problem: While PR adds comprehensive token redaction (good!), the NEW handlers don't consistently redact. For example:
Required Fix:
Reference: CLAUDE.md lines 446-450, security-standards.md lines 22-47 🟡 Major Issues6. No RBAC validation before returning session credentials Location: Problem: Endpoints verify session exists, extract userID, then return credentials without checking if the requesting user has permission to access that session. RBAC gap! // Gets session
obj, err := reqDyn.Resource(gvr).Namespace(project).Get(...)
// Extracts userID
userID := ... // from session spec
// Returns credentials for that userID
// BUT: No check that c.GetString("userID") == extracted userID!Attack scenario: User A could call endpoint for User B's session if they know the session name. Required Fix: // After extracting userID from session
requestingUser := c.GetString("userID")
if requestingUser != userID {
c.JSON(http.StatusForbidden, gin.H{"error": "Cannot access another user's credentials"})
return
}Reference: security-standards.md lines 52-70 7. Inconsistent error handling - some errors return 500, should be 404 Location: Problem: When // Line 74
c.JSON(http.StatusNotFound, gin.H{"error": err.Error()})Issue: Exposes internal errors ("failed to get secret") vs. user-facing errors ("not configured"). Required Fix: Distinguish between:
Reference: error-handling.md lines 9-41 8. Duplicate code across all 4 credential endpoints (DRY violation) Location: Problem: Lines 23-60 are identical in all 4 endpoints (get session, extract userID). ~150 lines of duplicate code. Required Fix: Extract common logic: func getUserIDFromSession(c *gin.Context, project, session string) (string, error) {
reqK8s, reqDyn := GetK8sClientsForRequest(c)
if reqK8s == nil {
return "", fmt.Errorf("unauthorized")
}
// ... common logic
return userID, nil
}Then: func GetGitHubTokenForSession(c *gin.Context) {
userID, err := getUserIDFromSession(c, c.Param("projectName"), c.Param("sessionName"))
if err != nil { /* handle */ }
// ... GitHub-specific logic
}9. GitLab validation allows HTTP URLs (security downgrade) Location: // Line 76
if parsedURL.Scheme != "https" {
return fmt.Errorf("instance URL must use HTTPS")
}Issue: Good validation! But error message says "must use HTTPS" while code checks Required Fix: Explicit check: if parsedURL.Scheme == "" || parsedURL.Scheme == "http" {
return fmt.Errorf("instance URL must use HTTPS (got: %s)", parsedURL.Scheme)
}10. Integration status validation makes 4 concurrent external API calls Location: Problem: Calls Performance impact: 4-6 seconds per status check (4 × 1-1.5s each). Required Fix:
🔵 Minor Issues11. Google token refresh error handling is too lenient Location: if err != nil {
log.Printf("Failed to refresh Google token: %v", err)
// Return existing token (may be expired, but better than nothing)
}Issue: Returning expired token is worse than returning error. Client will make failed API calls. Better: if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{
"error": "Failed to refresh expired Google token",
"action": "Please reconnect your Google account",
})
return
}12. Missing input validation for Jira URL Location: Problem: Jira URL is not validated (no HTTPS check, no format validation). Only checks if empty. Required Fix: Add similar validation as GitLab (HTTPS required, valid hostname). 13. Retry logic uses magic number 3 Location: Multiple files ( for i := 0; i < 3; i++ { // retry on conflictBetter: Extract constant: const maxRetries = 3
for i := 0; i < maxRetries; i++ {14. Inconsistent timestamp formats Location:
Required Fix: Use RFC3339 everywhere for consistency. 15. No tests for new credential endpoints Problem: No test coverage for:
Required: Add comprehensive tests covering:
Positive Highlights✅ Excellent token redaction improvements in ✅ Good security practices: HTTPS validation for GitLab, input sanitization, RBAC checks in global endpoints ✅ Well-documented GitHub token precedence (PAT > App > fallback) in ✅ Proper use of user-scoped clients in global auth endpoints ( ✅ Conflict retry logic for Secret updates (prevents race conditions) ✅ Comprehensive credential validation helpers in ✅ Clean separation between global auth and runtime credential endpoints RecommendationsBefore Merge (Priority Order)
Architecture ImprovementsConsider adding a credential service layer: This would:
Security Checklist
Overall Assessment: This PR makes significant architectural improvements over PR #562, but MUST address the authentication and security issues before merge. The runtime credential fetching design is sound, but the implementation has several critical security gaps that violate established patterns in CLAUDE.md. Estimated fixes: 4-6 hours (authentication model + RBAC + deduplication + tests) 🤖 Generated with Claude Code 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Claude Code ReviewSummaryPR #568 implements runtime credential fetching for GitHub PAT, GitLab, and Jira integrations. Strong architectural improvement with several critical security issues to address. Overall Assessment: Request changes - address critical security issues before merge. Critical Issues (MUST FIX)1. Token Logging Violation (runtime_credentials.go:78)Returns GitHub token in plain JSON - will be logged by Gin middleware. 2. Missing User Authorization CheckRuntime credential endpoints do NOT verify requesting user owns the session. 3. Unsafe Type AssertionsMissing ok checks on type assertions (runtime_credentials.go:49-50). 4. GitLab Token Validation BugLogic error - rejects valid tokens with hyphens/underscores. Major Issues
Minor Issues
Positive Highlights✅ User token authentication enforced RecommendationRequest Changes - Fix 4 critical security issues before merge. Architecture is excellent but security patterns need tightening. Estimated fix time: 2-4 hours for critical issues. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
|
See full review below |
Claude Code ReviewSummaryThis PR implements runtime credential fetching for GitHub PAT, GitLab PAT, and Jira API tokens, replacing the volume-mounting approach from PR #562. The architecture is sound: credentials are fetched just-in-time from the backend API, ensuring tokens are always fresh for long-running sessions. Overall Assessment: The implementation follows most project patterns correctly, but there are security and architectural issues that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. Critical RBAC Violation in Runtime Credentials Endpoints (runtime_credentials.go) Lines 28-33, 94-99, 182-187, 249-254 use user-scoped clients but then extract userID from the session CR. This creates a privilege escalation vulnerability. Problem: A malicious user could:
Required Fix: // After getting userID from session CR (line 49-54)
authenticatedUserID := c.GetString("userID")
if authenticatedUserID \!= userID {
log.Printf("RBAC violation: user %s attempted to access credentials for session owned by %s", authenticatedUserID, userID)
c.JSON(http.StatusForbidden, gin.H{"error": "Access denied: session belongs to different user"})
return
}This check already exists in lines 57-67 but needs to be added to ALL four credential endpoints (GitHub, Google, Jira, GitLab). Reference: See security-standards.md lines 208-220 for RBAC enforcement patterns. 2. Unauthorized Backend Service Account Usage (runtime_credentials.go:78, 224, 291) Lines 78, 224, 291 use backend service account ( Problem: Violates Critical Rule #1 from CLAUDE.md:435-439:
Required Fix: These are user-initiated operations (session runtime credential fetching), so they MUST use user-scoped clients: // Use reqDyn instead of DynamicClient to respect RBAC
obj, err := reqDyn.Resource(gvr).Namespace(project).Get(c.Request.Context(), session, v1.GetOptions{})Reference: See k8s-client-usage.md lines 19-26 for correct pattern. 🔴 Critical Issues3. Missing Error Context in Token Validation (integration_validation.go:29-31, 59-61, 88-90, 114-116) Lines 29, 59, 88, 114 suppress errors that could leak tokens: return false, fmt.Errorf("request failed") // Lost original error contextProblem: While preventing token leakage is correct, losing ALL error context makes debugging impossible. Recommended Fix: if err \!= nil {
log.Printf("GitHub token validation failed (request): %v", err) // Log for debugging
return false, fmt.Errorf("request failed") // Generic user message
}Reference: See error-handling.md lines 196-220 for error handling patterns. 4. Inconsistent RBAC Ownership Verification (runtime_credentials.go) Lines 57-67 include RBAC ownership verification ( Problem: If this logic needs to change, it must be updated in 4 places. Recommended Fix: Extract to shared helper function: func verifySessionOwnership(c *gin.Context, session *unstructured.Unstructured) error {
userID, found, err := unstructured.NestedString(session.Object, "spec", "userContext", "userId")
if \!found || err \!= nil || userID == "" {
return fmt.Errorf("user ID not found in session")
}
authenticatedUserID := c.GetString("userID")
if authenticatedUserID == "" {
return fmt.Errorf("missing authenticated userID")
}
if authenticatedUserID \!= userID {
log.Printf("RBAC violation: user %s attempted to access session owned by %s", authenticatedUserID, userID)
return fmt.Errorf("access denied: session belongs to different user")
}
return nil
}5. Potential Token Leakage in Logs (server/server.go:33-40) Lines 33-40 redact Authorization headers, but only for Bearer tokens: if strings.HasPrefix(auth, "Bearer ") {
authHeader = "Bearer [REDACTED]"
} else {
authHeader = "[REDACTED]" // What if it's "Token ghp_xyz..."?
}Problem: GitHub Personal Access Tokens might be sent as Recommended Fix: if auth := param.Request.Header.Get("Authorization"); auth \!= "" {
authHeader = "[REDACTED]" // Always redact, regardless of format
}Reference: See security-standards.md lines 22-50 for token redaction patterns. 🟡 Major Issues6. Missing Input Validation for Token Format (jira_auth.go, gitlab_auth.go) Jira: No validation of token format before storage (jira_auth.go:47-56) Problem: Invalid tokens get stored and fail later during session execution. Recommended Fix: Add validation helper in // ValidateGitHubTokenFormat checks PAT format (ghp_, gho_, ghs_, ghu_, etc.)
func ValidateGitHubTokenFormat(token string) error {
if \!strings.HasPrefix(token, "ghp_") && \!strings.HasPrefix(token, "gho_") {
return fmt.Errorf("invalid GitHub token format")
}
return nil
}Call validation before storing in 7. Inconsistent Error Messages (runtime_credentials.go, git/operations.go) Lines 81, 142, 227, 293 return different error messages for the same scenario:
Problem: Inconsistent UX, harder to debug. Recommended Fix: Standardize error messages across all credential endpoints: const (
ErrGitHubNotConfigured = "GitHub credentials not configured. Connect GitHub on the Integrations page"
ErrGitLabNotConfigured = "GitLab credentials not configured. Connect GitLab on the Integrations page"
ErrJiraNotConfigured = "Jira credentials not configured. Connect Jira on the Integrations page"
)8. Missing Token Expiry Handling for Non-Refreshable Tokens (runtime_credentials.go) Google OAuth tokens have refresh logic (lines 153-166), but GitHub PAT, GitLab PAT, and Jira API tokens do NOT. Problem: If a PAT expires or is revoked, sessions fail with generic "401 Unauthorized" from GitHub/GitLab API. Recommended Fix: Add validation before returning tokens: // For GitHub PAT (after line 78)
token, err := git.GetGitHubToken(...)
if err \!= nil {
// ... existing error handling
}
// Validate token is still valid
valid, _ := ValidateGitHubToken(c.Request.Context(), token)
if \!valid {
c.JSON(http.StatusUnauthorized, gin.H{"error": "GitHub token expired or revoked. Please reconnect on Integrations page"})
return
}🔵 Minor Issues9. Missing Unit Tests for New Endpoints No tests for:
Recommended: Add table-driven tests in func TestGetGitHubTokenForSession(t *testing.T) {
tests := []struct {
name string
session *unstructured.Unstructured
expectedStatus int
expectedError string
}{
{"valid session", validSession, http.StatusOK, ""},
{"session not found", nil, http.StatusNotFound, "Session not found"},
{"missing userID", sessionNoUserID, http.StatusInternalServerError, "User ID not found"},
{"RBAC violation", sessionDifferentUser, http.StatusForbidden, "Access denied"},
}
// ... test implementation
}10. Hardcoded Secret Names Without Constants (jira_auth.go:159, gitlab_auth.go:497) Lines 159, 497 use hardcoded secret names: const secretName = "jira-credentials"
const secretName = "gitlab-credentials"Problem: If secret naming changes, must update in multiple places. Recommended Fix: Define constants at package level: const (
GitHubPATSecretName = "github-pat-credentials"
GitLabSecretName = "gitlab-credentials"
JiraSecretName = "jira-credentials"
)11. Type Assertion Without Safety Check (runtime_credentials.go:72-76) Lines 72-76 use type assertion without checking: k8sClientset, ok := K8sClient.(*kubernetes.Clientset)
if \!ok {
log.Printf("Failed to convert K8sClient to *kubernetes.Clientset")
c.JSON(http.StatusInternalServerError, gin.H{"error": "Internal error"})
return
}Problem: While the check exists, the error message is too generic. Recommended Fix: if \!ok {
log.Printf("K8sClient type assertion failed: expected *kubernetes.Clientset, got %T", K8sClient)
c.JSON(http.StatusInternalServerError, gin.H{"error": "Internal configuration error"})
return
}12. Frontend: Missing Error Boundaries for New Components New components Recommended: Wrap in Suspense/ErrorBoundary per frontend patterns. Positive Highlights✅ Excellent Security: RBAC ownership verification (runtime_credentials.go:57-67) correctly prevents cross-user credential access ✅ Type-Safe Unstructured Access: Consistent use of ✅ Token Redaction: Comprehensive token redaction in logs (server/server.go:26-50, server_test.go:1-109) ✅ Retry Logic: Proper conflict retry handling for Secret updates (jira_auth.go:161-209, gitlab_auth.go:499-547) ✅ Google Token Refresh: Automatic token refresh before expiry (runtime_credentials.go:153-166) ✅ Operator Simplification: Removed 230 lines of volume mounting logic (operator/sessions.go:-230) ✅ Frontend React Query: Proper use of React Query hooks in new components RecommendationsPriority 1 (Must Fix Before Merge)
Priority 2 (Should Fix Before Merge)
Priority 3 (Can Address in Follow-Up)
Architecture Improvements
Testing ChecklistBefore merge, verify:
Overall: Strong architectural improvement over PR #562, but security issues must be fixed before merge. The runtime fetching approach is the right solution - just needs RBAC hardening. Generated with Claude Code Review 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Blocker Issues Fixed: 1. Type assertions: Use unstructured.NestedString() instead of unsafe type assertions 2. RBAC validation: Verify authenticated user owns session before returning credentials 3. Token redaction: Extend log redaction to include Authorization headers 4. Token logging: Remove error wrapping that could leak tokens in validation functions 5. Expired tokens: Return 401 error on refresh failure instead of expired token All 4 runtime credential endpoints now: - Use type-safe unstructured helpers (CLAUDE.md compliance) - Verify session ownership (prevent cross-user credential access) - Return 401 on token refresh failures (force re-authentication) - Don't leak tokens in error messages or logs
Claude Code ReviewSummaryThis PR implements runtime credential fetching for GitHub PAT, GitLab, Jira, and Google OAuth credentials. The architectural approach is sound—moving from volume mounting to on-demand API fetching ensures fresh credentials for long-running sessions. However, there are several critical security issues and code quality concerns that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. Token Logging in Server Redaction (components/backend/server/server.go:27-50)
2. Missing RBAC Validation in Runtime Credential Endpoints (components/backend/handlers/runtime_credentials.go)
ssar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Group: "vteam.ambient-code",
Resource: "agenticsessions",
Verb: "get",
Namespace: project,
},
},
}
res, err := reqK8s.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, ssar, v1.CreateOptions{})
if err != nil || !res.Status.Allowed {
c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized"})
return
}🔴 Critical Issues3. Service Account Used for Credential Writes Without User Permission Check (handlers/jira_auth.go, gitlab_auth.go)
// After getting reqK8s, BEFORE storing
ssar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Resource: "secrets",
Verb: "update",
Namespace: Namespace,
Name: "jira-credentials", // or gitlab-credentials
},
},
}
res, err := reqK8s.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, ssar, v1.CreateOptions{})
if err != nil || !res.Status.Allowed {
c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to manage credentials"})
return
}4. Type Assertion Without Check (runtime_credentials.go:71-76)
5. Validation Functions Return Bool + Error But Ignore Error in Status Checks (integrations_status.go:67, 115, 133)
valid, err := ValidateGitHubToken(ctx, patCreds.Token)
if err != nil {
status["pat"] = gin.H{
"configured": true,
"valid": false,
"validationError": "Unable to validate (network error)",
}
} else {
status["pat"] = gin.H{
"configured": true,
"valid": valid,
}
}6. Hardcoded Timeouts Without Context (integration_validation.go)
client := &http.Client{} // No timeout
ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
defer cancel()
// Use ctx in NewRequestWithContext🟡 Major Issues7. Excessive Code Duplication in Runtime Credential Handlers
func validateSessionAccess(c *gin.Context, project, session string) (userID string, err error) {
reqK8s, reqDyn := GetK8sClientsForRequest(c)
if reqK8s == nil {
return "", fmt.Errorf("unauthorized")
}
// Get session, extract userID, verify ownership
// Return userID or error
}
func GetGitHubTokenForSession(c *gin.Context) {
project := c.Param("projectName")
session := c.Param("sessionName")
userID, err := validateSessionAccess(c, project, session)
if err != nil {
c.JSON(http.StatusUnauthorized, gin.H{"error": err.Error()})
return
}
// Now just fetch GitHub token for userID
}8. Missing Input Sanitization in Jira/GitLab Connection
parsedURL, err := url.Parse(req.URL)
if err != nil || parsedURL.Scheme == "" || parsedURL.Host == "" {
c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid URL format"})
return
}
// Store normalized URL
creds.URL = parsedURL.String()9. Secret Update Race Conditions Not Handled Robustly
for i := 0; i < 5; i++ { // Increase retries
secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, secretName, v1.GetOptions{})
// ... update secret ...
if _, uerr := K8sClient.CoreV1().Secrets(Namespace).Update(ctx, secret, v1.UpdateOptions{}); uerr != nil {
if errors.IsConflict(uerr) {
time.Sleep(time.Duration(i*100) * time.Millisecond) // Exponential backoff
continue
}
return uerr
}
return nil
}10. Inconsistent Error Messages Expose Internal State
🔵 Minor Issues11. Unused Variable in GitLab/Jira Test Functions
12. Hardcoded Secret Names Without Constants
const (
JiraCredentialsSecret = "jira-credentials"
GitLabCredentialsSecret = "gitlab-credentials"
GitHubPATSecret = "github-pat-credentials"
)13. Missing godoc Comments on Public Functions
14. Frontend: Potential TypeScript
15. Adapter.py: Comment Says Credentials Fetched On-Demand But Doesn't Show Implementation
Positive Highlights✅ Excellent architectural improvement: Runtime fetching vs. volume mounting is much cleaner and ensures fresh credentials ✅ Good RBAC ownership checks: Verifying ✅ Type-safe unstructured access: Correctly using ✅ Comprehensive token redaction: server.go custom formatter redacts tokens in URLs and auth headers ✅ Proper retry logic for Secret conflicts: Using retry loops for concurrent Secret updates (though needs backoff) ✅ User-scoped secrets: Credentials keyed by sanitized userID for multi-tenant isolation ✅ Validation endpoints: Test connections before saving credentials is great UX RecommendationsPriority 1 (Must Fix Before Merge)
Priority 2 (Should Fix Before Merge)
Priority 3 (Can Fix Post-Merge)
Testing RecommendationsSecurity Testing:
Integration Testing:
Error Handling:
Overall AssessmentArchitecture: ✅ Excellent - runtime fetching is the right approach Security: ❌ Blocker issues must be fixed - RBAC gaps and potential token logging Code Quality: Recommendation: Request Changes - Fix blockers and critical issues, then re-review. 🤖 Generated with Claude Code 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
- Check parsedURL.User to catch user@host syntax - Test expects 400 BadRequest for invalid URL format - Prevents injection via user info in URL
Claude Code ReviewSummaryPR #568 implements runtime credential fetching for GitHub PAT, GitLab, and Jira tokens. The implementation demonstrates excellent adherence to established patterns with strong security compliance. The architectural shift from volume-mounted credentials to on-demand API fetching is a significant improvement. Recommendation: APPROVE ✅ (with 3 minor non-blocking suggestions) Issues by Severity🚫 Blocker IssuesNone - All critical security patterns are compliant. 🔴 Critical IssuesNone - RBAC, authentication, and token handling all follow CLAUDE.md standards. 🟡 Major IssuesNone - Type-safe unstructured access, error handling, and logging patterns are correct. 🔵 Minor Issues (Non-Blocking)1. Missing URL Validation in Jira HandlerFile: Issue: Basic validation only checks empty string, missing format validation unlike GitLab handler. if req.URL == "" {
c.JSON(http.StatusBadRequest, gin.H{"error": "Jira URL is required"})
return
}
// No further URL validation\!Recommendation: Add validation similar to parsedURL, err := url.Parse(req.URL)
if err \!= nil {
c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid URL format"})
return
}
if parsedURL.Scheme \!= "https" {
c.JSON(http.StatusBadRequest, gin.H{"error": "URL must use HTTPS"})
return
}
if parsedURL.User \!= nil {
c.JSON(http.StatusBadRequest, gin.H{"error": "URL must not contain userinfo"})
return
}Impact: Low (malformed URLs will fail at API call time) 2. Token Format Validation CompletenessFile: Issue: Missing if \!strings.HasPrefix(req.Token, "ghp_") && \!strings.HasPrefix(req.Token, "gho_") &&
\!strings.HasPrefix(req.Token, "ghu_") && \!strings.HasPrefix(req.Token, "ghs_") &&
\!strings.HasPrefix(req.Token, "github_pat_") {
// Missing: ghc_ for GitHub App tokensImpact: Very Low (GitHub App tokens are minted by backend, not user-provided) 3. Error Message ConsistencyFile: Current approach: // Don't wrap error - could leak token from request details
return false, fmt.Errorf("request failed")Note: This is good security practice (prevents token leakage), but generic messages may hinder debugging. Suggestion: Add internal structured logging while keeping user-facing errors generic: log.Printf("GitHub token validation failed: %v", err) // Internal debug
return false, fmt.Errorf("request failed") // Generic user messageImpact: Very Low (debugging convenience only) Positive Highlights🔒 Excellent Security Compliance1. User Token Authentication (All 4 Credential Endpoints)✅ reqK8s, reqDyn := GetK8sClientsForRequest(c)
if reqK8s == nil {
c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
return
}2. RBAC Enforcement - Session Ownership Verification✅ Excellent pattern prevents credential leakage between users ( authenticatedUserID := c.GetString("userID")
if authenticatedUserID \!= userID {
log.Printf("RBAC violation: user %s attempted to access credentials for session owned by %s",
authenticatedUserID, userID)
c.JSON(http.StatusForbidden, gin.H{"error": "Access denied"})
return
}3. Type-Safe Unstructured Access✅ All handlers use userID, found, err := unstructured.NestedString(obj.Object, "spec", "userContext", "userId")
if \!found || err \!= nil || userID == "" {
log.Printf("Failed to extract userID: found=%v, err=%v", found, err)
c.JSON(http.StatusInternalServerError, gin.H{"error": "User ID not found"})
return
}4. Token Redaction✅ No tokens logged anywhere, enhanced redaction in 🏗️ Architectural Improvements✅ Operator simplification: Removed ~230 lines of volume mounting logic 📝 Code Quality✅ Pattern consistency: All handlers follow established error handling patterns RecommendationsPriority 1: Before MergeNone - All critical patterns are compliant. Priority 2: Nice to Have (Post-Merge)
Pre-Commit Checklist Status
Files ReviewedBackend (Go)
Operator (Go)
Runner (Python)
SummaryThis PR is production-ready with excellent security compliance and code quality. The three minor issues identified are low-impact and do not block merge. The architectural shift from volume-mounted credentials to runtime API fetching is a significant security and reliability improvement. 🎉 Great work adhering to CLAUDE.md standards! 🤖 Generated by Claude Code Review 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Tests now expect 401 Unauthorized when auth is missing instead of 400 BadRequest. This aligns with security improvements where authentication is checked first. Updated tests: - ConnectGitLabGlobal: Now tests auth requirement (not project name) - GetGitLabStatusGlobal: Now tests auth requirement - DisconnectGitLabGlobal: Now tests auth requirement - Invalid user ID type: Expects 401 (GetString returns empty for non-string) - JSON validation: Don't check exact Gin validation message (varies)
Claude Code ReviewSummaryThis PR implements runtime credential fetching for GitHub PAT, GitLab, and Jira tokens, replacing the volume mounting approach from PR #562. The implementation follows CLAUDE.md standards and introduces critical security fixes that were identified during review. Overall Assessment: ✅ APPROVE with recommendations The code quality is excellent, with comprehensive security improvements addressing multiple RBAC and token handling issues. All backend patterns align with project standards. Issues by Severity🚫 Blocker IssuesNone - All critical security issues were fixed in commit e10bdcd. 🔴 Critical Issues (FIXED in this PR)The following critical issues were identified and resolved during development: 1. ✅ Type-Safe Unstructured Access (Fixed)Location: Issue: Original code used unsafe type assertions that could panic: // Before (unsafe)
if spec, ok := obj.Object["spec"].(map[string]interface{}); ok { ... }Resolution: Now uses type-safe // After (safe)
userID, found, err := unstructured.NestedString(obj.Object, "spec", "userContext", "userId")
if \!found || err \!= nil { /* handle error */ }Applied consistently to all 4 credential endpoints (lines 48, 115, 203, 270). 2. ✅ RBAC Session Ownership Validation (Fixed)Location: Issue: Original implementation lacked verification that the authenticated user owns the session being accessed. Any authenticated user could potentially access any session's credentials. Resolution: Added 3-layer RBAC validation to all 4 endpoints: // 1. Validate user token
reqK8s, reqDyn := GetK8sClientsForRequest(c)
if reqK8s == nil { return 401 }
// 2. Fetch session with user-scoped client (K8s RBAC)
obj, err := reqDyn.Resource(gvr).Namespace(project).Get(...)
// 3. Verify session ownership
authenticatedUserID := c.GetString("userID") // From middleware
if authenticatedUserID \!= sessionOwnerID {
log.Printf("RBAC violation: user %s attempted to access credentials for session owned by %s", authenticatedUserID, sessionOwnerID)
return 403 Forbidden
}Security Chain:
Applied to: GitHub (L56-67), Google (L122-133), Jira (L210-221), GitLab (L277-288) 3. ✅ Token Redaction in Logs (Fixed)Location: Issue: Error wrapping with Resolution: Removed error wrapping in all 4 validation functions: // Before (risky)
return false, fmt.Errorf("request failed: %w", err) // ← Could leak Authorization header
// After (safe)
return false, fmt.Errorf("request failed") // ← No token exposureApplied to: GitHub (L19), GitLab (L57), Jira (L86), Google (L112) 4. ✅ Authorization Header Redaction (Fixed)Location: Issue: HTTP access logs exposed Authorization headers. Resolution: Enhanced log formatter to redact Bearer tokens: authHeader := "[none]"
if auth := param.Request.Header.Get("Authorization"); auth \!= "" {
if strings.HasPrefix(auth, "Bearer ") {
authHeader = "Bearer [REDACTED]"
} else {
authHeader = "[REDACTED]"
}
}Log Output Example:
5. ✅ Expired Token Handling (Fixed)Location: Issue: Original code returned expired tokens as fallback when refresh failed. Resolution: Now returns 401 Unauthorized, forcing re-authentication: if err \!= nil {
log.Printf("Failed to refresh Google token for user %s: %v", userID, err)
c.JSON(http.StatusUnauthorized, gin.H{"error": "Google token expired and refresh failed. Please re-authenticate."})
return // ← Force re-auth instead of serving expired token
}🟡 Major Issues1. Missing Test Coverage for RBAC ValidationImpact: No automated tests verify that users cannot access other users' session credentials. Recommendation: Add integration tests for all 4 endpoints: // Test: User A attempts to access User B's session credentials
func TestGetGitHubTokenRBACViolation(t *testing.T) {
// Setup: Create session owned by user-b
session := createSession("user-b", "myproject")
// Act: User-a attempts to fetch credentials
resp := makeRequest("GET", "/credentials/github", tokenForUser("user-a"))
// Assert: Should return 403 Forbidden, not 200 OK
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
assert.Contains(t, resp.Body, "Access denied")
}Suggested test file: Priority: Medium - Current manual testing likely catches this, but automated coverage is important. 2. Inconsistent Error Message DetailLocation: Issue: GitHub token endpoint returns token, err := git.GetGitHubToken(...)
if err \!= nil {
c.JSON(http.StatusNotFound, gin.H{"error": err.Error()}) // ← Potential detail leakage
return
}Comparison: Other endpoints use generic messages:
Recommendation: Change to: if err \!= nil {
log.Printf("Failed to get GitHub token for user %s: %v", userID, err)
c.JSON(http.StatusNotFound, gin.H{"error": "GitHub credentials not found"})
return
}Priority: Low-Medium - Unlikely to leak sensitive data, but improves consistency. 🔵 Minor Issues1. Operator Volume Mounting CleanupLocation: Observation: PR removes 230 lines of volume mounting logic. Excellent simplification! Note: Comment at L409 references "runtime fetching", but no code enforces that credentials are fetched before use. Recommendation: Consider adding a startup check in the runner to fail fast if credential fetching fails: # adapter.py:_populate_runtime_credentials()
if not self._fetch_github_credentials():
raise RuntimeError("Failed to fetch GitHub credentials at startup")Priority: Low - Current implementation logs warnings, which is acceptable. 2. GitHub Token Precedence LogicLocation: Observation: New precedence order is well-documented:
Note: Lines 117-130 handle GitHub App token minting with service account K8sClient, which is correct per CLAUDE.md standards (backend SA for privileged operations). Recommendation: None - Implementation is correct and well-structured. Positive Highlights✅ Architecture Improvements
✅ Security Excellence
✅ Code Quality
✅ Frontend Integration
RecommendationsImmediate (Before Merge)
Follow-Up PRs
CLAUDE.md Compliance Review
Final VerdictStatus: ✅ APPROVE This PR represents high-quality, security-focused engineering. All critical backend patterns align with CLAUDE.md standards, and the architectural simplification (removing volume mounting) significantly improves maintainability. The comprehensive security fixes (RBAC, token redaction, type safety) demonstrate attention to detail and proper understanding of the platform's security model. The frontend changes follow established patterns with proper React Query usage and type safety. Recommendation: Merge after CI passes. Follow up with test coverage PR to validate RBAC behavior. Commit reviewed: e10bdcd 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
1 participant
Summary
Implements runtime credential fetching architecture for GitHub PAT, GitLab PAT, and Jira API tokens. This improves upon PR #562's volume mounting approach by fetching credentials just-in-time from the backend API, ensuring tokens are always fresh for long-running sessions.
Key Features
1. Cluster-Level Credential Storage
github-pat-credentialsSecret (user-scoped)gitlab-credentialsSecret (user-scoped)jira-credentialsSecret (user-scoped)2. Runtime Credential Fetching
3. GitHub Token Precedence (Priority-Based)
4. Backend API Endpoints
GET /api/projects/:project/agentic-sessions/:session/credentials/githubGET /api/projects/:project/agentic-sessions/:session/credentials/googleGET /api/projects/:project/agentic-sessions/:session/credentials/jiraGET /api/projects/:project/agentic-sessions/:session/credentials/gitlabAll endpoints require BOT_TOKEN authentication (session-scoped).
Architectural Improvements Over PR #562
PR #562 fixed Google MCP authentication using volume mounting. This PR supersedes that approach:
Components Changed
Backend
handlers/github_auth.go- GitHub PAT and App installation storage/retrievalhandlers/gitlab_auth.go- GitLab PAT storage/retrieval (cluster-level)handlers/jira_auth.go- Jira API token storage/retrievalhandlers/runtime_credentials.go- NEW - Session runtime credential endpointshandlers/integrations_status.go- NEW - Unified integration status endpointhandlers/integration_validation.go- NEW - Credential validation helpersgit/operations.go- Updated GitHub token precedence logicroutes.go- Added credential management routesserver/server.go- Enhanced token redaction in logsFrontend
components/github-connection-card.tsx- GitHub PAT configuration UIcomponents/gitlab-connection-card.tsx- NEW - GitLab connection UIcomponents/jira-connection-card.tsx- NEW - Jira connection UIapp/api/auth/github/pat/*- NEW - GitHub PAT API routesapp/api/auth/gitlab/*- NEW - GitLab API routesapp/api/auth/jira/*- NEW - Jira API routesapp/api/projects/[name]/agentic-sessions/[sessionName]/credentials/*- NEW - Runtime credential endpointsOperator
internal/handlers/sessions.go- Removed volume mounting logic, added runtime fetching commentsRunner
adapter.py-_populate_runtime_credentials()fetches all credentials at session startSecurity
Testing
Migration Notes
For users currently using project-level integration secrets:
ambient-non-vertex-integrationsSecret still supported as fallbackFor PR #562 changes:
🤖 Generated with Claude Code