-
Notifications
You must be signed in to change notification settings - Fork 8.9k
everything: zip tool: add outputType to control resource vs. resource link output #2831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cliffhall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How has this been tested? The Everything server doesn't have any file resources and if I try giving it a Resource URI it fails.
I tried fetch with a file url and Node v23.11.1 throws error, whether I included "file:///" or not:
TypeError: fetch failed
at node:internal/deps/undici/undici:13510:13
[cause]: Error: not implemented... yet...
When I ask Google about that, I get
The error TypeError: fetch failed with the cause Error: not implemented... yet... occurs because the fetch() function in Node.js does not support the file:// protocol for reading from the local file system.
The fetch() function is designed primarily for making network requests using protocols like http:// and https://.
To solve this issue, you must use Node.js's built-in File System (fs) module instead of fetch().
Seems like there might be at minimum a need to rewrite using fs.
import { readFile } from 'node:fs/promises';
async function useReadFile() {
try {
const filePath = '/Users/zaphod/question.txt';
const fileContent = await readFile(filePath');
console.log('File content:', fileContent);
} catch (error) {
console.error('Error reading file:', error);
}
}The Everything server doesn't have any resources that are files, so what is this tool supposed to fetch?
I'm worried that either implementation would allow fetching of arbitrary files.
Please do some testing and show your work so that we can duplicate it when testing on our side.
src/everything/everything.ts
Outdated
| if (name === ToolName.ZIP_RESOURCES) { | ||
| const { files } = ZipResourcesInputSchema.parse(args); | ||
|
|
||
| const { files, outputType } = ZipResourcesInputSchema.parse(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be some validation that the files are within roots or are valid resources? If I asked for /etc/passwd would it zip up and return it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks!
I'd only tested w/ http(s) & data URIs, now limiting to them / exploding when receiving other protocols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, didn't mean to add Wontfix label, only Waiting for submitter. My bad on that.
ochafik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cliffhall thanks for the review!
How has this been tested? The Everything server doesn't have any file resources and if I try giving it a Resource URI it fails.
...
Please do some testing and show your work so that we can duplicate it when testing on our side.
Sorry I forgot to put the testing instructions from #2830 back in, fixed.
I tried fetch with a file url and Node v23.11.1 throws error, whether I included "file:///" or not:
Somehow hadn't considered file:// urls, now explicitly disallowing them to avoid this can of worms (my motivational use case is data: URIs actually)
src/everything/everything.ts
Outdated
| if (name === ToolName.ZIP_RESOURCES) { | ||
| const { files } = ZipResourcesInputSchema.parse(args); | ||
|
|
||
| const { files, outputType } = ZipResourcesInputSchema.parse(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks!
I'd only tested w/ http(s) & data URIs, now limiting to them / exploding when receiving other protocols.
|
@cliffhall Added some file size limit + timeout to address @domdomegg 's comments on related pr. Also simplified the resource handling / reusing the existing resource array. |
- Changed tool from multi-file zip to single-file gzip compression - Replaced JSZip dependency with node:zlib (built-in) - Updated schema: renamed ZipResourcesInputSchema to GzipResourceInputSchema - Tool now accepts single file with name and data URI parameters - Updated tool name from ZIP_RESOURCES to GZIP_RESOURCE - Updated environment variables (ZIP_* to GZIP_*) - Removed jszip from package.json dependencies 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testing procedure and screenshots do not match what's currently on this branch.
When I run it, I see a different set of fields, with preloaded example.

When I run the tool, I see a successful response, but I do not see a new resource at the bottom of the list.
I now see that if I choose resourceLink output type it just returns the link and I do see the item at the bottom of the list.

It all looks good, but I made a request for a change in the tool description that explains the two forms of response.
Co-authored-by: Cliff Hall <[email protected]>
|
Thanks @cliffhall for testing & for the suggestion! (applied) |
|
@ochafik seems like the |
Follow up on #2830
GZIP_MAX_FETCH_SIZE)GZIP_MAX_FETCH_TIME_MILLISenv var)http:,https:(restricted to domains listed inGZIP_ALLOWED_DOMAINSto mitigate SSRF attacks; defaults to justraw.githubusercontent.com) anddata:URIs; nofile:cc/ @crondinini-ant
To test:
Run gzip tool w/ a url as input (must be on raw.githubusercontent.com)
If selecting
resourceLinkforoutputType, the resource can then be read in the resources tab (hitList More Resourcesuntil reached last page)