fix: address CodeRabbit review feedback on Gerrit integration

- Fix SSRF DNS rebinding (TOCTOU): add ssrfSafeTransport with custom
  dialer that validates resolved IPs at connection time, preventing
  DNS rebinding attacks between validation and HTTP request
- Fail closed on non-200 Gerrit responses in ValidateGerritToken
  instead of assuming valid (prevents storing invalid connections)
- Add isValidUserID() check to GetGerritStatus, DisconnectGerrit,
  and ListGerritInstances for consistency with ConnectGerrit
- Tighten test assertions: assert exact HTTP 200 instead of
  NotTo(Equal(400)) or BeElementOf(200, 404)
- Add timeout (15s) and error handling to frontend test/route.ts
- Use discriminated union types for GerritConnectRequest and
  GerritTestRequest in frontend API types
- Fix stale Gerrit config: call generate_gerrit_config() even when
  backend returns empty instances list, to clean up old config
- Add docstring clarifying MCP config regeneration timing
- Fix documentation: per-user Secret naming, pinned MCP server
  revision, multi-instance UX consistency, dedicated test request
  schema in OpenAPI contract

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: Sylvain Bauza

components/backend/handlers/gerrit_auth.go

@@ -45,6 +45,14 @@ func gerritSecretName(userID string) string {
 	return gerritSecretPrefix + userID
 }
 
+// isPrivateOrBlocked returns true if the IP is loopback, private,
+// link-local, or a cloud metadata address.
+func isPrivateOrBlocked(ip net.IP) bool {
+	return ip.IsLoopback() || ip.IsPrivate() ||
+		ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() ||
+		ip.Equal(net.ParseIP("169.254.169.254"))
+}
+
 // validateGerritURL validates a Gerrit URL for SSRF protection.
 // It ensures the scheme is HTTPS and the host does not resolve to
 // loopback, private, link-local, or metadata-service addresses.
@@ -70,17 +78,48 @@ func validateGerritURL(rawURL string) error {
 		if ip == nil {
 			continue
 		}
-		if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() {
+		if isPrivateOrBlocked(ip) {
 			return fmt.Errorf("URL must not resolve to a private or loopback address")
 		}
-		// Block cloud metadata endpoints (169.254.169.254)
-		if ip.Equal(net.ParseIP("169.254.169.254")) {
-			return fmt.Errorf("URL must not resolve to a metadata service address")
-		}
 	}
 	return nil
 }
 
