Skip to content

Commit 416af7c

Browse files
committed
fix(credentials): capture the correct provider identity on connect and rotate
Attio OAuth recorded an arbitrary workspace member instead of the authorizing user, so two members connecting under one Sim user collapsed into a single account row via the stale-sibling dedupe. Notion read `profile.person.email`, which never exists on a bot token. Synthetic connector emails were minted on live third-party domains. Google service-account rotation left the credential labeled with the old key's client_email and skipped audit metadata entirely. Box and Salesforce identity lookups failed silently with no logger in either file. Service-account principals are now a single ServiceAccountPrincipal union (user / tenant / lookup_failed / null) mirrored centrally into both audit and stored metadata, so a principal can no longer be captured and forgotten, and "which account is this credential?" is answerable from SQL.
1 parent b8ec114 commit 416af7c

51 files changed

Lines changed: 1273 additions & 270 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

apps/sim/app/api/credentials/route.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,8 @@ describe('POST /api/credentials', () => {
140140
providerId: 'zoom-service-account',
141141
encryptedServiceAccountKey: 'encrypted-blob',
142142
displayName: 'Zoom account acct_123',
143-
auditMetadata: { zoomAccountId: 'acct_123' },
143+
auditMetadata: { principalKind: 'tenant', principalId: 'acct_123' },
144+
principal: { kind: 'tenant', id: 'acct_123' },
144145
})
145146

146147
const req = createMockRequest('POST', {

apps/sim/app/api/credentials/route.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -654,9 +654,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
654654
resourceName: resolvedDisplayName,
655655
description: `Created ${type} credential "${resolvedDisplayName}"`,
656656
metadata: {
657+
// Provider metadata spreads first so this route's own keys stay
658+
// authoritative and can never be shadowed, matching the update path in
659+
// `lib/credentials/orchestration`.
660+
...extraAuditMetadata,
657661
credentialType: type,
658662
providerId: resolvedProviderId,
659-
...extraAuditMetadata,
660663
},
661664
request,
662665
})

apps/sim/lib/auth/auth.ts

