Repository navigation
Conversation
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Route the new filesystem tools through existing path permissions
src/tools/CsvTool/CsvTool.ts:82
CsvToolandJsonToolmark themselves read-only and then read arbitrary paths directly withreadFileSync, whileFileArchiveToolreturnsallowforlistand only asks generically forcreate/extract. None of these paths go through the existingcheckReadPermissionForTool/checkWritePermissionForToolflow thatFileReadTooland edit/write tools use, so a model can use these new tools to read or touch files outside the allowed working directories, ignore explicit read-deny rules, and skip the suspicious/UNC path checks. Please wire these tools into the same filesystem permission helpers, including checking every archive source path and the destination for create/extract. -
[P1] Restore bundle feature gates instead of hardcoding them
src/tools.ts:18
This PR replaces thefeature(...)gates insrc/tools.tswith literals, which permanently disables several feature-gated tools (SleepTool, remote triggers, push notifications, history snip, workflows, etc.) and permanently enables others (MonitorTool, coordinator mode) regardless of the bundle/build flags. That is a broad behavior change unrelated to the data-processing tools and will make builds expose or hide tools incorrectly. Please keep thebun:bundlefeature()checks and only add the three new tools. -
[P1] Do not extract archives before validating entry paths
src/tools/FileArchiveTool/FileArchiveTool.ts:83
The archive tool shells out tounzip -o/tar -xfdirectly, but the prompt promises that extraction validates paths to prevent directory traversal. As written, a malicious archive can be handed to this structured tool and extraction is delegated before the implementation inspects entries for absolute paths,..segments, symlinks/hardlinks, or paths that resolve outside the requested destination. Please list and validate entries before extraction, fail closed on unsafe members, and add coverage for traversal cases.
techbrewboss
left a comment
There was a problem hiding this comment.
Review summary
Thanks for putting this together. Structured CSV/JSON/archive tooling is directionally useful for OpenClaude, but this implementation crosses sensitive filesystem and tool-registration boundaries. I think this needs changes before merge because the new tools bypass existing permission checks, archive extraction is unsafe, and src/tools.ts changes unrelated feature gates.
Findings
-
src/tools.ts:18- Restore bundle feature gates instead of hardcoding them.
Impact: The PR permanently disables several gated tools and permanently enables others, includingMonitorTooland coordinator-mode paths, regardless of bundle flags. That is a broad runtime behavior change unrelated to data-processing tools and will make builds expose or hide tools incorrectly.
Suggested fix: Restoreimport { feature } from 'bun:bundle'and the originalfeature(...)checks; only add the three new tool imports/registrations. -
src/tools/CsvTool/CsvTool.ts:103,src/tools/JsonTool/JsonTool.ts:86,src/tools/FileArchiveTool/FileArchiveTool.ts:55- Route the new filesystem tools through existing read/write permissions.
Impact: These tools can read arbitrary files, list archives, or write/extract archives without using the samecheckReadPermissionForTool/checkWritePermissionForToolflow used byFileReadToolandFileWriteTool. That bypasses read-deny rules and filesystem safety checks.
Suggested fix: Wire every source path through read permissions and every destination/write path through write permissions, including all archive sources whensourceis an array. -
src/tools/FileArchiveTool/FileArchiveTool.ts:83- Validate archive entries before extraction.
Impact: The prompt promises traversal protection, butunzip -o/tar -xfrun directly. A crafted archive can write outside the requested destination or overwrite existing files before the implementation inspects entries.
Suggested fix: List archive entries first, reject absolute paths,.., unsafe symlinks/hardlinks, and paths resolving outside the destination, then extract only after validation. Please add traversal coverage. -
src/tools/JsonTool/prompt.ts:3- Remove or implement the advertisedusers[].namesyntax.
Impact: The prompt advertises array collection, but the implementation strips[]and then tries to readnamefrom the array object, returningundefined. Users will get behavior that contradicts the tool instructions.
Suggested fix: Either implement collection over array elements or remove the unsupported syntax and the “JMESPath-style” wording.
Validation
I inspected the PR metadata/diff, compared the new tools against existing OpenClaude tool permission patterns, and ran focused checks locally:
bun test src/tools/CsvTool/CsvTool.test.ts src/tools/JsonTool/JsonTool.test.ts src/tools/FileArchiveTool/FileArchiveTool.test.tspassed: 39 tests.bun run security:pr-scanreported no suspicious additions.bun run typecheckwas not useful as a PR signal in this checkout because it produced many broad existing/environment missing-module errors outside this change.
|
Closing this PR. Adding multiple new tools in a single PR without prior maintainer discussion is not the right approach for a 25k+ star project. If you want to contribute tools, please:
Bulk tool additions create review burden and maintenance overhead. |
|
Bulk tool addition without prior discussion. |
Summary
FileArchiveTool(zip/tar/tar.gz/gz archive management),CsvTool(CSV reading, filtering, statistics), andJsonTool(JSON reading, dot-notation querying, validation).Impact
buildTool({...})with propercall()contract,mapToolResultToToolResultBlockParam,getPath,checkPermissions. No new npm dependencies. FileArchive delegates to system CLIs (zip/unzip/tar/gzip). CsvTool/JsonTool use native readFileSync.Testing — 39/39 passing
Implementation Detail
Notes