Skip to content

Commit f6711ea

Browse files
committed
fix(secrets): read visible values by own property so prototype-named secrets cannot poison the list
1 parent 1848e66 commit f6711ea

4 files changed

Lines changed: 53 additions & 2 deletions

File tree

apps/sim/app/api/v2/secrets/route.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,37 @@ describe('GET /api/v2/secrets', () => {
176176
})
177177
})
178178

179+
it('never attaches an inherited prototype member as a missing value', async () => {
180+
mocks.list.mockResolvedValue({
181+
secrets: [
182+
{
183+
...secret,
184+
id: 'secret-proto',
185+
displayName: 'constructor',
186+
envKey: 'constructor',
187+
unredacted: true,
188+
},
189+
],
190+
/** The name is legal but its value is absent — a bare index would read Object's constructor. */
191+
values: {},
192+
userId: 'user-1',
193+
nextCursorKeys: null,
194+
sortBy: 'name',
195+
sortOrder: 'asc',
196+
})
197+
198+
const response = await GET(
199+
new NextRequest(`http://localhost:3000/api/v2/secrets?workspaceId=${WORKSPACE_ID}`, {
200+
headers: { 'x-api-key': 'key' },
201+
})
202+
)
203+
const body = await response.json()
204+
205+
expect(response.status).toBe(200)
206+
expect(body.data[0]).toMatchObject({ name: 'constructor', unredacted: true })
207+
expect(body.data[0]).not.toHaveProperty('value')
208+
})
209+
179210
/**
180211
* Pins the binding end-to-end — the mint in `present` and the read in
181212
* `mapInput` — because the contract-level sweep only checks a hand-maintained

apps/sim/app/api/v2/secrets/route.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,16 @@ export const GET = defineV2JsonRoute({
4242
useCase: listSecretsUseCase,
4343
present: ({ secrets, values, userId, nextCursorKeys }, { query }) => ({
4444
data: secrets.map((secret) =>
45-
toV2Secret(secret, userId, secret.envKey ? values[secret.envKey] : undefined)
45+
toV2Secret(
46+
secret,
47+
userId,
48+
/**
49+
* Own-property read: a secret may legally be named `constructor` or `toString`,
50+
* and a bare index on a missing key would hand the inherited function to the
51+
* serializer and fail response validation for the whole page.
52+
*/
53+
secret.envKey && Object.hasOwn(values, secret.envKey) ? values[secret.envKey] : undefined
54+
)
4655
),
4756
nextCursor: writeSortedCursor(
4857
nextCursorKeys,

apps/sim/lib/credentials/secret-values.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,4 +269,15 @@ describe('readWorkspaceSecretValues', () => {
269269
).resolves.toEqual({})
270270
expect(mockDecryptSecret).not.toHaveBeenCalled()
271271
})
272+
273+
it('never reads an inherited prototype member for a missing key', async () => {
274+
queueTableRows(schemaMock.workspaceEnvironment, [
275+
{ id: 'env-1', variables: { OTHER_KEY: 'encrypted-other' } },
276+
])
277+
278+
await expect(
279+
readWorkspaceSecretValues({ workspaceId: 'workspace-1', names: ['constructor', 'toString'] })
280+
).resolves.toEqual({})
281+
expect(mockDecryptSecret).not.toHaveBeenCalled()
282+
})
272283
})

apps/sim/lib/credentials/secret-values.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export async function readWorkspaceSecretValues(params: {
5151
const values: Record<string, string> = {}
5252
await Promise.all(
5353
params.names.map(async (name) => {
54-
const encrypted = variables[name]
54+
const encrypted = Object.hasOwn(variables, name) ? variables[name] : undefined
5555
if (!encrypted) return
5656
try {
5757
const { decrypted } = await decryptSecret(encrypted)

0 commit comments

Comments
 (0)