Lines changed: 156 additions & 51 deletions
Large diffs are not rendered by default.
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { syntheticConnectorEmail } from '@/lib/auth/connector-email'
6+
7+
describe('syntheticConnectorEmail', () => {
8+
it('namespaces the address by provider and identity', () => {
9+
expect(syntheticConnectorEmail('attio', 'abc123')).toBe('attio-abc123@connectors.sim.invalid')
10+
})
11+
12+
it('always lands on the RFC 2606 reserved .invalid TLD', () => {
13+
const providers: Array<[string, string]> = [
14+
['x', 'someuser'],
15+
['hubspot', '12345'],
16+
['salesforce', '005xx'],
17+
['docusign', 'sub-1'],
18+
['calcom', '77'],
19+
['atlassian', 'acct'],
20+
['wordpress', 'blogger'],
21+
]
22+
for (const [provider, id] of providers) {
23+
const email = syntheticConnectorEmail(provider, id)
24+
expect(email.endsWith('@connectors.sim.invalid')).toBe(true)
25+
}
26+
})
27+
28+
it('never emits a live third-party domain', () => {
29+
const email = syntheticConnectorEmail('x', 'jack')
30+
expect(email).not.toMatch(/@(x|hubspot|docusign|cal|salesforce|atlassian|wordpress)\.com$/)
31+
})
32+
33+
it('distinguishes the same external id across providers', () => {
34+
expect(syntheticConnectorEmail('zoom', '42')).not.toBe(syntheticConnectorEmail('spotify', '42'))
35+
})
36+
37+
it('is deterministic for the same input', () => {
38+
expect(syntheticConnectorEmail('monday', 99)).toBe(syntheticConnectorEmail('monday', 99))
39+
})
40+
41+
it('accepts numeric identifiers', () => {
42+
expect(syntheticConnectorEmail('monday', 99)).toBe('monday-99@connectors.sim.invalid')
43+
})
44+
45+
it('strips characters that are illegal in an unquoted local part', () => {
46+
expect(syntheticConnectorEmail('slack', 'T123-usr_U456')).toBe(
47+
'slack-T123-usr_U456@connectors.sim.invalid'
48+
)
49+
expect(syntheticConnectorEmail('reddit', 'some user!@#')).toBe(
50+
'reddit-someuser@connectors.sim.invalid'
51+
)
52+
})
53+
54+
it('keeps the local part inside the RFC 5321 64-character limit', () => {
55+
const email = syntheticConnectorEmail('a'.repeat(100), 'b'.repeat(100))
56+
const [localPart] = email.split('@')
57+
expect(localPart.length).toBeLessThanOrEqual(64)
58+
})
59+
60+
it('does not leave a dot or hyphen at either edge of a truncated segment', () => {
61+
const email = syntheticConnectorEmail('wealthbox', `${'c'.repeat(29)}...tail`)
62+
const [localPart] = email.split('@')
63+
expect(localPart.endsWith('.')).toBe(false)
64+
expect(localPart.startsWith('.')).toBe(false)
65+
})
66+
67+
it('falls back to placeholders rather than emitting an empty local part', () => {
68+
expect(syntheticConnectorEmail('notion', undefined)).toBe(
69+
'notion-unknown@connectors.sim.invalid'
70+
)
71+
expect(syntheticConnectorEmail('notion', '')).toBe('notion-unknown@connectors.sim.invalid')
72+
expect(syntheticConnectorEmail('', '')).toBe('connector-unknown@connectors.sim.invalid')
73+
expect(syntheticConnectorEmail('!!!', '###')).toBe('connector-unknown@connectors.sim.invalid')
74+
})
75+
76+
it('always returns a truthy value, which is what Better Auth 1.6.23 requires', () => {
77+
expect(syntheticConnectorEmail('', undefined)).toBeTruthy()
78+
})
79+
})
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/** RFC 2606 §2 reserved TLD — permanently unregistrable and unroutable. */
2+
const SYNTHETIC_EMAIL_DOMAIN = 'connectors.sim.invalid'
3+
4+
/** Longest local-part segment kept, so the address stays under the 64-char RFC 5321 limit. */
5+
const MAX_SEGMENT_LENGTH = 30
6+
7+
/**
8+
* Reduce an arbitrary upstream identifier to characters that are unambiguously
9+
* legal in an unquoted email local part.
10+
*/
11+
function sanitizeLocalPart(value: string): string {
12+
return (
13+
value
14+
.replace(/[^a-zA-Z0-9._-]/g, '')
15+
.slice(0, MAX_SEGMENT_LENGTH)
16+
// RFC 5321 `dot-string` is `Atom *("." Atom)`, so a run of separators is not
17+
// a legal local part — and stripping illegal characters readily creates one.
18+
.replace(/[._-]{2,}/g, '-')
19+
.replace(/^[._-]+|[._-]+$/g, '')
20+
)
21+
}
22+
23+
/**
24+
* Synthetic placeholder email for an OAuth connector identity.
25+
*
26+
* Many connector providers either never expose an email (X, Slack bot tokens,
27+
* TikTok, Reddit, Webflow) or expose one only when an optional scope was
28+
* granted. Better Auth still demands one: in `better-auth@1.6.23`,
29+
* `dist/plugins/generic-oauth/routes.mjs` hard-rejects a falsy `email` returned
30+
* from `getUserInfo` by throwing a redirect to `?error=email_is_missing`. There
31+
* is no option to disable that guard, so every `getUserInfo` must return a
32+
* truthy address or the connect flow dies at the callback.
33+
*
34+
* The value is never persisted. Sim's connectors go through the session-bound
35+
* `oauth2.link` path, the `account` table has no email column, and
36+
* `updateUserInfoOnLink` is unset — so Better Auth reads the address, satisfies
37+
* its own guard, and discards it. It is never shown to a user, never mailed to,
38+
* and never matched against a real account.
39+
*
40+
* The domain is `.invalid`, reserved by RFC 2606 §2 precisely so that it can
41+
* never be registered or routed. Earlier code synthesized addresses on live
42+
* third-party domains (`@x.com`, `@salesforce.com`, `@atlassian.com`, …), which
43+
* are owned by other companies and could in principle resolve to a real
44+
* mailbox.
45+
*
46+
* Delete this helper and return the upstream email directly once Better Auth
47+
* relaxes the guard (tracked in better-auth issue #9124, slated for v2).
48+
*
49+
* @param providerId - Connector provider id, e.g. `'attio'`. Namespaces the
50+
* address so two providers reporting the same external id do not collide.
51+
* @param stableId - Stable external identifier for the connected identity
52+
* (workspace member id, account id, username, …). Falsy or fully-unsupported
53+
* values degrade to `unknown`; uniqueness is best-effort because the address
54+
* is discarded either way.
55+
* @returns An RFC 5321-shaped address on a permanently unroutable domain.
56+
*/
57+
export function syntheticConnectorEmail(providerId: string, stableId?: string | number): string {
58+
const provider = sanitizeLocalPart(providerId) || 'connector'
59+
const identity = sanitizeLocalPart(stableId == null ? '' : String(stableId)) || 'unknown'
60+
return `${provider}-${identity}@${SYNTHETIC_EMAIL_DOMAIN}`
61+
}

