diff --git a/review/docmap.go b/review/docmap.go index 3945b8a..8916b6f 100644 --- a/review/docmap.go +++ b/review/docmap.go @@ -1,5 +1,4 @@ -// Package review provides doc-map parsing and doc injection for path-scoped -// design document context in AI code reviews. +// doc-map parsing and doc injection for path-scoped design document context in AI code reviews. package review import ( @@ -106,7 +105,7 @@ func mappingMatches(patterns, files []string) bool { // globMatch matches a path against a glob pattern that may contain **. // It supports: -// - Standard path.Match patterns (*, ?, [range]) +// - filepath.Match patterns (*, ?, [range]) // - ** as a path segment that matches zero or more segments // - Trailing /** to match a directory and all its contents // @@ -246,9 +245,13 @@ type docEntry struct { // If the path is a directory, all .md files under it are returned. // If it's a file, a single entry is returned. func loadDocEntries(ctx context.Context, fetcher DocFetcher, owner, repo, docPath string) ([]docEntry, error) { + if err := validateDocPath(docPath); err != nil { + return nil, fmt.Errorf("doc path %q rejected: %w", docPath, err) + } + // Try directory expansion first. - files, err := fetcher.GetAllFilesInPath(ctx, owner, repo, docPath) - if err == nil && len(files) > 0 { + files, dirErr := fetcher.GetAllFilesInPath(ctx, owner, repo, docPath) + if dirErr == nil && len(files) > 0 { // Filter for .md files only. var entries []docEntry for path, content := range files { @@ -261,6 +264,11 @@ func loadDocEntries(ctx context.Context, fetcher DocFetcher, owner, repo, docPat return entries, nil } + // Directory expansion returned nothing; log and fall through to single-file fetch. + if dirErr != nil { + slog.Debug("doc-map: directory expansion failed, trying as single file", "path", docPath, "error", dirErr) + } + // Try as a single file. content, fileErr := fetcher.GetFileContent(ctx, owner, repo, docPath) if fileErr != nil { @@ -290,8 +298,29 @@ func readFileBytes(path string) ([]byte, error) { return os.ReadFile(path) } +// validateDocPath rejects doc paths that could cause path traversal via the +// VCS API (absolute paths, any ".." segment). Defense-in-depth: the VCS API +// should already scope paths to the repo, but we validate locally to avoid +// any quirk in backend path handling. +func validateDocPath(p string) error { + if filepath.IsAbs(p) { + return fmt.Errorf("absolute paths not allowed") + } + for _, segment := range strings.Split(p, "/") { + if segment == ".." { + return fmt.Errorf("path traversal ('..' segment) not allowed") + } + } + return nil +} + // truncateUTF8 truncates s to at most maxBytes without splitting multi-byte // UTF-8 characters. Returns a valid UTF-8 string of at most maxBytes bytes. +// +// Note: an identical implementation exists in budget/budget.go. The two +// packages are intentionally separate (review does not import budget), so +// the duplication is accepted rather than introducing a shared internal +// package for a single small function. func truncateUTF8(s string, maxBytes int) string { if len(s) <= maxBytes { return s