Open
Conversation
OrKoN
force-pushed
the
orkon/igotnoroots
branch
3 times, most recently
from
eba7359 to
51e3291
Compare
OrKoN
changed the title
wolfib
approved these changes
Apr 23, 2026
OrKoN
force-pushed
the
orkon/igotnoroots
branch
2 times, most recently
from
a11f796 to
abf7160
Compare
Collaborator
Author
|
I realized I might have had a bug in index.ts. PT(another)L. |
wolfib
approved these changes
Apr 23, 2026
OrKoN
force-pushed
the
orkon/igotnoroots
branch
3 times, most recently
from
4823288 to
314d91d
Compare
Collaborator
Author
|
Sorry, I realised again that we should treat empty roots differently from undefined roots (rejecting any file access with empty roots). PTAL |
OrKoN
force-pushed
the
orkon/igotnoroots
branch
from
314d91d to
d910a7a
Compare
wolfib
approved these changes
Apr 23, 2026
Comment on lines
+677
to
+679
| const filepath = await getTempFilePath(filename); | ||
| this.validatePath(filepath); | ||
| await fs.writeFile(filepath, data); |
Contributor
There was a problem hiding this comment.
Should this be wrapped in a try..catch?
Collaborator
Author
There was a problem hiding this comment.
I think no, why would it need a try/catch?
Contributor
There was a problem hiding this comment.
fs.writeFile might throw an exception. Maybe it's already caught elsewhere, but it's not obvious.
The previous saveTemporaryFile did catch exceptions too.
| * Only add methods used by tools/*. | ||
| */ | ||
| export type Context = Readonly<{ | ||
| validatePath(filePath: string): void; |
Collaborator
There was a problem hiding this comment.
Should this be
Suggested change
| validatePath(filePath: string): void; | |
| validatePath(filePath?: string): void; |
So we can ignore some if statements?
Collaborator
Author
There was a problem hiding this comment.
Good idea, let me re-visit this next week.
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
3 participants
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.
This PR implements https://modelcontextprotocol.io/specification/2025-11-25/client/roots If client specifies roots, all reads and writes to the file system originating in tool calls will denied (including tmp files). The client specified empty list of roots, all filesystem access will be restricted.
Closes #1860