Harden trash path boundary checks#230
Conversation
![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: e6890039ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if strings.Contains(name, `\`) { | ||
| return fmt.Errorf("trash name must not contain backslash: %s", name) |
There was a problem hiding this comment.
On Windows, legitimate nested trash entries produced by List use filepath.Join(parentRel, name), so their TrashEntry.Name contains \ (for example org\skill). The TUI/server restore flows pass those listed entries back into Restore/RestoreAgent, and this new check rejects them with “must not contain backslash,” making nested restores fail on Windows even though the entries were created by the trash code itself. Normalize listed names to slash form before validation, or accept the OS separator for entries sourced from List.
Useful? React with 👍 / 👎.
| } | ||
| if !strings.HasPrefix(candidate, base+string(filepath.Separator)) { | ||
| return fmt.Errorf("path %q escapes base %q", candidate, base) |
There was a problem hiding this comment.
When a valid global config uses source: . or agents_source: ., restoring a flat item now computes destPath as skill-name, while ensureUnderBase cleans the base to . and then requires the child to start with ./. That rejects normal restores as escaping even though skill-name is inside the current-directory source; use filepath.Rel/absolute normalization instead of this string-prefix check for relative bases.
Useful? React with 👍 / 👎.
2 participants
Fix #229
Summary
This PR hardens path boundary checks in
internal/trash.It addresses a function-level contract weakness found during G703 / CWE-22 path traversal triage:
MoveToTrashpreviously accepted a caller-providednamethat could contain traversal segments and influence the computed trash path.This is treated as defense-in-depth hardening rather than a confirmed external exploit path. Current production callers appear to pass sanitized names, but the trash helpers perform filesystem write/move/delete/restore operations and should enforce their own invariants.
Changes
Add validation for trash-relative names.
Reject traversal-style names, including:
../outside../../outsidea/../../outside./..Preserve safe nested names such as:
org/team-skilldemo/my-agentAdd under-base checks before filesystem operations.
Add a stricter cleanup guard so cleanup cannot remove
trashBaseitself.Add regression tests in
internal/trash/trash_security_test.go.Scope
Modified files:
internal/trash/trash.gointernal/trash/trash_security_test.goNo unrelated server, install, sync, config, or git behavior is changed.
Security note
This PR does not claim a confirmed externally exploitable vulnerability. The issue was confirmed as a contract weakness: manually constructed malicious parameters could violate the intended path invariant, even though production callers currently appear to pass sanitized values.
The patch makes the invariant explicit inside the trash helpers.
Validation
go test ./...passes.go vet ./...passes.