docs(#150): fix stale comments in validateDocmapPath — reflect new in-repo-symlink semantic
Findings 1-3 from self-review (4dce8e4):
Finding 1 [NIT]: remove dead ModeSymlink check and its misleading
'defense-in-depth' comment. After filepath.EvalSymlinks, resolvedPath
is guaranteed symlink-free; fi.Mode()&os.ModeSymlink can never be set.
Dropped the unreachable branch; updated Lstat comment to say so.
Finding 2 [MINOR]: update validateDocmapPath godoc — invariant #2 now
reads 'The resolved path is within resolvedRoot' instead of 'The path
is not a symlink'. In-repo file-level symlinks whose resolved target
stays within the root are allowed; the confinement check enforces the
actual invariant.
Finding 3 [MINOR]: update inline comment in runValidateDocmap — the
bulleted list item now says 'Resolved target stays within the root
(in-repo symlinks allowed...)' instead of 'Is not a symlink'.
This commit is contained in:
@@ -23,8 +23,9 @@ const maxDocmapBytes int64 = 10 * 1024 * 1024 // 10 MB
|
||||
// 1. The path resolves to a regular file within resolvedRoot (path
|
||||
// confinement): prevents a PR-controlled --docmap from reading arbitrary
|
||||
// host files via absolute paths or ".." traversal.
|
||||
// 2. The path is not a symlink: prevents denial-of-service via /dev/zero or
|
||||
// information disclosure via symlinks that point outside the workspace.
|
||||
// 2. The resolved path is within resolvedRoot: in-repo file-level symlinks
|
||||
// are allowed when their resolved target is still inside the root;
|
||||
// symlinks that escape the root are rejected by the confinement check.
|
||||
// 3. The file does not exceed maxDocmapBytes: prevents memory exhaustion
|
||||
// from an oversized but legitimately committed doc-map file.
|
||||
//
|
||||
@@ -49,18 +50,13 @@ func validateDocmapPath(localPath, resolvedRoot string) error {
|
||||
return fmt.Errorf("cannot resolve path (symlink): %w", err)
|
||||
}
|
||||
|
||||
// Lstat the resolved path — at this point resolvedPath is symlink-free, so
|
||||
// ModeSymlink will never be set. We keep the check as defense-in-depth.
|
||||
// Lstat the resolved path — EvalSymlinks guarantees resolvedPath is
|
||||
// symlink-free, so ModeSymlink can never be set here; this is unreachable.
|
||||
fi, err := os.Lstat(resolvedPath)
|
||||
if err != nil {
|
||||
return fmt.Errorf("cannot stat file: %w", err)
|
||||
}
|
||||
|
||||
// Defense-in-depth: reject any remaining symlink indicator.
|
||||
if fi.Mode()&os.ModeSymlink != 0 {
|
||||
return fmt.Errorf("symlinks are not allowed")
|
||||
}
|
||||
|
||||
// Confine to resolvedRoot: use the fully-resolved path so that a directory
|
||||
// symlink inside the repo cannot carry the path outside the root.
|
||||
rel, err := filepath.Rel(resolvedRoot, resolvedPath)
|
||||
@@ -137,7 +133,8 @@ func runValidateDocmap(args []string) int {
|
||||
// may reference a PR-controlled file (e.g. .review-bot/doc-map.yml).
|
||||
// Validate that it:
|
||||
// 1. Resolves within resolvedRoot (prevent reading arbitrary host files).
|
||||
// 2. Is not a symlink (prevent /dev/zero or symlink-based host probing).
|
||||
// 2. Resolved target stays within the root (in-repo symlinks are allowed
|
||||
// if they resolve to a path inside the root).
|
||||
// 3. Does not exceed maxDocmapBytes (prevent memory exhaustion from an
|
||||
// oversized committed file).
|
||||
if err := validateDocmapPath(*docmapFlag, resolvedRoot); err != nil {
|
||||
|
||||
Reference in New Issue
Block a user