Skip to content

Commit af9eb7b

Browse files
Audit remediation: tests, isolate hardening, framework fixes, error hygiene (#465)
* test(ai-code-mode-skills): add unit test coverage for skill library The package had 13 source files with zero unit tests. Added 116 tests across 9 files covering trust strategies, memory + file storage, skill management tools (including name-validation boundaries for register_skill), bindings, skills-to-tools execution with mocked isolate driver, type generation, the system-prompt renderer, and skill selection with a mocked chat adapter. * feat(ai-isolate-cloudflare): support production deployments and harden tool-name handling The Worker was documented, commented, and configured as if unsafe_eval only worked in wrangler dev. Updated src/worker/index.ts, wrangler.toml, and the README to describe the production path (Cloudflare accounts with the unsafe_eval binding enabled), and pointed users to auth / rate limiting as the real production gate. Also added assertSafeToolName in wrap-code.ts to reject tool names that would break out of the generated function identifier, e.g. "foo'); process.exit(1); (function bar() {". Added tests covering quotes, backticks, whitespace, semicolons, newlines, empty strings, leading digits, and the valid identifier shapes. Added a new escape-attempts.test.ts covering JSON.stringify escaping of adversarial tool-result values and verifying the result lands in a plain object-literal assignment (never a template literal). * refactor(ai-ollama): extract tool-converter with test coverage Tool handling was inlined inside the text adapter with raw type casts. Extracted into src/tools/function-tool.ts + tool-converter.ts matching the structure used by ai-openai, ai-anthropic, ai-grok, and ai-groq. Re-exported as convertFunctionToolToAdapterFormat and convertToolsToProviderFormat from the package index. Added 29 unit tests covering the converter, client utilities (createOllamaClient, getOllamaHostFromEnv, generateId, estimateTokens), and the text adapter's streaming behaviour: RUN/TEXT_MESSAGE/tool-call lifecycle events, id synthesis when Ollama omits a tool-call id, tool forwarding to the SDK in provider format, and structured-output JSON parsing with error wrapping. The package previously had 73 source files and zero unit tests. * fix(frameworks): propagate useChat callback changes after re-render onResponse, onChunk, and onCustomEvent were captured by reference at ChatClient creation time. When a parent component re-rendered with fresh closures, the client kept calling the originals. - ai-react / ai-preact: wrap the three callbacks the same way onFinish/onError already were, reading from optionsRef.current at call time. - ai-vue / ai-solid: wrap the callbacks to read options.xxx at call time. This also fixes a subtler bug where using client.updateOptions to swap callbacks could not clear them (the "!== undefined" guard silently skipped undefined values). - ai-svelte: documented the capture-at-creation behaviour — Svelte's createChat runs once per instance and there's no per-render hook, so callbacks are frozen unless the caller mutates the options object or calls client.updateOptions imperatively. Added a React regression test that rerenders with a new onChunk and verifies the new callback fires while the original does not. * refactor(ai, ai-openai): narrow error handling and stop logging raw errors The three catch blocks that convert thrown values into RUN_ERROR events (stream-to-response.ts, activities/stream-generation-result.ts, activities/generateVideo/index.ts) were using catch(error: any) and dereferencing .message / .code without checks. Added a shared toRunErrorPayload(error, fallback) helper under activities/ that accepts Error instances, plain objects with message/code fields, or bare strings, and funnels all three sites through it with a per-site fallback message. Removed four console.error calls in the OpenAI text adapter's chatStream that dumped the full error object to stdout. SDK errors can carry the original request (including auth headers), so the library no longer logs them; upstream callers should convert errors into structured events. Added 8 unit tests for toRunErrorPayload including a leaked-properties test confirming the helper does not expose extra fields. * test(isolates): add sandbox escape-attempt tests for Node and QuickJS drivers Covers the attack surface a malicious skill / code-mode snippet might probe: process/require/fetch should be unavailable, prototype pollution must not leak to the host or between contexts, synchronous CPU-spin loops must be interrupted by the timeout (not hang), and Function- constructor escape attempts must execute inside the isolate (never returning a real host process object). QuickJS also gets a test that globalThis mutations inside one context do not bleed into a sibling context. * ci: apply automated fixes * fix: address PR review feedback - ai-preact: forward onCustomEvent in useChat (changeset claimed the fix covered preact but it was silently dropped before reaching ChatClient). - ai-isolate-cloudflare: reject JS reserved keywords as tool names (return, class, function, if, await, ...) so the wrapper fails fast at generation time instead of with a cryptic SyntaxError at eval. - ai/src/activities/error-payload: apply typeof string check to the Error branch's code field, matching the plain-object branch. Some SDKs attach numeric or Symbol codes to Error instances. - ai-ollama text-adapter test: strengthen OLLAMA_HOST assertion by tracking the mocked Ollama constructor args, so the test fails if the env var is ignored. - ai-ollama utils test: rename 'when OLLAMA_HOST is unset' to 'empty' since the setup stubs an empty string. - ai-code-mode-skills file-storage test: use vi.useFakeTimers() for the createdAt/updatedAt round-trip instead of a 5ms real sleep. * ci: apply automated fixes * fix(ai, ai-ollama): merge-driven regressions from CR Address CR findings after merging main: - ai-ollama tests: inject testLogger (from resolveDebugOption(false)) into every adapter.chatStream and adapter.structuredOutput call — main's #467 made `logger` required on TextOptions, the PR's new tests were written against the pre-#467 contract and crashed at runtime on `logger.errors`/`logger.request` dereference. - generateVideo: narrow `error` via toRunErrorPayload before handing it to logger.errors. Previously passed the raw error object through the logger meta, which would surface SDK request state (headers, payloads) to any user-supplied logger — defeating the hardening the PR applies to the RUN_ERROR event. - error-narrowing changeset: update wording to match actual code. The OpenAI text adapter's chatStream still logs under the merge, but now through the narrowed `{message, code}` payload rather than raw errors. Changeset previously claimed "the library now re-throws without logging", which didn't match shipped behavior. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 parent 008f015 commit af9eb7b

44 files changed

Lines changed: 3050 additions & 94 deletions

Some content is hidden

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

.changeset/error-narrowing.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@tanstack/ai': patch
3+
'@tanstack/ai-openai': patch
4+
---
5+
6+
refactor(ai, ai-openai): narrow error handling before logging
7+
8+
`catch (error: any)` sites in `stream-to-response.ts`, `activities/stream-generation-result.ts`, and `activities/generateVideo/index.ts` are now narrowed to `unknown` and funnel through a shared `toRunErrorPayload(error, fallback)` helper that extracts `message` / `code` without leaking the original error object (which can carry request state from an SDK).
9+
10+
Replaced four `console.error` calls in the OpenAI text adapter's `chatStream` catch block that dumped the full error object to stdout. SDK errors can carry the original request including auth headers, so the library now logs only the narrowed `{ message, code }` payload via the internal logger — any user-supplied logger receives the sanitized shape, not the raw SDK error.
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
'@tanstack/ai-isolate-cloudflare': patch
3+
---
4+
5+
feat(ai-isolate-cloudflare): support production deployments and close tool-name injection vector
6+
7+
The Worker now documents production-capable `unsafe_eval` usage (previously the code, wrangler.toml, and README all described it as dev-only). Tool names are validated against a strict identifier regex before being interpolated into the generated wrapper code, so a malicious tool name like `foo'); process.exit(1); (function bar() {` is rejected at generation time rather than breaking out of the wrapping function.
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
'@tanstack/ai-ollama': patch
3+
---
4+
5+
refactor(ai-ollama): extract tool conversion into `src/tools/` matching peer adapters
6+
7+
Tool handling lived inline inside the text adapter with raw type casts. It is now split into a dedicated `tool-converter.ts` / `function-tool.ts` pair (mirroring the structure used by `ai-openai`, `ai-anthropic`, `ai-grok`, and `ai-groq`) and re-exported from the package index as `convertFunctionToolToAdapterFormat` and `convertToolsToProviderFormat`. Runtime behavior is unchanged.
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@tanstack/ai-react': patch
3+
'@tanstack/ai-preact': patch
4+
'@tanstack/ai-vue': patch
5+
'@tanstack/ai-solid': patch
6+
---
7+
8+
fix(ai-react, ai-preact, ai-vue, ai-solid): propagate `useChat` callback changes
9+
10+
`onResponse`, `onChunk`, and `onCustomEvent` were captured by reference at client creation time. When a parent component re-rendered with fresh closures, the `ChatClient` kept calling the originals. Every framework now wraps these callbacks so the latest `options.xxx` is read at call time (via `optionsRef.current` in React/Preact, and direct option access in Vue/Solid, matching the pattern already used for `onFinish` / `onError`). Clearing a callback (setting it to `undefined`) now correctly no-ops instead of continuing to invoke the stale handler.
Lines changed: 302 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,302 @@
1+
import { describe, expect, it, vi } from 'vitest'
2+
import { createSkillManagementTools } from '../src/create-skill-management-tools'
3+
import { createMemorySkillStorage } from '../src/storage/memory-storage'
4+
import {
5+
createAlwaysTrustedStrategy,
6+
createDefaultTrustStrategy,
7+
} from '../src/trust-strategies'
8+
9+
const mockContext = () => ({ emitCustomEvent: vi.fn() })
10+
11+
function getTool(
12+
tools: ReturnType<typeof createSkillManagementTools>,
13+
name: string,
14+
) {
15+
const tool = tools.find((t) => t.name === name)
16+
if (!tool) throw new Error(`Tool ${name} not found`)
17+
return tool
18+
}
19+
20+
function validRegisterInput(
21+
overrides: Partial<{
22+
name: string
23+
description: string
24+
code: string
25+
inputSchema: string
26+
outputSchema: string
27+
usageHints: Array<string>
28+
dependsOn: Array<string>
29+
}> = {},
30+
) {
31+
return {
32+
name: 'fetch_data',
33+
description: 'A skill',
34+
code: 'return input;',
35+
inputSchema: '{"type":"object","properties":{}}',
36+
outputSchema: '{"type":"object","properties":{}}',
37+
usageHints: ['Use for fetching'],
38+
dependsOn: [],
39+
...overrides,
40+
}
41+
}
42+
43+
describe('createSkillManagementTools', () => {
44+
it('exposes search_skills, get_skill, and register_skill', () => {
45+
const storage = createMemorySkillStorage([])
46+
const tools = createSkillManagementTools({ storage })
47+
expect(tools.map((t) => t.name).sort()).toEqual([
48+
'get_skill',
49+
'register_skill',
50+
'search_skills',
51+
])
52+
})
53+
54+
describe('search_skills', () => {
55+
it('returns lightweight matching entries', async () => {
56+
const storage = createMemorySkillStorage([
57+
{
58+
id: '1',
59+
name: 'github_stats',
60+
description: 'GitHub stats',
61+
code: 'secret',
62+
inputSchema: {},
63+
outputSchema: {},
64+
usageHints: ['for github'],
65+
dependsOn: [],
66+
trustLevel: 'untrusted',
67+
stats: { executions: 0, successRate: 0 },
68+
createdAt: '',
69+
updatedAt: '',
70+
},
71+
])
72+
const tools = createSkillManagementTools({ storage })
73+
const tool = getTool(tools, 'search_skills')
74+
const results = (await tool.execute!(
75+
{ query: 'github', limit: 5 },
76+
mockContext() as any,
77+
)) as Array<Record<string, unknown>>
78+
expect(results).toHaveLength(1)
79+
expect(results[0]).not.toHaveProperty('code')
80+
expect(results[0]!.name).toBe('github_stats')
81+
})
82+
83+
it('respects the limit parameter', async () => {
84+
const storage = createMemorySkillStorage([
85+
{
86+
id: 'a',
87+
name: 'data_one',
88+
description: '',
89+
code: '',
90+
inputSchema: {},
91+
outputSchema: {},
92+
usageHints: [],
93+
dependsOn: [],
94+
trustLevel: 'untrusted',
95+
stats: { executions: 0, successRate: 0 },
96+
createdAt: '',
97+
updatedAt: '',
98+
},
99+
{
100+
id: 'b',
101+
name: 'data_two',
102+
description: '',
103+
code: '',
104+
inputSchema: {},
105+
outputSchema: {},
106+
usageHints: [],
107+
dependsOn: [],
108+
trustLevel: 'untrusted',
109+
stats: { executions: 0, successRate: 0 },
110+
createdAt: '',
111+
updatedAt: '',
112+
},
113+
])
114+
const tools = createSkillManagementTools({ storage })
115+
const tool = getTool(tools, 'search_skills')
116+
const results = (await tool.execute!(
117+
{ query: 'data', limit: 1 },
118+
mockContext() as any,
119+
)) as Array<unknown>
120+
expect(results).toHaveLength(1)
121+
})
122+
})
123+
124+
describe('get_skill', () => {
125+
it('returns an error object for a missing skill', async () => {
126+
const storage = createMemorySkillStorage([])
127+
const tools = createSkillManagementTools({ storage })
128+
const tool = getTool(tools, 'get_skill')
129+
const result = (await tool.execute!(
130+
{ name: 'missing' },
131+
mockContext() as any,
132+
)) as { error?: string }
133+
expect(result.error).toContain('not found')
134+
})
135+
136+
it('returns the full skill including code when found', async () => {
137+
const storage = createMemorySkillStorage([
138+
{
139+
id: '1',
140+
name: 'alpha',
141+
description: 'Alpha',
142+
code: 'return 1;',
143+
inputSchema: { type: 'object' },
144+
outputSchema: { type: 'number' },
145+
usageHints: ['hint'],
146+
dependsOn: [],
147+
trustLevel: 'untrusted',
148+
stats: { executions: 0, successRate: 0 },
149+
createdAt: '',
150+
updatedAt: '',
151+
},
152+
])
153+
const tools = createSkillManagementTools({ storage })
154+
const tool = getTool(tools, 'get_skill')
155+
const result = (await tool.execute!(
156+
{ name: 'alpha' },
157+
mockContext() as any,
158+
)) as {
159+
name?: string
160+
code?: string
161+
inputSchema?: string
162+
}
163+
expect(result.name).toBe('alpha')
164+
expect(result.code).toBe('return 1;')
165+
expect(result.inputSchema).toBe('{"type":"object"}')
166+
})
167+
})
168+
169+
describe('register_skill', () => {
170+
it('rejects names starting with external_', async () => {
171+
const storage = createMemorySkillStorage([])
172+
const tools = createSkillManagementTools({ storage })
173+
const tool = getTool(tools, 'register_skill')
174+
const result = (await tool.execute!(
175+
validRegisterInput({ name: 'external_evil' }),
176+
mockContext() as any,
177+
)) as { error?: string }
178+
expect(result.error).toContain("cannot start with 'external_'")
179+
})
180+
181+
it('rejects names starting with skill_ (redundant prefix)', async () => {
182+
const storage = createMemorySkillStorage([])
183+
const tools = createSkillManagementTools({ storage })
184+
const tool = getTool(tools, 'register_skill')
185+
const result = (await tool.execute!(
186+
validRegisterInput({ name: 'skill_duplicate' }),
187+
mockContext() as any,
188+
)) as { error?: string }
189+
expect(result.error).toContain("should not include the 'skill_' prefix")
190+
})
191+
192+
it('rejects malformed JSON inputSchema', async () => {
193+
const storage = createMemorySkillStorage([])
194+
const tools = createSkillManagementTools({ storage })
195+
const tool = getTool(tools, 'register_skill')
196+
const result = (await tool.execute!(
197+
validRegisterInput({ inputSchema: 'not valid json' }),
198+
mockContext() as any,
199+
)) as { error?: string }
200+
expect(result.error).toContain('inputSchema must be a valid JSON string')
201+
})
202+
203+
it('rejects malformed JSON outputSchema', async () => {
204+
const storage = createMemorySkillStorage([])
205+
const tools = createSkillManagementTools({ storage })
206+
const tool = getTool(tools, 'register_skill')
207+
const result = (await tool.execute!(
208+
validRegisterInput({ outputSchema: '{' }),
209+
mockContext() as any,
210+
)) as { error?: string }
211+
expect(result.error).toContain('outputSchema must be a valid JSON string')
212+
})
213+
214+
it('rejects a duplicate name', async () => {
215+
const storage = createMemorySkillStorage([
216+
{
217+
id: '1',
218+
name: 'existing',
219+
description: '',
220+
code: '',
221+
inputSchema: {},
222+
outputSchema: {},
223+
usageHints: [],
224+
dependsOn: [],
225+
trustLevel: 'untrusted',
226+
stats: { executions: 0, successRate: 0 },
227+
createdAt: '',
228+
updatedAt: '',
229+
},
230+
])
231+
const tools = createSkillManagementTools({ storage })
232+
const tool = getTool(tools, 'register_skill')
233+
const result = (await tool.execute!(
234+
validRegisterInput({ name: 'existing' }),
235+
mockContext() as any,
236+
)) as { error?: string }
237+
expect(result.error).toContain('already exists')
238+
})
239+
240+
it('persists a valid skill with defaults', async () => {
241+
const storage = createMemorySkillStorage([])
242+
const tools = createSkillManagementTools({ storage })
243+
const tool = getTool(tools, 'register_skill')
244+
const result = (await tool.execute!(
245+
validRegisterInput({ name: 'valid_skill' }),
246+
mockContext() as any,
247+
)) as { success?: boolean; skillId?: string }
248+
expect(result.success).toBe(true)
249+
expect(result.skillId).toMatch(/^[0-9a-f-]{36}$/)
250+
251+
const saved = await storage.get('valid_skill')
252+
expect(saved).not.toBeNull()
253+
expect(saved!.stats).toEqual({ executions: 0, successRate: 0 })
254+
})
255+
256+
it('applies the trust strategy to set initial trust level', async () => {
257+
const storage = createMemorySkillStorage([])
258+
const tools = createSkillManagementTools({
259+
storage,
260+
trustStrategy: createAlwaysTrustedStrategy(),
261+
})
262+
const tool = getTool(tools, 'register_skill')
263+
await tool.execute!(
264+
validRegisterInput({ name: 's1' }),
265+
mockContext() as any,
266+
)
267+
const saved = await storage.get('s1')
268+
expect(saved!.trustLevel).toBe('trusted')
269+
})
270+
271+
it('prefers explicit trustStrategy over storage.trustStrategy', async () => {
272+
const storage = createMemorySkillStorage({
273+
trustStrategy: createAlwaysTrustedStrategy(),
274+
})
275+
const tools = createSkillManagementTools({
276+
storage,
277+
trustStrategy: createDefaultTrustStrategy(),
278+
})
279+
const tool = getTool(tools, 'register_skill')
280+
await tool.execute!(
281+
validRegisterInput({ name: 's1' }),
282+
mockContext() as any,
283+
)
284+
const saved = await storage.get('s1')
285+
expect(saved!.trustLevel).toBe('untrusted')
286+
})
287+
288+
it('falls back to storage.trustStrategy when none provided', async () => {
289+
const storage = createMemorySkillStorage({
290+
trustStrategy: createAlwaysTrustedStrategy(),
291+
})
292+
const tools = createSkillManagementTools({ storage })
293+
const tool = getTool(tools, 'register_skill')
294+
await tool.execute!(
295+
validRegisterInput({ name: 's1' }),
296+
mockContext() as any,
297+
)
298+
const saved = await storage.get('s1')
299+
expect(saved!.trustLevel).toBe('trusted')
300+
})
301+
})
302+
})

0 commit comments

Comments
 (0)