fix(#141): harden checkStaleDocs against path traversal
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 32s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m12s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m13s
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 32s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m12s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m13s
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.
This commit is contained in:
@@ -83,6 +83,8 @@ func runValidateDocmap(args []string) int {
|
||||
}
|
||||
|
||||
// --- Check 2: Stale docs ---
|
||||
// checkStaleDocs validates each path before touching the filesystem; see
|
||||
// its documentation for the path-traversal hardening applied.
|
||||
repoRoot := filepath.Clean(*repoRootFlag)
|
||||
staleDocs := checkStaleDocs(cfg, repoRoot)
|
||||
if len(staleDocs) > 0 {
|
||||
@@ -101,7 +103,14 @@ func runValidateDocmap(args []string) int {
|
||||
return 0
|
||||
}
|
||||
|
||||
// checkStaleDocs returns deduplicated docs: entries that do not exist under repoRoot.
|
||||
// checkStaleDocs returns deduplicated docs: entries that do not exist under
|
||||
// repoRoot.
|
||||
//
|
||||
// Path-traversal hardening: each docPath is validated with
|
||||
// review.ValidateDocPath (rejects absolute paths and ".." segments) and then
|
||||
// confined to repoRoot via filepath.Clean + filepath.Rel before os.Stat is
|
||||
// called. Paths that fail either check are treated as invalid (reported as
|
||||
// stale) without touching the host filesystem.
|
||||
func checkStaleDocs(cfg *review.DocMapConfig, repoRoot string) []string {
|
||||
seen := make(map[string]struct{})
|
||||
var stale []string
|
||||
@@ -116,7 +125,25 @@ func checkStaleDocs(cfg *review.DocMapConfig, repoRoot string) []string {
|
||||
}
|
||||
seen[docPath] = struct{}{}
|
||||
|
||||
fullPath := filepath.Join(repoRoot, filepath.FromSlash(docPath))
|
||||
// Guard 1: reject absolute paths and ".." segments sourced from
|
||||
// PR-controlled YAML before joining with repoRoot.
|
||||
if err := review.ValidateDocPath(docPath); err != nil {
|
||||
stale = append(stale, docPath)
|
||||
continue
|
||||
}
|
||||
|
||||
// Guard 2: verify the cleaned joined path does not escape repoRoot.
|
||||
// filepath.Clean resolves any remaining ".." after the join; the
|
||||
// filepath.Rel check confirms the path is still under repoRoot.
|
||||
fullPath := filepath.Clean(filepath.Join(repoRoot, filepath.FromSlash(docPath)))
|
||||
rel, err := filepath.Rel(repoRoot, fullPath)
|
||||
if err != nil || strings.HasPrefix(rel, "..") {
|
||||
stale = append(stale, docPath)
|
||||
continue
|
||||
}
|
||||
|
||||
// Safe to stat: path is relative, contains no "..", and is
|
||||
// confined within repoRoot.
|
||||
if _, err := os.Stat(fullPath); err != nil {
|
||||
stale = append(stale, docPath)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user