Skip to content

Commit 1e60042

Browse files
authored
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
1 parent 0c4e674 commit 1e60042

5 files changed

Lines changed: 104 additions & 62 deletions

File tree

.agents/skills/ship/SKILL.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,16 @@ improvement(scope): description for enhancements
100100
chore(scope): description for maintenance
101101
```
102102

103+
## What to Omit
104+
105+
The repo is public. Keep the title and description to the code change and its reasoning — never:
106+
107+
- Customer, company, or user names; workspace/user/org IDs; email addresses
108+
- Prod or staging operational data: log lines, DB rows, metrics, timestamps, incident details, canary/alert output
109+
- Infrastructure specifics: hostnames, ARNs, internal URLs, env var values, secret names
110+
111+
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".
112+
103113
## PR Description Format
104114

105115
Use this exact template in the user's voice (concise, bullet points):

.claude/commands/ship.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,16 @@ improvement(scope): description for enhancements
9999
chore(scope): description for maintenance
100100
```
101101

102+
## What to Omit
103+
104+
The repo is public. Keep the title and description to the code change and its reasoning — never:
105+
106+
- Customer, company, or user names; workspace/user/org IDs; email addresses
107+
- Prod or staging operational data: log lines, DB rows, metrics, timestamps, incident details, canary/alert output
108+
- Infrastructure specifics: hostnames, ARNs, internal URLs, env var values, secret names
109+
110+
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".
111+
102112
## PR Description Format
103113

104114
Use this exact template in the user's voice (concise, bullet points):

.claude/rules/sim-architecture.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,27 @@ Use the `migrate-application-operation` skill before creating or migrating a pro
5757

5858
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`.
5959

60+
## The app/worker runtime boundary
61+
62+
Server code runs in two runtimes with **different environments**. The app container loads the
63+
full env from `SIM_ENV_SECRET_ID` (Secrets Manager). Trigger.dev workers — which execute
64+
workflows, so every block handler and every tool call — get their env from the Trigger.dev
65+
dashboard, and `trigger.config.ts` syncs only `DB_APP_NAME`. The repo cannot see what the
66+
dashboard holds.
67+
68+
So before replacing a worker's HTTP call to our own API with an in-process call, ask what env
69+
that work reads *on the app side*. Anything gated by a `require*Capability` helper is the sharp
70+
case: those **throw** when the variable is absent (`requireOAuthClientCapability`
71+
`EnvCapabilityConfigurationError`), and the throw may be caught and reported as something
72+
unrelated. OAuth token refresh is the known example — moving it into the worker turns every
73+
expired credential into `Failed to refresh access token`, while a still-valid token hides the
74+
bug entirely, so it surfaces hours later and only for whoever's token lapsed first.
75+
76+
An in-process conversion is safe when the same work already runs in that runtime (the agent
77+
block has always called `executeProviderRequest` in-process, so router and evaluator joining it
78+
is proven), or when the caller and the callee are both the app (a route calling a lib module, an
79+
RSC prefetch reading the data layer). It is not safe on reasoning alone.
80+
6081
## Feature Organization
6182

6283
Features live under `app/workspace/[workspaceId]/`:

.cursor/commands/ship.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,16 @@ improvement(scope): description for enhancements
9494
chore(scope): description for maintenance
9595
```
9696

97+
## What to Omit
98+
99+
The repo is public. Keep the title and description to the code change and its reasoning — never:
100+
101+
- Customer, company, or user names; workspace/user/org IDs; email addresses
102+
- Prod or staging operational data: log lines, DB rows, metrics, timestamps, incident details, canary/alert output
103+
- Infrastructure specifics: hostnames, ARNs, internal URLs, env var values, secret names
104+
105+
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".
106+
97107
## PR Description Format
98108

99109
Use this exact template in the user's voice (concise, bullet points):

apps/sim/tools/index.ts

