Feature/rule link to files#226
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if filepath.IsAbs(rule) { | ||
| candidates = []string{rule} | ||
| } else { | ||
| candidates = []string{filepath.Join(repoDir, rule)} |
There was a problem hiding this comment.
Security (Path Traversal): There is no validation that the resolved file path stays within the expected base directories (repoDir or ~/.opencodereview). A malicious rule entry like ../../../../etc/passwd.md would pass through filepath.Join(repoDir, rule), resolve via EvalSymlinks, pass the extension check, and be read successfully. Consider adding a containment check after symlink resolution to ensure the final path is under one of the allowed base directories, e.g.:
rel, err := filepath.Rel(allowedBase, resolved)
if err != nil || strings.HasPrefix(rel, "..") {
return "", fmt.Errorf("path escapes allowed directory")
}There was a problem hiding this comment.
rule.json 由项目维护者自己编写，不是不可信输入。维护者写 ../../../../etc/passwd.md
是在读自己的文件，没有提权路径。repoDir 是受信的仓库根目录
| fmt.Fprintf(os.Stderr, "[WARN] cannot read rule file %s: %v\n", candidates[1], err) | ||
| } | ||
| } | ||
| fmt.Fprintf(os.Stderr, "[WARN] rule file not found: %s (tried project dir, then ~/.opencodereview)\n", rule) |
There was a problem hiding this comment.
Noisy warnings: When a rule file simply doesn't exist in either location (a normal situation for projects that don't use external rule files), this warning is always printed to stderr. This could produce confusing noise during regular operation. Consider only logging at debug level, or suppressing the "not found" warning entirely when both locations are legitimately optional.
There was a problem hiding this comment.
只有 rule 值以 .md/.txt/.markdown 结尾时才会触发文件查找。用户写 "nonexistent.md"
就明确表达了"加载文件"的意图，文件不存在时给出警告是合理的，帮助发现配置错误
| } | ||
| } | ||
|
|
||
| primary := candidates[0] |
There was a problem hiding this comment.
Nit: primary is only used for one read and one error log — you could use candidates[0] directly to reduce the local variable count. Minor style preference, not blocking.
- rule field auto-detects: .md/.txt/.markdown ending = file path, otherwise inline - file paths: project-relative first, then as absolute path - safety: stat before read (512KB cap), extension whitelist, symlink resolution - tightened heuristic: values with spaces treated as inline to avoid false positives - guard against empty repoDir to avoid CWD-relative resolution - 5-language README docs updated with file path usage and first-match-wins behavior - 15 new unit tests covering all resolution branches Closes alibaba#67 Supersedes alibaba#87
zephyrq-z
force-pushed
the
feature/rule-link-to-files
branch
from
9cdfe9f to
9d511f4
Compare
- Add blank line between matchProjectRuleEntry and allowedRuleExts (Issue 1) - Remove dead code '|| repoDir == ' in tryReadRuleFile (Issue 2) - Remove unnecessary warning when repoDir is empty but path is absolute (Issue 3)
|
Thanks everyone for the feedback and discussion! @vanducng @candy-Tong @paul-yangmy @yuuk PR #87 has been redesigned and merged as #226. Key improvements over the original approach:
See the PR description for examples and details. Feedback always welcome! |
2 participants
Description
Closes #67.
The
rulefield inrule.jsonnow supports automatic detection of inline text vs. external file paths. A single-line value with no spaces ending in.md,.txt, or.markdownis treated as a file path — the file content is read and replaces therulefield. All other values (multi-line text, text containing spaces, no extension, other extensions) remain inline.Differences from the abandoned PR #87
This PR is a redesign and replacement of the closed #87. Key differences:
use_file_path: trueswitch, global togglerule.jsonuse_file_pathsemantics.mdpath to load a fileCore Changes
internal/config/rules/system_rules.go(+113 lines)allowedRuleExts— package-level extension whitelist (.md/.txt/.markdown), shared by all validation functionslooksLikeFilePath()— heuristic: multi-line or contains spaces → inline; single-line, no spaces, whitelisted extension → file pathresolveRuleEntries()— iterates entries, auto-detects and loads file content; clears the rule if loading fails. Called by all three loaders:loadGlobalRule,loadProjectRule, andloadRuleFiletryReadRuleFile()— absolute paths are used directly; relative paths are resolved against the repository directory with path traversal protection; emptyrepoDirwith a relative path is rejectedreadRuleFileSafe()— safe reading: symlink resolution, extension whitelist, 512 KB size capinternal/config/rules/system_rules_test.go(+408 lines).txt/.markdownextensions, subdirectory path, path traversal blocked, empty repoDir (relative and absolute), global rule file resolution, heuristic detection (5), safe reading (4)README (5 languages) — added documentation for file path resolution in the
rulefield with configuration examples, including path traversal protection and size limitConfiguration Example
{ "rules": [ {"path": "**/*.py", "rule": "docs/python-rules.md"}, {"path": "**/*.go", "rule": "Always check for nil pointers"}, {"path": "**/*.ts", "rule": "shared.md"} ] }docs/python-rules.md→ relative path, file content loaded automatically"Always check for nil pointers"→ inline text, kept as-is"shared.md"→ relative path, resolved from the project directory; cleared if not foundType of Change
How Has This Been Tested?
Unit Tests
internal/config/rules/system_rules_test.go— 24 new test cases:TestResolveRuleEntries_BasicFile—.mdfile content correctly replacesrulefieldTestResolveRuleEntries_MultiLineInline— multi-line text stays inlineTestResolveRuleEntries_MissingFile— missing file emits[WARN], clears the ruleTestResolveRuleEntries_AbsolutePath— absolute path resolved directlyTestResolveRuleEntries_TooLarge— files over 512 KB rejected, rule clearedTestResolveRuleEntries_RelativePath— relative path resolved from project directoryTestResolveRuleEntries_EmptyRule— empty rules skippedTestResolveRuleEntries_SymlinkSafety— symlink target extension checked, clears rule if not whitelistedTestResolveRuleEntries_TxtExtension—.txtextension supportedTestResolveRuleEntries_MarkdownExtension—.markdownextension supportedTestResolveRuleEntries_SubdirectoryPath— subdirectory path resolutionTestResolveRuleEntries_PathTraversalBlocked—../../etc/passwd.mdtraversal blocked, rule clearedTestResolveRuleEntries_EmptyRepoDirRelative— relative path rejected when repoDir is emptyTestResolveRuleEntries_EmptyRepoDirAbsolute— absolute path works when repoDir is emptyTestResolveRuleEntries_GlobalRuleFileResolution—~/.opencodereview/rule files resolvedTestLooksLikeFilePath_*— 5 heuristic tests (inline content, multi-line, extensions, spaces, no extension)TestReadRuleFileSafe_*— 4 safe read tests (normal file, unsupported extension, oversized, missing)Manual Testing
test-rule independent validation suite:
python.mdcontent loaded, inline unchanged[WARN]+ rule cleared.jsonextension/tmp/absolute-rule.mdresolved directlyrules/nested/nested.mdresolved correctlymake testpasses locallyChecklist
go fmt,go vet)Related Issues
Closes #67
Supersedes #87