fix: validateDocmapPath — add EvalSymlinks to close directory-symlink bypass #150

New Issue

2026-05-15T08:21:29Z

rodin commented

2026-05-15 08:21:29 +00:00

Summary

validateDocmapPath in cmd/review-bot/validatedocmap.go has a directory-symlink bypass that contradicts its stated security invariant.

Background

PR #142 added path-confinement hardening to prevent reading arbitrary host files via --docmap. The hardening correctly:

Calls filepath.EvalSymlinks on --repo-root to resolve it
Uses os.Lstat to reject file-level symlinks
Uses filepath.Rel for confinement check

However, os.Lstat(absPath) only avoids following the final component of the path. It does follow intermediate directory symlinks.

Bypass

If a PR commits .review-bot/ as a directory symlink pointing outside the repo, and CI is configured with --docmap .review-bot/doc-map.yml:

filepath.Abs("/.review-bot/doc-map.yml") → textually inside repo ✅
os.Lstat(absPath) follows the dir symlink, finds a regular file outside repo — no symlink bit on final component ✅
filepath.Rel(resolvedRoot, absPath) passes because path is textually inside repo ✅
File gets read → bypass

This was verified with a reproducible Go test.

Fix

In validateDocmapPath, after filepath.Abs, call filepath.EvalSymlinks and use the resolved path for all subsequent checks:

absPath, err := filepath.Abs(localPath)
if err != nil {
    return fmt.Errorf("cannot resolve path: %w", err)
}

// Resolve symlinks in ALL components (not just the final one).
// This catches directory symlinks that could bypass the confinement check.
resolvedPath, err := filepath.EvalSymlinks(absPath)
if err != nil {
    return fmt.Errorf("cannot resolve path (symlink): %w", err)
}

// Use resolvedPath from here on for Rel check and Lstat
fi, err := os.Lstat(resolvedPath)
// ...
rel, err := filepath.Rel(resolvedRoot, resolvedPath)

Note: after EvalSymlinks the path is already symlink-free, so the ModeSymlink check can be removed or kept as defense-in-depth (it will never fire, but the comment is still educational).

Also add test

func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) {
    repoDir := t.TempDir()
    outsideDir := t.TempDir()
    
    // Secret file outside repo  
    os.WriteFile(filepath.Join(outsideDir, "secret.yml"), []byte("mappings: []"), 0o644)
    
    // Directory symlink inside repo pointing outside
    os.Symlink(outsideDir, filepath.Join(repoDir, ".review-bot"))
    
    // Attempt to read outside file via directory symlink
    attackPath := filepath.Join(repoDir, ".review-bot", "secret.yml")
    err := validateDocmapPath(attackPath, repoDir)
    if err == nil {
        t.Error("expected rejection of dir-symlink bypass, got nil")
    }
}

Severity

MINOR — requires the PR author to commit .review-bot/ as a directory symlink (aggressive modification) and relies on the CI-configured --docmap path landing on a parseable YAML outside the repo. Not easily exploitable in typical usage.

Discovered during self-review of PR #142 (post-merge).

## Summary `validateDocmapPath` in `cmd/review-bot/validatedocmap.go` has a directory-symlink bypass that contradicts its stated security invariant. ## Background PR #142 added path-confinement hardening to prevent reading arbitrary host files via `--docmap`. The hardening correctly: - Calls `filepath.EvalSymlinks` on `--repo-root` to resolve it - Uses `os.Lstat` to reject file-level symlinks - Uses `filepath.Rel` for confinement check However, `os.Lstat(absPath)` only avoids following the **final** component of the path. It **does** follow intermediate directory symlinks. ## Bypass If a PR commits `.review-bot/` as a directory symlink pointing outside the repo, and CI is configured with `--docmap .review-bot/doc-map.yml`: 1. `filepath.Abs("/.review-bot/doc-map.yml")` → textually inside repo ✅ 2. `os.Lstat(absPath)` follows the dir symlink, finds a regular file outside repo — no symlink bit on final component ✅ 3. `filepath.Rel(resolvedRoot, absPath)` passes because path is textually inside repo ✅ 4. File gets read → **bypass** This was verified with a reproducible Go test. ## Fix In `validateDocmapPath`, after `filepath.Abs`, call `filepath.EvalSymlinks` and use the resolved path for all subsequent checks: ```go absPath, err := filepath.Abs(localPath) if err != nil { return fmt.Errorf("cannot resolve path: %w", err) } // Resolve symlinks in ALL components (not just the final one). // This catches directory symlinks that could bypass the confinement check. resolvedPath, err := filepath.EvalSymlinks(absPath) if err != nil { return fmt.Errorf("cannot resolve path (symlink): %w", err) } // Use resolvedPath from here on for Rel check and Lstat fi, err := os.Lstat(resolvedPath) // ... rel, err := filepath.Rel(resolvedRoot, resolvedPath) ``` Note: after `EvalSymlinks` the path is already symlink-free, so the `ModeSymlink` check can be removed or kept as defense-in-depth (it will never fire, but the comment is still educational). ## Also add test ```go func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) { repoDir := t.TempDir() outsideDir := t.TempDir() // Secret file outside repo os.WriteFile(filepath.Join(outsideDir, "secret.yml"), []byte("mappings: []"), 0o644) // Directory symlink inside repo pointing outside os.Symlink(outsideDir, filepath.Join(repoDir, ".review-bot")) // Attempt to read outside file via directory symlink attackPath := filepath.Join(repoDir, ".review-bot", "secret.yml") err := validateDocmapPath(attackPath, repoDir) if err == nil { t.Error("expected rejection of dir-symlink bypass, got nil") } } ``` ## Severity MINOR — requires the PR author to commit `.review-bot/` as a directory symlink (aggressive modification) and relies on the CI-configured `--docmap` path landing on a parseable YAML outside the repo. Not easily exploitable in typical usage. Discovered during self-review of PR #142 (post-merge).

rodin referenced this issue from a commit

2026-05-15 08:29:45 +00:00

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

rodin referenced a pull request that will close this issue

2026-05-15 08:30:02 +00:00

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

rodin referenced this issue from a commit

2026-05-15 08:37:37 +00:00

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

sonnet-review-bot referenced this issue

2026-05-15 08:38:18 +00:00

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