vrr/open-code-review
2
0
Fork 0
mirror of https://github.com/alibaba/open-code-review.git synced 2026-07-09 17:28:58 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

fix(tool): constrain workspace file reads to repo (#109)

Browse source
This commit is contained in:
MuoDoo 2026-06-12 17:57:28 +08:00 • committed by GitHub
parent 76b4d5a3b5
commit 9f81cbe486
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 180 additions and 2 deletions
Download patch file Download diff file Expand all files Collapse all files

136
internal/tool/file_read_test.go
View file

@ -2,9 +2,11 @@ package tool
import (
"context"
"errors"
"fmt"
"os"
"path/filepath"
"runtime"
"strings"
"testing"
)
@ -161,6 +163,140 @@ func TestReadLines_GitShow_Window(t *testing.T) {
}
}
func TestReadLines_Disk_RejectsParentTraversal(t *testing.T) {
base := t.TempDir()
repoDir := filepath.Join(base, "repo")
if err := os.Mkdir(repoDir, 0755); err != nil {
t.Fatal(err)
}
secretPath := filepath.Join(base, "secret.txt")
if err := os.WriteFile(secretPath, []byte("outside-secret\n"), 0644); err != nil {
t.Fatal(err)
}
escapePath, err := filepath.Rel(repoDir, secretPath)
if err != nil {
t.Fatal(err)
}
fr := &FileReader{RepoDir: repoDir, Mode: ModeWorkspace}
if _, _, err := fr.ReadLines(context.Background(), escapePath, 1, 10); err == nil || !strings.Contains(err.Error(), "outside repository") {
t.Fatalf("ReadLines(%q) error = %v, want outside repository", escapePath, err)
}
if _, err := fr.Read(context.Background(), escapePath); err == nil || !strings.Contains(err.Error(), "outside repository") {
t.Fatalf("Read(%q) error = %v, want outside repository", escapePath, err)
}
}
func TestReadLines_Disk_AllowsParentSegmentWithinRepo(t *testing.T) {
dir := t.TempDir()
if err := os.Mkdir(filepath.Join(dir, "pkg"), 0755); err != nil {
t.Fatal(err)
}
writeTestFile(t, dir, "target.txt", "inside\n")
fr := &FileReader{RepoDir: dir, Mode: ModeWorkspace}
lines, _, err := fr.ReadLines(context.Background(), filepath.Join("pkg", "..", "target.txt"), 1, 10)
if err != nil {
t.Fatal(err)
}
if len(lines) == 0 || lines[0] != "inside" {
t.Fatalf("ReadLines(pkg/../target.txt) = %q, want inside", lines)
}
}
func TestReadLines_Disk_AbsolutePathStaysUnderRepo(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("absolute path syntax varies on Windows")
}
dir := t.TempDir()
if err := os.Mkdir(filepath.Join(dir, "etc"), 0755); err != nil {
t.Fatal(err)
}
writeTestFile(t, dir, filepath.Join("etc", "passwd"), "repo-passwd\n")
fr := &FileReader{RepoDir: dir, Mode: ModeWorkspace}
lines, _, err := fr.ReadLines(context.Background(), "/etc/passwd", 1, 10)
if err != nil {
t.Fatal(err)
}
if len(lines) == 0 || lines[0] != "repo-passwd" {
t.Fatalf("ReadLines(/etc/passwd) = %q, want repo-passwd", lines)
}
}
func TestReadLines_Disk_MissingFilePreservesReadError(t *testing.T) {
dir := t.TempDir()
fr := &FileReader{RepoDir: dir, Mode: ModeWorkspace}
_, _, err := fr.ReadLines(context.Background(), "missing.txt", 1, 10)
if err == nil {
t.Fatal("ReadLines(missing.txt) error = nil, want not exist")
}
if !errors.Is(err, os.ErrNotExist) {
t.Fatalf("ReadLines(missing.txt) error = %v, want os.ErrNotExist", err)
}
if !strings.Contains(err.Error(), `read file "missing.txt"`) || strings.Contains(err.Error(), "resolve file") {
t.Fatalf("ReadLines(missing.txt) error = %v, want read file error", err)
}
_, err = fr.Read(context.Background(), "missing.txt")
if err == nil {
t.Fatal("Read(missing.txt) error = nil, want not exist")
}
if !errors.Is(err, os.ErrNotExist) {
t.Fatalf("Read(missing.txt) error = %v, want os.ErrNotExist", err)
}
if !strings.Contains(err.Error(), `read file "missing.txt"`) || strings.Contains(err.Error(), "resolve file") {
t.Fatalf("Read(missing.txt) error = %v, want read file error", err)
}
}
func TestReadLines_Disk_RejectsSymlinkOutsideRepo(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("symlink privileges vary on Windows")
}
base := t.TempDir()
repoDir := filepath.Join(base, "repo")
if err := os.Mkdir(repoDir, 0755); err != nil {
t.Fatal(err)
}
secretPath := filepath.Join(base, "secret.txt")
if err := os.WriteFile(secretPath, []byte("outside-secret\n"), 0644); err != nil {
t.Fatal(err)
}
if err := os.Symlink(secretPath, filepath.Join(repoDir, "link.txt")); err != nil {
t.Fatal(err)
}
fr := &FileReader{RepoDir: repoDir, Mode: ModeWorkspace}
if _, _, err := fr.ReadLines(context.Background(), "link.txt", 1, 10); err == nil || !strings.Contains(err.Error(), "outside repository") {
t.Fatalf("ReadLines(link.txt) error = %v, want outside repository", err)
}
}
func TestReadLines_Disk_AllowsSymlinkInsideRepo(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("symlink privileges vary on Windows")
}
dir := t.TempDir()
writeTestFile(t, dir, "target.txt", "inside\n")
if err := os.Symlink(filepath.Join(dir, "target.txt"), filepath.Join(dir, "link.txt")); err != nil {
t.Fatal(err)
}
fr := &FileReader{RepoDir: dir, Mode: ModeWorkspace}
lines, _, err := fr.ReadLines(context.Background(), "link.txt", 1, 10)
if err != nil {
t.Fatal(err)
}
if len(lines) == 0 || lines[0] != "inside" {
t.Fatalf("ReadLines(link.txt) = %q, want inside", lines)
}
}
func TestExecute_Truncation(t *testing.T) {
dir := t.TempDir()

46
internal/tool/filereader.go
View file

@ -76,7 +76,10 @@ func (fr *FileReader) Read(ctx context.Context, path string) (string, error) {
}
func (fr *FileReader) readFromDisk(path string) (string, error) {
fullPath := filepath.Join(fr.RepoDir, path)
fullPath, err := fr.resolveWorkspacePath(path)
if err != nil {
return "", err
}
content, err := os.ReadFile(fullPath)
if err != nil {
return "", fmt.Errorf("read file %q: %w", path, err)
@ -84,6 +87,42 @@ func (fr *FileReader) readFromDisk(path string) (string, error) {
return string(content), nil
}
func (fr *FileReader) resolveWorkspacePath(path string) (string, error) {
repoRoot, err := filepath.Abs(fr.RepoDir)
if err != nil {
return "", fmt.Errorf("resolve repository path %q: %w", fr.RepoDir, err)
}
repoRoot, err = filepath.EvalSymlinks(repoRoot)
if err != nil {
return "", fmt.Errorf("resolve repository path %q: %w", fr.RepoDir, err)
}
fullPath := filepath.Join(repoRoot, path)
if !pathWithinBase(repoRoot, fullPath) {
return "", fmt.Errorf("file path %q is outside repository", path)
}
resolvedPath, err := filepath.EvalSymlinks(fullPath)
if err != nil {
if os.IsNotExist(err) {
return fullPath, nil
}
return "", fmt.Errorf("resolve file %q: %w", path, err)
}
if !pathWithinBase(repoRoot, resolvedPath) {
return "", fmt.Errorf("file path %q is outside repository", path)
}
return resolvedPath, nil
}
func pathWithinBase(base, target string) bool {
rel, err := filepath.Rel(base, target)
if err != nil {
return false
}
return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)))
}
func (fr *FileReader) readFromGitShow(parentCtx context.Context, path string) (string, error) {
ctx, cancel := context.WithTimeout(parentCtx, 30*time.Second)
defer cancel()
@ -160,7 +199,10 @@ func scanLines(r io.Reader, startLine, maxLines int) ([]string, int, error) {
}
func (fr *FileReader) readLinesFromDisk(path string, startLine, maxLines int) ([]string, int, error) {
fullPath := filepath.Join(fr.RepoDir, path)
fullPath, err := fr.resolveWorkspacePath(path)
if err != nil {
return nil, 0, err
}
f, err := os.Open(fullPath)
if err != nil {
return nil, 0, fmt.Errorf("read file %q: %w", path, err)