Feature/gitlab support v2

[aegeiger]

This is a re-vamp of #251 after rebase.
Most credit is due to nafridma@redhat.com .

[github-actions]

Claude Code Review

Summary

This PR adds comprehensive GitLab support (SaaS and self-hosted) to the Ambient Code Platform. Overall Assessment: Strong implementation with excellent documentation, but critical security issues must be addressed before merge.

Key Strengths:

• Well-structured provider abstraction
• Comprehensive error messages with remediation guidance
• Extensive documentation (5 new docs, 3000+ lines)
• Integration test coverage

---

Issues by Severity

🚫 Blocker Issues

1. Secret Storage Uses Wrong Namespace (Security Risk)

components/backend/git/operations.go:113-144

GitLab tokens stored in backend namespace instead of project namespaces, breaking multi-tenant isolation. All projects share the same secret, violating RBAC boundaries.

Fix Required: Refactor to use project-scoped secrets like GitHub tokens (see GetGitHubToken lines 62-111).

---

2. Service Account Client Used for User Operations

components/backend/handlers/gitlab_auth.go:195-209

Global wrapper functions use backend service account client instead of user-scoped clients.

Violates CLAUDE.md Rule: "FORBIDDEN: Using backend service account for user-initiated API operations"

Fix Required: Use GetK8sClientsForRequest(c) to get user-scoped clients and validate RBAC permissions.

---

3. Missing RBAC Validation in Auth Handlers

components/backend/handlers/gitlab_auth.go:48-103

Handler checks authentication but bypasses authorization by using service account client.

Fix Required: Validate user has permission to create secrets in their namespace before storing tokens.

---

🔴 Critical Issues

4. Race Condition in Secret Updates

components/backend/k8s/secrets.go:47-63

Get-Modify-Update pattern is inherently racy between concurrent requests.

Recommendation: Use optimistic concurrency control with ResourceVersion checks or Patch operation.

---

5. Unvalidated User Input

components/backend/handlers/gitlab_auth.go:50-57

• instanceUrl not validated (could be malicious URL)
• No length limits on personalAccessToken
• Redundant statusCode in response body

Recommendation: Validate instanceUrl format (must be HTTPS), limit token length, remove redundant status codes.

---

🟡 Major Issues

6. Incomplete Error Handling in Repo Seeding

components/backend/git/operations.go:385-401

Validation failures don't indicate if previous repos were modified, leaving workspace in inconsistent state.

Recommendation: Validate ALL repos before modifying ANY, or add rollback guidance.

---

7. Inconsistent Error Response Format

Some responses include statusCode in body, others don't. Remove for consistency (already in HTTP header).

---

8. Magic Numbers in Pagination

components/backend/gitlab/client.go:263

Hard-coded 100-page limit (10K items max) might be insufficient for large repos.

Recommendation: Make configurable or increase to 500 pages.

---

🔵 Minor Issues

9. Missing Logging Context

GitLab API requests don't log user ID, project context, or token length for debugging.

10. Potential URL Encoding Issues

Owner/repo names with special characters need additional validation.

11. Frontend Missing Error States

No client-side validation, loading states, or network error handling in settings UI.

12. Documentation Namespace References

Docs reference "vteam" namespace - update to "ambient-code" or clarify backward compatibility.

13. Test Coverage Gaps

Ensure tests cover: invalid/expired tokens, 403/404 responses, rate limiting, self-hosted instances, concurrent updates.

---

Positive Highlights

✅ Excellent Provider Abstraction - Clean separation via types.ProviderType
✅ Outstanding Documentation - 5 comprehensive docs with setup guides
✅ User-Friendly Error Messages - GitLabAPIError with remediation guidance
✅ Proper Token Redaction - URL redaction in logs, request ID tracking
✅ Integration Tests - Dedicated test suite with README
✅ No panic() calls - Proper error handling throughout

---

Recommendations

Before Merge (REQUIRED)

