fix: bound attachment filenames - #62
Conversation
vycdev
left a comment
There was a problem hiding this comment.
Found one path-safety blocker. The normal filename and traversal tests pass, but truncation can alter path normalization after the existing validation.
| ) { | ||
| throw new Error("Attachment filename resolves outside the images directory"); | ||
| } | ||
| const filePath = path.resolve(IMAGES_DIR, boundFilename(filename)); |
There was a problem hiding this comment.
The resolved path is validated before boundFilename() changes it, so the final path can escape IMAGES_DIR. I reproduced this with "../" + "a".repeat(300) + "/../images/safe.png": the original path normalizes inside images, passes the check, then truncation removes the suffix that brought it back inside and fs.writeFileSync writes into the parent directory. Please validate the final bounded path (or bound only a validated basename) and add this normalization/truncation case as a regression test.
There was a problem hiding this comment.
Addressed in the current head (f4adf4f): downloadAttachment() now rejects any input that is not already a basename before passing it to boundFilename(), so bounding only operates on a validated basename. The regression test uses the reported ../${"a".repeat(300)}/../images/safe.png normalization/truncation case and verifies it is rejected before fetch or write. Re-ran npm test: 111 tests passed.
This was generated by an AI agent (vycdev2). Please verify any changes before merging or applying.
f16855d to
f4adf4f
Compare
|
Addressed the path-safety review by rebuilding the change on current This was generated by an AI agent (vycdev2). Please verify any changes before merging or applying. |
Summary
Verification
npm test— passed: TypeScript build and all 111 testsgit diff --check main...HEAD— passedRisk
This was generated by an AI agent (vycdev2). Please verify any changes before merging or applying.