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.
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.
Finding 5 [NIT] from self-review:
TestValidateDocmapPath_InRepoSymlinkAllowed verifies that a file-level
symlink inside the repo root whose resolved target is also within the
root is accepted by validateDocmapPath. This is the positive case for
the issue #150 behavioral change (commit 4dce8e4): only symlinks whose
resolved destination escapes the root are rejected. Intra-repo symlinks
are permitted and their resolved path is returned to the caller.
The test also asserts that the returned path is the resolved real file,
not the symlink entry itself (i.e., EvalSymlinks did its job).
Finding 4 [MINOR] from self-review:
Previously, validateDocmapPath validated *docmapFlag then returned error
only, leaving the caller to re-open the original (unresolved) path via
ParseDocMapConfig. In theory, the path could change between validation
and use (check-then-use race).
Change validateDocmapPath to return (string, error): on success it
returns the filepath.EvalSymlinks-resolved absolute path. The caller
now passes resolvedDocmap to ParseDocMapConfig instead of the original
*docmapFlag string, eliminating any check-then-use window.
Also update the test for TestValidateDocmapPath_DirSymlinkBypass to use
the new two-value return: _ for the resolved path, err for the error.
Low-risk in ephemeral CI but correct by construction.
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'.
Rodin