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
Finding #1 [MAJOR]: replace os.Stat with os.Lstat in checkStaleDocs to
prevent symlink traversal. Symlinks under repoRoot could probe arbitrary
host file existence; Lstat never follows them. Symlinked docs are now
treated as stale.
Finding #2 [MINOR]: resolve --repo-root with filepath.Abs +
filepath.EvalSymlinks before passing to checkStaleDocs, so a symlinked
repo-root cannot bypass the filepath.Rel escape guard.
Finding #3 [NIT]: reject backslashes in ValidateDocPath to prevent
Windows platform edge cases where a path separator may be normalised
differently by the host OS or VCS backend.
Tests added:
- TestCheckStaleDocs_SymlinkOutside: symlink inside repo → outside
- TestCheckStaleDocs_SymlinkInsideRepo: intra-repo symlink also rejected
- TestRunValidateDocmap_SymlinkRepoRoot: symlinked --repo-root resolves OK
- TestValidateDocPath_Backslash: backslash paths rejected
- Backslash cases added to TestValidateDocPath invalid slice
All go test ./... pass, go vet ./... clean.
Export review.ValidateDocPath and use it in checkStaleDocs before
calling os.Stat. Add filepath.Clean + filepath.Rel confinement check
as defense-in-depth to ensure doc paths from PR-controlled YAML
cannot probe filesystem locations outside repoRoot.
Also add tests covering: ../../etc/passwd, /etc/passwd, ../outside,
a valid present path, and a valid missing path.
Addresses security finding from security-review-bot on PR #142.
TestRunValidateDocmap_Clean was reading real os.Stdin (fragile in CI).
Switch to stdinValidateDocmap with a covered file and empty-stdin test
already covered by TestRunValidateDocmap_EmptyStdin.
Adds 'review-bot validate-docmap' for CI hard-fail on docmap coverage gaps.
Usage:
git diff --name-only origin/main HEAD | \
review-bot validate-docmap --docmap .review-bot/doc-map.yml --repo-root .
Flags:
--docmap (required) path to doc-map YAML file
--repo-root (optional, default '.') root for resolving docs: paths
Two checks, both always run:
1. Coverage: every stdin file must match at least one paths: glob.
2. Stale docs: every docs: entry must exist on disk under --repo-root.
Exit codes: 0=clean, 1=failures found, 2=usage/parse error.
Tests cover: clean pass, uncovered file, stale doc, both failures,
empty stdin, blank-line stdin, and duplicate docs: deduplication.
Adds FileCoveredByDocMap(cfg *DocMapConfig, file string) bool — a thin wrapper
over the existing unexported mappingMatches that lets cmd/ check per-file docmap
coverage without duplicating glob logic.
Also adds unit tests covering matched globs, non-matching paths, empty file,
and empty config.
- budget/budget_test.go: add TestFit_DesignDocsInSystemPrompt,
TestFit_DesignDocsTrimmedBeforeFileContext, TestFit_DesignDocsEmptyNoHeading
to cover the new DesignDocs section through Fit() and buildResult()
- Remove PLAN-137.md (contained raw thinking stream, not suitable as repo doc)
- Add docs/DESIGN-137-doc-map.md with clean architectural decision record
- New --doc-map flag (DOC_MAP_FILE env var): path to YAML config mapping
source path globs to governing design docs
- New --doc-map-max-bytes flag (DOC_MAP_MAX_BYTES env var): cap on total
injected doc content, default 100KB
- review/docmap.go: DocMapConfig parsing, glob matching with ** support,
doc loading via VCS with directory expansion and size guard
- budget.Sections: new DesignDocs field, trimmed after conventions
- budget.buildResult: injects DesignDocs under ## Design Documents heading
- action.yml: doc-map and doc-map-max-bytes inputs wired to env vars
- CHANGELOG.md: created with unreleased entry
- Tests: ParseDocMapConfig, MatchDocs, globMatch, LoadMatchingDocs
Removed github/review.go and github/identity.go which were untracked orphan files
from an incomplete refactor (issue #130). They referenced a non-existent vcs package
and duplicated methods already in github/client.go.
All 6 packages pass: go test -count=1 ./... ✅
go build ./... and go vet ./... clean ✅
Updated TODO.md with current cycle status.
Rodin
rodin
claw