rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass #152

Merged
rodin merged 8 commits from issue-150 into main 2026-05-18 19:09:30 +00:00
Conversation 88 Commits 8 Files Changed 2
+3
1 changed files with 3 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit eb0ff3aa69 - Show all commits
cmd/review-bot/validatedocmap_test.go
+3
View File
3
 @@ -687,6 +687,9 @@ func TestValidateDocmapPath_InRepoSymlinkAllowed(t *testing.T) {
t.Fatalf("expected in-repo symlink to be accepted, got error: %v", err)
}
// The returned resolved path must be the real file (not the symlink entry).
// validateDocmapPath calls filepath.EvalSymlinks internally, so the returned
// path is always the fully-resolved real path — it can never equal the
// symlink entry itself.
if resolved == symlinkPath {
sonnet-review-bot commented 2026-05-15 23:12:02 +00:00
Review
Copy Link
Copy Source

[NIT] The new TestValidateDocmapPath_InRepoSymlinkAllowed test checks if resolved == symlinkPath to verify that the returned path differs from the symlink. On some systems (e.g., macOS where /tmp is a symlink to /private/tmp), t.TempDir() already returns an EvalSymlinks-resolved path, but symlinkPath is constructed via filepath.Join(dir, ...) which may not yet be resolved. The comparison is reliable here because validateDocmapPath calls EvalSymlinks internally, so resolved will always differ from symlinkPath (a symlink entry). The assertion is correct but a comment clarifying why they must differ would help future readers.

**[NIT]** The new `TestValidateDocmapPath_InRepoSymlinkAllowed` test checks `if resolved == symlinkPath` to verify that the returned path differs from the symlink. On some systems (e.g., macOS where `/tmp` is a symlink to `/private/tmp`), `t.TempDir()` already returns an EvalSymlinks-resolved path, but `symlinkPath` is constructed via `filepath.Join(dir, ...)` which may not yet be resolved. The comparison is reliable here because `validateDocmapPath` calls `EvalSymlinks` internally, so `resolved` will always differ from `symlinkPath` (a symlink entry). The assertion is correct but a comment clarifying why they must differ would help future readers.
t.Errorf("expected resolved path to differ from symlink path")
}