From 2dbc6d7b2e2e2d5163e7100d16413d214d9eaa31 Mon Sep 17 00:00:00 2001 From: zephyrq-z Date: Mon, 8 Jun 2026 15:45:46 +0800 Subject: [PATCH 1/4] feat: support rule_file to load external .md/.txt rules - Added 'rule_file' field to ProjectRuleEntry in rule.json - Implemented resolveRuleFiles to read, validate, and merge external rule files - Security: prevents path traversal (../) outside the base directory - Limits supported extensions to .md and .txt, max size 100KB - Merges contents if both 'rule' and 'rule_file' are provided - Added unit tests for security, missing files, large files, etc. - Updated README.md and README.zh-CN.md Resolves alibaba#67 --- README.md | 13 +++- README.zh-CN.md | 11 ++- internal/config/rules/system_rules.go | 86 +++++++++++++++++++++- internal/config/rules/system_rules_test.go | 79 ++++++++++++++++++++ 4 files changed, 182 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 4df7c9c..89f9bd2 100644 --- a/README.md +++ b/README.md @@ -353,15 +353,24 @@ Layers 1–3 share the same JSON format: }, { "path": "**/*mapper*.xml", - "rule": "Check SQL for injection risks, parameter errors, and missing closing tags" + "rule_file": "docs/sql-rules.md" + }, + { + "path": "web/**/*.ts", + "rule": "Focus on XSS vulnerabilities.", + "rule_file": "docs/frontend-rules.md" } ] } ``` - `path` supports `**` recursive matching and `{java,kt}` brace expansion. +- `rule` is used for inline rule text. +- `rule_file` references an external `.md` or `.txt` file as rule content. The path is relative to the directory containing the current `rule.json`. +- If both `rule` and `rule_file` are provided, their contents are merged. +- For security reasons, `rule_file` cannot reference files outside its base directory (no `../` path traversal), and the file size must not exceed 100KB. - Within each layer, rules are evaluated in declaration order — the first match wins. -- If a rule file does not exist, it is silently skipped. +- Missing rule files are skipped silently. ### Path Filtering diff --git a/README.zh-CN.md b/README.zh-CN.md index 82e9162..5f10d38 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -343,13 +343,22 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 }, { "path": "**/*mapper*.xml", - "rule": "检查 SQL 注入风险、参数错误和缺少闭合标签" + "rule_file": "docs/sql-rules.md" + }, + { + "path": "web/**/*.ts", + "rule": "重点关注 XSS 漏洞。", + "rule_file": "docs/frontend-rules.md" } ] } ``` - `path` 支持 `**` 递归匹配和 `{java,kt}` 大括号展开。 +- `rule` 字段用于直接编写内联规则文本。 +- `rule_file` 字段支持引用外部的 `.md` 或 `.txt` 文件作为规则内容。路径相对于当前 `rule.json` 所在的目录。 +- 如果同时指定了 `rule` 和 `rule_file`，系统会将两者的内容合并后作为最终规则。 +- 出于安全考虑，`rule_file` 不能引用所在目录之外的文件（禁止 `../` 路径穿越），且文件大小不能超过 100KB。 - 在每一层内，规则按声明顺序评估 —— 首次匹配生效。 - 如果规则文件不存在，将被静默跳过。 diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index d3740ff..e7972a2 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -176,8 +176,9 @@ func expandBraces(s string) []string { // ProjectRuleEntry is a single entry in .opencodereview/rule.json. type ProjectRuleEntry struct { - Path string `json:"path"` - Rule string `json:"rule"` + Path string `json:"path"` + Rule string `json:"rule"` + RuleFile string `json:"rule_file,omitempty"` } // ProjectRule holds rules loaded from /.opencodereview/rule.json. @@ -302,12 +303,85 @@ func buildFileFilter(layers ...*ProjectRule) *FileFilter { return nil } +// resolveRuleFiles reads external rule files and merges their content into the Rule field. +func resolveRuleFiles(pr *ProjectRule, baseDir string) { + if pr == nil || baseDir == "" { + return + } + + absBase, err := filepath.Abs(baseDir) + if err != nil { + fmt.Fprintf(os.Stderr, "[WARN] Failed to get absolute path for base dir %s: %v\n", baseDir, err) + return + } + // Ensure base directory path ends with separator for proper prefix matching + if !strings.HasSuffix(absBase, string(filepath.Separator)) { + absBase += string(filepath.Separator) + } + + for i := range pr.Rules { + entry := &pr.Rules[i] + if entry.RuleFile == "" { + continue + } + + absFile, err := filepath.Abs(filepath.Join(baseDir, entry.RuleFile)) + if err != nil { + fmt.Fprintf(os.Stderr, "[WARN] Failed to get absolute path for rule file %s: %v\n", entry.RuleFile, err) + continue + } + + // Security check: prevent directory traversal + if !strings.HasPrefix(absFile+string(filepath.Separator), absBase) && absFile != strings.TrimSuffix(absBase, string(filepath.Separator)) { + fmt.Fprintf(os.Stderr, "[WARN] Security violation: rule file %s is outside the base directory %s\n", entry.RuleFile, baseDir) + continue + } + + // Extension check + ext := strings.ToLower(filepath.Ext(absFile)) + if ext != ".md" && ext != ".txt" { + fmt.Fprintf(os.Stderr, "[WARN] Unsupported rule file extension %s for %s (only .md and .txt are allowed)\n", ext, entry.RuleFile) + continue + } + + // File size check + info, err := os.Stat(absFile) + if err != nil { + if os.IsNotExist(err) { + fmt.Fprintf(os.Stderr, "[WARN] Rule file not found: %s\n", absFile) + } else { + fmt.Fprintf(os.Stderr, "[WARN] Failed to stat rule file %s: %v\n", absFile, err) + } + continue + } + if info.Size() > 100*1024 { + fmt.Fprintf(os.Stderr, "[WARN] Rule file %s is too large (%d bytes, max 100KB)\n", absFile, info.Size()) + continue + } + + // Read file content + content, err := os.ReadFile(absFile) + if err != nil { + fmt.Fprintf(os.Stderr, "[WARN] Failed to read rule file %s: %v\n", absFile, err) + continue + } + + fileContent := strings.TrimRight(string(content), "\n") + if entry.Rule != "" { + entry.Rule = entry.Rule + "\n\n" + fileContent + } else { + entry.Rule = fileContent + } + } +} + func loadGlobalRule() (*ProjectRule, error) { home, err := os.UserHomeDir() if err != nil { return nil, nil } - path := filepath.Join(home, ".opencodereview", "rule.json") + baseDir := filepath.Join(home, ".opencodereview") + path := filepath.Join(baseDir, "rule.json") data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { @@ -319,6 +393,7 @@ func loadGlobalRule() (*ProjectRule, error) { if err := json.Unmarshal(data, &pr); err != nil { return nil, fmt.Errorf("unmarshal global rule: %w", err) } + resolveRuleFiles(&pr, baseDir) return &pr, nil } @@ -331,11 +406,13 @@ func loadRuleFile(path string) (*ProjectRule, error) { if err := json.Unmarshal(data, &pr); err != nil { return nil, fmt.Errorf("unmarshal rule file %s: %w", path, err) } + resolveRuleFiles(&pr, filepath.Dir(path)) return &pr, nil } func loadProjectRule(repoDir string) (*ProjectRule, error) { - path := filepath.Join(repoDir, ".opencodereview", "rule.json") + baseDir := filepath.Join(repoDir, ".opencodereview") + path := filepath.Join(baseDir, "rule.json") data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { @@ -347,6 +424,7 @@ func loadProjectRule(repoDir string) (*ProjectRule, error) { if err := json.Unmarshal(data, &pr); err != nil { return nil, fmt.Errorf("unmarshal project rule: %w", err) } + resolveRuleFiles(&pr, baseDir) return &pr, nil } diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 00e45dc..d2045b2 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -670,3 +670,82 @@ func TestNewResolver_BraceExpansionInProjectRule(t *testing.T) { }) } } + +func TestResolveRuleFiles_Basic(t *testing.T) { + dir := t.TempDir() + mdContent := "# Rule from file\nCheck for memory leaks." + err := os.WriteFile(filepath.Join(dir, "rule.md"), []byte(mdContent), 0644) + if err != nil { + t.Fatal(err) + } + + pr := &ProjectRule{ + Rules: []ProjectRuleEntry{ + {Path: "*.go", Rule: "Inline rule.", RuleFile: "rule.md"}, + {Path: "*.py", RuleFile: "rule.md"}, + }, + } + resolveRuleFiles(pr, dir) + + if !strings.Contains(pr.Rules[0].Rule, "Inline rule.") || !strings.Contains(pr.Rules[0].Rule, "Check for memory leaks.") { + t.Errorf("expected merged rule, got %q", pr.Rules[0].Rule) + } + if pr.Rules[1].Rule != mdContent { + t.Errorf("expected file content only, got %q", pr.Rules[1].Rule) + } +} + +func TestResolveRuleFiles_Security(t *testing.T) { + dir := t.TempDir() + pr := &ProjectRule{ + Rules: []ProjectRuleEntry{ + {Path: "*.go", RuleFile: "../outside.md"}, + }, + } + resolveRuleFiles(pr, dir) + if pr.Rules[0].Rule != "" { + t.Errorf("expected empty rule due to security violation, got %q", pr.Rules[0].Rule) + } +} + +func TestResolveRuleFiles_UnsupportedExtension(t *testing.T) { + dir := t.TempDir() + os.WriteFile(filepath.Join(dir, "rule.json"), []byte("{}"), 0644) + pr := &ProjectRule{ + Rules: []ProjectRuleEntry{ + {Path: "*.go", RuleFile: "rule.json"}, + }, + } + resolveRuleFiles(pr, dir) + if pr.Rules[0].Rule != "" { + t.Errorf("expected empty rule due to unsupported extension, got %q", pr.Rules[0].Rule) + } +} + +func TestResolveRuleFiles_TooLarge(t *testing.T) { + dir := t.TempDir() + largeContent := strings.Repeat("a", 101*1024) + os.WriteFile(filepath.Join(dir, "large.md"), []byte(largeContent), 0644) + pr := &ProjectRule{ + Rules: []ProjectRuleEntry{ + {Path: "*.go", RuleFile: "large.md"}, + }, + } + resolveRuleFiles(pr, dir) + if pr.Rules[0].Rule != "" { + t.Errorf("expected empty rule due to large file, got %q", pr.Rules[0].Rule) + } +} + +func TestResolveRuleFiles_MissingFile(t *testing.T) { + dir := t.TempDir() + pr := &ProjectRule{ + Rules: []ProjectRuleEntry{ + {Path: "*.go", Rule: "Keep me", RuleFile: "missing.md"}, + }, + } + resolveRuleFiles(pr, dir) + if pr.Rules[0].Rule != "Keep me" { + t.Errorf("expected original rule to be kept, got %q", pr.Rules[0].Rule) + } +} From 4c0352fddb09a4908f86a5a783cc3a41d0c14ebd Mon Sep 17 00:00:00 2001 From: zephyrq-z Date: Tue, 9 Jun 2026 15:15:43 +0800 Subject: [PATCH 2/4] feat: update rule handling to support use_file_path for external rules --- README.md | 12 ++++---- README.zh-CN.md | 12 ++++---- internal/config/rules/system_rules.go | 27 ++++++++---------- internal/config/rules/system_rules_test.go | 32 ++++++++++------------ 4 files changed, 40 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 89f9bd2..917a380 100644 --- a/README.md +++ b/README.md @@ -353,12 +353,14 @@ Layers 1–3 share the same JSON format: }, { "path": "**/*mapper*.xml", - "rule_file": "docs/sql-rules.md" + "rule": "docs/sql-rules.md", + "use_file_path": true }, { "path": "web/**/*.ts", "rule": "Focus on XSS vulnerabilities.", - "rule_file": "docs/frontend-rules.md" + "rule": "docs/frontend-rules.md", + "use_file_path": true } ] } @@ -366,9 +368,9 @@ Layers 1–3 share the same JSON format: - `path` supports `**` recursive matching and `{java,kt}` brace expansion. - `rule` is used for inline rule text. -- `rule_file` references an external `.md` or `.txt` file as rule content. The path is relative to the directory containing the current `rule.json`. -- If both `rule` and `rule_file` are provided, their contents are merged. -- For security reasons, `rule_file` cannot reference files outside its base directory (no `../` path traversal), and the file size must not exceed 100KB. +- When `use_file_path` is set to `true`, the `rule` field is treated as a relative path to an external `.md` or `.txt` file containing the rule content. The path is relative to the directory containing the current `rule.json`. +- The content of the external file will overwrite the `rule` field. +- For security reasons, the referenced file cannot be outside its base directory (no `../` path traversal), and the file size must not exceed 100KB. - Within each layer, rules are evaluated in declaration order — the first match wins. - Missing rule files are skipped silently. diff --git a/README.zh-CN.md b/README.zh-CN.md index 5f10d38..365098b 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -343,12 +343,14 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 }, { "path": "**/*mapper*.xml", - "rule_file": "docs/sql-rules.md" + "rule": "docs/sql-rules.md", + "use_file_path": true }, { "path": "web/**/*.ts", "rule": "重点关注 XSS 漏洞。", - "rule_file": "docs/frontend-rules.md" + "rule": "docs/frontend-rules.md", + "use_file_path": true } ] } @@ -356,9 +358,9 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 - `path` 支持 `**` 递归匹配和 `{java,kt}` 大括号展开。 - `rule` 字段用于直接编写内联规则文本。 -- `rule_file` 字段支持引用外部的 `.md` 或 `.txt` 文件作为规则内容。路径相对于当前 `rule.json` 所在的目录。 -- 如果同时指定了 `rule` 和 `rule_file`，系统会将两者的内容合并后作为最终规则。 -- 出于安全考虑，`rule_file` 不能引用所在目录之外的文件（禁止 `../` 路径穿越），且文件大小不能超过 100KB。 +- 当设置 `use_file_path: true` 时，`rule` 字段的值将被视为指向外部 `.md` 或 `.txt` 文件的相对路径。该路径相对于当前 `rule.json` 所在的目录。 +- 外部文件的内容将直接覆盖 `rule` 字段的值。 +- 出于安全考虑，引用的文件不能位于所在目录之外（禁止 `../` 路径穿越），且文件大小不能超过 100KB。 - 在每一层内，规则按声明顺序评估 —— 首次匹配生效。 - 如果规则文件不存在，将被静默跳过。 diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index e7972a2..f935bfa 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -176,9 +176,9 @@ func expandBraces(s string) []string { // ProjectRuleEntry is a single entry in .opencodereview/rule.json. type ProjectRuleEntry struct { - Path string `json:"path"` - Rule string `json:"rule"` - RuleFile string `json:"rule_file,omitempty"` + Path string `json:"path"` + Rule string `json:"rule"` + UseFilePath bool `json:"use_file_path,omitempty"` } // ProjectRule holds rules loaded from /.opencodereview/rule.json. @@ -303,7 +303,7 @@ func buildFileFilter(layers ...*ProjectRule) *FileFilter { return nil } -// resolveRuleFiles reads external rule files and merges their content into the Rule field. +// resolveRuleFiles reads external rule files if UseFilePath is true and replaces the Rule field with the file content. func resolveRuleFiles(pr *ProjectRule, baseDir string) { if pr == nil || baseDir == "" { return @@ -321,26 +321,27 @@ func resolveRuleFiles(pr *ProjectRule, baseDir string) { for i := range pr.Rules { entry := &pr.Rules[i] - if entry.RuleFile == "" { + if !entry.UseFilePath || entry.Rule == "" { continue } - absFile, err := filepath.Abs(filepath.Join(baseDir, entry.RuleFile)) + filePath := entry.Rule + absFile, err := filepath.Abs(filepath.Join(baseDir, filePath)) if err != nil { - fmt.Fprintf(os.Stderr, "[WARN] Failed to get absolute path for rule file %s: %v\n", entry.RuleFile, err) + fmt.Fprintf(os.Stderr, "[WARN] Failed to get absolute path for rule file %s: %v\n", filePath, err) continue } // Security check: prevent directory traversal if !strings.HasPrefix(absFile+string(filepath.Separator), absBase) && absFile != strings.TrimSuffix(absBase, string(filepath.Separator)) { - fmt.Fprintf(os.Stderr, "[WARN] Security violation: rule file %s is outside the base directory %s\n", entry.RuleFile, baseDir) + fmt.Fprintf(os.Stderr, "[WARN] Security violation: rule file %s is outside the base directory %s\n", filePath, baseDir) continue } // Extension check ext := strings.ToLower(filepath.Ext(absFile)) if ext != ".md" && ext != ".txt" { - fmt.Fprintf(os.Stderr, "[WARN] Unsupported rule file extension %s for %s (only .md and .txt are allowed)\n", ext, entry.RuleFile) + fmt.Fprintf(os.Stderr, "[WARN] Unsupported rule file extension %s for %s (only .md and .txt are allowed)\n", ext, filePath) continue } @@ -366,12 +367,8 @@ func resolveRuleFiles(pr *ProjectRule, baseDir string) { continue } - fileContent := strings.TrimRight(string(content), "\n") - if entry.Rule != "" { - entry.Rule = entry.Rule + "\n\n" + fileContent - } else { - entry.Rule = fileContent - } + // Overwrite the Rule field with the file content + entry.Rule = strings.TrimRight(string(content), "\n") } } diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index d2045b2..fe126ad 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -681,17 +681,13 @@ func TestResolveRuleFiles_Basic(t *testing.T) { pr := &ProjectRule{ Rules: []ProjectRuleEntry{ - {Path: "*.go", Rule: "Inline rule.", RuleFile: "rule.md"}, - {Path: "*.py", RuleFile: "rule.md"}, + {Path: "*.go", Rule: "rule.md", UseFilePath: true}, }, } resolveRuleFiles(pr, dir) - if !strings.Contains(pr.Rules[0].Rule, "Inline rule.") || !strings.Contains(pr.Rules[0].Rule, "Check for memory leaks.") { - t.Errorf("expected merged rule, got %q", pr.Rules[0].Rule) - } - if pr.Rules[1].Rule != mdContent { - t.Errorf("expected file content only, got %q", pr.Rules[1].Rule) + if pr.Rules[0].Rule != mdContent { + t.Errorf("expected file content only, got %q", pr.Rules[0].Rule) } } @@ -699,12 +695,12 @@ func TestResolveRuleFiles_Security(t *testing.T) { dir := t.TempDir() pr := &ProjectRule{ Rules: []ProjectRuleEntry{ - {Path: "*.go", RuleFile: "../outside.md"}, + {Path: "*.go", Rule: "../outside.md", UseFilePath: true}, }, } resolveRuleFiles(pr, dir) - if pr.Rules[0].Rule != "" { - t.Errorf("expected empty rule due to security violation, got %q", pr.Rules[0].Rule) + if pr.Rules[0].Rule != "../outside.md" { + t.Errorf("expected rule to remain unchanged due to security violation, got %q", pr.Rules[0].Rule) } } @@ -713,12 +709,12 @@ func TestResolveRuleFiles_UnsupportedExtension(t *testing.T) { os.WriteFile(filepath.Join(dir, "rule.json"), []byte("{}"), 0644) pr := &ProjectRule{ Rules: []ProjectRuleEntry{ - {Path: "*.go", RuleFile: "rule.json"}, + {Path: "*.go", Rule: "rule.json", UseFilePath: true}, }, } resolveRuleFiles(pr, dir) - if pr.Rules[0].Rule != "" { - t.Errorf("expected empty rule due to unsupported extension, got %q", pr.Rules[0].Rule) + if pr.Rules[0].Rule != "rule.json" { + t.Errorf("expected rule to remain unchanged due to unsupported extension, got %q", pr.Rules[0].Rule) } } @@ -728,12 +724,12 @@ func TestResolveRuleFiles_TooLarge(t *testing.T) { os.WriteFile(filepath.Join(dir, "large.md"), []byte(largeContent), 0644) pr := &ProjectRule{ Rules: []ProjectRuleEntry{ - {Path: "*.go", RuleFile: "large.md"}, + {Path: "*.go", Rule: "large.md", UseFilePath: true}, }, } resolveRuleFiles(pr, dir) - if pr.Rules[0].Rule != "" { - t.Errorf("expected empty rule due to large file, got %q", pr.Rules[0].Rule) + if pr.Rules[0].Rule != "large.md" { + t.Errorf("expected rule to remain unchanged due to large file, got %q", pr.Rules[0].Rule) } } @@ -741,11 +737,11 @@ func TestResolveRuleFiles_MissingFile(t *testing.T) { dir := t.TempDir() pr := &ProjectRule{ Rules: []ProjectRuleEntry{ - {Path: "*.go", Rule: "Keep me", RuleFile: "missing.md"}, + {Path: "*.go", Rule: "missing.md", UseFilePath: true}, }, } resolveRuleFiles(pr, dir) - if pr.Rules[0].Rule != "Keep me" { + if pr.Rules[0].Rule != "missing.md" { t.Errorf("expected original rule to be kept, got %q", pr.Rules[0].Rule) } } From 48e443a4b5dfc5ff146c7d15238610ad21700cbd Mon Sep 17 00:00:00 2001 From: zephyrq-z Date: Tue, 9 Jun 2026 19:23:44 +0800 Subject: [PATCH 3/4] refactor(rules): move use_file_path to top-level and fix symlink vulnerability - Move use_file_path from per-entry to top-level ProjectRule field (simplifies config: no need to repeat flag for every rule entry) - Add filepath.EvalSymlinks to prevent symlink-based directory traversal (resolves symlinks on both file path and base directory for consistent prefix matching across platforms like macOS /var -> /private/var) - Add warning when use_file_path=true but rule path is empty - Update tests to use new top-level UseFilePath structure - Update README.md and README.zh-CN.md with new config format --- README.md | 18 +++---- README.zh-CN.md | 14 ++--- internal/config/rules/system_rules.go | 63 ++++++++++++++-------- internal/config/rules/system_rules_test.go | 15 ++++-- 4 files changed, 64 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 917a380..c6dc15d 100644 --- a/README.md +++ b/README.md @@ -346,6 +346,7 @@ Layers 1–3 share the same JSON format: ```json { + "use_file_path": true, "rules": [ { "path": "force-api/**/*.java", @@ -353,26 +354,21 @@ Layers 1–3 share the same JSON format: }, { "path": "**/*mapper*.xml", - "rule": "docs/sql-rules.md", - "use_file_path": true + "rule": "docs/sql-rules.md" }, { "path": "web/**/*.ts", - "rule": "Focus on XSS vulnerabilities.", - "rule": "docs/frontend-rules.md", - "use_file_path": true + "rule": "docs/frontend-rules.md" } ] } ``` -- `path` supports `**` recursive matching and `{java,kt}` brace expansion. -- `rule` is used for inline rule text. -- When `use_file_path` is set to `true`, the `rule` field is treated as a relative path to an external `.md` or `.txt` file containing the rule content. The path is relative to the directory containing the current `rule.json`. -- The content of the external file will overwrite the `rule` field. -- For security reasons, the referenced file cannot be outside its base directory (no `../` path traversal), and the file size must not exceed 100KB. +- `use_file_path` is a top-level flag. When set to `true`, all `rule` fields are treated as relative paths to external `.md` or `.txt` files containing the rule content. The paths are relative to the directory containing the current `rule.json`. +- The content of external files will overwrite the `rule` field values. +- For security reasons, referenced files cannot be outside their base directory (no `../` path traversal), and file sizes must not exceed 100KB. - Within each layer, rules are evaluated in declaration order — the first match wins. -- Missing rule files are skipped silently. +- Missing rule files will generate a warning and be skipped. ### Path Filtering diff --git a/README.zh-CN.md b/README.zh-CN.md index 365098b..8ec5e9c 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -336,6 +336,7 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 ```json { + "use_file_path": true, "rules": [ { "path": "force-api/**/*.java", @@ -343,26 +344,21 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 }, { "path": "**/*mapper*.xml", - "rule": "docs/sql-rules.md", - "use_file_path": true + "rule": "docs/sql-rules.md" }, { "path": "web/**/*.ts", - "rule": "重点关注 XSS 漏洞。", - "rule": "docs/frontend-rules.md", - "use_file_path": true + "rule": "docs/frontend-rules.md" } ] } ``` -- `path` 支持 `**` 递归匹配和 `{java,kt}` 大括号展开。 -- `rule` 字段用于直接编写内联规则文本。 -- 当设置 `use_file_path: true` 时，`rule` 字段的值将被视为指向外部 `.md` 或 `.txt` 文件的相对路径。该路径相对于当前 `rule.json` 所在的目录。 +- `use_file_path` 是顶层标志。当设置为 `true` 时，所有 `rule` 字段将被视为指向外部 `.md` 或 `.txt` 文件的相对路径，文件内容包含实际的规则文本。路径相对于当前 `rule.json` 所在的目录。 - 外部文件的内容将直接覆盖 `rule` 字段的值。 - 出于安全考虑，引用的文件不能位于所在目录之外（禁止 `../` 路径穿越），且文件大小不能超过 100KB。 - 在每一层内，规则按声明顺序评估 —— 首次匹配生效。 -- 如果规则文件不存在，将被静默跳过。 +- 如果规则文件不存在，将生成警告并被跳过。 ### 路径过滤 diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index f935bfa..489213c 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -176,16 +176,16 @@ func expandBraces(s string) []string { // ProjectRuleEntry is a single entry in .opencodereview/rule.json. type ProjectRuleEntry struct { - Path string `json:"path"` - Rule string `json:"rule"` - UseFilePath bool `json:"use_file_path,omitempty"` + Path string `json:"path"` + Rule string `json:"rule"` } // ProjectRule holds rules loaded from /.opencodereview/rule.json. type ProjectRule struct { - Rules []ProjectRuleEntry `json:"rules"` - Include []string `json:"include,omitempty"` - Exclude []string `json:"exclude,omitempty"` + UseFilePath bool `json:"use_file_path,omitempty"` + Rules []ProjectRuleEntry `json:"rules"` + Include []string `json:"include,omitempty"` + Exclude []string `json:"exclude,omitempty"` } // FileFilter holds the merged user-configured include/exclude glob patterns @@ -309,11 +309,21 @@ func resolveRuleFiles(pr *ProjectRule, baseDir string) { return } + // Early exit if UseFilePath is not enabled at the top level + if !pr.UseFilePath { + return + } + absBase, err := filepath.Abs(baseDir) if err != nil { fmt.Fprintf(os.Stderr, "[WARN] Failed to get absolute path for base dir %s: %v\n", baseDir, err) return } + // Resolve symlinks on base directory for consistent prefix matching + // (e.g. on macOS /var -> /private/var) + if resolved, err := filepath.EvalSymlinks(absBase); err == nil { + absBase = resolved + } // Ensure base directory path ends with separator for proper prefix matching if !strings.HasSuffix(absBase, string(filepath.Separator)) { absBase += string(filepath.Separator) @@ -321,7 +331,10 @@ func resolveRuleFiles(pr *ProjectRule, baseDir string) { for i := range pr.Rules { entry := &pr.Rules[i] - if !entry.UseFilePath || entry.Rule == "" { + + // Warn if Rule path is empty + if entry.Rule == "" { + fmt.Fprintf(os.Stderr, "[WARN] Rule entry for path %q has use_file_path=true but empty rule path, skipping\n", entry.Path) continue } @@ -332,20 +345,7 @@ func resolveRuleFiles(pr *ProjectRule, baseDir string) { continue } - // Security check: prevent directory traversal - if !strings.HasPrefix(absFile+string(filepath.Separator), absBase) && absFile != strings.TrimSuffix(absBase, string(filepath.Separator)) { - fmt.Fprintf(os.Stderr, "[WARN] Security violation: rule file %s is outside the base directory %s\n", filePath, baseDir) - continue - } - - // Extension check - ext := strings.ToLower(filepath.Ext(absFile)) - if ext != ".md" && ext != ".txt" { - fmt.Fprintf(os.Stderr, "[WARN] Unsupported rule file extension %s for %s (only .md and .txt are allowed)\n", ext, filePath) - continue - } - - // File size check + // File size check and existence check (before symlink resolution) info, err := os.Stat(absFile) if err != nil { if os.IsNotExist(err) { @@ -360,6 +360,27 @@ func resolveRuleFiles(pr *ProjectRule, baseDir string) { continue } + // Security: resolve symlinks to prevent directory traversal via symlink + resolvedFile, err := filepath.EvalSymlinks(absFile) + if err != nil { + fmt.Fprintf(os.Stderr, "[WARN] Failed to resolve symlinks for rule file %s: %v\n", filePath, err) + continue + } + absFile = resolvedFile + + // Security check: prevent directory traversal + if !strings.HasPrefix(absFile+string(filepath.Separator), absBase) && absFile != strings.TrimSuffix(absBase, string(filepath.Separator)) { + fmt.Fprintf(os.Stderr, "[WARN] Security violation: rule file %s is outside the base directory %s\n", filePath, baseDir) + continue + } + + // Extension check + ext := strings.ToLower(filepath.Ext(absFile)) + if ext != ".md" && ext != ".txt" { + fmt.Fprintf(os.Stderr, "[WARN] Unsupported rule file extension %s for %s (only .md and .txt are allowed)\n", ext, filePath) + continue + } + // Read file content content, err := os.ReadFile(absFile) if err != nil { diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index fe126ad..01ef2db 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -680,8 +680,9 @@ func TestResolveRuleFiles_Basic(t *testing.T) { } pr := &ProjectRule{ + UseFilePath: true, Rules: []ProjectRuleEntry{ - {Path: "*.go", Rule: "rule.md", UseFilePath: true}, + {Path: "*.go", Rule: "rule.md"}, }, } resolveRuleFiles(pr, dir) @@ -694,8 +695,9 @@ func TestResolveRuleFiles_Basic(t *testing.T) { func TestResolveRuleFiles_Security(t *testing.T) { dir := t.TempDir() pr := &ProjectRule{ + UseFilePath: true, Rules: []ProjectRuleEntry{ - {Path: "*.go", Rule: "../outside.md", UseFilePath: true}, + {Path: "*.go", Rule: "../outside.md"}, }, } resolveRuleFiles(pr, dir) @@ -708,8 +710,9 @@ func TestResolveRuleFiles_UnsupportedExtension(t *testing.T) { dir := t.TempDir() os.WriteFile(filepath.Join(dir, "rule.json"), []byte("{}"), 0644) pr := &ProjectRule{ + UseFilePath: true, Rules: []ProjectRuleEntry{ - {Path: "*.go", Rule: "rule.json", UseFilePath: true}, + {Path: "*.go", Rule: "rule.json"}, }, } resolveRuleFiles(pr, dir) @@ -723,8 +726,9 @@ func TestResolveRuleFiles_TooLarge(t *testing.T) { largeContent := strings.Repeat("a", 101*1024) os.WriteFile(filepath.Join(dir, "large.md"), []byte(largeContent), 0644) pr := &ProjectRule{ + UseFilePath: true, Rules: []ProjectRuleEntry{ - {Path: "*.go", Rule: "large.md", UseFilePath: true}, + {Path: "*.go", Rule: "large.md"}, }, } resolveRuleFiles(pr, dir) @@ -736,8 +740,9 @@ func TestResolveRuleFiles_TooLarge(t *testing.T) { func TestResolveRuleFiles_MissingFile(t *testing.T) { dir := t.TempDir() pr := &ProjectRule{ + UseFilePath: true, Rules: []ProjectRuleEntry{ - {Path: "*.go", Rule: "missing.md", UseFilePath: true}, + {Path: "*.go", Rule: "missing.md"}, }, } resolveRuleFiles(pr, dir) From 72a3018c41d0ac8168fd6b5de7ba8a5faf12477d Mon Sep 17 00:00:00 2001 From: zephyrq-z Date: Wed, 10 Jun 2026 10:29:46 +0800 Subject: [PATCH 4/4] feat: add support for complex rules using top-level use_file_path flag in README files --- README.ja-JP.md | 27 +++++++++++++++++++++++++-- README.ko-KR.md | 27 +++++++++++++++++++++++++-- README.md | 18 ++++++++++++++++-- README.zh-CN.md | 18 ++++++++++++++++-- 4 files changed, 82 insertions(+), 8 deletions(-) diff --git a/README.ja-JP.md b/README.ja-JP.md index 3d7d799..4f4ec9f 100644 --- a/README.ja-JP.md +++ b/README.ja-JP.md @@ -359,9 +359,32 @@ OCRは4層の優先度チェーンを使ってレビュールールを解決し } ``` +複雑なルールを別ファイルで管理したい場合は、トップレベルの `use_file_path` フラグを有効にします： + +```json +{ + "use_file_path": true, + "rules": [ + { + "path": "**/*mapper*.xml", + "rule": "docs/sql-rules.md" + }, + { + "path": "web/**/*.ts", + "rule": "docs/frontend-rules.md" + } + ] +} +``` + +- `use_file_path` はトップレベルのフラグです。`true` に設定すると、すべての `rule` フィールドは実際のルールテキストを含む外部 `.md` または `.txt` ファイルへの相対パスとして扱われます。パスは現在の `rule.json` を含むディレクトリからの相対パスです。 +- `false` または省略した場合、`rule` フィールドはインライン文字列ルールとして使用されます（上記の最初の例のように）。 +- 外部ファイルの内容は `rule` フィールドの値を上書きします。 +- セキュリティ上の理由から、参照されるファイルはベースディレクトリの外側に配置できません（`../` によるパストラバーサルは禁止）。また、ファイルサイズは100KBを超えてはなりません。 +- 各層内でルールは宣言順に評価され、最初にマッチしたものが採用されます。 +- ルールファイルが存在しない場合は警告を出力してスキップされます。 + - `path`は`**`による再帰マッチと`{java,kt}`のブレース展開をサポートします。 -- 各層の中では、ルールは宣言順に評価されます — 最初にマッチしたものが採用されます。 -- ルールファイルが存在しない場合は、何も出力せずスキップされます。 ### パスフィルタリング diff --git a/README.ko-KR.md b/README.ko-KR.md index 9870de3..246c4d3 100644 --- a/README.ko-KR.md +++ b/README.ko-KR.md @@ -359,9 +359,32 @@ OCR은 네 계층의 priority chain으로 review rule을 해석합니다. 각 } ``` -- `path`는 `**` recursive matching과 `{java,kt}` brace expansion을 지원합니다. +복잡한 rule을 별도 파일로 관리하고 싶을 때는 최상위 `use_file_path` flag를 활성화합니다: + +```json +{ + "use_file_path": true, + "rules": [ + { + "path": "**/*mapper*.xml", + "rule": "docs/sql-rules.md" + }, + { + "path": "web/**/*.ts", + "rule": "docs/frontend-rules.md" + } + ] +} +``` + +- `use_file_path`는 최상위 flag입니다. `true`로 설정하면 모든 `rule` field는 실제 rule text가 포함된 외부 `.md` 또는 `.txt` file에 대한 상대 경로로 취급됩니다. 경로는 현재 `rule.json`이 있는 directory 기준 상대 경로입니다. +- `false`로 설정하거나 생략하면 `rule` field는 inline string rule로 사용됩니다 (위 첫 번째 예시처럼). +- 외부 file의 내용이 `rule` field 값을 덮어씁니다. +- 보안상 이유로 참조되는 file은 base directory 밖에 위치할 수 없으며 (`../` path traversal 금지), file 크기는 100KB를 초과할 수 없습니다. - 각 계층 안에서는 rule이 선언 순서대로 평가되며 첫 번째 match가 선택됩니다. -- rule file이 없으면 조용히 건너뜁니다. +- Rule file이 없으면 warning을 출력하고 건너뜁니다. + +- `path`는 `**` recursive matching과 `{java,kt}` brace expansion을 지원합니다. ## Configuration Reference diff --git a/README.md b/README.md index c6dc15d..5376c0d 100644 --- a/README.md +++ b/README.md @@ -346,12 +346,25 @@ Layers 1–3 share the same JSON format: ```json { - "use_file_path": true, "rules": [ { "path": "force-api/**/*.java", "rule": "All new methods must validate required parameters for null values" }, + { + "path": "**/*mapper*.xml", + "rule": "Check SQL for injection risks, parameter errors, and missing closing tags" + } + ] +} +``` + +When complex rules are better kept in separate files, enable the top-level `use_file_path` flag: + +```json +{ + "use_file_path": true, + "rules": [ { "path": "**/*mapper*.xml", "rule": "docs/sql-rules.md" @@ -364,7 +377,8 @@ Layers 1–3 share the same JSON format: } ``` -- `use_file_path` is a top-level flag. When set to `true`, all `rule` fields are treated as relative paths to external `.md` or `.txt` files containing the rule content. The paths are relative to the directory containing the current `rule.json`. +- `use_file_path` is a top-level toggle. When set to `true`, all `rule` fields are treated as relative paths to external `.md` or `.txt` files whose content is the actual rule text. Paths are relative to the directory containing the current `rule.json`. +- When set to `false` or omitted, `rule` fields are used as inline string rules (as in the first example above). - The content of external files will overwrite the `rule` field values. - For security reasons, referenced files cannot be outside their base directory (no `../` path traversal), and file sizes must not exceed 100KB. - Within each layer, rules are evaluated in declaration order — the first match wins. diff --git a/README.zh-CN.md b/README.zh-CN.md index 8ec5e9c..717b135 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -336,12 +336,25 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 ```json { - "use_file_path": true, "rules": [ { "path": "force-api/**/*.java", "rule": "所有新方法必须对必填参数进行空值校验" }, + { + "path": "**/*mapper*.xml", + "rule": "检查 SQL 是否存在注入风险、参数错误及缺失闭合标签" + } + ] +} +``` + +当需要使用外部文件来存放较复杂的规则时，可设置顶层 `use_file_path` 开关： + +```json +{ + "use_file_path": true, + "rules": [ { "path": "**/*mapper*.xml", "rule": "docs/sql-rules.md" @@ -354,7 +367,8 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 } ``` -- `use_file_path` 是顶层标志。当设置为 `true` 时，所有 `rule` 字段将被视为指向外部 `.md` 或 `.txt` 文件的相对路径，文件内容包含实际的规则文本。路径相对于当前 `rule.json` 所在的目录。 +- `use_file_path` 是顶层开关。设为 `true` 时，所有 `rule` 字段均被视为指向外部 `.md` 或 `.txt` 文件的相对路径，文件内容即为实际的规则文本。路径相对于当前 `rule.json` 所在目录。 +- 设为 `false` 或省略时，`rule` 字段作为内联字符串规则使用（即上方第一个示例）。 - 外部文件的内容将直接覆盖 `rule` 字段的值。 - 出于安全考虑，引用的文件不能位于所在目录之外（禁止 `../` 路径穿越），且文件大小不能超过 100KB。 - 在每一层内，规则按声明顺序评估 —— 首次匹配生效。