Fix trash restore regressions from path hardening#232
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b07ed4c6d
ℹ️ 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".
| // filepath.ToSlash handles OS-native separators. ReplaceAll also handles | ||
| // literal backslashes when Windows-style parent paths are simulated on Unix. | ||
| parentRel = filepath.ToSlash(parentRel) | ||
| parentRel = strings.ReplaceAll(parentRel, `\`, "/") |
There was a problem hiding this comment.
Leave Unix backslashes invalid instead of separators
On Unix, backslash is a legal filename character, not an OS separator. Replacing it here means a malformed or legacy trash parent such as ..\outside is emitted by List() as ../outside/<name>, violating the existing invariant that listed names can be joined under a destination without escaping; filepath.ToSlash already handles real Windows separators when running on Windows, so literal Unix backslashes should remain invalid rather than being translated into logical path separators.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed. I removed the literal backslash replacement and now only rely on filepath.ToSlash() for OS-native separators. Added a Unix guard test to ensure literal backslashes are not translated into /, while validateTrashName() still rejects backslash input.
- List() now uses trashLogicalName() to produce slash-separated logical names, preventing Windows backslash paths that validateTrashName rejects. - ensureUnderBase/ensureStrictlyUnderBase now use filepath.Abs+filepath.Rel instead of string prefix, fixing false escapes when base is '.'. - Add regression tests for Restore(entry, '.'), nested List->Restore roundtrip, sibling prefix rejection, and validateTrashName backslash enforcement.
5b07ed4 to
aa3151e
Compare
|
Thanks! LGTM. |
Summary
This follow-up fixes two compatibility regressions introduced by the trash path hardening change:
List()could use OS-native separators inTrashEntry.Name. On Windows this produced names likeorg\demo, which were then rejected byRestore()becausevalidateTrashName()correctly rejects backslashes.source: ./agents_source: ., was incorrectly rejected by the previous string-prefix containment check.Changes
trashLogicalName(parentRel, name)to build slash-separated logical trash names.List()to use slash-separated logical names likeorg/demoforTrashEntry.Name.TrashEntry.Pathas an OS-native filesystem path.ensureUnderBase/ensureStrictlyUnderBaseto usefilepath.Abs+filepath.Relcontainment checks instead of string-prefix checks.validateTrashName()backslash rejection unchanged.Tests
Added regression and guard coverage:
TestRestoreAllowsCurrentDirectoryDestinationTestEnsureUnderBaseAllowsChildOfDotButRejectsEscapesTestRestoreNestedEntryReturnedByListTestTrashLogicalNameNormalizesOSNativeSeparatorsTestTrashLogicalNameDoesNotTranslateLiteralUnixBackslashesTestEnsureUnderBaseRejectsSiblingPrefixTestValidateTrashNameRejectsBackslashSecurity boundary
This patch preserves the original hardening semantics:
skills-evilare rejected.Cleanup()still refuses to operate on the trash base itself.Validation
go test ./...reaches pre-existing integration failures that require a built CLI binary in PATH. The trash package tests pass, and the observed full-suite failure is unrelated to this patch.