Compare commits

..

2 Commits

Author SHA1 Message Date
Rodin eb0ff3aa69 nit(#150): clarify why resolved != symlinkPath in InRepoSymlinkAllowed test
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m0s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m3s
Add a comment explaining that validateDocmapPath calls EvalSymlinks
internally, so the returned path is always the fully-resolved real path
and can never equal the symlink entry itself.

Addresses sonnet bot NIT (review 4810) against d6bab7a9.
2026-05-15 16:23:11 -07:00
Rodin c76e7dcd2e fix(#150): add os.SameFile check after open to close Lstat→open TOCTOU window
After getting the resolved path from validateDocmapPath, Lstat the path
immediately before os.Open, then compare with f.Stat() after open using
os.SameFile. If the file was swapped between validation and open (e.g.,
replaced with a symlink pointing outside the repo), the inode comparison
catches it and returns an error.

Also changes defer f.Close() // nolint:errcheck to
defer func() { _ = f.Close() }() to follow the project convention of
explicit ignores over suppressor comments.

Addresses security bot finding (review 4812) against d6bab7a9.
2026-05-15 16:23:07 -07:00
2 changed files with 25 additions and 1 deletions
+22 -1
View File
@@ -156,12 +156,33 @@ func runValidateDocmap(args []string) int {
// window between the Lstat size check in validateDocmapPath and the file open
// here. The limit is maxDocmapBytes+1 so we can detect a file that grew past
// the cap after the stat without reading unbounded bytes.
//
// Defense-in-depth: stat the path immediately before and after open so we can
// detect a file swap between validateDocmapPath's validation and this open via
// os.SameFile. An attacker with workspace write access could otherwise replace
// the validated file with a symlink in the gap between validation and use.
preStat, err := os.Lstat(resolvedDocmap)
if err != nil {
fmt.Fprintf(errWriter, "Error: failed to stat docmap before open %q: %v\n", *docmapFlag, err)
return 2
}
f, err := os.Open(resolvedDocmap)
if err != nil {
fmt.Fprintf(errWriter, "Error: failed to open docmap %q: %v\n", *docmapFlag, err)
return 2
}
defer f.Close() // nolint:errcheck
defer func() { _ = f.Close() }()
// Verify we opened the same file that was validated — rejects a swap between
// the pre-open Lstat and the open call.
postStat, err := f.Stat()
if err != nil {
fmt.Fprintf(errWriter, "Error: failed to stat open docmap %q: %v\n", *docmapFlag, err)
return 2
}
if !os.SameFile(preStat, postStat) {
fmt.Fprintf(errWriter, "Error: --docmap %q changed between validation and open\n", *docmapFlag)
return 2
}
docmapData, err := io.ReadAll(io.LimitReader(f, maxDocmapBytes+1))
if err != nil {
fmt.Fprintf(errWriter, "Error: failed to read docmap %q: %v\n", *docmapFlag, err)
+3
View File
@@ -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 {
t.Errorf("expected resolved path to differ from symlink path")
}