+// ssrfSafeTransport returns an *http.Transport with a custom dialer that
+// rejects connections to private/loopback/metadata IPs. This prevents DNS
+// rebinding attacks where a hostname resolves to a public IP during
+// validateGerritURL but to a private IP when the HTTP request is made.
+func ssrfSafeTransport() *http.Transport {
+	return &http.Transport{
+		DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
+			host, port, err := net.SplitHostPort(addr)
+			if err != nil {
+				return nil, fmt.Errorf("invalid address: %w", err)
+			}
+			ips, err := net.DefaultResolver.LookupHost(ctx, host)
+			if err != nil {
+				return nil, fmt.Errorf("cannot resolve host: %w", err)
+			}
+			for _, ipStr := range ips {
+				ip := net.ParseIP(ipStr)
+				if ip != nil && isPrivateOrBlocked(ip) {
+					return nil, fmt.Errorf("connection to private/loopback address blocked")
+				}
+			}
+			// Dial using the resolved IPs to prevent rebinding
+			var dialer net.Dialer
+			for _, ipStr := range ips {
+				conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(ipStr, port))
+				if dialErr == nil {
+					return conn, nil
+				}
+				err = dialErr
+			}
+			return nil, err
+		},
+	}
+}
+
 // ConnectGerrit handles POST /api/auth/gerrit/connect
 // Validates and stores Gerrit credentials for a user instance
 func ConnectGerrit(c *gin.Context) {
@@ -201,6 +240,10 @@ func GetGerritStatus(c *gin.Context) {
 		c.JSON(http.StatusUnauthorized, gin.H{"error": "User authentication required"})
 		return
 	}
+	if !isValidUserID(userID) {
+		c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid user identifier"})
+		return
+	}
 
 	instanceName := c.Param("instanceName")
 	if instanceName == "" {
@@ -246,6 +289,10 @@ func DisconnectGerrit(c *gin.Context) {
 		c.JSON(http.StatusUnauthorized, gin.H{"error": "User authentication required"})
 		return
 	}
+	if !isValidUserID(userID) {
+		c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid user identifier"})
+		return
+	}
 
 	instanceName := c.Param("instanceName")
 	if instanceName == "" {
@@ -277,6 +324,10 @@ func ListGerritInstances(c *gin.Context) {
 		c.JSON(http.StatusUnauthorized, gin.H{"error": "User authentication required"})
 		return
 	}
+	if !isValidUserID(userID) {
+		c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid user identifier"})
+		return
+	}
 
 	instances, err := listGerritCredentials(c.Request.Context(), userID)
 	if err != nil {

components/backend/handlers/gerrit_auth_test.go

@@ -274,8 +274,7 @@ var _ = Describe("Gerrit Auth Handler", Label(test_constants.LabelUnit, test_con
 
 			ConnectGerrit(context)
 
-			status := httpUtils.GetResponseRecorder().Code
-			Expect(status).NotTo(Equal(http.StatusBadRequest), "Should accept valid http_basic credentials")
+			httpUtils.AssertHTTPStatus(http.StatusOK)
 		})
 
 		It("Should return 401 when credentials are invalid", func() {
@@ -361,8 +360,7 @@ var _ = Describe("Gerrit Auth Handler", Label(test_constants.LabelUnit, test_con
 
 			GetGerritStatus(context)
 
-			status := httpUtils.GetResponseRecorder().Code
-			Expect(status).To(BeElementOf(http.StatusOK, http.StatusNotFound))
+			httpUtils.AssertHTTPStatus(http.StatusOK)
 		})
 	})
 

components/backend/handlers/integration_validation.go

@@ -153,7 +153,10 @@ func ValidateGerritToken(ctx context.Context, gerritURL, authMethod, username, h
 		return false, fmt.Errorf("Gerrit URL is required")
 	}
 
-	client := &http.Client{Timeout: 15 * time.Second}
+	client := &http.Client{
+		Timeout:   15 * time.Second,
+		Transport: ssrfSafeTransport(),
+	}
 	apiURL := fmt.Sprintf("%s/a/accounts/self", gerritURL)
 
 	req, err := http.NewRequestWithContext(ctx, "GET", apiURL, nil)
@@ -191,16 +194,16 @@ func ValidateGerritToken(ctx context.Context, gerritURL, authMethod, username, h
 	}
 	defer resp.Body.Close()
 
-	// 200 = valid, 401/403 = invalid
+	// 200 = valid credentials; anything else (including 5xx, redirects,
+	// wrong base URL) is treated as invalid to fail closed.
 	if resp.StatusCode == http.StatusOK {
 		return true, nil
 	}
 	if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden {
 		return false, nil
 	}
 
-	// Other status codes - can't validate, assume valid to avoid false negatives
-	return true, nil
+	return false, fmt.Errorf("unexpected status %d from Gerrit /a/accounts/self", resp.StatusCode)
 }
 
 // parseGitcookies extracts the cookie value for a given Gerrit URL from gitcookies content.

components/frontend/src/app/api/auth/gerrit/test/route.ts

@@ -5,12 +5,21 @@ export async function POST(request: Request) {
   const headers = await buildForwardHeadersAsync(request)
   const body = await request.text()
 
-  const resp = await fetch(`${BACKEND_URL}/auth/gerrit/test`, {
-    method: 'POST',
-    headers,
-    body,
-  })
+  try {
+    const resp = await fetch(`${BACKEND_URL}/auth/gerrit/test`, {
+      method: 'POST',
+      headers,
+      body,
+      signal: AbortSignal.timeout(15_000),
+    })
 
-  const data = await resp.text()
-  return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } })
+    const data = await resp.text()
+    return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } })
+  } catch (err) {
+    const message = err instanceof Error ? err.message : 'Backend request failed'
+    return new Response(JSON.stringify({ valid: false, error: message }), {
+      status: 502,
+      headers: { 'Content-Type': 'application/json' },
+    })
+  }
 }

components/frontend/src/services/api/gerrit-auth.ts

@@ -2,22 +2,25 @@ import { apiClient } from './client'
 
 export type GerritAuthMethod = 'http_basic' | 'git_cookies'
 
