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'.
"ERROR: stale docmap docs: entries" had a vestigial "docs:" fragment
that reads awkwardly (looks like a YAML reference).
Change to: "ERROR: stale docmap entries (paths do not exist):"
Addresses NIT finding in review #4175.
Non-git tools (e.g. `find`, `ls`) can emit paths with a "./" prefix.
Without stripping this, "./cmd/foo.go" would not match the glob "cmd/**",
producing a false-positive uncovered-file failure.
Fix: add strings.TrimPrefix(f, "./") after backslash normalization.
Test: TestRunValidateDocmap_DotSlashPrefix
Addresses MINOR finding in review #4175.
Add IsRegular() check after Lstat so directories, FIFOs, and device nodes
produce a clear error ("docmap must be a regular file") instead of a
confusing downstream parse error.
Test: TestValidateDocmapPath_NonRegularFile
Addresses MINOR finding in review #4175.
When --doc-map-trusted-ref is provided, the --doc-map value is used as a
VCS API path parameter, not a local filesystem path. The early call to
validateWorkspacePath (which requires the file to exist locally) blocked
the trusted-ref code path when the doc-map did not exist in the local
checkout — defeating the feature's purpose in sparse checkouts or when
the file is only on the default branch.
Fix: guard the early validation with `&& *docMapTrustedRef == ""`.
Also fixes:
- review/docmap.go: correct ParseDocMapConfigContent godoc example to
match actual source format "owner/repo@ref:path"
- cmd/review-bot/main_test.go: add TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation
to prevent regression
The doc-map YAML config was previously read from the local workspace
(the PR branch checkout). A malicious PR author could modify
.review-bot/doc-map.yml to map any path glob to sensitive design docs,
causing review-bot to fetch and inject those docs into the LLM prompt.
Fix: add --doc-map-trusted-ref (DOC_MAP_TRUSTED_REF) flag. When set to
a trusted ref (e.g. 'main'), the doc-map config is fetched from the VCS
API at that ref instead of from local workspace. A 404 from VCS is a
hard error (no silent fallback to local copy).
When unset, the local workspace is used with a security warning in the
logs pointing operators to the new flag.
Changes:
- review/docmap.go: add ParseDocMapConfigContent + parseDocMapBytes
helper to parse from in-memory content (fetched via VCS API)
- cmd/review-bot/main.go: add --doc-map-trusted-ref flag; Step 6c
branches on trusted-ref to fetch vs local-workspace load
- .gitea/actions/review/action.yml: add doc-map-trusted-ref input
- README.md: document new input
- CHANGELOG.md: security and feature entries
Tests:
- TestParseDocMapConfigContent_Valid/Empty/InvalidYAML/UnknownKeys
in review/docmap_test.go
Coverage: 53.0% cmd/review-bot
validateurl.go is VCS-generic but imported gitea.IsBlockedIP, creating an
unexpected generic→Gitea-specific dependency. Extract IsBlockedIP and its
CIDR list to internal/netutil/ipcheck.go (a neutral shared package).
- gitea/ipcheck.go becomes a thin forwarding wrapper (preserves API compat
for callers within the gitea package)
- gitea/ipcheck_test.go replaced with a forwarding smoke test; full coverage
moves to internal/netutil/ipcheck_test.go
- validateurl.go now imports internal/netutil directly
The field was named NewPosition with a misleading comment 'Gitea: absolute
line; GitHub: diff hunk position'. In reality both adapters use it as an
absolute new-file line number (Gitea maps it to new_position, GitHub maps it
to Line+Side:RIGHT). Rename to NewLine to match actual semantics and update
comments to explain per-adapter mapping.
- CLI example used $GITEA_TOKEN which is not an actual env var; rename to
$REVIEWER_TOKEN (the correct env var the binary reads)
- Env var table referenced GITEA_REPO without noting GitHub support; add
a note and include VCS_TYPE row so users know they can override detection
The binary detects VCS type from VCS_TYPE env var, but action.yml did not
pass it to the Run review step. This caused the binary to fall back to a
URL heuristic (github.com substring), which misclassifies GitHub Enterprise
Server hosts whose URL does not contain 'github'.
The 'Determine version' step already outputs vcs_type — wire it through to
the Run review env block so explicit VCS_TYPE always takes precedence.
The previous implementation called os.Lstat(absPath) which only avoids
following the *final* path component. A PR committing .review-bot/ as a
directory symlink pointing outside the repo would pass the filepath.Rel
confinement check because the textual path was inside the root while
the resolved destination was not.
Fix: call filepath.EvalSymlinks after filepath.Abs to resolve ALL symlink
components before the confinement check. If EvalSymlinks fails (dangling
symlink, nonexistent target) the path is rejected. The filepath.Rel check
then operates on the fully-resolved path.
Semantic change: file-level in-repo symlinks (target also within root) are
now allowed — the invariant is about where the content lives, not whether
the entry is a symlink. The test TestValidateDocmapPath_Symlink is updated
to test an out-of-repo symlink target, which must still be rejected.
Tests:
- TestValidateDocmapPath_DirSymlinkBypass: reproduces the attack vector
(dir symlink bypassing textual confinement check) and verifies it is
now rejected
- TestValidateDocmapPath_Symlink: updated to test out-of-repo symlink
Coverage: 54.0%
Address security-review-bot REQUEST_CHANGES findings on PR #142:
MAJOR (Finding #1): Docmap file path was read directly without validating it
is within --repo-root or checking for symlinks. A malicious PR could create
.review-bot/doc-map.yml as a symlink to /dev/zero (resource exhaustion) or an
arbitrary host file (information disclosure).
Fix: Add validateDocmapPath() called before ParseDocMapConfig(). It:
- Resolves --repo-root first (filepath.Abs + EvalSymlinks), moved up before
docmap parsing so both checks share the same resolved root
- Uses os.Lstat to detect symlinks and rejects them outright
- Confirms the docmap path is within resolvedRoot via filepath.Rel
- Checks file size against maxDocmapBytes (10 MB) before reading
MINOR (Finding #2): No upper bound on docmap YAML size.
Fix: os.Lstat size check enforces maxDocmapBytes cap before os.ReadFile.
Tests:
- TestValidateDocmapPath_Symlink: docmap is a symlink → exit 2
- TestValidateDocmapPath_OutsideRepoRoot: docmap outside repo-root → exit 2
- TestValidateDocmapPath_SizeLimit: docmap exceeds 10 MB cap → exit 2
- Updated all existing tests to use makeDocmapInDir() so the docmap
lives inside the repo-root, satisfying the new confinement check
Rodin
aweiker
rodin