Skip to content

Commit 9176362

Browse files
committed
fix(tools): harden the metadata accessors against inherited keys
Review found two real defects in the generated-metadata layer. `JSON.parse` returns an object with the normal prototype, so a bare bracket lookup resolved inherited members: `getToolMetadata('constructor')` returned a *function* typed as `ToolMetadata`, and `getToolOutputsMetadata('toString')` likewise — silently violating the accessors' documented "undefined if unknown" contract. Guarded with `Object.hasOwn`, with a parameterised regression test over `constructor`, `toString`, `valueOf`, `hasOwnProperty` and `__proto__`. The generator's no-functions scan also gave up past ten levels of nesting. Param and output schemas nest arbitrarily, so a deeper closure would have been dropped silently by `JSON.stringify` while generation reported success — shipping an incomplete schema and defeating the guarantee the scan exists to provide. The depth cap is gone; a `WeakSet` handles the cycles that exposes.
1 parent 07e5f74 commit 9176362

4 files changed

Lines changed: 53 additions & 13 deletions

File tree

apps/sim/tools/metadata-outputs.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,11 @@ type ToolOutputs = NonNullable<ToolConfig['outputs']>
2222
*/
2323
const outputs: Record<string, ToolOutputs> = rawOutputs as Record<string, ToolOutputs>
2424

25-
/** Declared outputs for a built-in tool, or `undefined` if it declares none. */
25+
/**
26+
* Declared outputs for a built-in tool, or `undefined` if it declares none.
27+
*
28+
* `Object.hasOwn` rather than a bare lookup — see `getToolMetadata` for why.
29+
*/
2630
export function getToolOutputsMetadata(toolId: string): ToolOutputs | undefined {
27-
return outputs[toolId]
31+
return Object.hasOwn(outputs, toolId) ? outputs[toolId] : undefined
2832
}

apps/sim/tools/metadata.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,21 @@ describe('generated tool metadata', () => {
3232
expect(getToolOutputsMetadata('gmail_send')).toBeDefined()
3333
})
3434

35+
/**
36+
* `JSON.parse` yields an object with the normal prototype, so a bare bracket
37+
* lookup returns inherited members — `getToolMetadata('constructor')` handed
38+
* back a function typed as tool metadata.
39+
*/
40+
it.each(['constructor', 'toString', 'valueOf', 'hasOwnProperty', '__proto__'])(
41+
'treats inherited key %s as an unknown tool',
42+
(key) => {
43+
expect(getToolMetadata(key)).toBeUndefined()
44+
expect(getToolParams(key)).toBeUndefined()
45+
expect(getToolOutputsMetadata(key)).toBeUndefined()
46+
expect(hasToolMetadata(key)).toBe(false)
47+
}
48+
)
49+
3550
/**
3651
* The registry contains a null param entry (`stt_deepgram_v2`), which crashes
3752
* any consumer that iterates params unguarded. The generator strips those, so

apps/sim/tools/metadata.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,19 @@ export function hasToolMetadata(toolId: string): boolean {
4343
return Object.hasOwn(metadata, toolId)
4444
}
4545

46-
/** Serializable metadata for a built-in tool, or `undefined` if unknown. */
46+
/**
47+
* Serializable metadata for a built-in tool, or `undefined` if unknown.
48+
*
49+
* `Object.hasOwn` rather than a bare lookup: `JSON.parse` yields an object with
50+
* the normal prototype, so a tool id colliding with `constructor`, `toString` or
51+
* `__proto__` would otherwise return an inherited function typed as tool
52+
* metadata.
53+
*/
4754
export function getToolMetadata(toolId: string): ToolMetadata | undefined {
48-
return metadata[toolId]
55+
return Object.hasOwn(metadata, toolId) ? metadata[toolId] : undefined
4956
}
5057

5158
/** Declared parameters for a built-in tool, or `undefined` if unknown. */
5259
export function getToolParams(toolId: string): ToolConfig['params'] | undefined {
53-
return metadata[toolId]?.params
60+
return getToolMetadata(toolId)?.params
5461
}

scripts/sync-tool-metadata.ts

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,35 @@ const METADATA_FIELDS = ['name', 'description', 'version', 'params', 'oauth'] as
5252

5353
type ToolRecord = Record<string, ToolConfig>
5454

55-
/** Recursively locates any function value, which must never reach the artifacts. */
56-
function findFunctionPaths(value: unknown, path: string, found: string[], depth = 0): void {
57-
if (found.length >= 10 || depth > 10 || value == null) return
55+
/**
56+
* Recursively locates any function value, which must never reach the artifacts.
57+
*
58+
* Unbounded in depth on purpose: param and output schemas nest arbitrarily, and
59+
* a depth cap would let a deeply-nested closure through — `JSON.stringify` drops
60+
* it silently, so the artifact would ship an incomplete schema while generation
61+
* reported success. `seen` guards the cycles that removing the cap exposes.
62+
*/
63+
function findFunctionPaths(
64+
value: unknown,
65+
path: string,
66+
found: string[],
67+
seen = new WeakSet<object>()
68+
): void {
69+
if (found.length >= 10 || value == null) return
5870
if (typeof value === 'function') {
5971
found.push(path)
6072
return
6173
}
74+
if (typeof value !== 'object') return
75+
if (seen.has(value as object)) return
76+
seen.add(value as object)
77+
6278
if (Array.isArray(value)) {
63-
value.forEach((item, i) => findFunctionPaths(item, `${path}[${i}]`, found, depth + 1))
79+
value.forEach((item, i) => findFunctionPaths(item, `${path}[${i}]`, found, seen))
6480
return
6581
}
66-
if (typeof value === 'object') {
67-
for (const [key, item] of Object.entries(value as object)) {
68-
findFunctionPaths(item, `${path}.${key}`, found, depth + 1)
69-
}
82+
for (const [key, item] of Object.entries(value as object)) {
83+
findFunctionPaths(item, `${path}.${key}`, found, seen)
7084
}
7185
}
7286

0 commit comments

Comments
 (0)