Lines changed: 53 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -1732,77 +1732,68 @@ async function executeToolImplementation(
17321732
const callerUserId =
17331733
userId && contextParams._context?.enforceCredentialAccess ? userId : undefined
17341734

1735-
let data: CredentialTokenPayload
1735+
const baseUrl = getInternalApiBaseUrl()
1736+
logger.info(`[${requestId}] Fetching access token from ${baseUrl}/api/auth/oauth/token`)
17361737

1737-
if (typeof window === 'undefined') {
1738-
// Server-side runs resolve the credential through the same application
1739-
// operation the route calls, rather than minting an internal JWT and
1740-
// POSTing to ourselves through the load balancer. The synthesized
1741-
// `AuthResult` is exactly what verifying that self-issued token would
1742-
// have produced, so authorization, refresh, and audit are unchanged —
1743-
// including failing closed when the run carries no user id.
1744-
const { resolveCredentialToken } = await import('@/lib/oauth/token-resolution')
1745-
const result = await resolveCredentialToken(
1746-
{ success: true, authType: 'internal_jwt', userId },
1747-
{
1748-
requestId,
1749-
credentialId: contextParams.credential as string,
1750-
workflowId,
1751-
scopes: tokenPayload.scopes,
1752-
impersonateEmail: tokenPayload.impersonateEmail,
1753-
callerUserId,
1754-
}
1755-
)
1738+
const tokenUrlObj = new URL('/api/auth/oauth/token', baseUrl)
1739+
if (workflowId) {
1740+
tokenUrlObj.searchParams.set('workflowId', workflowId)
1741+
}
1742+
if (callerUserId) {
1743+
tokenUrlObj.searchParams.set('userId', callerUserId)
1744+
}
17561745

1757-
if (!result.ok) {
1758-
logger.error(`[${requestId}] Token fetch failed for ${toolId}:`, {
1759-
status: result.status,
1760-
error: result.error,
1761-
})
1762-
const toolLabel = tool?.name || toolId
1763-
throw new Error(`Failed to obtain credential for ${toolLabel}: ${result.error}`)
1746+
/**
1747+
* Deliberately an HTTP hop rather than an in-process call to
1748+
* `resolveCredentialToken`, even though both run the same authorization rule.
1749+
*
1750+
* An OAuth refresh needs the provider's client id and secret
1751+
* (`requireOAuthClientCapability`, which THROWS when they are absent). Only the
1752+
* app container loads those, from `SIM_ENV_SECRET_ID`. Tool calls execute inside
1753+
* the Trigger.dev worker, whose environment does not carry them, so resolving
1754+
* in-process there turns every credential whose access token has expired into
1755+
* `Failed to refresh access token`. A still-valid token hides it — the refresh
1756+
* path is only reached once the token lapses.
1757+
*
1758+
* Moving this in-process requires the worker to hold the OAuth client config,
1759+
* not just a code change.
1760+
*/
1761+
const tokenHeaders: Record<string, string> = { 'Content-Type': 'application/json' }
1762+
if (typeof window === 'undefined') {
1763+
try {
1764+
const internalToken = await generateInternalToken(userId)
1765+
tokenHeaders.Authorization = `Bearer ${internalToken}`
1766+
} catch (_e) {
1767+
// Swallow token generation errors; the request will fail and be reported upstream
17641768
}
1769+
}
17651770

1766-
data = result.token
1767-
} else {
1768-
const baseUrl = getInternalApiBaseUrl()
1769-
logger.info(`[${requestId}] Fetching access token from ${baseUrl}/api/auth/oauth/token`)
1770-
1771-
const tokenUrlObj = new URL('/api/auth/oauth/token', baseUrl)
1772-
if (workflowId) {
1773-
tokenUrlObj.searchParams.set('workflowId', workflowId)
1774-
}
1775-
if (callerUserId) {
1776-
tokenUrlObj.searchParams.set('userId', callerUserId)
1777-
}
1771+
// boundary-raw-fetch: same-origin token route, authenticated by internal JWT on the server and the session cookie in the browser
1772+
const response = await fetch(tokenUrlObj.toString(), {
1773+
method: 'POST',
1774+
headers: tokenHeaders,
1775+
body: JSON.stringify(tokenPayload),
1776+
})
17781777

1779-
// boundary-raw-fetch: browser-side tool runs authenticate with the session cookie against the same-origin token route
1780-
const response = await fetch(tokenUrlObj.toString(), {
1781-
method: 'POST',
1782-
headers: { 'Content-Type': 'application/json' },
1783-
body: JSON.stringify(tokenPayload),
1778+
if (!response.ok) {
1779+
const errorText = await response.text()
1780+
logger.error(`[${requestId}] Token fetch failed for ${toolId}:`, {
1781+
status: response.status,
1782+
error: errorText,
17841783
})
1785-
1786-
if (!response.ok) {
1787-
const errorText = await response.text()
1788-
logger.error(`[${requestId}] Token fetch failed for ${toolId}:`, {
1789-
status: response.status,
1790-
error: errorText,
1791-
})
1792-
let parsedError = errorText
1793-
try {
1794-
const parsed = JSON.parse(errorText)
1795-
if (parsed.error) parsedError = parsed.error
1796-
} catch {
1797-
// Use raw text
1798-
}
1799-
const toolLabel = tool?.name || toolId
1800-
throw new Error(`Failed to obtain credential for ${toolLabel}: ${parsedError}`)
1784+
let parsedError = errorText
1785+
try {
1786+
const parsed = JSON.parse(errorText)
1787+
if (parsed.error) parsedError = parsed.error
1788+
} catch {
1789+
// Use raw text
18011790
}
1802-
1803-
data = (await response.json()) as CredentialTokenPayload
1791+
const toolLabel = tool?.name || toolId
1792+
throw new Error(`Failed to obtain credential for ${toolLabel}: ${parsedError}`)
18041793
}
18051794

1795+
const data = (await response.json()) as CredentialTokenPayload
1796+
18061797
contextParams.accessToken = data.accessToken
18071798
if (data.idToken) {
18081799
contextParams.idToken = data.idToken

0 commit comments

Comments
 (0)