fix(google-oauth): set correct OAuth redirect URI for workspace-mcp in runner pods#1480
Merged
mergify[bot] merged 2 commits intomainfrom Apr 29, 2026
Merged
Conversation
…er pods workspace-mcp defaults to http://localhost:8000/oauth2callback when GOOGLE_OAUTH_REDIRECT_URI is not set. This causes Google OAuth to fail with "redirect_uri_mismatch" because localhost:8000 is not a registered redirect URI in the Google Cloud Console app. The fix threads the backend's public URL through the operator so that runner pods receive the correct redirect URI pointing to the Ambient backend's /oauth2callback endpoint (which is already registered and handled by handlers/oauth.go). Changes: - operator/config.go: add BackendPublicURL field, read from BACKEND_PUBLIC_URL env - operator/handlers/sessions.go: inject GOOGLE_OAUTH_REDIRECT_URI into runner pod env when BackendPublicURL is configured - manifests/base/core/operator-deployment.yaml: source BACKEND_PUBLIC_URL from google-workflow-app-secret (key: BACKEND_URL, optional) — reuses the same secret already configured by platform admins - runners/ambient-runner/.mcp.json: forward GOOGLE_OAUTH_REDIRECT_URI into the workspace-mcp process environment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds a propagated backend public URL: secret-sourced env var is added to the operator deployment, read into operator config, used to set GOOGLE_OAUTH_REDIRECT_URI in runners; also adds scheduled session types to the Go SDK and minor MCP JSON arg/env formatting changes. Changes
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/operator/internal/handlers/sessions.go`:
- Around line 1083-1087: The redirect URI concatenation is fragile; normalize
appConfig.BackendPublicURL by trimming whitespace and any trailing slashes
before appending "/oauth2callback" to avoid double or missing slashes. Update
the code that builds the EnvVar (the Value for "GOOGLE_OAUTH_REDIRECT_URI" where
appConfig.BackendPublicURL is used) to call strings.TrimSpace then
strings.TrimRight(baseURL, "/") (or equivalent) and then construct the final
value with baseURL + "/oauth2callback"; remember to add the strings import if
missing.
- Around line 1083-1088: When the code decides not to inject
GOOGLE_OAUTH_REDIRECT_URI because appConfig.BackendPublicURL is empty, add a
warning so misconfiguration is visible: detect when Google OAuth client
configuration is present (e.g., GOOGLE_OAUTH_CLIENT_ID and/or
GOOGLE_OAUTH_CLIENT_SECRET in the environment or the corresponding appConfig
fields) and when appConfig.BackendPublicURL == "" emit a warning log (use the
same logger/context used in this handler) stating that BACKEND_PUBLIC_URL is
empty and redirect URI injection is skipped which may cause
redirect_uri_mismatch at runtime; keep the existing conditional that appends
GOOGLE_OAUTH_REDIRECT_URI to base unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69888b2e-ebe3-492e-8b03-bab483fe2dc0
📒 Files selected for processing (4)
components/manifests/base/core/operator-deployment.yamlcomponents/operator/internal/config/config.gocomponents/operator/internal/handlers/sessions.gocomponents/runners/ambient-runner/.mcp.json
Comment on lines
+1083
to
+1087
| if appConfig.BackendPublicURL != "" { | ||
| base = append(base, corev1.EnvVar{ | ||
| Name: "GOOGLE_OAUTH_REDIRECT_URI", | ||
| Value: fmt.Sprintf("%s/oauth2callback", appConfig.BackendPublicURL), | ||
| }) |
Contributor
There was a problem hiding this comment.
Normalize the redirect base URL before concatenation.
fmt.Sprintf("%s/oauth2callback", appConfig.BackendPublicURL) is fragile: a trailing slash or whitespace in BACKEND_PUBLIC_URL can produce a mismatched redirect URI and break OAuth.
Proposed fix
- if appConfig.BackendPublicURL != "" {
+ redirectBase := strings.TrimRight(strings.TrimSpace(appConfig.BackendPublicURL), "/")
+ if redirectBase != "" {
base = append(base, corev1.EnvVar{
Name: "GOOGLE_OAUTH_REDIRECT_URI",
- Value: fmt.Sprintf("%s/oauth2callback", appConfig.BackendPublicURL),
+ Value: fmt.Sprintf("%s/oauth2callback", redirectBase),
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if appConfig.BackendPublicURL != "" { | |
| base = append(base, corev1.EnvVar{ | |
| Name: "GOOGLE_OAUTH_REDIRECT_URI", | |
| Value: fmt.Sprintf("%s/oauth2callback", appConfig.BackendPublicURL), | |
| }) | |
| redirectBase := strings.TrimRight(strings.TrimSpace(appConfig.BackendPublicURL), "/") | |
| if redirectBase != "" { | |
| base = append(base, corev1.EnvVar{ | |
| Name: "GOOGLE_OAUTH_REDIRECT_URI", | |
| Value: fmt.Sprintf("%s/oauth2callback", redirectBase), | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/operator/internal/handlers/sessions.go` around lines 1083 - 1087,
The redirect URI concatenation is fragile; normalize appConfig.BackendPublicURL
by trimming whitespace and any trailing slashes before appending
"/oauth2callback" to avoid double or missing slashes. Update the code that
builds the EnvVar (the Value for "GOOGLE_OAUTH_REDIRECT_URI" where
appConfig.BackendPublicURL is used) to call strings.TrimSpace then
strings.TrimRight(baseURL, "/") (or equivalent) and then construct the final
value with baseURL + "/oauth2callback"; remember to add the strings import if
missing.
Comment on lines
+1083
to
+1088
| if appConfig.BackendPublicURL != "" { | ||
| base = append(base, corev1.EnvVar{ | ||
| Name: "GOOGLE_OAUTH_REDIRECT_URI", | ||
| Value: fmt.Sprintf("%s/oauth2callback", appConfig.BackendPublicURL), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Add an explicit warning when redirect URI injection is skipped.
When Google OAuth client vars are present but BACKEND_PUBLIC_URL is empty, the runner can still fall back to localhost behavior and fail with redirect_uri_mismatch at runtime. Emitting a warning here makes misconfiguration immediately visible.
Proposed fix
- if appConfig.BackendPublicURL != "" {
+ redirectBase := strings.TrimRight(strings.TrimSpace(appConfig.BackendPublicURL), "/")
+ if redirectBase != "" {
base = append(base, corev1.EnvVar{
Name: "GOOGLE_OAUTH_REDIRECT_URI",
- Value: fmt.Sprintf("%s/oauth2callback", appConfig.BackendPublicURL),
+ Value: fmt.Sprintf("%s/oauth2callback", redirectBase),
})
+ } else if strings.TrimSpace(os.Getenv("GOOGLE_OAUTH_CLIENT_ID")) != "" &&
+ strings.TrimSpace(os.Getenv("GOOGLE_OAUTH_CLIENT_SECRET")) != "" {
+ log.Printf("Warning: BACKEND_PUBLIC_URL is empty; GOOGLE_OAUTH_REDIRECT_URI will not be set and workspace-mcp may default to localhost callback")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/operator/internal/handlers/sessions.go` around lines 1083 - 1088,
When the code decides not to inject GOOGLE_OAUTH_REDIRECT_URI because
appConfig.BackendPublicURL is empty, add a warning so misconfiguration is
visible: detect when Google OAuth client configuration is present (e.g.,
GOOGLE_OAUTH_CLIENT_ID and/or GOOGLE_OAUTH_CLIENT_SECRET in the environment or
the corresponding appConfig fields) and when appConfig.BackendPublicURL == ""
emit a warning log (use the same logger/context used in this handler) stating
that BACKEND_PUBLIC_URL is empty and redirect URI injection is skipped which may
cause redirect_uri_mismatch at runtime; keep the existing conditional that
appends GOOGLE_OAUTH_REDIRECT_URI to base unchanged.
The scheduledSessions plugin and ambient-sdk go-sdk referenced openapi.ScheduledSession, openapi.ScheduledSessionList, openapi.ScheduledSessionPatchRequest, and types.ScheduledSession / types.ScheduledSessionPatch / types.ScheduledSessionList which were never generated. This caused ambient-api-server and ambient-control-plane Docker builds to fail with "undefined" compile errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-sdk/go-sdk/types/scheduled_session.go`:
- Around line 90-104: The Build methods are returning pointers to internal
mutable builder fields (e.g., ScheduledSessionBuilder.Build returning
&b.resource), allowing later builder calls to mutate previously built objects;
change Build (and the analogous builder method that returns &b.patch) to return
a pointer to a copy instead of the internal field by creating a local copy of
b.resource (or b.patch) and returning its address so the builder's internal
state remains immutable to callers.
- Around line 46-49: ScheduledSessionBuilder currently keeps a persistent errs
slice on the struct which is appended across Build() calls; change validation to
use a local errs variable inside the Build() method (remove reliance on the
builder's errs field) so each Build() starts fresh. Locate
ScheduledSessionBuilder and its Build() method and: 1) make errs a local []error
in Build(); 2) update any validation helpers called by Build() (or their call
sites) to return errors (or append to the local errs) instead of mutating
b.errs; and 3) keep the resource population logic (resource field) as-is but
only return accumulated local errs from Build() so prior failures don't persist
across builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33f6ac0d-a342-4dc0-8a31-828fc50c33ad
⛔ Files ignored due to path filters (3)
components/ambient-api-server/pkg/api/openapi/model_scheduled_session.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_scheduled_session_list.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_scheduled_session_patch_request.gois excluded by!**/pkg/api/openapi/**
📒 Files selected for processing (1)
components/ambient-sdk/go-sdk/types/scheduled_session.go
Comment on lines
+46
to
+49
| type ScheduledSessionBuilder struct { | ||
| resource ScheduledSession | ||
| errs []error | ||
| } |
Contributor
There was a problem hiding this comment.
Builder validation errors persist across Build() calls
errs is stored on the builder and appended on each Build(). After one failed call, later calls can keep failing even when inputs are fixed (Line 100). Make validation errors local to Build().
Suggested fix
type ScheduledSessionBuilder struct {
resource ScheduledSession
- errs []error
}
@@
func (b *ScheduledSessionBuilder) Build() (*ScheduledSession, error) {
+ var errs []error
if b.resource.Name == "" {
- b.errs = append(b.errs, fmt.Errorf("name is required"))
+ errs = append(errs, fmt.Errorf("name is required"))
}
if b.resource.AgentID == "" {
- b.errs = append(b.errs, fmt.Errorf("agent_id is required"))
+ errs = append(errs, fmt.Errorf("agent_id is required"))
}
if b.resource.Schedule == "" {
- b.errs = append(b.errs, fmt.Errorf("schedule is required"))
+ errs = append(errs, fmt.Errorf("schedule is required"))
}
- if len(b.errs) > 0 {
- return nil, fmt.Errorf("validation failed: %w", errors.Join(b.errs...))
+ if len(errs) > 0 {
+ return nil, fmt.Errorf("validation failed: %w", errors.Join(errs...))
}
return &b.resource, nil
}As per coding guidelines, **/*: - Prioritize Critical and Major severity issues. Minimize Minor and Trivial findings.
Also applies to: 90-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-sdk/go-sdk/types/scheduled_session.go` around lines 46 -
49, ScheduledSessionBuilder currently keeps a persistent errs slice on the
struct which is appended across Build() calls; change validation to use a local
errs variable inside the Build() method (remove reliance on the builder's errs
field) so each Build() starts fresh. Locate ScheduledSessionBuilder and its
Build() method and: 1) make errs a local []error in Build(); 2) update any
validation helpers called by Build() (or their call sites) to return errors (or
append to the local errs) instead of mutating b.errs; and 3) keep the resource
population logic (resource field) as-is but only return accumulated local errs
from Build() so prior failures don't persist across builds.
Comment on lines
+90
to
+104
| func (b *ScheduledSessionBuilder) Build() (*ScheduledSession, error) { | ||
| if b.resource.Name == "" { | ||
| b.errs = append(b.errs, fmt.Errorf("name is required")) | ||
| } | ||
| if b.resource.AgentID == "" { | ||
| b.errs = append(b.errs, fmt.Errorf("agent_id is required")) | ||
| } | ||
| if b.resource.Schedule == "" { | ||
| b.errs = append(b.errs, fmt.Errorf("schedule is required")) | ||
| } | ||
| if len(b.errs) > 0 { | ||
| return nil, fmt.Errorf("validation failed: %w", errors.Join(b.errs...)) | ||
| } | ||
| return &b.resource, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Build() exposes mutable internal builder state
Both builders return pointers to internal fields (&b.resource, &b.patch). If the same builder is reused, later setter calls can mutate previously built objects (Lines 103 and 146). Return a copy instead.
Suggested fix
func (b *ScheduledSessionBuilder) Build() (*ScheduledSession, error) {
@@
- return &b.resource, nil
+ out := b.resource
+ return &out, nil
}
@@
func (b *ScheduledSessionPatchBuilder) Build() *ScheduledSessionPatch {
- return &b.patch
+ out := b.patch
+ return &out
}As per coding guidelines, **/*: - Prioritize Critical and Major severity issues. Minimize Minor and Trivial findings.
Also applies to: 145-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-sdk/go-sdk/types/scheduled_session.go` around lines 90 -
104, The Build methods are returning pointers to internal mutable builder fields
(e.g., ScheduledSessionBuilder.Build returning &b.resource), allowing later
builder calls to mutate previously built objects; change Build (and the
analogous builder method that returns &b.patch) to return a pointer to a copy
instead of the internal field by creating a local copy of b.resource (or
b.patch) and returning its address so the builder's internal state remains
immutable to callers.
Contributor
Merge Queue Status
This pull request spent 13 seconds in the queue, including 1 second running CI. Required conditions to merge |
Contributor
|
| File | Component | Mode |
|---|---|---|
components/runners/ambient-runner/.mcp.json |
runner | warn |
No action required — these components are in warn mode. Consider using the component's agent workflow for future changes.
📖 Specs: Runner Spec · Runner Constitution
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
workspace-mcpdefaults tohttp://localhost:8000/oauth2callbackfor its OAuth redirect URI whenGOOGLE_OAUTH_REDIRECT_URIis not set. Google rejects this withredirect_uri_mismatchbecauselocalhost:8000is not registered in the Google Cloud Console OAuth app.GOOGLE_OAUTH_REDIRECT_URIconfiguration, leavingworkspace-mcpto fall back to itslocalhost:8000default when credentials are absent or expired.GOOGLE_OAUTH_REDIRECT_URI=${BACKEND_PUBLIC_URL}/oauth2callback, which points to the already-registered and already-handled/oauth2callbackendpoint inhandlers/oauth.go.Changes
operator/internal/config/config.goBackendPublicURLfield; read fromBACKEND_PUBLIC_URLenv varoperator/internal/handlers/sessions.goGOOGLE_OAUTH_REDIRECT_URIinto runner pod envmanifests/base/core/operator-deployment.yamlBACKEND_PUBLIC_URLfromgoogle-workflow-app-secretkeyBACKEND_URL(optional) — same secret already used for OAuth client credentialsrunners/ambient-runner/.mcp.jsonGOOGLE_OAUTH_REDIRECT_URIinto theworkspace-mcpprocess envAdmin action required
No new secrets are needed. The
BACKEND_URLkey already exists ingoogle-workflow-app-secret(it's sourced by the backend deployment). The operator will now read it too — no changes needed if that secret is already populated.If
BACKEND_PUBLIC_URLis not set, the conditional block insessions.gois a no-op and behavior is unchanged (degrades gracefully).Test plan
BACKEND_PUBLIC_URLredirect_uri_mismatcherror from Googlelocalhost:8000go build ./...passes incomponents/operator/🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements