fix(ai-code-mode): warn when tool parameters look like secrets#431
Open
AlemTuzlak wants to merge 6 commits intomainfrom
Open
fix(ai-code-mode): warn when tool parameters look like secrets#431AlemTuzlak wants to merge 6 commits intomainfrom
AlemTuzlak wants to merge 6 commits intomainfrom
Conversation
Code Mode executes LLM-generated code in a sandbox. If a tool's input schema includes parameters like apiKey, token, or password, the LLM-generated code can access those values and potentially exfiltrate them via tool calls. Add warnIfBindingsExposeSecrets() that scans tool input schemas for secret-like parameter names and emits console.warn during development.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughA secret-parameter detection system was added to Code Mode tools. It scans tool input schemas for parameter names resembling secrets (e.g., Changes
Sequence DiagramsequenceDiagram
actor Dev as Developer
participant CM as createCodeModeTool
participant VB as warnIfBindingsExposeSecrets
participant H as Handler
Dev->>CM: Create tool with config & static external_* bindings
activate CM
CM->>VB: Scan static bindings (pass dedupCache)
activate VB
VB->>VB: Traverse schemas, resolve local $defs, detect matches
VB-->>H: Invoke handler (warn/throw/ignore/callback)
deactivate VB
Note over CM: store secretDedupCache
CM-->>Dev: Return tool
deactivate CM
Dev->>CM: Execute tool with dynamic skill bindings
activate CM
CM->>VB: Scan dynamic bindings (reuse dedupCache)
activate VB
VB->>VB: Traverse schemas + dedupe matches
alt new secret match
VB-->>H: Invoke handler with SecretParameterInfo
end
deactivate VB
deactivate CM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🚀 Changeset Version Preview12 package(s) bumped directly, 21 bumped as dependents. 🟥 Major bumps
🟨 Minor bumps
🟩 Patch bumps
|
|
View your CI Pipeline Execution ↗ for commit 2dea51e
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-anthropic
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/typescript/ai-code-mode/src/validate-bindings.ts (1)
91-136: Optional: sharedseenacross siblings suppresses reports for duplicated$ref/subschema references.
seenis threaded through the entire traversal, so once a subschema object (e.g. a$defs/Credstarget) has been visited via one reference site, any other reference site to the same object is skipped entirely. In practice this means: ifcreds1andcreds2both$refthe same definition that containsaccessToken, onlycreds1.accessTokenwill be reported;creds2.accessTokenwon't appear infoundat all. With the per-instancededupCacheincreate-code-mode-tool.ts, the user still gets a warning for the tool, so impact is low — but the reportedparamPathlist under-represents the real exposure surface.If you want full coverage, track cycle-breaking per-path (e.g. a
Setofobjectseeded only with ancestors on the current DFS stack, removed after recursion) rather than a single globalseen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai-code-mode/src/validate-bindings.ts` around lines 91 - 136, The traversal currently uses a global `seen` that permanently marks visited subschema objects, which suppresses reporting when the same schema object is referenced from different paths; in function findSecretParams change `seen` to act as an ancestor/stack-local cycle detector by removing the schema from `seen` before returning (i.e., after all recursive calls complete, call seen.delete(schema)), so the object is only treated as visited for the current DFS path and other sibling/reference sites will be explored; keep using the same `seen` Set parameter but ensure you delete the schema at the end of findSecretParams.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/typescript/ai-code-mode/src/validate-bindings.ts`:
- Around line 91-136: The traversal currently uses a global `seen` that
permanently marks visited subschema objects, which suppresses reporting when the
same schema object is referenced from different paths; in function
findSecretParams change `seen` to act as an ancestor/stack-local cycle detector
by removing the schema from `seen` before returning (i.e., after all recursive
calls complete, call seen.delete(schema)), so the object is only treated as
visited for the current DFS path and other sibling/reference sites will be
explored; keep using the same `seen` Set parameter but ensure you delete the
schema at the end of findSecretParams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9159be81-e9e3-4194-af32-e06b98bcf0ae
📒 Files selected for processing (5)
.changeset/fix-code-mode-secret-warning.mdpackages/typescript/ai-code-mode/src/create-code-mode-tool.tspackages/typescript/ai-code-mode/src/types.tspackages/typescript/ai-code-mode/src/validate-bindings.tspackages/typescript/ai-code-mode/tests/validate-bindings.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/typescript/ai-code-mode/tests/validate-bindings.test.ts (4)
333-348: Also assert noconsole.warnon the'throw'path.This test doesn’t spy on
console.warn, so two things are left unverified/unsafe:
- If the implementation happens to call
console.warnbefore throwing, that output leaks into the test runner’s stderr.- The intended contract —
handler: 'throw'throws instead of warning — isn’t actually asserted.Adding a spy and a
not.toHaveBeenCalled()assertion tightens the contract:♻️ Suggested change
it('handler: "throw" throws on the first match', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) expect(() => warnIfBindingsExposeSecrets( [ { name: 't', inputSchema: { type: 'object', properties: { apiKey: { type: 'string' } }, }, }, ], { handler: 'throw' }, ), ).toThrow(/apiKey/) + expect(warnSpy).not.toHaveBeenCalled() + warnSpy.mockRestore() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` around lines 333 - 348, Update the test for handler: "throw" to also spy on console.warn and assert it was not called: before invoking warnIfBindingsExposeSecrets create a jest.spyOn(console, 'warn').mockImplementation(() => {}) (or equivalent test spy), run the function expecting toThrow(/apiKey/), then assert the spy.not.toHaveBeenCalled() and finally restore the spy (spy.mockRestore()) to avoid cross-test pollution; reference the test case and the warnIfBindingsExposeSecrets invocation to locate where to add the spy and assertions.
350-383: Order-dependent assertion onmatches[0]/matches[1].
toHaveLength(2)plus indexedtoMatchObjectassumes the traversal emitsapiKeystrictly beforenested.token. That happens to match insertion order today, but it couples the test to the implementation’s DFS order rather than the observable contract ("each match is reported exactly once"). Consider asserting set-membership instead so a later traversal refactor (e.g., BFS,anyOfreordering) doesn't break the test:♻️ Suggested change
- expect(matches).toHaveLength(2) - expect(matches[0]).toMatchObject({ - toolName: 'callApi', - paramName: 'apiKey', - paramPath: ['apiKey'], - }) - expect(matches[1]).toMatchObject({ - toolName: 'callApi', - paramName: 'token', - paramPath: ['nested', 'token'], - }) + expect(matches).toHaveLength(2) + expect(matches).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + toolName: 'callApi', + paramName: 'apiKey', + paramPath: ['apiKey'], + }), + expect.objectContaining({ + toolName: 'callApi', + paramName: 'token', + paramPath: ['nested', 'token'], + }), + ]), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` around lines 350 - 383, The test currently assumes traversal order by checking matches[0] and matches[1]; update the assertions in the 'handler: function receives each match' spec to be order-independent: collect the pushed SecretParameterInfo objects from warnIfBindingsExposeSecrets (the matches array) and assert that it has length 2 and contains both expected objects using set-membership style assertions (e.g., expect.arrayContaining or multiple expect(matches).toContainEqual calls) referencing the same toolName 'callApi' and paramName/paramPath shapes rather than relying on index-based checks.
80-161: Nice coverage of naming variants — consider one more edge case.The
secretNames/safeNamesmatrix is thorough. One gap worth adding given the PR’s goal of catching "the common antipattern of passing API keys as tool parameters": a case-insensitive variant of a compound name likeOPENAI_API_KEYorSLACK_WEBHOOK_SECRET(SCREAMING_SNAKE_CASE env-style names often copy-pasted into schemas). If the detector already normalizes case, it’s a free test; if it doesn’t, this surfaces a real gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` around lines 80 - 161, Add tests covering SCREAMING_SNAKE_CASE secret-like names so the detector handles case-insensitive compound env-style names: extend the secretNames array used by the warnIfBindingsExposeSecrets tests to include examples like 'OPENAI_API_KEY' and 'SLACK_WEBHOOK_SECRET' (or other UPPER_SNAKE variants), then ensure the existing test that expects a warning for each secretName still passes; reference the warnIfBindingsExposeSecrets call and the secretNames array in validate-bindings.test.ts to locate where to add these entries.
7-7: Consider centralizingconsole.warnspy restoration viaafterEach.Every test manually creates a
vi.spyOn(console, 'warn')and callswarnSpy.mockRestore()at the end. If anyexpectin a test fails,mockRestore()is skipped and the spy leaks into subsequent tests, which can mask or alter assertions (e.g.,toHaveBeenCalledTimesin the dedup suite). A singleafterEach(() => vi.restoreAllMocks())(plus removing the per-testmockRestore()calls) makes the suite leak-safe and a bit less repetitive.♻️ Sketch
import { describe, expect, it, vi } from 'vitest' +import { afterEach } from 'vitest' ... +afterEach(() => { + vi.restoreAllMocks() +})Also applies to: 27-27, 47-47, 71-71, 110-110, 146-146, 165-165, 189-189, 216-216, 248-248, 275-275, 297-297, 314-314, 388-388, 407-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` at line 7, Centralize console.warn spy cleanup by adding an afterEach(() => vi.restoreAllMocks()) in the test suite and remove all per-test warnSpy.mockRestore() calls; keep the existing vi.spyOn(console, 'warn') usages (e.g., the const warnSpy = vi.spyOn(console, 'warn') lines) but rely on global restoration so tests won't leak spies and assertions like toHaveBeenCalledTimes remain reliable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts`:
- Around line 333-348: Update the test for handler: "throw" to also spy on
console.warn and assert it was not called: before invoking
warnIfBindingsExposeSecrets create a jest.spyOn(console,
'warn').mockImplementation(() => {}) (or equivalent test spy), run the function
expecting toThrow(/apiKey/), then assert the spy.not.toHaveBeenCalled() and
finally restore the spy (spy.mockRestore()) to avoid cross-test pollution;
reference the test case and the warnIfBindingsExposeSecrets invocation to locate
where to add the spy and assertions.
- Around line 350-383: The test currently assumes traversal order by checking
matches[0] and matches[1]; update the assertions in the 'handler: function
receives each match' spec to be order-independent: collect the pushed
SecretParameterInfo objects from warnIfBindingsExposeSecrets (the matches array)
and assert that it has length 2 and contains both expected objects using
set-membership style assertions (e.g., expect.arrayContaining or multiple
expect(matches).toContainEqual calls) referencing the same toolName 'callApi'
and paramName/paramPath shapes rather than relying on index-based checks.
- Around line 80-161: Add tests covering SCREAMING_SNAKE_CASE secret-like names
so the detector handles case-insensitive compound env-style names: extend the
secretNames array used by the warnIfBindingsExposeSecrets tests to include
examples like 'OPENAI_API_KEY' and 'SLACK_WEBHOOK_SECRET' (or other UPPER_SNAKE
variants), then ensure the existing test that expects a warning for each
secretName still passes; reference the warnIfBindingsExposeSecrets call and the
secretNames array in validate-bindings.test.ts to locate where to add these
entries.
- Line 7: Centralize console.warn spy cleanup by adding an afterEach(() =>
vi.restoreAllMocks()) in the test suite and remove all per-test
warnSpy.mockRestore() calls; keep the existing vi.spyOn(console, 'warn') usages
(e.g., the const warnSpy = vi.spyOn(console, 'warn') lines) but rely on global
restoration so tests won't leak spies and assertions like toHaveBeenCalledTimes
remain reliable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3cc6a73-a4a6-46a3-a941-1476498eb472
📒 Files selected for processing (1)
packages/typescript/ai-code-mode/tests/validate-bindings.test.ts
Address CR findings on the secret-parameter warning heuristic:
- Recurse into nested object properties, array items, anyOf/oneOf/allOf
branches, additionalProperties, and `$ref` targets — previously only
top-level properties were scanned, so `{ auth: { token: string } }`
slipped through.
- Replace the narrow anchored regex with a two-stage matcher
(camelCase/snake/kebab word tokenization + compound-substring check)
so common names now hit: `accessToken`, `bearerToken`, `refreshToken`,
`sessionToken`, `clientSecret`, `x-api-key`, `openaiApiKey`,
`passcode`, `pwd`, `jwt`, `Authorization`. Safe names stay safe:
`tokenizer`, `tokens`, `foreignKey`, `sortKey`, `email`, `username`.
- Add `onSecretParameter` config option with `'warn' | 'throw' |
'ignore' | fn` variants so consumers can route matches (throw in CI,
ignore in trusted environments, log to an observability pipeline).
- Dedupe per `(toolName, paramPath)` across a single code-mode instance
to stop the same binding warning on every execute call.
- Scan dynamic `getSkillBindings()` output too, with the same dedup
cache; skill bindings are in the same exfiltration threat model.
Tests: 56 cases covering every pattern/safe-name pair, nested +
array + union + $ref + additionalProperties + cycle safety, and each
handler variant + dedup behavior.
AlemTuzlak
force-pushed
the
fix/code-mode-secret-warning
branch
from
99163d6 to
6287faa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
warnIfBindingsExposeSecrets()that scans tool input schemas for secret-like parameter names (apiKey,token,password,credential,secret,access_key,private_key,auth_token) and emitsconsole.warnduring developmentTest plan
@tanstack/ai-code-modetests passSummary by CodeRabbit