feat(connect): managed connectors via public org — cloud auth, local data#1038
Merged
Conversation
Nango-style managed OAuth, Lobu-native. An oauth_app profile can encode a
`__broker` reference ({url, org, pat}) instead of client_id/secret. When set,
the local instance delegates the secret-requiring OAuth steps to a remote Lobu
"broker" over HTTP, authenticating with a Lobu PAT (Bearer). The broker holds
the real provider app credentials, performs build-authorize-url / exchange /
refresh on the caller's behalf, and returns ONLY the user's tokens — the
client_secret never leaves the broker.
- getBrokerRef() helper + __broker preservation in normalizeAuthData (no DB
migration; reuses profile_kind=oauth_app)
- POST /broker/oauth/{authorize-url,exchange,refresh} — PAT-gated router
- local delegation hooks in the connect start + callback paths
SPIKE — not for merge.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements OAuth broker delegation, enabling distributed instances to delegate managed OAuth credentials to a broker via a PAT-gated endpoint. It adds broker-reference auth profile support, shared PAT authentication primitives, broker HTTP routes, gateway integration, runtime token resolution, and comprehensive integration tests. ChangesOAuth Broker Delegation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Member
Author
|
bug_free 86, simplicity 78, slop 0, bugs 0, 0 blockers Suites passed. Booted server with embedded file DB; /api/health returned 200 and POST /oauth/connection-token without PAT returned 401. [env] initial exploratory boot without DATABASE_URL failed, so retried with file:// DB. New PAT-gated token endpoint is security-sensitive but has strong integration coverage. Full verdict JSON{
"bug_free_confidence": 86,
"bugs": 0,
"slop": 0,
"simplicity": 78,
"blockers": [],
"change_type": "feat",
"behavior_change_risk": "high",
"tests_adequate": true,
"suggested_fixes": [],
"notes": "Suites passed. Booted server with embedded file DB; /api/health returned 200 and POST /oauth/connection-token without PAT returned 401. [env] initial exploratory boot without DATABASE_URL failed, so retried with file:// DB. New PAT-gated token endpoint is security-sensitive but has strong integration coverage.",
"categories": {
"src": 888,
"tests": 1133,
"docs": 0,
"config": 0,
"deps": 0,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts (1)
286-292: 💤 Low valueConsider simplifying type assertions.
The double-casting pattern (
as unknown as Array<...>) appears in several SQL queries. While functional, you could define explicit types for your query results to improve readability.♻️ Optional: Define explicit result types
type AuthProfileRow = { auth_data: Record<string, unknown> }; // Then use: const stored = await sql<AuthProfileRow[]>` SELECT auth_data FROM auth_profiles WHERE id = ${brokerProfile.id} `;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts` around lines 286 - 292, The test uses double-casting for SQL results (e.g., const stored = (await sql`...`) as unknown as Array<{ auth_data: Record<string, unknown> }>) which is verbose; define an explicit row type (e.g., type AuthProfileRow = { auth_data: Record<string, unknown> }) near the test and use the sql generic to type the result (const stored = await sql<AuthProfileRow[]>`SELECT auth_data ...`) so you can remove the double-cast and simplify subsequent assertions that reference stored[0].auth_data and its __broker property.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/connect/broker-routes.ts`:
- Around line 179-180: Requests currently accept caller-controlled token_url and
use it to send broker-managed OAuth client credentials (token_url and
token_endpoint_auth_method are present); this allows exfiltration. Change the
code so that any token exchange or refresh (including the /oauth/refresh flow)
never uses token_url from the request body: instead resolve the token endpoint
from trusted provider metadata (e.g., the provider's well-known/openid
configuration) or validate against a strict allowlist of approved token
endpoints before sending client credentials. Locate all usages of token_url and
token_endpoint_auth_method in the broker route handlers (the token exchange and
refresh handlers) and replace them with the resolved/validated token endpoint,
rejecting requests that attempt to override the endpoint.
In `@packages/server/src/connect/routes.ts`:
- Around line 552-557: The PKCE verifier is created unconditionally into
pkceCodeVerifier but may not be persisted, causing a mismatch between the
challenge sent and the verifier used during callback; change the flow so that
buildPkceVerifier() is only called when you will persist it (i.e. when
needsUpdate is true) or immediately assign the generated verifier into
authConfig.pkceCodeVerifier before sending the challenge; update the logic
around authConfig.usePkce, pkceCodeVerifier, needsUpdate and the code that sends
the challenge (the block that sends the challenge at the later lines) to ensure
the exact same verifier stored on authConfig is used for both sending the
challenge and for callback exchange.
- Around line 1041-1049: The POST to `${broker.url}/broker/oauth/${endpoint}`
uses fetch without a timeout; wrap the fetch in an AbortController, pass
controller.signal to fetch (the call in routes.ts where the fetch uses
broker.url, broker.pat and endpoint), start a setTimeout that calls
controller.abort() after a sensible timeout (e.g. configurable constant), and
clear the timeout when fetch resolves; also catch the abort error (check for
error.name === 'AbortError') and convert it to a controlled timeout
error/response so the connect flow can fail fast instead of hanging
indefinitely.
---
Nitpick comments:
In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts`:
- Around line 286-292: The test uses double-casting for SQL results (e.g., const
stored = (await sql`...`) as unknown as Array<{ auth_data: Record<string,
unknown> }>) which is verbose; define an explicit row type (e.g., type
AuthProfileRow = { auth_data: Record<string, unknown> }) near the test and use
the sql generic to type the result (const stored = await
sql<AuthProfileRow[]>`SELECT auth_data ...`) so you can remove the double-cast
and simplify subsequent assertions that reference stored[0].auth_data and its
__broker property.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f1d01cc-0616-4000-9de0-a110fd61a992
📒 Files selected for processing (6)
packages/server/src/__tests__/integration/connectors/oauth-broker.test.tspackages/server/src/connect/broker-routes.tspackages/server/src/connect/routes.tspackages/server/src/http/spa-route-filter.tspackages/server/src/index.tspackages/server/src/utils/auth-profiles.ts
…rifier reuse Security blocker: the broker exchange/refresh used caller-supplied token_url / authorization_url, so any valid PAT could redirect the broker's client_secret to an attacker endpoint. Now the broker resolves authorizationUrl / tokenUrl / tokenEndpointAuthMethod (+ client id/secret key names) SERVER-SIDE from its OWN org's connector_definitions auth_schema for the given connector_key/provider, and rejects (400) connectors it doesn't manage. token_url / authorization_url removed from all three endpoint request bodies; local delegation caller updated to match. PKCE: /connect/:token/oauth/start regenerated the verifier on every call, so a repeat start built the challenge from a verifier the callback never sees (the persisted one is unchanged when needsUpdate is false). Reuse the persisted verifier (authConfig.pkceCodeVerifier ?? buildPkceVerifier()) so start/callback stay consistent; fixed in both the broker branch and the pre-existing local path. Test: + caller-supplied token_url is ignored (attacker endpoint gets 0 hits) + unknown-connector 400; all prior assertions kept. 7 passed.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts (3)
340-364: ⚡ Quick winUse the seeded broker org slug instead of a hardcoded value.
At Line 362,
'broker-org'couples the test to slug-generation internals. Use the org returned byseedBrokerOrg()to keep this test robust.Proposed change
- const { pat } = await seedBrokerOrg(); + const { pat, org } = await seedBrokerOrg(); ... - __broker: { url: brokerBaseUrl, org: 'broker-org', pat: pat.token }, + __broker: { url: brokerBaseUrl, org: org.slug, pat: pat.token },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts` around lines 340 - 364, Replace the hardcoded broker org slug used when creating the broker-backed auth profile with the actual slug returned by seedBrokerOrg(); specifically, use the org object/value from the await seedBrokerOrg() call (the same place you destructure pat) when building authData.__broker.org for createAuthProfile so the test uses the seeded organization's slug (alongside brokerBaseUrl and pat.token) instead of the literal 'broker-org'.
301-328: ⚡ Quick winAdd a malicious
authorization_urloverride assertion for/broker/oauth/authorize-url.You already guard against
token_urloverride in exchange. Add the same regression guard here by including caller-suppliedauthorization_urland asserting broker-owned metadata still wins.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts` around lines 301 - 328, Update the test "authorize-url builds the provider consent URL..." to include a caller-supplied authorization_url override in the POST body (e.g., "http://malicious.example/authorize") and assert that the broker-owned connector metadata still wins: after fetching (res -> body -> url) assert `${url.origin}${url.pathname}` equals the broker endpoint ('https://demo.example/authorize') and that search params like client_id remain 'broker-cid' and state is 'state-xyz', ensuring the request's authorization_url override is ignored.
167-329: ⚡ Quick winAdd parity coverage for
/broker/oauth/refreshendpoint ownership.This PR’s security hardening includes refresh path endpoint resolution too. A refresh integration test with a malicious caller-supplied
token_urlwould lock in that guarantee and catch regressions early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts` around lines 167 - 329, Add an integration test for the broker-owned refresh flow: mirror the existing exchange/token_url test but POST to '/broker/oauth/refresh' using buildBrokerApp() and a valid PAT from seedBrokerOrg(), send body with connector_key:'demo.oauth', provider:'demo', refresh_token:CANNED.refresh_token plus a malicious token_url:attackerTokenUrl and any client_secret_key/token_endpoint_auth_method fields, then assert res.status is 200, response contains the expected CANNED access_token/refresh_token and does not leak secrets, and assert attackerHits === 0 to ensure the broker resolved and called its own token endpoint rather than the supplied token_url.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts`:
- Around line 340-364: Replace the hardcoded broker org slug used when creating
the broker-backed auth profile with the actual slug returned by seedBrokerOrg();
specifically, use the org object/value from the await seedBrokerOrg() call (the
same place you destructure pat) when building authData.__broker.org for
createAuthProfile so the test uses the seeded organization's slug (alongside
brokerBaseUrl and pat.token) instead of the literal 'broker-org'.
- Around line 301-328: Update the test "authorize-url builds the provider
consent URL..." to include a caller-supplied authorization_url override in the
POST body (e.g., "http://malicious.example/authorize") and assert that the
broker-owned connector metadata still wins: after fetching (res -> body -> url)
assert `${url.origin}${url.pathname}` equals the broker endpoint
('https://demo.example/authorize') and that search params like client_id remain
'broker-cid' and state is 'state-xyz', ensuring the request's authorization_url
override is ignored.
- Around line 167-329: Add an integration test for the broker-owned refresh
flow: mirror the existing exchange/token_url test but POST to
'/broker/oauth/refresh' using buildBrokerApp() and a valid PAT from
seedBrokerOrg(), send body with connector_key:'demo.oauth', provider:'demo',
refresh_token:CANNED.refresh_token plus a malicious token_url:attackerTokenUrl
and any client_secret_key/token_endpoint_auth_method fields, then assert
res.status is 200, response contains the expected CANNED
access_token/refresh_token and does not leak secrets, and assert attackerHits
=== 0 to ensure the broker resolved and called its own token endpoint rather
than the supplied token_url.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1389f292-bbea-4a4a-9166-16fb6590dc5f
📒 Files selected for processing (3)
packages/server/src/__tests__/integration/connectors/oauth-broker.test.tspackages/server/src/connect/broker-routes.tspackages/server/src/connect/routes.ts
Lean the OAuth broker delegation (behavior-preserving; grant stays local):
1. Dedup PAT auth: extract shared authenticatePat + extractPatBearer into
auth/pat-auth.ts; broker router AND createLobuAuthBridge now use the one
implementation (gateway.ts inline PAT block ~120 -> ~33 lines).
2. CredentialSource seam: replace scattered getBrokerRef branches in the
connect start + callback paths with one resolveCredentialSource(...) returning
{kind:'local'|'broker'}; each path branches once on source.kind.
3. Dedup provider/credential resolution: factor resolveConnectorOAuthMethod +
resolveOAuthClientCredentials into connect/oauth-resolution.ts; broker reuses
the existing connector-auth helpers instead of a parallel query, and routes.ts
drops its private copy.
4. Runtime validation: broker endpoints validate bodies via @sinclair/typebox
TypeCompiler (matching repo pattern) and return 400 on malformed/missing
fields instead of casting; added a malformed-body -> 400 test (now 8 tests).
Server-side endpoint resolution preserved (no caller-supplied token_url).
broker-routes.ts 404 -> 315 lines; all 8 broker tests + 14 gateway-auth tests green.
buremba
force-pushed
the
feat/oauth-broker-spike
branch
from
a3a7c14 to
fd735aa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts (1)
461-463: 💤 Low valueConsider adding
attackerHitsassertion to strengthen end-to-end security coverage.The exchange handshake test at line 249 explicitly verifies
attackerHits === 0to prove the secret wasn't redirected. This end-to-end test exercises the same broker exchange path but doesn't include that assertion.🛡️ Suggested addition after token assertions
expect(accounts[0].accessToken).toBe(CANNED.access_token); expect(accounts[0].refreshToken).toBe(CANNED.refresh_token); + + // Broker exchange must not have hit the attacker endpoint. + expect(attackerHits).toBe(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts` around lines 461 - 463, Add an assertion that attackerHits remains 0 after the broker exchange to mirror the handshake test: after the existing expectations on accounts, assert attackerHits === 0 (use the same attackerHits variable used in the other test) to ensure the secret wasn't redirected in this end-to-end exchange; locate this in the oauth-broker.test.ts around the block validating accounts and append the attackerHits check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/server/src/__tests__/integration/connectors/oauth-broker.test.ts`:
- Around line 461-463: Add an assertion that attackerHits remains 0 after the
broker exchange to mirror the handshake test: after the existing expectations on
accounts, assert attackerHits === 0 (use the same attackerHits variable used in
the other test) to ensure the secret wasn't redirected in this end-to-end
exchange; locate this in the oauth-broker.test.ts around the block validating
accounts and append the attackerHits check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b727ae8-2d42-4a14-bccc-94db200d2bf1
📒 Files selected for processing (6)
packages/server/src/__tests__/integration/connectors/oauth-broker.test.tspackages/server/src/auth/pat-auth.tspackages/server/src/connect/broker-routes.tspackages/server/src/connect/oauth-resolution.tspackages/server/src/connect/routes.tspackages/server/src/lobu/gateway.ts
Rewrite the OAuth broker spike from grant-local (the local instance ran the
flow and delegated authorize-url/exchange/refresh to the broker per-step) to
grant-on-broker: the broker OWNS the grant (managed app creds + oauth_account)
and runs consent/refresh through its EXISTING /connect flow + CredentialService.
The local instance only fetches a fresh access token at runtime.
- broker-routes.ts: replace 3 delegation endpoints (authorize-url/exchange/
refresh) with one PAT-gated POST /broker/oauth/token; org-scoped (cross-org
connection -> 403); returns only { access_token, expires_at } via the existing
resolveExecutionAuth path; never the refresh token or client secret.
- execution-context.ts: runtime hook — a broker-backed connection (oauth_app
auth_data has __broker) fetches its access token from the broker; the local
(non-broker) path is unchanged.
- routes.ts: restored to main (broker branch in /oauth/start + callback removed).
- oauth-resolution.ts: deleted (now unused).
- auth-profiles.ts: BrokerRef gains connection_id (the broker-side connection).
- pat-auth.ts + gateway.ts: shared authenticatePat retained (used by both).
Production insertions vs main: 826 -> 484 (~41% smaller); broker-routes 315->160.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/utils/auth-profiles.ts (1)
138-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop local credential fields when
__brokeris present.This merge keeps every normalized string key alongside
__broker, so a broker-backedoauth_appcan still persistclient_id/client_secretlocally. That breaks the broker model’s core guarantee that secret-bearing OAuth data stays on the broker.Suggested fix
if (profileKind === 'oauth_app') { // A broker-ref oauth_app carries a `__broker` object instead of string // client credentials. normalizeAuthValues() would strip the object, so // preserve it explicitly while still normalizing any sibling string keys. const broker = getBrokerRef(raw); const normalized = normalizeAuthValues(raw); - return broker ? { ...normalized, __broker: broker } : normalized; + return broker ? { __broker: broker } : normalized; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/utils/auth-profiles.ts` around lines 138 - 140, When a broker is present (broker = getBrokerRef(raw)), drop local secret-bearing credential fields from the normalized auth object so broker-backed providers don't keep secrets locally: in the code that currently builds and returns "{ ...normalized, __broker: broker }", remove keys like client_id and client_secret (and any token-bearing fields your code treats as secrets such as access_token and refresh_token) from normalized before merging. Concretely, after calling normalizeAuthValues(raw) and obtaining normalized, create a sanitized version that omits those secret keys (e.g., via destructuring or a small omit helper) and then return the sanitized object with __broker when broker is truthy; otherwise return normalized unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/utils/execution-context.ts`:
- Around line 160-167: The broker token fetch call (the fetch to
`${broker.url}/broker/oauth/token` using broker.pat and broker.connection_id)
needs a request timeout to avoid hanging; wrap the fetch in an AbortController,
start a timer (e.g., configurable default like 5s via a constant or env var)
that calls controller.abort() on expiry, pass signal: controller.signal to
fetch, and clear the timer on success or error so the request is properly
cleaned up; ensure any AbortError is handled/mapped to a clear timeout error
upstream in the execution context logic.
- Around line 51-71: The early return in the getBrokerRef/fetchBrokerAccessToken
branch discards existing connectionCredentials, so update the logic around
getBrokerRef and fetchBrokerAccessToken to merge and preserve
connectionCredentials instead of returning immediately: after building
credentials (in the block that sets
provider/accessToken/refreshToken/expiresAt), construct connectionCredentials by
shallow-merging params.connectionCredentials with any non-secret fields from
appAuthProfile (e.g., oauth_app fields and env-profile values) so env-profile
values and sibling non-secret fields are retained; then return the same shaped
object (credentials, connectionCredentials, sessionState, browserUserDataDir)
without dropping existing connectionCredentials. Ensure you reference
getBrokerRef, fetchBrokerAccessToken, credentials, and
params.connectionCredentials when implementing the merge.
---
Outside diff comments:
In `@packages/server/src/utils/auth-profiles.ts`:
- Around line 138-140: When a broker is present (broker = getBrokerRef(raw)),
drop local secret-bearing credential fields from the normalized auth object so
broker-backed providers don't keep secrets locally: in the code that currently
builds and returns "{ ...normalized, __broker: broker }", remove keys like
client_id and client_secret (and any token-bearing fields your code treats as
secrets such as access_token and refresh_token) from normalized before merging.
Concretely, after calling normalizeAuthValues(raw) and obtaining normalized,
create a sanitized version that omits those secret keys (e.g., via destructuring
or a small omit helper) and then return the sanitized object with __broker when
broker is truthy; otherwise return normalized 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed5f5535-b6bf-415b-9b7d-dd072a305f4f
📒 Files selected for processing (5)
packages/server/src/__tests__/integration/connectors/oauth-broker.test.tspackages/server/src/connect/broker-routes.tspackages/server/src/index.tspackages/server/src/utils/auth-profiles.tspackages/server/src/utils/execution-context.ts
Comment on lines
+51
to
+71
| const broker = getBrokerRef(appAuthProfile?.auth_data ?? null); | ||
| if (broker) { | ||
| const accessToken = await fetchBrokerAccessToken(broker, { | ||
| ...params.logContext, | ||
| connection_id: params.connectionId, | ||
| }); | ||
| if (accessToken) { | ||
| credentials = { | ||
| provider: appAuthProfile?.provider ?? 'broker', | ||
| accessToken: accessToken.access_token, | ||
| refreshToken: null, | ||
| expiresAt: accessToken.expires_at ?? null, | ||
| scope: null, | ||
| }; | ||
| } | ||
| return { | ||
| credentials, | ||
| connectionCredentials: {}, | ||
| sessionState: null, | ||
| browserUserDataDir: null, | ||
| }; |
There was a problem hiding this comment.
Preserve existing connection credentials for broker-backed runs.
This early return skips the normal connectionCredentials merge, so any env-profile values or non-secret sibling fields on the oauth_app disappear as soon as __broker is present. That changes connector inputs for broker-backed runs.
Suggested fix
let credentials: ExecutionOAuthCredentials | null = null;
+ const connectionCredentials = {
+ ...normalizeAuthValues(appAuthProfile?.auth_data ?? {}),
+ ...normalizeAuthValues(
+ authProfile?.profile_kind === 'env' ? (authProfile.auth_data ?? {}) : {}
+ ),
+ };
// Broker-backed connection: the grant lives on a REMOTE Lobu "broker", which
// holds the managed client_id/secret + the user's refresh token. Fetch a
// fresh access token at runtime instead of reading/refreshing a local grant. ONLY this branch changes for broker-backed connections; the
// local (non-broker) path below is unchanged.
const broker = getBrokerRef(appAuthProfile?.auth_data ?? null);
if (broker) {
const accessToken = await fetchBrokerAccessToken(broker, {
...params.logContext,
connection_id: params.connectionId,
});
if (accessToken) {
credentials = {
provider: appAuthProfile?.provider ?? 'broker',
accessToken: accessToken.access_token,
refreshToken: null,
expiresAt: accessToken.expires_at ?? null,
scope: null,
};
}
return {
credentials,
- connectionCredentials: {},
+ connectionCredentials,
sessionState: null,
browserUserDataDir: null,
};
}
-
- const connectionCredentials = {
- ...normalizeAuthValues(appAuthProfile?.auth_data ?? {}),
- ...normalizeAuthValues(
- authProfile?.profile_kind === 'env' ? (authProfile.auth_data ?? {}) : {}
- ),
- };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/utils/execution-context.ts` around lines 51 - 71, The
early return in the getBrokerRef/fetchBrokerAccessToken branch discards existing
connectionCredentials, so update the logic around getBrokerRef and
fetchBrokerAccessToken to merge and preserve connectionCredentials instead of
returning immediately: after building credentials (in the block that sets
provider/accessToken/refreshToken/expiresAt), construct connectionCredentials by
shallow-merging params.connectionCredentials with any non-secret fields from
appAuthProfile (e.g., oauth_app fields and env-profile values) so env-profile
values and sibling non-secret fields are retained; then return the same shaped
object (credentials, connectionCredentials, sessionState, browserUserDataDir)
without dropping existing connectionCredentials. Ensure you reference
getBrokerRef, fetchBrokerAccessToken, credentials, and
params.connectionCredentials when implementing the merge.
Comment on lines
+160
to
+167
| const response = await fetch(`${broker.url}/broker/oauth/token`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${broker.pat}`, | ||
| }, | ||
| body: JSON.stringify({ connection_id: broker.connection_id }), | ||
| }); |
There was a problem hiding this comment.
Add a timeout to the broker token fetch.
This is a new cross-instance network hop on the credential-resolution path. Without a timeout, a slow or blackholed broker can hold the caller open until the platform-level timeout, which is an avoidable reliability risk here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/utils/execution-context.ts` around lines 160 - 167, The
broker token fetch call (the fetch to `${broker.url}/broker/oauth/token` using
broker.pat and broker.connection_id) needs a request timeout to avoid hanging;
wrap the fetch in an AbortController, start a timer (e.g., configurable default
like 5s via a constant or env var) that calls controller.abort() on expiry, pass
signal: controller.signal to fetch, and clear the timer on success or error so
the request is properly cleaned up; ensure any AbortError is handled/mapped to a
clear timeout error upstream in the execution context logic.
Org membership alone authorized any member PAT to fetch raw OAuth access tokens for any connection in the org (token exfiltration by a low-privilege member). Enforce least-privilege via a dedicated PAT scope instead of an admin role (the runtime fetch happens in automated/worker runs, so an admin *user* would be wrong). - scopes.ts: add `broker:token` to AVAILABLE_SCOPES (NOT in DEFAULT_SCOPES, so a broad mcp:* PAT never carries it). - pat-auth.ts: authenticatePat now also returns the PAT's `scopes`. - broker-routes.ts: the /oauth/* gate rejects (403, insufficient_scope) any PAT lacking `broker:token` — in ADDITION to the existing org-scope 403. - execution-context.ts: document that the __broker.pat must be minted with `broker:token` (lobu token create --scope broker:token). - test-fixtures.ts: createTestPAT accepts an optional scope. - oauth-broker.test.ts: happy paths mint the PAT with broker:token; new case — a valid same-org PAT WITHOUT broker:token -> 403. gateway.ts path unchanged (it ignores the new scopes field).
Replace the __broker magic-key hack (a reserved __-prefixed object smuggled into an oauth_app profile's flat credential blob) with a real profile_kind = 'oauth_broker' in auth_profiles. The whole profile IS the broker descriptor; its auth_data holds typed, named fields (broker_url / broker_org / broker_connection_id) plus the PAT (broker_pat). No __-prefixed keys anywhere. - Migration adds 'oauth_broker' to the auth_profiles.profile_kind CHECK (DROP + re-ADD, with a reversible down). Applies up + down clean on a fresh DB. - Typed CredentialSource + resolveCredentialSource accessor: loads the connection's app_auth_profile and branches on profile_kind (oauth_broker -> broker, oauth_app -> local). Readers never sniff keys. - execution-context resolves broker creds via parseBrokerCredential gated on profile_kind === 'oauth_broker'; the non-broker token path is unchanged. - manage_auth_profiles validates broker fields on create + update for oauth_broker (rejects missing broker_url/org/pat/connection_id). - Connection linkage reuses the existing app_auth_profile_id FK: a broker-backed connection points it at an oauth_broker profile. - Deleted getBrokerRef / BrokerRef and every __broker code path. Security properties intact: PAT auth + broker:token scope check + org-scope 403 on /broker/oauth/token; runtime hook only affects broker-backed connections; no secret leaves the broker.
…CredentialSource seam
BLOCKER: resolveConnectionAuthSelection early-returned before honoring an
explicit oauth_broker app_auth_profile_slug when there was no local
oauth_account — but a broker-backed connection HAS no local oauth_account
(the grant lives on the broker). Now:
- resolveConnectionAuthSelection resolves the explicit app profile up front
and short-circuits to a broker-backed selection (appAuthProfile = the
oauth_broker profile, no runtime authProfile) BEFORE the no-auth-profile
early-return.
- manage_connections treats an OAuth connector as satisfied when the app
profile is oauth_broker (no forced pending_auth / missing-oauth_account
rejection), so connections.create yields an ACTIVE broker-backed connection.
- Test: creates a broker-backed connection THROUGH manage_connections
(referencing an oauth_broker profile), asserting active + FK linkage +
no local auth_profile_id.
DEAD CODE: resolveCredentialSource + CredentialSource were exported but
unused. execution-context now calls resolveCredentialSource() and branches
on source.kind ('broker' -> fetch from broker; 'local' -> unchanged path);
parseBrokerCredential stays as the internal helper it uses. One typed branch
point, no dead export, non-broker path unchanged.
…e validation
Polish pass (no behavior change to the happy path):
1. Dead code: resolveCredentialSource's {kind:'local'} arm was never consumed
(execution-context only used the broker case; the local path stays on its
own unchanged code). Collapsed to a broker-only resolver
resolveBrokerCredentialForConnection(orgId, appAuthProfileId) -> BrokerCredential | null
and removed the CredentialSource union entirely. execution-context branches
on the non-null result; the local path is unchanged.
2. broker_url hardening: parseBrokerCredential now rejects a broker_url that is
not an absolute http:/https: URL (new URL() + protocol check), closing the
broker-URL gap.
3. oauth_broker update validation: validate the payload that will actually be
persisted (auth_data, else normalized credentials), not just args.auth_data,
so a partial/bad update (e.g. string-only credentials that can't carry the
numeric broker_connection_id) is rejected instead of silently wiping the
profile.
Test: added a broker_url-invalid rejection case (no scheme, relative, ftp:).
The local instance POSTs its broker PAT to broker_url, so an attacker-controlled oauth_broker profile could point that fetch at an internal service or the cloud metadata endpoint (169.254.169.254). Gate it with an operator-configured ORIGIN ALLOWLIST (no IP/host denylist — denylists are bypassable via DNS rebinding + encodings): - New env LOBU_ALLOWED_BROKER_ORIGINS: comma-separated scheme://host[:port] origins this instance may delegate OAuth to. Parsed once. - Enforced at the fetch boundary in fetchBrokerAccessToken (before any outbound request): compute the request URL's origin and reject (return null + clear warn log, no fetch) unless it's an exact allowlist member. - Unset/empty allowlist → reject ALL broker fetches (fail-closed). - https required for non-loopback origins; http allowed only for an explicitly-allowlisted loopback origin (opt-in dev / self-broker). Keeps the existing new URL() + http/https parse check. - Comment: full DNS-rebinding protection (resolve host + verify the connected IP isn't private at fetch time) is a production hardening follow-up; the origin allowlist is the primary control. Tests: set LOBU_ALLOWED_BROKER_ORIGINS to the in-test self-broker loopback origin so existing runtime-hook tests fetch; added a not-allowlisted-origin case and an unset-allowlist fail-closed case — both assert no credentials and zero outbound request (the fake provider is never hit).
…roker)
Replace the bespoke oauth_broker model with the simpler public-org model: a
managed connector lives in a public org with a managed oauth_app; a user joins
it and connects normally (a connection owned by them via created_by). Their
local Lobu fetches its own user's connection token from the cloud at runtime
and runs the connector locally. The cloud secret never leaves the cloud.
Delete the broker layer: oauth_broker profile kind + its (never-on-main)
migration, BrokerCredential/parseBrokerCredential/resolveBrokerCredentialForConnection,
broker_* auth_data fields, the LOBU_ALLOWED_BROKER_ORIGINS SSRF allowlist, the
broker:token scope, and connect/broker-routes.ts. Keep pat-auth.ts.
Add POST /oauth/connection-token (connect/connection-token-route.ts), PAT-gated
via authenticatePat: 403 non-member, 404/owner-scoped on created_by, returns
only { access_token, expires_at } via the existing CredentialService.
Add managedBy to the connection (defineConnection({ connector, managedBy })),
folded into connection config; execution-context.ts fetches the token from the
cloud when a connection is managedBy, caching until near expiry. The local
(non-managed) path is unchanged.
buremba
changed the title
…dpoint to public consent-only grants
Address two security blockers + one cache-key bug on the public-org managed
connector model:
- PAT exfiltration: fetchManagedConnectionToken sent LOBU_CLOUD_PAT to a
connection-config-controlled managedBy.url, letting a malicious connection
redirect the PAT to an arbitrary host. The PAT is now ALWAYS sent to the
instance-configured LOBU_CLOUD_URL only; drop the per-connection url override
entirely (managedBy is just { org }) in execution-context, the CLI
defineConnection ManagedBy type, and map-config.
- Over-broad token endpoint: /oauth/connection-token returned a token for any
owner-created active connection in any org the user belongs to (could export
private-org connection tokens). The lookup now requires the org to be
visibility='public' AND the connection to be a consent-only managed
grant-holder (config.consent_only=true); otherwise 404 (same not-found shape,
no leak). Keeps the member + created_by owner checks.
- Cache-key bug: managed-token cache key used literal NUL separators; replaced
with JSON.stringify([org, connectorKey, baseUrl]) so keys can't collide.
Tests: connection-token suite extended — non-consent-only and private-org
connections -> 404; managedBy.url ignored (PAT always targets LOBU_CLOUD_URL);
map-config managedBy fold is org-only. server + cli tsc clean.
The managed-connector token endpoint minted OAuth access tokens but accepted any valid PAT (e.g. a default mcp:read mcp:write member PAT) — too permissive for a token-minting endpoint. - Add a dedicated least-privilege scope connections:token to AVAILABLE_SCOPES (NOT in DEFAULT_SCOPES, so a default member PAT never carries it). - /oauth/connection-token now rejects 403 insufficient_scope (in the auth middleware, before any org/connection lookup) unless the PAT carries connections:token. The local instance's LOBU_CLOUD_PAT must be minted with it (lobu token create --scope connections:token). Keeps the existing member + owner(created_by) + public-org + consent_only checks — this is in addition. - Resolve the request org by EITHER organization.id OR slug, then use the resolved id consistently in the membership + connection queries. Tests: connection-token suite extended — a right-user/org PAT WITHOUT connections:token -> 403; passing the org slug resolves (200); happy-path PATs mint with the scope; consent-only-feeds token test updated to carry it.
…d authedOrgId The /oauth/connection-token endpoint requires the connections:token scope, but the PAT-creation route's allowlist (AVAILABLE_PAT_SCOPES) rejected it, so the documented LOBU_CLOUD_PAT could never be minted. Add it there (still NOT a default scope). Also remove the unused authedOrgId context var.
This was referenced May 25, 2026
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
2 participants
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.
What this is
Managed OAuth connectors via a public org. An admin configures a managed OAuth app (e.g. Spotify) in a public org; members join and connect normally (consent against the managed app) — no per-user app setup, no client secret on their side. Each member's local Lobu fetches a short-lived, owner-scoped access token from the cloud at runtime and runs the connector locally, so feeds + data stay on the user's machine while the secret + grant stay in the cloud.
Where things live
events)How it works
oauth_app+ connector.created_by) holds the grant.managedBy: { org }; at runtime its resolver callsPOST /oauth/connection-token { org, connector_key }(user PAT) → cloud returns only{ access_token, expires_at }, refreshed server-side with the managed secret.Endpoint security —
POST /oauth/connection-tokenauthenticatePat).org(a PAT's org-binding does NOT imply membership in an arbitrary org).created_by); 404 otherwise."Data stays local" — by construction
A grant-holder connection is consent-only (
config.consent_only): feed creation on it is rejected, so the cloud can never sync a copy — the data only ever lives on the local instance. (defineConnection({ consentOnly: true }); declaringconsentOnly+ feeds is a config error.)Tests (real app + Postgres + fake provider)
owner 200 (refreshed via managed secret, no secret/refresh leak) · different-member-same-org → 404 (owner-scope) · non-member → 403 · no/bad PAT → 401 · malformed → 400 · no-connection → 404 · local
managedByresolver fetches via the endpoint · non-managed path unchanged · consent-only → feed creation rejected · normal connection → feed created. All green;tscclean (server + cli).Status
Foundation for managed connectors — mergeable as plumbing. Deferred (post-merge productization): the consent deep-link UX (local "Connect" → cloud connect page) so it's user-reachable end-to-end, and a
lobuskill section documentingmanagedBy/consentOnly.🤖 Generated with Claude Code