fix(penpal): address Codex security findings from PR #297#302
Conversation
- Bind HTTP server to 127.0.0.1 instead of all interfaces - Add path traversal protection to all endpoints that join untrusted input with project paths (handleRawFile, handlePublish, clipboard copy, file delete, add source) - Validate comment sidecar paths stay within the .penpal/comments directory - Add isSubpath helper with tests Closes PENPAL-1 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5311fca680
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Verify the file exists and is under the project directory | ||
| if !strings.HasPrefix(fullPath, project.Path+"/") { | ||
| resolved, err := filepath.Abs(fullPath) | ||
| if err != nil || !isSubpath(project.Path, resolved) { |
There was a problem hiding this comment.
isSubpath(project.Path, resolved) treats resolved == project.Path as valid, so a request with path=. now passes validation and reaches os.Remove(resolved). Unlike the previous project.Path + "/" prefix check, this allows deleting the project directory itself when it is empty (or returning a confusing 500 when it is not), which is a data-loss regression for crafted API calls against empty projects.
Useful? React with 👍 / 👎.
isSubpath(parent, child) previously returned true when child == parent, allowing path=. to pass validation and reach os.Remove in the delete handler. This could delete the project directory itself on empty projects. Make the check strictly "inside parent" so equal paths are rejected. Addresses Codex review comment on PR #302. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Addresses 3 P1 security issues flagged by Codex on #297 (plus 2 additional instances of the same pattern):
":%d"to"127.0.0.1:%d"so penpal only listens on localhostisSubpath()helper and applied it to all 5 endpoints that join untrusted input with project paths:handleRawFile,handlePublish, clipboard copy, file delete, and add sourcecommentsPath()now resolves and validates the full path stays within.penpal/comments/Closes PENPAL-1
Test plan
go test ./...passes inapps/penpalTestIsSubpathcovers valid subpaths, sibling paths, and..traversalcurl localhost:8082/api/raw?project=...&path=../../etc/passwdreturns 400🤖 Generated with Claude Code