fix(gateway): gate agent API handlers with ownership check to prevent cross-tenant access#285
Merged
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7e775e322
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+602
to
+604
| const settingsSession = verifySettingsSession(c); | ||
| if (settingsSession) { | ||
| const access = await verifyOwnedAgentAccess( |
There was a problem hiding this comment.
requireAgentOwnership returns immediately when a settings session cookie is present, so a valid admin bearer token is never evaluated in that case. This breaks the documented “admin password bypass” behavior for browser requests that carry both Authorization: Bearer <admin> and a non-admin lobu_settings_session cookie, causing legitimate cross-tenant admin actions to return 403 instead of bypassing ownership checks.
Useful? React with 👍 / 👎.
buremba
added a commit
that referenced
this pull request
Apr 21, 2026
Per Codex review on #285: if a caller sends both a valid admin bearer token AND a non-admin settings-session cookie, the settings-session short-circuit used to win, making 'admin password bypass' silently fail for those requests. Re-order so admin-bearer is evaluated first.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e70b0c033f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+607
to
+611
| const settingsSession = verifySettingsSession(c); | ||
| if (settingsSession) { | ||
| const access = await verifyOwnedAgentAccess( | ||
| settingsSession, | ||
| resolvedAgentId, |
There was a problem hiding this comment.
When a request carries both a settings-session cookie and an Authorization bearer token, this branch authorizes using the cookie identity first and never evaluates the bearer identity. That regresses valid bearer flows (worker/CLI/external), because a stale or unrelated cookie can force a 403 even though the bearer token is valid for the target agent; this is especially visible for ephemeral sessions that have no ownership metadata for the cookie principal. Evaluate bearer-based principals first when a bearer token is present, and only fall back to settings-session ownership when no bearer auth is supplied.
Useful? React with 👍 / 👎.
… cross-tenant access Every agent API handler (create/get/delete/events/messages) now verifies the caller owns the resolved agentId via requireAgentOwnership, which accepts the full matrix of auth methods used by the route middleware: - admin password → full access (checked BEFORE any cookie) - worker token → scoped to its own agentId - settings session → verifyOwnedAgentAccess - CLI JWT / external → synthesized external-platform payload → verifyOwnedAgentAccess Path params are usually sessionKeys, so each handler resolves session.agentId before calling requireAgentOwnership. Admin bearer is evaluated before the settings-session short-circuit so a caller holding both a valid admin token AND a non-admin cookie cannot be silently downgraded.
buremba
force-pushed
the
fix/agent-api-cross-tenant
branch
from
e70b0c0 to
5f586f7
Compare
This was referenced Apr 21, 2026
6 tasks
buremba
pushed a commit
that referenced
this pull request
Apr 23, 2026
Second pass on the 2-week PR review. Five more gaps closed: - gateway: unit tests for verifyOwnedAgentAccess covering owner, cross-tenant, cross-platform, agent-scoped, admin-bypass, unknown-agent, and external OAuth mismatches (#285 follow-up). Closes the test hole in the cross-tenant ownership guard. - owletto-backend: validate each CSP frame-ancestor entry against a strict host-source / scheme-source grammar before joining (#246 follow-up). Malformed env entries like `https:// lobu.ai` are now dropped instead of silently rendered into the directive. - owletto-backend: introduce normalizeHost() in utils/public-origin and use it from getSubdomainZone, extractSubdomainOrg, getCanonicalRedirectUrl, and the BetterAuth trustedOrigins wiring (#234/#224/#214 follow-up). Unifies the ad-hoc .toLowerCase()/.replace() patterns and adds IDN→punycode so `müller.lobu.ai` matches the ASCII zone configured in env. - owletto-backend: redact member emails that surface via template_data and tab template_data in resolve_path, not only on the single resolved entity (#309 follow-up). A dashboard data source that enumerates $member entities no longer leaks emails to non-admin callers. New utils/member-redaction helper plus unit coverage. - owletto-backend: treat #311 as already closed — ToolContext.memberRole is `string | null` (required, not optional), so TypeScript already catches future literal omissions at construction.
buremba
pushed a commit
that referenced
this pull request
Apr 23, 2026
Second pass on the 2-week PR review. Five more gaps closed: - gateway: unit tests for verifyOwnedAgentAccess covering owner, cross-tenant, cross-platform, agent-scoped, admin-bypass, unknown-agent, and external OAuth mismatches (#285 follow-up). Closes the test hole in the cross-tenant ownership guard. - owletto-backend: validate each CSP frame-ancestor entry against a strict host-source / scheme-source grammar before joining (#246 follow-up). Malformed env entries like `https:// lobu.ai` are now dropped instead of silently rendered into the directive. - owletto-backend: introduce normalizeHost() in utils/public-origin and use it from getSubdomainZone, extractSubdomainOrg, getCanonicalRedirectUrl, and the BetterAuth trustedOrigins wiring (#234/#224/#214 follow-up). Unifies the ad-hoc .toLowerCase()/.replace() patterns and adds IDN→punycode so `müller.lobu.ai` matches the ASCII zone configured in env. - owletto-backend: redact member emails that surface via template_data and tab template_data in resolve_path, not only on the single resolved entity (#309 follow-up). A dashboard data source that enumerates $member entities no longer leaks emails to non-admin callers. New utils/member-redaction helper plus unit coverage. - owletto-backend: treat #311 as already closed — ToolContext.memberRole is `string | null` (required, not optional), so TypeScript already catches future literal omissions at construction.
buremba
added a commit
that referenced
this pull request
Apr 23, 2026
…instr guard, MCP join rate-limit) (#325) * fix: address gaps found in post-merge review of last 2 weeks of PRs Follow-ups from an aggregated teammate review of 128 PRs merged between 2026-04-09 and 2026-04-23. Five concrete gaps patched: - worker: constrain UploadUserFile to the workspace root (#203 follow-up). path.join allowed `../` and absolute paths to escape the workspace. Now resolves and rejects anything outside workspaceDir when one is set. - core: flip Sentry sendDefaultPii to false (#172 follow-up). User content and identifiers flow through this stack; the schema has no scrubbing so PII-by-default was unsafe. - gateway: make SlackInstructionProvider extend BaseInstructionProvider (#269 follow-up). Sibling Skills/Network providers are wrapped in a try/catch that returns "" on error; Slack was bypassing it and would crash session-context assembly if listConnections threw. - owletto-backend: rate-limit the join_organization MCP tool to match the REST endpoint (#296 follow-up). Keyed on userId since MCP calls don't carry a client IP. Skipped one reviewer finding: removing the process.env fallback for API keys at worker.ts:1099/1109 (the inconsistency with #225 base-URL code). Embedded/dev workers depend on that fallback since credentialStore is only populated from gateway-supplied session context. * fix: address remaining gaps from post-merge review Second pass on the 2-week PR review. Five more gaps closed: - gateway: unit tests for verifyOwnedAgentAccess covering owner, cross-tenant, cross-platform, agent-scoped, admin-bypass, unknown-agent, and external OAuth mismatches (#285 follow-up). Closes the test hole in the cross-tenant ownership guard. - owletto-backend: validate each CSP frame-ancestor entry against a strict host-source / scheme-source grammar before joining (#246 follow-up). Malformed env entries like `https:// lobu.ai` are now dropped instead of silently rendered into the directive. - owletto-backend: introduce normalizeHost() in utils/public-origin and use it from getSubdomainZone, extractSubdomainOrg, getCanonicalRedirectUrl, and the BetterAuth trustedOrigins wiring (#234/#224/#214 follow-up). Unifies the ad-hoc .toLowerCase()/.replace() patterns and adds IDN→punycode so `müller.lobu.ai` matches the ASCII zone configured in env. - owletto-backend: redact member emails that surface via template_data and tab template_data in resolve_path, not only on the single resolved entity (#309 follow-up). A dashboard data source that enumerates $member entities no longer leaks emails to non-admin callers. New utils/member-redaction helper plus unit coverage. - owletto-backend: treat #311 as already closed — ToolContext.memberRole is `string | null` (required, not optional), so TypeScript already catches future literal omissions at construction. --------- Co-authored-by: Claude <noreply@anthropic.com>
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
The
createAgentApifactory inpackages/gateway/src/routes/public/agent.tsregistered multiple route handlers that accepted anagentIdpath parameter (really asessionKey/conversationId) but never verified that the authenticated caller owned the underlying agent. A signed-in user could read, modify, stream, or send messages against another tenant's agent by swapping the id in the URL.This change wires the existing
verifyOwnedAgentAccesshelper into everyagentId-bearing handler in the file. A new in-filerequireAgentOwnershiphelper handles the five auth methods the agent-API middleware accepts:verifyOwnedAgentAccessexternal-platform settings payload, thenverifyOwnedAgentAccessfetchUserInfoto resolvesubFor handlers where the path param is a composite session key, we resolve the real
agentIdviasessionManager.getSession(...)before the ownership check so lookups run against the actual owning agent. The DELETE handler reuses the session fetch it needs anyway.Handler audit
POST /api/v1/agents(create)agentId. Ephemeral creates (auto-generated UUID) stay open so unauthenticated-style create flows keep working.GET /api/v1/agents/{agentId}(status)requireAgentOwnership.DELETE /api/v1/agents/{agentId}GET /api/v1/agents/{agentId}/events(SSE)connectedevent, the SSE backlog replay at line ~1031 (the issue the task specifically called out), and the heartbeat loop.POST /api/v1/agents/{agentId}/messagesPOST /api/v1/agents/approveagentIdpath parameter. Authorization is carried by therequestIdpending-tool secret that is atomicallyGETDEL'd from Redis (pending-tool:{requestId}). That per-request secret is the intended capability check. Not modified.Wiring
AgentApiConfiggrew two optional fields,userAgentsStoreandagentMetadataStore, populated inpackages/gateway/src/cli/gateway.tsfromcoreServices.getUserAgentsStore()/getAgentMetadataStore(). The existingagentConfigStore.getMetadatais used as a fallback metadata source when no dedicatedAgentMetadataStoreis provided.Test plan
make build-packages- cleanbun run typecheck- cleanbun test packages/gateway/src/__tests__- 488 pass / 0 failGET /api/v1/agents/{agent-B-session}-> 403