Skip to content

Commit 296eb16

Browse files
committed
fix(credentials): address review round 1
- A scoped 403 whose body can't be read is provider_unavailable, never accepted - Provider error snippets read through the bounded reader - Props interfaces for KeylessTrustFields and useWorkloadIdentitySetup; TSDoc on issuer responses
1 parent 15fe0ad commit 296eb16

7 files changed

Lines changed: 72 additions & 12 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/integrations/components/connect-service-account-modal/keyless-connection.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,12 @@ export function keylessSubjectFields(setup: KeylessSetup) {
7474
return setup.trust ? { subject: setup.trust.subject, subjectProof: setup.trust.subjectProof } : {}
7575
}
7676

77+
interface KeylessTrustFieldsProps {
78+
setup: KeylessSetup
79+
}
80+
7781
/** Reconnect choice plus the issuer, audience, and subject the customer configures. */
78-
export function KeylessTrustFields({ setup }: { setup: KeylessSetup }) {
82+
export function KeylessTrustFields({ setup }: KeylessTrustFieldsProps) {
7983
return (
8084
<>
8185
{setup.isReconnect && (

‎apps/sim/hooks/queries/scoped-credentials.ts‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -130,18 +130,21 @@ const workloadIdentityKeys = {
130130
/** A setup's subject is minted once per modal session and never refreshed. */
131131
const WORKLOAD_IDENTITY_SETUP_STALE_TIME = Number.POSITIVE_INFINITY
132132

133+
interface UseWorkloadIdentitySetupProps {
134+
workspaceId?: string
135+
provider: WorkloadIdentityProvider
136+
/** One per modal session, so each open gets its own subject. */
137+
sessionKey: string
138+
enabled: boolean
139+
}
140+
133141
/**
134142
* The trust setup for a new keyless AWS role or Databricks service principal:
135143
* the issuer to register and the signed subject it must trust. Each fetch
136144
* mints a new subject, so the key carries the modal session and the result
137145
* never goes stale or refetches while the person is copying it.
138146
*/
139-
export function useWorkloadIdentitySetup(input: {
140-
workspaceId?: string
141-
provider: WorkloadIdentityProvider
142-
sessionKey: string
143-
enabled: boolean
144-
}) {
147+
export function useWorkloadIdentitySetup(input: UseWorkloadIdentitySetupProps) {
145148
return useQuery({
146149
queryKey: workloadIdentityKeys.setup(input.workspaceId, input.provider, input.sessionKey),
147150
queryFn: async ({ signal }) =>

‎apps/sim/lib/credentials/token-service-accounts/errors.test.ts‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,24 @@ describe('token service-account error helpers', () => {
124124
expect(snippet).toBe(`${'x'.repeat(500)}...`)
125125
})
126126

127+
it('stops reading an oversized error body instead of buffering all of it', async () => {
128+
let pulled = 0
129+
const chunk = new Uint8Array(16 * 1024).fill(120)
130+
const body = new ReadableStream<Uint8Array>({
131+
pull(controller) {
132+
pulled++
133+
if (pulled > 1000) controller.close()
134+
else controller.enqueue(chunk)
135+
},
136+
})
137+
138+
await expect(readProviderErrorSnippet(new Response(body, { status: 500 }))).resolves.toBe('')
139+
expect(pulled).toBeLessThan(20)
140+
})
141+
127142
it('never throws on an unreadable body', async () => {
128143
const res = new Response('gone', { status: 500 })
129-
vi.spyOn(res, 'text').mockRejectedValue(new Error('body stream already read'))
144+
await res.text()
130145

131146
await expect(readProviderErrorSnippet(res)).resolves.toBe('')
132147
})

‎apps/sim/lib/credentials/token-service-accounts/errors.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import { getErrorMessage } from '@sim/utils/errors'
22
import { truncate } from '@sim/utils/string'
3+
import {
4+
DEFAULT_MAX_ERROR_BODY_BYTES,
5+
readResponseTextWithLimit,
6+
} from '@/lib/core/utils/stream-limits'
37

48
/**
59
* Discrete validation failure codes returned to the client for token
@@ -142,7 +146,12 @@ export async function parseProviderJson<T>(res: Response, step: string): Promise
142146
*/
143147
export async function readProviderErrorSnippet(res: Response): Promise<string> {
144148
try {
145-
return truncate(await res.text(), ERROR_SNIPPET_MAX_LENGTH)
149+
// Bounded: a provider or proxy can answer with an arbitrarily large error body.
150+
const text = await readResponseTextWithLimit(res, {
151+
maxBytes: DEFAULT_MAX_ERROR_BODY_BYTES,
152+
label: 'provider error response',
153+
})
154+
return truncate(text, ERROR_SNIPPET_MAX_LENGTH)
146155
} catch {
147156
return ''
148157
}

‎apps/sim/lib/credentials/token-service-accounts/validators/http.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,19 @@ export function tokenCheckValidator(check: TokenCheck): TokenServiceAccountValid
8383
response.status === 403 &&
8484
(response.headers.get('content-type') ?? '').includes('application/json')
8585
if (check.rejectsKey && !response.ok) {
86-
const errorBody = await readJsonBody(response, check.serviceLabel).catch(() => null)
87-
if (check.rejectsKey(errorBody)) {
86+
const unreadable = Symbol('unreadable')
87+
const errorBody = await readJsonBody(response, check.serviceLabel).catch(() => unreadable)
88+
if (errorBody !== unreadable && check.rejectsKey(errorBody)) {
8889
throw new TokenServiceAccountValidationError('invalid_credentials', 400, { step: 'verify' })
8990
}
9091
if (scopedForbidden) {
92+
// Only a body that was read and doesn't name a bad key proves the token.
93+
if (errorBody === unreadable) {
94+
throw new TokenServiceAccountValidationError('provider_unavailable', response.status, {
95+
step: 'verify',
96+
reason: 'unreadable 403 body',
97+
})
98+
}
9199
return accepted
92100
}
93101
const callerError =
@@ -97,7 +105,10 @@ export function tokenCheckValidator(check: TokenCheck): TokenServiceAccountValid
97105
throw new TokenServiceAccountValidationError(
98106
callerError ? 'invalid_credentials' : 'provider_unavailable',
99107
response.status,
100-
{ step: 'verify', body: truncate(JSON.stringify(errorBody) ?? '', 500) }
108+
{
109+
step: 'verify',
110+
body: errorBody === unreadable ? '' : truncate(JSON.stringify(errorBody) ?? '', 500),
111+
}
101112
)
102113
}
103114
if (!scopedForbidden) {

‎apps/sim/lib/credentials/token-service-accounts/validators/token-checks.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,22 @@ describe('Brex saved token verification', () => {
6161
})
6262
})
6363

64+
it('does not accept a 403 whose body cannot be read to tell a bad token apart', async () => {
65+
vi.stubGlobal(
66+
'fetch',
67+
vi.fn().mockResolvedValue(
68+
new Response('{"message": "Invalid or Revoked', {
69+
status: 403,
70+
headers: { 'Content-Type': 'application/json' },
71+
})
72+
)
73+
)
74+
75+
await expect(validateBrexServiceAccount({ apiToken: 'bxt_unknown' })).rejects.toMatchObject({
76+
code: 'provider_unavailable',
77+
})
78+
})
79+
6480
it('accepts a valid token that only lacks the user scope', async () => {
6581
vi.stubGlobal(
6682
'fetch',

‎apps/sim/lib/credentials/workload-identity/responses.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,13 @@ function notConfigured() {
2020
return new NextResponse(null, { status: 404 })
2121
}
2222

23+
/** The issuer's OpenID discovery document, or 404 when this deployment has no usable signing key. */
2324
export async function workloadIdentityDiscoveryResponse() {
2425
if (!(await isWorkloadIdentityConfigured())) return notConfigured()
2526
return NextResponse.json(getWorkloadIdentityDiscoveryDocument(), { headers: DISCOVERY_HEADERS })
2627
}
2728

29+
/** The issuer's public signing keys (JWKS), or 404 when this deployment has no usable signing key. */
2830
export async function workloadIdentityJwksResponse() {
2931
const jwks = await getWorkloadIdentityJwks().catch(() => null)
3032
if (!jwks) return notConfigured()

0 commit comments

Comments
 (0)