1. Fix Blocker Security Issues:

   • Move GitLab tokens to project-scoped secrets (Issue Outcome: Reduce Refinement Time with agent System #1)
   • Replace SA client with user-scoped clients (Issue Epic: RAT Architecture & Design #2)
   • Add RBAC validation to auth handlers (Issue Epic: Data Source Integration #3)

2. Address Critical Issues:

   • Fix race condition in secret updates (Issue Epic: AI Agent Development #4)
   • Add input validation for instanceUrl (Issue Epic: Jira Integration & Workflow #5)

3. Testing:

   • Verify RBAC isolation between projects
   • Test concurrent token updates
   • Validate all error scenarios

Future Improvements

• Add metrics/observability for GitLab API
• Implement token refresh for expired PATs
• Add GitLab OAuth support
• Cache validated tokens to reduce API calls

---

Compliance with CLAUDE.md

Rule	Status	Notes
User Token Auth Required	❌ FAIL	Issues #2, #3
Never Panic	✅ PASS	No panic() found
Token Redaction	✅ PASS	Proper redaction
RBAC Enforcement	❌ FAIL	Bypasses user RBAC

---

Verdict: Request changes. The implementation is well-designed, but critical security issues create risks in multi-tenant environments. Fixing Issues #1-3 is required before merge.

[Gkrumbach07]

Tracked in Jira: https://issues.redhat.com/browse/RHOAIENG-39120

[github-actions]

Claude Code Review

Summary

This PR successfully implements comprehensive GitLab support for the Ambient Code Platform, enabling integration with both GitLab.com (SaaS) and self-hosted GitLab instances. The implementation follows established patterns from the existing GitHub integration and demonstrates solid engineering practices.

Overall Assessment: The code quality is high with strong attention to security, error handling, and multi-tenancy. However, there are several critical security issues and architectural concerns that must be addressed before merging.

---

Issues by Severity

🚫 Blocker Issues

1. Secret Update Race Condition (k8s/secrets.go:48-60)

• Issue: Setting secret.Data = nil clears ALL existing user tokens in the secret when updating a single user's token
• Impact: When user A updates their GitLab token, it deletes tokens for users B, C, D, etc. in the same namespace
• Fix: Read existing secret.Data, convert to StringData, merge the new token, then update
• Location: components/backend/k8s/secrets.go:48-60

2. User Token Authentication Not Enforced in Global Handlers (handlers/gitlab_auth.go:281-344)

• Issue: Global wrapper functions create handlers with user-scoped K8s client but don't verify authentication succeeded
• CLAUDE.md Violation: Must return 401 Unauthorized if user token is missing or invalid
• Impact: Could potentially execute operations with nil client, causing runtime panics
• Location: components/backend/handlers/gitlab_auth.go:281-344

3. Missing Error Response for Failed ConfigMap Cleanup (gitlab/connection.go:104-107)

• Issue: Partial failures during connection deletion are not returned to the user
• Impact: User believes connection is fully deleted when token Secret still exists
• Location: components/backend/gitlab/connection.go:104-107

---

🔴 Critical Issues

4. Token Logging Risk - Error messages from URL parsing/injection could contain the injected token (git/operations.go:697)

5. Unbounded Pagination Loop - Hard limit of 100 pages could fail for large repositories (gitlab/client.go:262-266)

6. SSH URL Detection Incomplete - Missing support for ssh:// protocol URLs (types/provider.go:27-31)

7. Frontend Token Storage in Local State - Tokens should be write-only in the UI (settings-section.tsx:38-77)

---

🟡 Major Issues

8. Inconsistent Error Handling - Some HTTP response reads don't check io.ReadAll errors before unmarshaling

9. Provider Detection Too Broad - strings.Contains(hostname, "gitlab") matches non-GitLab hosts (types/provider.go:59)

---

Positive Highlights

✅ Excellent Provider Abstraction - Clean separation enabling future Git providers

✅ Strong Error Handling - GitLabAPIError type provides structured errors with remediation guidance

✅ Comprehensive Logging - UUID-based request tracking with redacted URLs

✅ Multi-Tenancy Security - Tokens stored per-project namespace with RBAC checks

✅ Backward Compatibility - No breaking changes to CRDs or existing GitHub integration

✅ Self-Hosted GitLab Support - Correct API paths for custom domains and ports

✅ Extensive Documentation - 5 detailed markdown docs with API examples

✅ Comprehensive Integration Tests - End-to-end test covering full user journey

---

Final Verdict

Recommendation: Approve with Required Changes

This PR represents excellent engineering work with comprehensive GitLab support. However, the Blocker Issues (especially the secret update race condition) must be fixed before merging to prevent data loss in multi-user environments.

Estimated Fix Time: ~4-6 hours to address blockers

Great work on this feature! 🎉 The GitLab integration is well-architected and will be a valuable addition to the platform.

[github-actions]

Claude Code Review

Summary

This PR adds comprehensive GitLab support to the platform, enabling users to work with both GitHub and GitLab repositories (SaaS and self-hosted). The implementation follows established CLAUDE.md patterns with strong authentication, proper RBAC enforcement, and multi-tenant isolation. The code quality is high with excellent error handling, security practices, and extensive documentation.

Overall Assessment: ✅ Approved with minor recommendations

This is well-architected feature work that properly extends the existing platform capabilities while maintaining security and code quality standards.

---

Issues by Severity

🔴 Critical Issues

1. Token Validation Length Check May Be Too Restrictive

• Location: components/backend/handlers/gitlab_auth.go:78-82
• Issue: Minimum token length validation set to exactly 20 characters

if len(token) < 20 {
    return fmt.Errorf("token must be at least 20 characters")
}

• Problem: GitLab PATs can vary in length. While current GitLab PATs are 20 characters (glpat-*), older tokens or future formats may differ. This could block valid tokens.
• Impact: Users with valid tokens from older GitLab versions or different formats may be unable to connect
• Recommendation: Lower minimum to 16 characters or make this configurable. Add token format validation instead (check for glpat- prefix for modern tokens).

---

🟡 Major Issues

1. Missing Rate Limiting Protection

• Location: components/backend/handlers/gitlab_auth.go:100-192, components/backend/gitlab/client.go:42-83
• Issue: No rate limiting on GitLab API calls or auth endpoints
• Impact: Could exhaust GitLab API quota (300 req/min for GitLab.com) during high usage
• Recommendation: Implement rate limiting at handler level and add retry logic with exponential backoff in the GitLab client

2. Secret Update Race Condition Mitigation Could Be Improved

• Location: components/backend/k8s/secrets.go:21-88
• Issue: Optimistic concurrency control with only 3 retries and linear backoff

time.Sleep(time.Millisecond * 100 * time.Duration(attempt+1))

• Problem: Under high concurrency (multiple users connecting GitLab simultaneously), this could still fail
• Impact: Connection failures during concurrent operations
• Recommendation:
  • Increase retries to 5
  • Use exponential backoff: time.Sleep(time.Millisecond * 100 * (1 << attempt))
  • Add jitter to prevent thundering herd

3. Git Operations Missing Timeout Protection

• Location: components/backend/git/operations.go:376-747 (PerformRepoSeeding)
• Issue: Git clone/push operations use context but no explicit timeout
• Impact: Hung git operations could block indefinitely
• Recommendation: Add timeout to git operations:

ctx, cancel := context.WithTimeout(ctx, 5*time.Minute)
defer cancel()

4. Error Messages May Leak Repository Structure

• Location: components/backend/git/operations.go:283-289
• Issue: Error messages include full repository paths which could expose internal structure

return false, fmt.Errorf("supporting repo #%d (%s) access validation failed: %w", i+1, repo.GetURL(), err)

• Impact: Information disclosure in error logs
• Recommendation: Sanitize URLs in error messages using existing sanitizeURLForError() function

5. Missing Input Validation for Custom GitLab Domains

• Location: components/backend/handlers/gitlab_auth.go:51-97
• Issue: Instance URL validation checks for HTTPS and hostname but doesn't validate against SSRF attacks
• Impact: Potential SSRF if malicious user provides internal URLs like https://169.254.169.254/
• Recommendation: Add blocklist for RFC 1918 addresses, localhost, metadata endpoints:

if isPrivateIP(parsedURL.Host) || isMetadataEndpoint(parsedURL.Host) {
    return fmt.Errorf("instance URL cannot point to private networks or metadata endpoints")
}

---

🔵 Minor Issues

1. Inconsistent Logging Patterns

• Location: Multiple files use different logging approaches
• Examples:
  • components/backend/gitlab/client.go: Uses custom LogInfo, LogError
  • components/backend/handlers/gitlab_auth.go: Uses gitlab.LogError
  • components/backend/git/operations.go: Uses standard log.Printf
• Recommendation: Standardize on one approach (prefer structured logging with log levels)

2. Magic Numbers in Pagination

• Location: components/backend/gitlab/client.go:219, 302

if perPage == 0 {
    perPage = 100 // Max page size for GitLab API
}

• Recommendation: Define as constants:

const (
    GitLabMaxPageSize = 100
    GitLabDefaultPageSize = 100
)

3. Duplicate Code in Global Handlers

• Location: components/backend/handlers/gitlab_auth.go:338-404
• Issue: Three nearly identical global wrapper functions with repeated project validation
• Recommendation: Extract common logic:

func withProjectAuth(c *gin.Context, handler func(*gin.Context, *kubernetes.Clientset, string)) {
    project := c.Param("projectName")
    if project == "" {
        c.JSON(http.StatusBadRequest, gin.H{"error": "Project name is required"})
        return
    }
    reqK8s, _ := GetK8sClientsForRequest(c)
    if reqK8s == nil {
        c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
        return
    }
    handler(c, reqK8s, project)
}

4. Hard-Coded User Agent

• Location: components/backend/gitlab/client.go:62
• Issue: No User-Agent header set for GitLab API requests
• Impact: Cannot track platform usage in GitLab API logs
• Recommendation: Add User-Agent header:

req.Header.Set("User-Agent", "AmbientCode-Platform/1.0")

5. URL Encoding Not Consistently Applied

• Location: components/backend/gitlab/client.go:306
• Issue: Query parameters constructed without proper escaping

apiPath := fmt.Sprintf("/projects/%s/repository/tree?ref=%s&page=%d&per_page=%d", projectID, ref, page, perPage)

• Problem: Branch names with special characters (e.g., feature/test) won't work
• Recommendation: Use url.QueryEscape for all query parameters

6. Missing Documentation for Public Functions

• Location: components/backend/types/errors.go, components/backend/types/provider.go
• Issue: Several exported functions lack godoc comments
• Recommendation: Add documentation for:
  • GetProviderSpecificGuidance
  • FormatMixedProviderError
  • CreateProviderResult
  • AggregateProviderResults

7. Frontend Settings Section Has Minimal Error Handling

• Location: components/frontend/src/components/workspace-sections/settings-section.tsx
• Issue: GitLab integration UI added but error states not fully handled
• Recommendation: Add error boundaries and loading states for GitLab connection status

---

Positive Highlights

✅ Excellent Security Practices:

• Proper RBAC enforcement with user-scoped K8s clients (gitlab_auth.go:154-171, 225-243)
• Token redaction in logs (gitlab/logger.go, gitlab/client.go:52)
• HTTPS enforcement for instance URLs (gitlab_auth.go:61-63)
• Optimistic concurrency control for secret updates (k8s/secrets.go:20-88)

✅ Strong Error Handling:

• User-friendly error messages with remediation guidance (types/errors.go, gitlab/client.go:128-168)
• Provider-specific error messages (git/operations.go:833-919)
• Comprehensive GitLab API error mapping (gitlab/client.go:85-125)

✅ Comprehensive Testing:

• Integration test suite covering full workflow (tests/integration/gitlab/gitlab_integration_test.go)
• Phase-based test organization (6 test phases)
• Backward compatibility tests (tests/regression/backward_compat_test.go)

✅ Excellent Documentation:

• 6 comprehensive markdown docs (gitlab-integration.md, gitlab-token-setup.md, gitlab-self-hosted.md, etc.)
• Clear API documentation (docs/api/gitlab-endpoints.md)
• Testing procedures documented (gitlab-testing-procedures.md)
• README updated with GitLab features

✅ Architecture Follows CLAUDE.md Standards:

• Project-scoped authentication (multi-tenant isolation)
• User token for operations, service account only for CR writes
• No panic() calls in production code (verified via grep)
• Proper error logging with context
• Type-safe unstructured access using helpers

✅ Clean Code Organization:

• Clear package separation (gitlab/, git/, handlers/, k8s/, types/)
• No cyclic dependencies
• Reusable abstractions (Client, ConnectionManager)
• Provider abstraction layer (types/provider.go)

✅ Performance Optimizations:

• HTTP client with 15-second timeout (gitlab/client.go:34-36)
• Pagination support for large repositories (gitlab/client.go:189-297)
• Configurable pagination limits via environment variable (gitlab/client.go:250-259)

---

Recommendations

High Priority

1. Add SSRF protection to instance URL validation
2. Relax token length validation to support legacy tokens
3. Add rate limiting to GitLab API client and auth endpoints
4. Improve concurrent secret update handling with exponential backoff

Medium Priority

5. Add timeouts to git operations (clone, push, fetch)
6. Sanitize URLs in error messages to prevent information disclosure
7. Add URL encoding for all query parameters in GitLab API calls
8. Standardize logging across all components

Low Priority

9. Add User-Agent header to GitLab API requests
10. Extract duplicate logic in global handler wrappers
11. Add godoc comments for exported functions
12. Define magic numbers as named constants

Documentation

13. Add troubleshooting section to gitlab-integration.md for common errors
14. Document rate limits and quotas in gitlab-self-hosted.md
15. Add migration guide for users switching from GitHub to GitLab

---

Testing Coverage Assessment

Excellent:

• ✅ Integration tests cover full workflow (6 phases)
• ✅ Unit tests for token validation, URL parsing, error handling
• ✅ Backward compatibility tests
• ✅ Security tests (token redaction, URL sanitization)

Suggestions:

• Add performance tests for pagination with large repositories (1000+ branches)
• Add chaos tests for concurrent secret updates
• Add E2E tests with actual GitLab instance (currently skipped without INTEGRATION_TESTS=true)

---

Final Verdict: This PR represents high-quality work that properly extends the platform with GitLab support. The critical and major issues are relatively minor and can be addressed in follow-up PRs if time-sensitive. The security practices, error handling, and documentation are exemplary.

Recommended Action: Approve with follow-up issues created for the critical and major items listed above.

[github-actions]

Claude Code Review

Summary

PR #339 adds comprehensive GitLab support. Implementation is architecturally sound with strong security practices, but has critical issues requiring fixes before merge.

Overall Assessment: Conditional Approval - Fix blocker issues before merging.

Files Changed: 36 files, +9,094/-298 lines

---

Blocker Issues

1. Race Condition in ConfigMap Updates

Location: components/backend/k8s/configmap.go:56-66

ConfigMap updates lack optimistic concurrency control. Multiple concurrent GitLab connections can cause lost updates.

Fix: Implement retry logic with exponential backoff (like k8s/secrets.go:24-87).

2. RBAC Bypass After Validation

Location: components/backend/handlers/gitlab_auth.go:154-174

After validating user permissions, code uses backend service account to write Secrets/ConfigMaps, bypassing RBAC.

CLAUDE.md Violation: FORBIDDEN - Using backend service account for user-initiated operations

Fix: Pass user-scoped clientset to ConnectionManager.

3. Orphaned Tokens on Transaction Failure

Location: components/backend/gitlab/connection.go:50-60

Token stored before ConfigMap creation. If ConfigMap fails, cleanup errors are silently ignored.

Fix: Log cleanup failures explicitly, add reconciliation for orphaned tokens.

4. HTTP to HTTPS Silent Upgrade Inconsistency

Location: gitlab/parser.go:85 + handlers/gitlab_auth.go:60-64

Parser silently upgrades HTTP to HTTPS, but auth handler rejects HTTP - inconsistent behavior.

Fix: Either remove silent upgrade OR document auto-upgrade behavior.

---

Critical Issues

• Context Deadline Enforcement: gitlab/client.go hard-coded 15s timeout ignores context deadlines
• Error Handling: fmt.Sscanf errors ignored in pagination parsing
• Pagination Inconsistency: Inconsistent pagination limits across methods
• Test Coverage Gaps: No RBAC tests, no concurrency tests, no E2E tests

---

Positive Highlights

• Clean provider abstraction foundation
• Multi-tenancy support with namespace isolation
• Token redaction in logs
• 4,460+ lines of comprehensive documentation
• 396-line integration test suite
• Backward compatibility tests

---

Recommendations

Pre-Merge (Required)

1. Fix ConfigMap race condition
2. Fix RBAC bypass
3. Fix orphaned token cleanup
4. Fix HTTP/HTTPS inconsistency

Post-Merge

5. Add RBAC integration tests
6. Add concurrency tests with -race flag
7. Add E2E Cypress tests for GitLab

Long-Term

8. Provider interface pattern
9. Split git/operations.go (1,897 lines)
10. Token lifecycle management

---

Testing Summary

Category	Grade
Functional	B+
Security	D
Concurrency	F
Documentation	A

Overall: B- (68%)

---

Conclusion

Solid engineering work with strong foundation for multi-provider support. The 4 blocker issues must be resolved before production use.

Estimated Fix Time: 4-8 hours

Review by: Claude Code (Sonnet 4.5) with specialized agents