mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-21 13:00:04 +08:00
fix(tools): resolve credentials over HTTP again so token refresh keeps the app's OAuth config (#6662)
* fix(tools): resolve credentials over HTTP again so token refresh keeps the app's OAuth config * chore(ship): note what to keep out of PR titles and descriptions
This commit is contained in:
Waleed
@@ -100,6 +100,16 @@ improvement(scope): description for enhancements
|
||||
chore(scope): description for maintenance
|
||||
```
|
||||
|
||||
## What to Omit
|
||||
|
||||
The repo is public. Keep the title and description to the code change and its reasoning — never:
|
||||
|
||||
- Customer, company, or user names; workspace/user/org IDs; email addresses
|
||||
- Prod or staging operational data: log lines, DB rows, metrics, timestamps, incident details, canary/alert output
|
||||
- Infrastructure specifics: hostnames, ARNs, internal URLs, env var values, secret names
|
||||
|
||||
Describe the bug by its mechanism, not by how you found it. "Expired OAuth credentials fail to refresh in the worker" — not "the Sheets canary failed at 16:31Z for workspace abc-123".
|
||||
|
||||
## PR Description Format
|
||||
|
||||
Use this exact template in the user's voice (concise, bullet points):
|
||||
|
||||
@@ -99,6 +99,16 @@ improvement(scope): description for enhancements
|
||||
chore(scope): description for maintenance
|
||||
```
|
||||
|
||||
## What to Omit
|
||||
|
||||
The repo is public. Keep the title and description to the code change and its reasoning — never:
|
||||
|
||||
- Customer, company, or user names; workspace/user/org IDs; email addresses
|
||||
- Prod or staging operational data: log lines, DB rows, metrics, timestamps, incident details, canary/alert output
|
||||
- Infrastructure specifics: hostnames, ARNs, internal URLs, env var values, secret names
|
||||
|
||||
Describe the bug by its mechanism, not by how you found it. "Expired OAuth credentials fail to refresh in the worker" — not "the Sheets canary failed at 16:31Z for workspace abc-123".
|
||||
|
||||
## PR Description Format
|
||||
|
||||
Use this exact template in the user's voice (concise, bullet points):
|
||||
|
||||
@@ -57,6 +57,27 @@ Use the `migrate-application-operation` skill before creating or migrating a pro
|
||||
|
||||
Every export of a `'use client'` module becomes a *client reference* on the server — server-evaluated code (RSC pages/layouts, `prefetch.ts`, route handlers, block definitions, triggers) can only *render* it as a component or pass it as a prop, never *call* it (doing so throws at runtime, e.g. `tableKeys.list is not a function`; `next build` does not catch it). Keep server-importable query primitives (key factories, fetchers, mappers, constants) in non-`'use client'` modules — see `.claude/rules/sim-queries.md`. Enforced by `scripts/check-client-boundary-imports.ts`.
|
||||
|
||||
## The app/worker runtime boundary
|
||||
|
||||
Server code runs in two runtimes with **different environments**. The app container loads the
|
||||
full env from `SIM_ENV_SECRET_ID` (Secrets Manager). Trigger.dev workers — which execute
|
||||
workflows, so every block handler and every tool call — get their env from the Trigger.dev
|
||||
dashboard, and `trigger.config.ts` syncs only `DB_APP_NAME`. The repo cannot see what the
|
||||
dashboard holds.
|
||||
|
||||
So before replacing a worker's HTTP call to our own API with an in-process call, ask what env
|
||||
that work reads *on the app side*. Anything gated by a `require*Capability` helper is the sharp
|
||||
case: those **throw** when the variable is absent (`requireOAuthClientCapability` →
|
||||
`EnvCapabilityConfigurationError`), and the throw may be caught and reported as something
|
||||
unrelated. OAuth token refresh is the known example — moving it into the worker turns every
|
||||
expired credential into `Failed to refresh access token`, while a still-valid token hides the
|
||||
bug entirely, so it surfaces hours later and only for whoever's token lapsed first.
|
||||
|
||||
An in-process conversion is safe when the same work already runs in that runtime (the agent
|
||||
block has always called `executeProviderRequest` in-process, so router and evaluator joining it
|
||||
is proven), or when the caller and the callee are both the app (a route calling a lib module, an
|
||||
RSC prefetch reading the data layer). It is not safe on reasoning alone.
|
||||
|
||||
## Feature Organization
|
||||
|
||||
Features live under `app/workspace/[workspaceId]/`:
|
||||
|
||||
@@ -94,6 +94,16 @@ improvement(scope): description for enhancements
|
||||
chore(scope): description for maintenance
|
||||
```
|
||||
|
||||
## What to Omit
|
||||
|
||||
The repo is public. Keep the title and description to the code change and its reasoning — never:
|
||||
|
||||
- Customer, company, or user names; workspace/user/org IDs; email addresses
|
||||
- Prod or staging operational data: log lines, DB rows, metrics, timestamps, incident details, canary/alert output
|
||||
- Infrastructure specifics: hostnames, ARNs, internal URLs, env var values, secret names
|
||||
|
||||
Describe the bug by its mechanism, not by how you found it. "Expired OAuth credentials fail to refresh in the worker" — not "the Sheets canary failed at 16:31Z for workspace abc-123".
|
||||
|
||||
## PR Description Format
|
||||
|
||||
Use this exact template in the user's voice (concise, bullet points):
|
||||
|
||||
+59
-68
@@ -1732,76 +1732,67 @@ async function executeToolImplementation(
|
||||
const callerUserId =
|
||||
userId && contextParams._context?.enforceCredentialAccess ? userId : undefined
|
||||
|
||||
let data: CredentialTokenPayload
|
||||
const baseUrl = getInternalApiBaseUrl()
|
||||
logger.info(`[${requestId}] Fetching access token from ${baseUrl}/api/auth/oauth/token`)
|
||||
|
||||
if (typeof window === 'undefined') {
|
||||
// Server-side runs resolve the credential through the same application
|
||||
// operation the route calls, rather than minting an internal JWT and
|
||||
// POSTing to ourselves through the load balancer. The synthesized
|
||||
// `AuthResult` is exactly what verifying that self-issued token would
|
||||
// have produced, so authorization, refresh, and audit are unchanged —
|
||||
// including failing closed when the run carries no user id.
|
||||
const { resolveCredentialToken } = await import('@/lib/oauth/token-resolution')
|
||||
const result = await resolveCredentialToken(
|
||||
{ success: true, authType: 'internal_jwt', userId },
|
||||
{
|
||||
requestId,
|
||||
credentialId: contextParams.credential as string,
|
||||
workflowId,
|
||||
scopes: tokenPayload.scopes,
|
||||
impersonateEmail: tokenPayload.impersonateEmail,
|
||||
callerUserId,
|
||||
}
|
||||
)
|
||||
|
||||
if (!result.ok) {
|
||||
logger.error(`[${requestId}] Token fetch failed for ${toolId}:`, {
|
||||
status: result.status,
|
||||
error: result.error,
|
||||
})
|
||||
const toolLabel = tool?.name || toolId
|
||||
throw new Error(`Failed to obtain credential for ${toolLabel}: ${result.error}`)
|
||||
}
|
||||
|
||||
data = result.token
|
||||
} else {
|
||||
const baseUrl = getInternalApiBaseUrl()
|
||||
logger.info(`[${requestId}] Fetching access token from ${baseUrl}/api/auth/oauth/token`)
|
||||
|
||||
const tokenUrlObj = new URL('/api/auth/oauth/token', baseUrl)
|
||||
if (workflowId) {
|
||||
tokenUrlObj.searchParams.set('workflowId', workflowId)
|
||||
}
|
||||
if (callerUserId) {
|
||||
tokenUrlObj.searchParams.set('userId', callerUserId)
|
||||
}
|
||||
|
||||
// boundary-raw-fetch: browser-side tool runs authenticate with the session cookie against the same-origin token route
|
||||
const response = await fetch(tokenUrlObj.toString(), {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify(tokenPayload),
|
||||
})
|
||||
|
||||
if (!response.ok) {
|
||||
const errorText = await response.text()
|
||||
logger.error(`[${requestId}] Token fetch failed for ${toolId}:`, {
|
||||
status: response.status,
|
||||
error: errorText,
|
||||
})
|
||||
let parsedError = errorText
|
||||
try {
|
||||
const parsed = JSON.parse(errorText)
|
||||
if (parsed.error) parsedError = parsed.error
|
||||
} catch {
|
||||
// Use raw text
|
||||
}
|
||||
const toolLabel = tool?.name || toolId
|
||||
throw new Error(`Failed to obtain credential for ${toolLabel}: ${parsedError}`)
|
||||
}
|
||||
|
||||
data = (await response.json()) as CredentialTokenPayload
|
||||
const tokenUrlObj = new URL('/api/auth/oauth/token', baseUrl)
|
||||
if (workflowId) {
|
||||
tokenUrlObj.searchParams.set('workflowId', workflowId)
|
||||
}
|
||||
if (callerUserId) {
|
||||
tokenUrlObj.searchParams.set('userId', callerUserId)
|
||||
}
|
||||
|
||||
/**
|
||||
* Deliberately an HTTP hop rather than an in-process call to
|
||||
* `resolveCredentialToken`, even though both run the same authorization rule.
|
||||
*
|
||||
* An OAuth refresh needs the provider's client id and secret
|
||||
* (`requireOAuthClientCapability`, which THROWS when they are absent). Only the
|
||||
* app container loads those, from `SIM_ENV_SECRET_ID`. Tool calls execute inside
|
||||
* the Trigger.dev worker, whose environment does not carry them, so resolving
|
||||
* in-process there turns every credential whose access token has expired into
|
||||
* `Failed to refresh access token`. A still-valid token hides it — the refresh
|
||||
* path is only reached once the token lapses.
|
||||
*
|
||||
* Moving this in-process requires the worker to hold the OAuth client config,
|
||||
* not just a code change.
|
||||
*/
|
||||
const tokenHeaders: Record<string, string> = { 'Content-Type': 'application/json' }
|
||||
if (typeof window === 'undefined') {
|
||||
try {
|
||||
const internalToken = await generateInternalToken(userId)
|
||||
tokenHeaders.Authorization = `Bearer ${internalToken}`
|
||||
} catch (_e) {
|
||||
// Swallow token generation errors; the request will fail and be reported upstream
|
||||
}
|
||||
}
|
||||
|
||||
// boundary-raw-fetch: same-origin token route, authenticated by internal JWT on the server and the session cookie in the browser
|
||||
const response = await fetch(tokenUrlObj.toString(), {
|
||||
method: 'POST',
|
||||
headers: tokenHeaders,
|
||||
body: JSON.stringify(tokenPayload),
|
||||
})
|
||||
|
||||
if (!response.ok) {
|
||||
const errorText = await response.text()
|
||||
logger.error(`[${requestId}] Token fetch failed for ${toolId}:`, {
|
||||
status: response.status,
|
||||
error: errorText,
|
||||
})
|
||||
let parsedError = errorText
|
||||
try {
|
||||
const parsed = JSON.parse(errorText)
|
||||
if (parsed.error) parsedError = parsed.error
|
||||
} catch {
|
||||
// Use raw text
|
||||
}
|
||||
const toolLabel = tool?.name || toolId
|
||||
throw new Error(`Failed to obtain credential for ${toolLabel}: ${parsedError}`)
|
||||
}
|
||||
|
||||
const data = (await response.json()) as CredentialTokenPayload
|
||||
|
||||
contextParams.accessToken = data.accessToken
|
||||
if (data.idToken) {
|
||||
|
||||
Reference in New Issue
Block a user