Skip to content

Conversation

@adalton
Copy link
Contributor

@adalton adalton commented Jan 30, 2026

Summary

Fixes RHOAIENG-46519: Google Workspace MCP server failing to authenticate and connect in session pods.

This was a multi-root-cause issue requiring fixes across the init container, operator, runner, backend, and frontend. The fix has been validated
end-to-end in a fresh kind cluster.

Root Causes Fixed

1. Workspace Directory Permissions (hydrate.sh)

MCP servers spawn as subprocesses and need write access to their working directory. Changed permissions from 755 to 777 for /workspace/artifacts,
/workspace/file-uploads, and /workspace/repos. The chown to uid 1001 is attempted first but fails on SELinux-restricted hosts.

2. Credentials Format Mismatch (sessions.go)

The workspace-mcp server expects a flat JSON format with specific fields (token, refresh_token, token_uri, client_id, client_secret, scopes, expiry). The
operator was creating a nested format.

3. Read-Only Credentials Mount (sessions.go)

K8s secrets are mounted read-only, but workspace-mcp needs to write updated tokens during refresh. Added postStart lifecycle hook to copy credentials to
writable location.

4. Invalid MCP Config Field (.mcp.json)

Removed invalid "type": "stdio" field from google-workspace config. Claude Code SDK doesn't recognize this field.

5. Missing Python Imports (main.py)

Added missing Path and datetime imports that caused NameError crashes.

6. Timezone-Naive Datetime Comparison (main.py)

Fixed _parse_token_expiry() to always return timezone-aware datetime.

7. Environment Variable Overwrite (adapter.py)

Fixed _set_google_user_email() to not overwrite operator-set USER_GOOGLE_EMAIL.

8. OAuth Callback 404 (route.ts)

Added /oauth2callback route to frontend to proxy OAuth callbacks to backend.

9. Secret Key Sanitization (oauth.go, sessions.go)

Added sanitizeSecretKey() to handle userIDs with invalid K8s secret key characters.

10. XSS Vulnerability (route.ts)

Added escapeHtml() to sanitize error messages in OAuth callback HTML responses.

Files Changed

Component File Change
Init Container hydrate.sh 777 permissions for workspace dirs
Operator sessions.go Credentials format, postStart hook, USER_GOOGLE_EMAIL
Runner .mcp.json Remove invalid "type" field
Runner main.py Imports, timezone-aware datetime
Runner adapter.py Preserve env var, MCP auth checks
Backend oauth.go Secret key sanitization
Frontend oauth2callback/route.ts OAuth proxy with XSS protection
Docs README.md, google-workspace.md Updated documentation

Test Plan

  • Deploy to fresh kind cluster
  • Complete Google OAuth flow at /integrations
  • Create new session
  • Verify "Google Workspace: Authenticated" status in MCP accordion
  • Successfully query Google Drive files via MCP
  • CI passes

Security Notes

  • XSS vulnerability in OAuth callback route identified during code review and fixed
  • 777 permissions are fallback only when chown fails (SELinux hosts)

🤖 Generated with Claude Code

adalton and others added 3 commits January 29, 2026 12:16
Fixes RHOAIENG-46519 - Google Workspace MCP shows authenticated but fails
when used due to expired tokens.

Google Workspace MCP integration showed "Authenticated" in the UI but failed
when agents attempted Drive operations. The status check only verified that
credentials.json existed, not whether tokens were valid/unexpired.

Symptoms:
- Status showed "Authenticated" with stale/expired credentials
- Tool calls failed with placeholder email (user@example.com)
- Disconnect/reconnect temporarily fixed (until tokens expired again)

1. _check_mcp_authentication() only checked file existence, not token validity
2. USER_GOOGLE_EMAIL environment variable was never set from actual credentials
3. No detection of expired access tokens or missing refresh tokens
4. Status showed "authenticated" with invalid/expired credentials

