You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
add a dedicated local attachment reader that revalidates the server-managed attachments/ key
require both lexical and realpath() containment under the upload root
open the verified canonical path with O_NOFOLLOW where supported and accept regular files only
route Agent text attachment extraction through that boundary
cover regular files, final symlink escapes, directory symlink escapes, missing files, invalid namespaces, and directories
Security context
PR #66 removed the remotely controllable URL and traversal path, but its local read boundary still relied on lexical resolve() containment. A symbolic link already present inside UPLOAD_DIR could therefore point the Agent reader outside the upload tree.
This PR closes that defense-in-depth gap. No normal remote path for creating such a symbolic link was identified, so this is not presented as a confirmed remote exploit chain.
Confirmed the gap is real on main: the local branch does resolve(root, key) plus a startsWith check and then readFile, so a link anywhere under the upload tree would be followed. And I appreciate you saying plainly that you found no remote way to create one — hardening described as hardening is more useful than hardening dressed up as an exploit.
The implementation is the right shape: realpath() on both root and candidate rather than the candidate alone, relative() containment instead of startsWith (which has its own /rootfoo edge), O_NOFOLLOW to close the window between resolving and opening, isFile() so a directory or device node can't be read, and re-running normalizeStorageKey inside the helper so it doesn't inherit the caller's trust. Checked the call site too — if (!buf) return null sits right below, so returning null fails closed rather than falling through.
Your CI checklist is fully green, so I've marked it ready and merged. Going out in the release now.
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
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
attachments/keyrealpath()containment under the upload rootO_NOFOLLOWwhere supported and accept regular files onlySecurity context
PR #66 removed the remotely controllable URL and traversal path, but its local read boundary still relied on lexical
resolve()containment. A symbolic link already present insideUPLOAD_DIRcould therefore point the Agent reader outside the upload tree.This PR closes that defense-in-depth gap. No normal remote path for creating such a symbolic link was identified, so this is not presented as a confirmed remote exploit chain.
Validation
Local
node --import tsx --test server/src/__tests__/storage-keys.test.ts server/src/__tests__/local-attachment-files.test.ts(11/11)npm run lintnpm run typechecknpm run server:typechecknpm run guard:big-brainnpm run guard:llm-trackedGitHub CI