apps/sim/lib/credentials/atlassian-service-account.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,16 @@ async function assertAtlassianResponseOk(
8080
export async function validateAtlassianServiceAccount(
8181
apiToken: string,
8282
domain: string
83-
): Promise<{ accountId: string; displayName: string; cloudId: string }> {
83+
): Promise<{
84+
accountId: string
85+
displayName: string
86+
cloudId: string
87+
/**
88+
* Only present when the site's profile-visibility settings expose it to the
89+
* calling token; absence is never a validation failure.
90+
*/
91+
emailAddress?: string
92+
}> {
8493
assertAtlassianCloudHost(domain)
8594

8695
const tenantInfoRes = await fetch(`https://${domain}/_edge/tenant_info`, {
@@ -123,5 +132,6 @@ export async function validateAtlassianServiceAccount(
123132
accountId: myself.accountId,
124133
displayName: myself.displayName || myself.emailAddress || domain,
125134
cloudId,
135+
...(myself.emailAddress ? { emailAddress: myself.emailAddress } : {}),
126136
}
127137
}

apps/sim/lib/credentials/client-credential-accounts/minters/box.test.ts

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ describe('mintBoxServiceAccountToken', () => {
6767
.mockResolvedValueOnce(jsonResponse(200, { access_token: 'box-access', expires_in: 3600 }))
6868
.mockResolvedValueOnce(
6969
jsonResponse(200, {
70+
id: '33445566',
7071
name: 'Sim Automation',
7172
login: 'AutomationUser_123_abc@boxdevedition.com',
7273
})
@@ -79,22 +80,21 @@ describe('mintBoxServiceAccountToken', () => {
7980
expiresInSeconds: 3600,
8081
identity: {
8182
displayName: 'Sim Automation',
82-
auditMetadata: {
83-
boxEnterpriseId: '1234567',
84-
boxServiceAccountLogin: 'AutomationUser_123_abc@boxdevedition.com',
85-
},
86-
storedMetadata: {
87-
enterpriseId: '1234567',
88-
serviceAccountLogin: 'AutomationUser_123_abc@boxdevedition.com',
83+
principal: {
84+
kind: 'user',
85+
id: '33445566',
86+
label: 'AutomationUser_123_abc@boxdevedition.com',
8987
},
88+
auditMetadata: { boxEnterpriseId: '1234567' },
89+
storedMetadata: { enterpriseId: '1234567' },
9090
},
9191
})
9292
expect(mockFetch).toHaveBeenCalledTimes(2)
9393
expectMintCall()
9494
expectIdentityCall()
9595
})
9696

97-
it('still succeeds with a fallback identity when users/me fails', async () => {
97+
it('marks the principal as lookup_failed when users/me fails', async () => {
9898
mockFetch
9999
.mockResolvedValueOnce(jsonResponse(200, { access_token: 'box-access', expires_in: 2400 }))
100100
.mockResolvedValueOnce(jsonResponse(500, { message: 'boom' }))
@@ -105,7 +105,22 @@ describe('mintBoxServiceAccountToken', () => {
105105
expect(result.expiresInSeconds).toBe(2400)
106106
expect(result.identity).toEqual({
107107
displayName: 'Box enterprise 1234567',
108+
principal: { kind: 'lookup_failed', reason: 'HTTP 500' },
108109
auditMetadata: { boxEnterpriseId: '1234567' },
110+
storedMetadata: { enterpriseId: '1234567' },
111+
})
112+
})
113+
114+
it('marks the principal as lookup_failed when users/me omits the user id', async () => {
115+
mockFetch
116+
.mockResolvedValueOnce(jsonResponse(200, { access_token: 'box-access', expires_in: 3600 }))
117+
.mockResolvedValueOnce(jsonResponse(200, { name: 'Sim Automation' }))
118+
119+
const result = await mintBoxServiceAccountToken(FIELDS)
120+
121+
expect(result.identity?.principal).toEqual({
122+
kind: 'lookup_failed',
123+
reason: 'response missing user id',
109124
})
110125
})
111126

@@ -118,6 +133,10 @@ describe('mintBoxServiceAccountToken', () => {
118133

119134
expect(result.accessToken).toBe('box-access')
120135
expect(result.identity?.displayName).toBe('Box enterprise 1234567')
136+
expect(result.identity?.principal).toEqual({
137+
kind: 'lookup_failed',
138+
reason: 'provider_unavailable (HTTP 502)',
139+
})
121140
})
122141

123142
it('throws invalid_credentials on 400 invalid_client', async () => {

apps/sim/lib/credentials/client-credential-accounts/minters/box.ts

Lines changed: 53 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { createLogger } from '@sim/logger'
2+
import { getErrorMessage } from '@sim/utils/errors'
13
import type {
24
ClientCredentialAccountFields,
35
ClientCredentialAccountIdentity,
@@ -8,10 +10,15 @@ import {
810
fetchProvider,
911
isTransientProviderStatus,
1012
parseProviderJson,
13+
providerFailureReason,
1114
readProviderErrorSnippet,
1215
TokenServiceAccountValidationError,
1316
} from '@/lib/credentials/token-service-accounts/errors'
1417

18+
const logger = createLogger('BoxServiceAccountMinter')
19+
20+
const IDENTITY_STEP = 'box_identity'
21+
1522
const BOX_TOKEN_URL = 'https://api.box.com/oauth2/token'
1623
const BOX_CURRENT_USER_URL = 'https://api.box.com/2.0/users/me'
1724

@@ -20,7 +27,14 @@ interface BoxTokenResponse {
2027
expires_in?: number
2128
}
2229

30+
/**
31+
* `id`, `name`, and `login` are all in the standard field set `GET /2.0/users/me`
32+
* returns without a `fields` parameter, so capturing the Service Account's user
33+
* id costs no extra request.
34+
* @see https://developer.box.com/reference/get-users-me/
35+
*/
2336
interface BoxCurrentUserResponse {
37+
id?: string
2438
name?: string
2539
login?: string
2640
}
@@ -53,40 +67,61 @@ function boxErrorHint(body: string): string | undefined {
5367

5468
/**
5569
* Best-effort identity lookup for the app's Service Account user. A failure
56-
* never fails the mint — the caller falls back to an Enterprise-ID-derived
57-
* display name.
70+
* never fails the mint — the credential degrades to an Enterprise-ID-derived
71+
* display name with a `lookup_failed` principal, so the audit record shows the
72+
* identity was not captured rather than implying none exists.
5873
*/
5974
async function fetchBoxServiceAccountIdentity(
6075
accessToken: string,
6176
orgId: string
6277
): Promise<ClientCredentialAccountIdentity> {
63-
const fallback: ClientCredentialAccountIdentity = {
78+
const degraded = (reason: string): ClientCredentialAccountIdentity => ({
6479
displayName: `Box enterprise ${orgId}`,
80+
principal: { kind: 'lookup_failed', reason },
6581
auditMetadata: { boxEnterpriseId: orgId },
66-
}
82+
storedMetadata: { enterpriseId: orgId },
83+
})
6784
try {
6885
const res = await fetchProvider(
6986
BOX_CURRENT_USER_URL,
7087
{ headers: { Authorization: `Bearer ${accessToken}`, Accept: 'application/json' } },
71-
'box_identity'
88+
IDENTITY_STEP
7289
)
73-
if (!res.ok) return fallback
74-
const user = await parseProviderJson<BoxCurrentUserResponse>(res, 'box_identity')
90+
if (!res.ok) {
91+
logger.warn('Box service-account identity lookup failed', {
92+
step: IDENTITY_STEP,
93+
status: res.status,
94+
enterpriseId: orgId,
95+
})
96+
return degraded(`HTTP ${res.status}`)
97+
}
98+
const user = await parseProviderJson<BoxCurrentUserResponse>(res, IDENTITY_STEP)
99+
const id = typeof user.id === 'string' && user.id ? user.id : undefined
75100
const login = typeof user.login === 'string' && user.login ? user.login : undefined
76101
const name = typeof user.name === 'string' && user.name ? user.name : undefined
77-
return {
78-
displayName: name ?? login ?? fallback.displayName,
79-
auditMetadata: {
80-
boxEnterpriseId: orgId,
81-
...(login ? { boxServiceAccountLogin: login } : {}),
82-
},
83-
storedMetadata: {
102+
if (!id) {
103+
logger.warn('Box service-account identity response carried no user id', {
104+
step: IDENTITY_STEP,
105+
status: res.status,
84106
enterpriseId: orgId,
85-
...(login ? { serviceAccountLogin: login } : {}),
86-
},
107+
})
108+
return degraded('response missing user id')
87109
}
88-
} catch {
89-
return fallback
110+
return {
111+
displayName: name ?? login ?? `Box enterprise ${orgId}`,
112+
// The Service Account is a real Box user; `enterpriseId` is shared by
113+
// every app in the enterprise and so is kept as separate context.
114+
principal: { kind: 'user', id, ...(login ? { label: login } : {}) },
115+
auditMetadata: { boxEnterpriseId: orgId },
116+
storedMetadata: { enterpriseId: orgId },
117+
}
118+
} catch (error) {
119+
logger.warn('Box service-account identity lookup threw', {
120+
step: IDENTITY_STEP,
121+
enterpriseId: orgId,
122+
error: getErrorMessage(error),
123+
})
124+
return degraded(providerFailureReason(error))
90125
}
91126
}
92127

0 commit comments

Comments
 (0)