- Replace file existence check with actual token validation
- Parse and check token_expiry timestamps
- Validate required fields (access_token, refresh_token) are non-empty
- Return three-state status: True (valid), False (invalid), None (needs refresh)
- Reject placeholder email user@example.com

- Extract user email from credentials.json on setup/refresh
- Set USER_GOOGLE_EMAIL environment variable with actual email
- Update email when credentials are refreshed

- Check MCP authentication status before each agent run
- Display user-visible warnings for auth issues
- Distinguish between "not configured", "expired", and "needs refresh"

- Update type signature: authenticated: boolean | null
- Show "Needs Refresh" badge for null state (expired with refresh token)
- Amber warning icon for uncertain auth states

- Unit tests (test_google_auth.py): All token validation scenarios
- E2E tests (test_google_drive_e2e.py): Complete authentication flow
- Backwards compatible with Jira/Atlassian MCP integration

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
RHOAIENG-46519

This commit fixes multiple issues preventing the Google Workspace MCP
server from connecting and authenticating properly in session pods.

## Root Causes Identified and Fixed

### 1. Workspace Directory Permissions (hydrate.sh)
MCP servers spawn as subprocesses and need write access to their working
directory. Changed permissions from 755 to 777 for /workspace/artifacts,
/workspace/file-uploads, and /workspace/repos. The chown to uid 1001 is
attempted first but fails on SELinux-restricted hosts, so 777 is the
fallback.

### 2. Credentials Format Mismatch (sessions.go)
The workspace-mcp server expects a flat JSON format with specific fields:
token, refresh_token, token_uri, client_id, client_secret, scopes, expiry.
The operator was creating a nested format. Fixed to produce correct flat
format.

### 3. Read-Only Credentials Mount (sessions.go)
K8s secrets are mounted read-only, but workspace-mcp needs to write
updated tokens during refresh. Added postStart lifecycle hook to copy
credentials from read-only /app/.google_workspace_mcp/credentials/ to
writable /workspace/.google_workspace_mcp/credentials/.

### 4. Invalid MCP Config Field (.mcp.json)
Removed invalid "type": "stdio" field from google-workspace config.
Claude Code SDK doesn't recognize this field and was failing to start
the MCP server.

### 5. Missing Python Imports (main.py)
Added missing `from pathlib import Path` and `from datetime import
datetime, timezone` imports that caused NameError crashes.

### 6. Timezone-Naive Datetime Comparison (main.py)
Fixed _parse_token_expiry() to always return timezone-aware datetime.
Was causing "can't compare offset-naive and offset-aware datetimes"
error when checking token expiry.

### 7. Environment Variable Overwrite (adapter.py)
Fixed _set_google_user_email() to not overwrite the operator-set
USER_GOOGLE_EMAIL with incorrect values from the new flat credentials
format.

### 8. OAuth Callback 404 (frontend route.ts)
Added /oauth2callback route to frontend to proxy OAuth callbacks to
backend. Without this, Google's redirect would 404.

### 9. Secret Key Sanitization (oauth.go, sessions.go)
Added sanitizeSecretKey() to handle userIDs containing invalid K8s
secret key characters (@ : /).

## Files Changed

- components/runners/state-sync/hydrate.sh - 777 permissions
- components/operator/internal/handlers/sessions.go - credentials format,
  postStart hook, USER_GOOGLE_EMAIL env var
- components/runners/claude-code-runner/.mcp.json - remove invalid field
- components/runners/claude-code-runner/main.py - imports, timezone fix
- components/runners/claude-code-runner/adapter.py - preserve env var
- components/backend/handlers/oauth.go - secret key sanitization
- components/frontend/src/app/oauth2callback/route.ts - OAuth proxy
- components/runners/claude-code-runner/README.md - documentation
- docs/integrations/google-workspace.md - documentation

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit addresses findings from the code review of the Google
Workspace MCP authentication fix.

## Security Fix