+type GerritHttpBasicFields = {
+  authMethod: 'http_basic'
+  username: string
+  httpToken: string
+}
+
+type GerritGitCookiesFields = {
+  authMethod: 'git_cookies'
+  gitcookiesContent: string
+}
+
 export type GerritConnectRequest = {
   instanceName: string
   url: string
-  authMethod: GerritAuthMethod
-  username?: string
-  httpToken?: string
-  gitcookiesContent?: string
-}
+} & (GerritHttpBasicFields | GerritGitCookiesFields)
 
 export type GerritTestRequest = {
   url: string
-  authMethod: GerritAuthMethod
-  username?: string
-  httpToken?: string
-  gitcookiesContent?: string
-}
+} & (GerritHttpBasicFields | GerritGitCookiesFields)
 
 export type GerritTestResponse = {
   valid: boolean

components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py

@@ -39,6 +39,12 @@ def generate_gerrit_config(instances: list[dict]) -> None:
     - .gitcookies: Combined gitcookies content (if any instances use git_cookies auth)
 
     Sets GERRIT_CONFIG_PATH env var to point to the generated config.
+
+    This is called by populate_runtime_credentials() before each SDK run,
+    so the config is always written before the CLI subprocess (and its MCP
+    servers) are spawned. On credential refresh the config files on disk
+    are regenerated; the Gerrit MCP server re-reads the config from disk
+    on each request, so changes take effect without restarting it.
     """
     config_dir = Path("/tmp/gerrit-mcp")
 

components/runners/ambient-runner/ambient_runner/platform/auth.py

@@ -419,14 +419,19 @@ async def populate_runtime_credentials(context: RunnerContext) -> None:
             git_user_email = github_creds["email"]
 
     # Gerrit credentials (generate config file for MCP server)
+    # Always call generate_gerrit_config (even with empty list) so stale
+    # config from a previous refresh is cleaned up when all instances are removed.
     if isinstance(gerrit_instances, Exception):
         logger.warning(f"Failed to fetch Gerrit credentials: {gerrit_instances}")
-    elif gerrit_instances:
+    else:
         try:
             from ambient_runner.bridges.claude.mcp import generate_gerrit_config
 
-            generate_gerrit_config(gerrit_instances)
-            logger.info("Generated Gerrit MCP config from backend credentials")
+            generate_gerrit_config(gerrit_instances if gerrit_instances else [])
+            if gerrit_instances:
+                logger.info("Generated Gerrit MCP config from backend credentials")
+            else:
+                logger.info("Cleared stale Gerrit MCP config (no instances)")
         except Exception as e:
             logger.warning(f"Failed to generate Gerrit config: {e}")
 

docs/internal/integrations/gerrit-integration.md

@@ -439,7 +439,7 @@ A: Use the `/api/auth/gerrit/test` endpoint. It validates credentials against th
 A: The instance name is a user-chosen identifier that distinguishes between multiple Gerrit instances. It is used in API paths (e.g., `/api/auth/gerrit/openstack/status`) and as part of the credential storage key.
 
 **Q: Can two users connect the same Gerrit instance with the same instance name?**
-A: Yes. Instance names are scoped per user. Two different users can both have an instance named "openstack" without conflict, as credentials are stored with a compound key of `instanceName.userID`.
+A: Yes. Instance names are scoped per user. Each user has their own Kubernetes Secret (`gerrit-credentials-{userID}`), so two different users can both have an instance named "openstack" without conflict.
 
 ---
 

specs/001-gerrit-integration/contracts/frontend-types.ts

@@ -22,6 +22,19 @@ export type GerritConnectRequest =
       gitcookiesContent: string
     }
 
+export type GerritTestRequest =
+  | {
+      url: string
+      authMethod: 'http_basic'
+      username: string
+      httpToken: string
+    }
+  | {
+      url: string
+      authMethod: 'git_cookies'
+      gitcookiesContent: string
+    }
+
 export interface GerritConnectResponse {
   message: string
   instanceName: string

specs/001-gerrit-integration/contracts/gerrit-api.yaml

@@ -97,7 +97,7 @@ paths:
         content:
           application/json:
             schema:
-              $ref: '#/components/schemas/GerritConnectRequest'
+              $ref: '#/components/schemas/GerritTestRequest'
       responses:
         '200':
           description: Validation result
@@ -177,15 +177,51 @@ components:
           type: string
           enum: [http_basic, git_cookies]
           description: Authentication method
-        username:
-          type: string
-          description: Gerrit username (required for http_basic)
-        httpToken:
+      oneOf:
+        - properties:
+            authMethod:
+              const: http_basic
+            username:
+              type: string
+            httpToken:
+              type: string
+          required: [username, httpToken]
+        - properties:
+            authMethod:
+              const: git_cookies
+            gitcookiesContent:
+              type: string
+          required: [gitcookiesContent]
+
+    GerritTestRequest:
+      description: Test Gerrit credentials without storing (no instanceName required)
+      type: object
+      required: [url, authMethod]
+      properties:
+        url:
           type: string
-          description: HTTP password/access token (required for http_basic)
-        gitcookiesContent:
+          format: uri
+          description: Gerrit instance base URL
+          example: https://review.opendev.org
+        authMethod:
           type: string
-          description: Raw gitcookies file content (required for git_cookies)
+          enum: [http_basic, git_cookies]
+          description: Authentication method
+      oneOf:
+        - properties:
+            authMethod:
+              const: http_basic
+            username:
+              type: string
+            httpToken:
+              type: string
+          required: [username, httpToken]
+        - properties:
+            authMethod:
+              const: git_cookies
+            gitcookiesContent:
+              type: string
+          required: [gitcookiesContent]
 
     GerritConnectResponse:
       type: object