Skip to content

Commit 7a32187

Browse files
Sylvain Bauzaclaude
andcommitted
fix: address third round of CodeRabbit review feedback
- Honor gitcookie subdomain flag (column 2) in parseGitcookies: only allow subdomain matching when flag is TRUE, preventing credential scope widening on mistyped URLs - Add cluster URL validation to Gerrit MCP auth check fallback to prevent BOT_TOKEN exfiltration via user-overridden env vars - Propagate Gerrit PermissionError into auth_failures list for consistent fail-fast behavior with other providers - Align spec type name GerritInstancesListResponse → GerritInstancesResponse to match implementation - Update spec assumption to reflect per-user Secret model - Add discriminator keyword to OpenAPI oneOf schemas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent ef57b27 commit 7a32187

6 files changed

Lines changed: 33 additions & 6 deletions

File tree

components/backend/handlers/integration_validation.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,9 @@ func ValidateGerritToken(ctx context.Context, gerritURL, authMethod, username, h
224224
}
225225

226226
// parseGitcookies extracts the cookie value for a given Gerrit URL from gitcookies content.
227-
// Gitcookies format: host\tFALSE\t/\tTRUE\t2147483647\to\tvalue
227+
// Gitcookies format: host\tsubdomain_flag\t/\tTRUE\t2147483647\to\tvalue
228+
// The subdomain flag (column 2) controls whether subdomains are allowed:
229+
// "TRUE" means any subdomain matches; "FALSE" means exact host match only.
228230
func parseGitcookies(gerritURL, content string) string {
229231
parsed, err := url.Parse(gerritURL)
230232
if err != nil {
@@ -241,7 +243,13 @@ func parseGitcookies(gerritURL, content string) string {
241243
fields := strings.Split(line, "\t")
242244
if len(fields) >= 7 {
243245
cookieHost := strings.TrimPrefix(fields[0], ".")
244-
if cookieHost == host || strings.HasSuffix(host, "."+cookieHost) {
246+
subdomainFlag := strings.ToUpper(strings.TrimSpace(fields[1]))
247+
248+
if cookieHost == host {
249+
return fields[5] + "=" + fields[6]
250+
}
251+
// Only allow subdomain matching when the flag is TRUE
252+
if subdomainFlag == "TRUE" && strings.HasSuffix(host, "."+cookieHost) {
245253
return fields[5] + "=" + fields[6]
246254
}
247255
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from datetime import datetime, timezone
1313
from pathlib import Path
1414
from typing import Any, Dict
15+
from urllib.parse import urlparse
1516

1617
from ambient_runner.platform.context import RunnerContext
1718
from ambient_runner.platform.utils import get_bot_token
@@ -358,6 +359,18 @@ def check_mcp_authentication(server_name: str) -> tuple[bool | None, str | None]
358359
session_id = os.getenv("SESSION_ID", "")
359360

360361
if base and project and session_id:
362+
# Reject non-cluster URLs to prevent token exfiltration
363+
parsed_base = urlparse(base)
364+
if parsed_base.hostname and not (
365+
parsed_base.hostname.endswith(".svc.cluster.local")
366+
or parsed_base.hostname == "localhost"
367+
or parsed_base.hostname == "127.0.0.1"
368+
):
369+
logger.error(
370+
f"Refusing to send credentials to external host: {parsed_base.hostname}"
371+
)
372+
return False, "Gerrit not configured - connect on Integrations page"
373+
361374
url = f"{base}/projects/{project.strip()}/agentic-sessions/{session_id}/credentials/gerrit"
362375
req = _urllib_request.Request(url, method="GET")
363376
bot = (os.getenv("BOT_TOKEN") or "").strip()

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,8 @@ async def populate_runtime_credentials(context: RunnerContext) -> None:
423423
# config from a previous refresh is cleaned up when all instances are removed.
424424
if isinstance(gerrit_instances, Exception):
425425
logger.warning(f"Failed to fetch Gerrit credentials: {gerrit_instances}")
426+
if isinstance(gerrit_instances, PermissionError):
427+
auth_failures.append(str(gerrit_instances))
426428
else:
427429
try:
428430
from ambient_runner.bridges.claude.mcp import generate_gerrit_config

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ export interface GerritTestResponse {
5656
error?: string
5757
}
5858

59-
export interface GerritInstancesListResponse {
59+
export interface GerritInstancesResponse {
6060
instances: GerritInstanceStatus[]
6161
}
6262

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ paths:
117117
content:
118118
application/json:
119119
schema:
120-
$ref: '#/components/schemas/GerritInstancesListResponse'
120+
$ref: '#/components/schemas/GerritInstancesResponse'
121121

122122
/api/projects/{projectName}/agentic-sessions/{sessionName}/credentials/gerrit:
123123
get:
@@ -177,6 +177,8 @@ components:
177177
type: string
178178
enum: [http_basic, git_cookies]
179179
description: Authentication method
180+
discriminator:
181+
propertyName: authMethod
180182
oneOf:
181183
- properties:
182184
authMethod:
@@ -207,6 +209,8 @@ components:
207209
type: string
208210
enum: [http_basic, git_cookies]
209211
description: Authentication method
212+
discriminator:
213+
propertyName: authMethod
210214
oneOf:
211215
- properties:
212216
authMethod:
@@ -260,7 +264,7 @@ components:
260264
type: string
261265
description: Error message if validation failed
262266

263-
GerritInstancesListResponse:
267+
GerritInstancesResponse:
264268
type: object
265269
properties:
266270
instances:

specs/001-gerrit-integration/spec.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ A platform user who contributes to multiple Gerrit-hosted projects (e.g., OpenSt
122122
- The open-source Gerrit MCP server from `gerrit.googlesource.com/gerrit-mcp-server` is suitable for bundling into the platform's runner environment.
123123
- Two authentication methods are supported: HTTP credentials (username + HTTP password/access token) and gitcookies file content. Both are widely used across Gerrit deployments.
124124
- The Gerrit MCP server will be run in STDIO mode within the runner pod, consistent with how other MCP servers (e.g., mcp-atlassian, google-workspace) are launched.
125-
- The platform's existing generic MCP server credential storage pattern (`mcp-server-credentials` secret with `serverName:userID` keying) can be extended to support Gerrit.
125+
- Gerrit credentials are stored in per-user Kubernetes Secrets named `gerrit-credentials-{userID}` with `instanceName` keys, providing user isolation and avoiding unbounded Secret growth.
126126
- Users are responsible for obtaining their own credentials: either generating an HTTP password from their Gerrit instance's Settings page, or obtaining their gitcookies file content.
127127

128128
## Dependencies

0 commit comments

Comments
 (0)