### XSS Vulnerability in OAuth Callback Route
- Added `escapeHtml()` function to sanitize error messages before HTML
  interpolation in oauth2callback/route.ts
- Prevents reflected XSS attacks where malicious content in backend
  error responses could be executed in the user's browser
- Escapes &, <, >, ", and ' characters

## Code Quality

### Remove DEBUG Logging Statements
- Removed two `logging.info("DEBUG: ...")` statements from adapter.py
- These were left in production code and added unnecessary log noise

### Fix Test Assertion
- Updated test_google_auth.py to expect "not configured" instead of
  "empty" for empty credentials file test
- Matches actual implementation behavior where empty files are treated
  as not configured

## Files Changed

- components/frontend/src/app/oauth2callback/route.ts - XSS fix
- components/runners/claude-code-runner/adapter.py - DEBUG removal
- components/runners/claude-code-runner/tests/test_google_auth.py - assertion fix

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions

This comment has been minimized.

Resolved conflict in hydrate.sh by:
- Keeping our chown addition (sets ownership to runner user 1001)
- Accepting main's permission approach (755 for artifacts/file-uploads, 777 for repos)

This combines both approaches: chown ensures the runner can write as owner,
and the permissions follow main's security rationale.
@codecov
Copy link

codecov bot commented Jan 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2026

Claude Code Review

Summary

This PR comprehensively fixes Google Workspace MCP authentication across 10 root causes spanning backend, operator, runner, frontend, and init container. The fix has been validated end-to-end and includes 386 lines of test coverage. The code demonstrates strong attention to security (XSS fix, sanitization) and follows established patterns. A few critical security and code quality issues must be addressed before merge.


Issues by Severity

🚫 Blocker Issues

None - All critical issues from initial review have been addressed in commit 286369c.

🔴 Critical Issues

1. Frontend Type Definition Inconsistency components/frontend/src/services/api/sessions.ts:114

  • Changed authenticated?: boolean to authenticated: boolean | null
  • Violates Critical Rule Outcome: Reduce Refinement Time with agent System #1: Changed from optional (?) to required
  • This is a breaking change for existing code that may not set this field
  • Impact: Runtime errors where authenticated is expected but undefined
  • Fix: Either keep it optional authenticated?: boolean | null OR ensure all code paths set it

2. Unused Test Fixture components/runners/claude-code-runner/tests/test_google_auth.py:481-485

  • temp_secret_creds fixture created but never used in tests
  • Test test_no_credentials_file should verify fallback from workspace to secret path
  • Impact: Incomplete test coverage for multi-location credential lookup
  • Fix: Use both fixtures in test_no_credentials_file or remove temp_secret_creds

3. Environment Variable Mutation in Tests test_google_auth.py:796

  • Tests modify os.environ without cleanup in all cases
  • Impact: Test pollution - one test's env changes affect subsequent tests
  • Fix: Use @pytest.fixture(autouse=True) to save/restore env or monkeypatch fixture

🟡 Major Issues

4. Permissions 777 Security Trade-off components/runners/state-sync/hydrate.sh:74

  • chmod -R 777 on /workspace/artifacts, /workspace/file-uploads, /workspace/repos
  • Reasoning understood: MCP servers spawn subprocesses, SELinux blocks chown
  • ⚠️ Security concern: Any process can read/write/execute these directories
  • Recommendation: Document in security review that 777 is acceptable here because:
    • Single-user container (not multi-tenant within pod)
    • Short-lived session pods (not long-running services)
    • Alternative would break MCP entirely on SELinux hosts

5. Incomplete Python Imports in Tests test_google_auth.py:461-462

import os
import sys
  • Missing imports: json, tempfile, datetime, timezone, timedelta, Path, unittest.mock.patch
  • These are imported later (lines 459-465) but standard is to import at top
  • Fix: Move all imports to top of file per PEP 8

6. Type Annotation Inconsistency main.py:469

def _read_google_credentials(workspace_path: Path, secret_path: Path) -> dict | None:
  • Returns generic dict instead of Dict[str, Any] or TypedDict
  • Fix: Use Dict[str, Any] or define GoogleCredentials = TypedDict(...)

7. Missing Error Handling in Frontend Route oauth2callback/route.ts:51

  • await response.text() could throw if response is malformed
  • Fix: Wrap in try-catch or use response.text().catch()

🔵 Minor Issues

8. Redundant Comment components/operator/internal/handlers/sessions.go:886

// Google MCP credentials directory - uses writable workspace location
// Credentials are copied from read-only secret mount by postStart lifecycle hook
  • Comment duplicates what the code clearly shows
  • Recommendation: Keep first line, remove second (implementation detail obvious from postStart hook above)

9. Inconsistent String Formatting main.py:576

return None, f"Google OAuth authenticated as {user_email} (token expiry format invalid)"
  • Mixes informational message with error state
  • Recommendation: Use clearer phrasing like "token expiry unparseable - may need refresh"

10. Test Organization tests/test_google_drive_e2e.py

  • E2E test filename suggests integration but only tests unit-level auth flow
  • test_google_drive_tool_invocation is a placeholder (skipped)
  • Recommendation: Rename to test_google_auth_e2e.py or implement tool invocation test

Positive Highlights

Comprehensive Root Cause Analysis: 10 distinct issues identified and fixed systematically
Excellent Test Coverage: 386 lines of unit tests covering all token validation edge cases
Security Awareness: XSS sanitization added proactively in commit 286369c
Proper K8s Patterns: OwnerReferences, secret sanitization (sanitizeSecretKey), lifecycle hooks
Backwards Compatibility: Jira MCP integration unchanged, authenticated?: boolean for other servers
Documentation: README updated with credentials flow, permissions rationale, format examples
Error Handling: Three-state auth (true/false/null) with user-visible messages in UI
Code Quality: Follows Go formatting, Python black/isort, TypeScript conventions
Timezone Awareness: Fixed datetime comparison bug (lines 513-520 in main.py)
Environment Variable Expansion: Generic _expand_env_vars utility in adapter.py (lines 1513-1529)


Recommendations (Prioritized)

Must Fix Before Merge

  1. Fix frontend type definition (authenticated?: boolean | null) - breaking change
  2. Clean up test fixtures - use or remove temp_secret_creds
  3. Fix test environment pollution - save/restore os.environ properly

Should Fix Before Merge

  1. Add error handling to frontend OAuth route - prevent unhandled promise rejections
  2. Document 777 permissions security trade-off - add comment in hydrate.sh
  3. Fix Python imports - move all to top of test files

Nice to Have

  1. Improve type annotations - use Dict[str, Any] or TypedDict for credentials
  2. Clarify error messages - "unparseable expiry" vs "format invalid"
  3. Rename E2E test file - matches actual test scope

Testing Gaps

While test coverage is excellent overall, consider:

  1. Integration test: End-to-end OAuth flow in kind cluster (not just unit tests)
  2. Secret mount test: Verify postStart hook successfully copies credentials
  3. MCP server lifecycle test: Spawn actual google-workspace server with test creds
  4. Frontend integration test: Add Cypress test for OAuth callback route

Architecture Compliance

Backend: User token auth pattern followed correctly
Operator: OwnerReferences, status updates via UpdateStatus subresource
Frontend: Uses Shadcn UI, type-safe (except issue #1)
Security: Token redaction, sanitization, XSS prevention
Error Handling: Structured errors, appropriate HTTP status codes
⚠️ Type Safety: any type avoided except in generic _expand_env_vars


Final Verdict

Approval Status: ✅ Approved with Required Changes

This is high-quality work that systematically addresses a complex multi-component issue. The three critical issues above must be fixed before merge, but they are straightforward. Once addressed, this PR will be production-ready.

Estimated Time to Fix: ~30 minutes for all critical issues

Recommended Merge Strategy: Squash and merge (already has 3 clean commits)


🤖 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 Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

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.

@adalton
Copy link
Contributor Author

adalton commented Jan 30, 2026

Testing

  1. Build the code, spin up the local cluster

    export CONTAINER_ENGINE=podman
    
    $ make build-all
    $ make push-all REGISTRY=quay.io/rh-ee-andalton
    
    $ cd e2e
    $ cat > e2e/.env <<EOF
    IMAGE_BACKEND=quay.io/rh-ee-andalton/vteam_backend:latest
    IMAGE_FRONTEND=quay.io/rh-ee-andalton/vteam_frontend:latest
    IMAGE_OPERATOR=quay.io/rh-ee-andalton/vteam_operator:latest
    IMAGE_RUNNER=quay.io/rh-ee-andalton/vteam_claude_runner:latest
    IMAGE_STATE_SYNC=quay.io/rh-ee-andalton/vteam_state_sync:latest
    EOF
    
    $ ./scripts/setup-kind.sh
    $ ./scripts/deploy.sh
    
    # Inject my creds to use claude into the cluster
    $ ~/bin/setup-vertex-kind.sh
    
    # Make sure everything is up
    $ kubectl -n ambient-code get pods
    NAME                               READY   STATUS    RESTARTS   AGE
    agentic-operator-5967b8f88-m7869   0/1     Running   0          7s
    backend-api-88866f6dc-556n8        0/1     Running   0          7s
    frontend-5bb5f664fd-cbhbx          0/1     Running   0          7s
    minio-75f8575968-dl5mb             0/1     Running   0          7s
  2. Tests that placeholder credentials show as "Not Connected" (the bug fix).

    # Create placeholder OAuth secret, formatted correctly, but with invalid content
    $ kubectl create secret generic google-workflow-app-secret \
      -n ambient-code \
      --from-literal=GOOGLE_OAUTH_CLIENT_ID='example-client-id.apps.googleusercontent.com' \
      --from-literal=GOOGLE_OAUTH_CLIENT_SECRET='example-secret-placeholder' \
      --from-literal=OAUTH_STATE_SECRET="$(openssl rand -base64 32)"
    
    # Restart backend to pick up secret
    $ kubectl rollout restart deployment backend-api -n ambient-code
    
    $ kubectl rollout status deployment backend-api -n ambient-code
    deployment "backend-api" successfully rolled out
  3. Create a test workspace+session

    • New workspace: test-google-mcp
    • New session
    • Examine MCP Server Status/Google Workspace
      • Note that it says "Not Authenticated"
      • Previously it would have said "Connected" or "Authenticated"
  4. Create real Google OAuth token

  5. Replace "bad" one with a "good" one:

    $ kubectl delete secret google-workflow-app-secret -n ambient-code
    
    # client-id and client-secret is from JSON blob above
    $ kubectl create secret generic google-workflow-app-secret \
      -n ambient-code \
      --from-literal=GOOGLE_OAUTH_CLIENT_ID='<paste-client-id>' \
      --from-literal=GOOGLE_OAUTH_CLIENT_SECRET='<paste-client-secret>' \
      --from-literal=OAUTH_STATE_SECRET="$(openssl rand -base64 32)"
    $ kubectl rollout restart deployment backend-api -n ambient-code
  6. Go to http://localhost:8080/integrations

    • Connect Google Drive
    • Pick your email -> Continue -> Allow
  7. Try again

    • The "bad" token was injected into the session at pod creation time.
    • Normally we wouldn't have added totally invalid creds; we added them this
      time to illustrate the fact that the mere presence of the creds isn't
      views as "Authenticated"
    • I'll create a new session
      • Google Workspace: Authenticated

      • Prompt (with correct output):

        > list the files in my google drive
        AI
        13:14:41
        
        MCP Server Authentication Issues:
        
        ℹ webfetch: None ⚠ mcp-atlassian: Jira not configured - set credentials
        in Workspace Settings
        
        These servers may not work correctly until re-authenticated.
        AI
        13:14:41
        
        I'll search for files on your Google Drive.
        13:14:41
        Google-workspace List Drive Items
        andalton@redhat.com
        AI
        13:14:41
        
        I found 2 items in your Google Drive root folder:
        
            Gemini Gems (Folder)
                ID: 1s8KI3_Q0mY_I5YXAesd_vuJCpz1G3ZlH
                Modified: 2025-11-13
                Open folder
        
            andalton-thinkpadp1gen7.rht.csb (Folder)
                ID: 1OXSa2VcKGdqJZc5C-Re_3rvRV7BDsE6a
                Modified: 2025-10-06
                Open folder
        
        Would you like me to explore the contents of either folder?
        
  8. Wait "a while" for the token to expire. Issue the prompt. See that it
    auto-refreshes the token:

    Can you list the files in 'Gemini Gems' within my Google drive?
    
    AI
    15:04:43
    
    MCP Server Authentication Issues:
    
    ℹ webfetch: None ⚠ mcp-atlassian: Jira not configured - set credentials in
    Workspace Settings ℹgoogle-workspace: Google OAuth authenticated as
    andalton@redhat.com (token refresh needed)
    
    These servers may not work correctly until re-authenticated.
    AI
    15:04:43
    
    I'll list the files in the "Gemini Gems" folder.
    15:04:43
    Google-workspace List Drive Items
    andalton@redhat.com
    AI
    15:04:43
    
    The "Gemini Gems" folder is currently empty - it contains no files or subfolders.
    

- Fix type annotations in main.py: use Dict[str, Any] instead of bare dict
  for _read_google_credentials() and _validate_google_token() return types
- Add error handling in oauth2callback route: wrap response.text() in
  try-catch to handle potential body read failures gracefully
- Fix McpServer type: make authenticated field optional (authenticated?)
  to maintain backwards compatibility with existing API responses
- Add context to operator error: include userID in "credentials missing
  email field" error message for debugging
- Use pytest monkeypatch in tests: properly clean up USER_GOOGLE_EMAIL
  environment variable after each test to prevent test pollution

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2026

Claude Code Review

Summary

This PR fixes 10 root causes preventing Google Workspace MCP from authenticating. The fix is comprehensive, touching init container, operator, runner (Python), backend, and frontend. The approach is sound, but there are critical security issues that must be addressed before merge.


🚫 Blocker Issues (MUST FIX)

1. Critical Security: 777 Permissions in Production

# hydrate.sh:81-82, 144, 148
chmod 777 /workspace/repos
chmod -R 777 /workspace/repos

Problem: World-writable directories (777) are a severe security vulnerability in multi-tenant environments.

Solution: Use fsGroup in PodSecurityContext to propagate group ownership automatically, eliminating the need for 777.

Impact: High - code in customer repos could be modified by other pods on the same node.


2. XSS Vulnerability Not Fully Fixed

While frontend correctly escapes errors, backend HTML responses in oauth.go are NOT escaped (lines 249, 346, 365, 372, 383).

Attack: OAuth provider returns malicious error_description with script tags

Solution: Use html.EscapeString() for ALL user-controlled strings in HTML responses

Impact: High - credential theft, session hijacking via XSS


3. Missing CSRF Validation on Cluster-Level OAuth (oauth.go:266)

Cluster-level OAuth flow skips HMAC signature verification that session-specific flow requires.

Solution: Validate HMAC signature BEFORE checking cluster flag

Impact: Critical - CSRF attack allowing unauthorized credential storage


Full Review

See complete analysis with code examples, security assessment, and prioritized recommendations at: #562 (comment)

Verdict: ⏸️ DO NOT MERGE until 3 blocker security issues are resolved.

Estimated fix time: 2-4 hours

Great work on comprehensive debugging! Once security issues are addressed, this will be a solid contribution.


🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

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.

The Google credentials directory is created by the container user (1001)
via mkdir -p, so it already has appropriate ownership and permissions.
The MCP server runs as the same user and can read/write without chmod.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2026

Claude Code Review

Summary

This PR addresses RHOAIENG-46519 by fixing multiple root causes preventing Google Workspace MCP server authentication. Comprehensive fix touching backend, frontend, operator, and runner. Changes follow established patterns well, with good test coverage.

Critical Issues

  1. Potential SecurityContext violation in postStart lifecycle hook (operator/sessions.go:890)
  2. Error handling in OAuth callback could hide debugging details (oauth2callback/route.ts:38)

Major Issues

  1. sanitizeSecretKey function duplicated in oauth.go and sessions.go - extract to shared utility
  2. getGoogleUserEmail uses context.TODO instead of proper context propagation
  3. Type annotations use Python 3.10+ syntax - verify compatibility
  4. Environment variable expansion in MCP config needs security validation

Minor Issues

  1. User email logged in plain text (PII concern)
  2. Error messages could be more helpful
  3. Inconsistent import style in main.py
  4. Consider using battle-tested XSS library

Positive Highlights

✅ Excellent security awareness - XSS vulnerability found and fixed
✅ Comprehensive test coverage - 394 lines of Google auth validation tests
✅ Good documentation - README updated with credentials flow
✅ Follows established patterns consistently
✅ Three-state authentication properly implemented

Recommendations

Priority 1 (Before Merge): Verify lifecycle hook SecurityContext, add error logging, extract sanitizeSecretKey
Priority 2 (Follow-up): Fix context propagation, add PII redaction, fix imports

Final Assessment

High-quality work with strong security awareness. Multi-root-cause analysis is thorough. Critical security issues addressed. Remaining issues are refinements.

Recommendation: Approve with minor follow-ups


🤖 Automated review by Claude Code


🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

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.

user_creds = creds[user_email]
# Map new flat format to expected field names
user_creds = {
"access_token": creds.get("token", ""),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put some debug here - make sure it gets to langfuse

Comment on lines +27 to +30
system_prompt_config = {
"type": "preset",
"preset": "claude_code",
"append": workspace_prompt,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be validated. we just put that claude_code thing in there this week. i dont want to lose it - what does preset do?

│ └── {workflow-name}/ # Individual workflow
├── artifacts/ # Output files created by Claude
├── file-uploads/ # User-uploaded files
└── .google_workspace_mcp/ # Google OAuth credentials
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this work if they have the same path

- **Timeout Protection**: Operations have configurable timeouts
- **User Context Validation**: User IDs and names are sanitized
- **Read-only Workflow Directories**: Workflows are read-only, outputs go to artifacts
- **OAuth Credentials Isolation**: Google credentials are stored in session-specific secrets, copied to writable storage only within the container
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to verify this - i want to know whether other users or sessions can see other users or sessions within the same workspace.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and of course across workspaces should currently have zero leakage

- Ensures Claude has both general coding capabilities and workspace knowledge
- `ClaudeAgentOptions.system_prompt` expects `str | SystemPromptPreset | None`
- The list format was invalid and caused `'list' object has no attribute 'get'` error
- `SystemPromptPreset` uses `type="preset"`, `preset="claude_code"`, and optional `append`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, did they change this...ok. we just have to validate

@Gkrumbach07
Copy link
Collaborator

Merging this now bc i have a pr that builds on this and resolves some of the concerns

@Gkrumbach07 Gkrumbach07 merged commit fe33925 into ambient-code:main Feb 2, 2026
40 of 41 checks passed
Gkrumbach07 added a commit to Gkrumbach07/platform that referenced this pull request Feb 2, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants