fix(security): resolve open CodeQL code-scanning alerts#19
Merged
Conversation
Address the open CodeQL findings on main: - Path traversal (High): confine the untrusted `dir`/`cwd` query parameter of the permission-catalog handler to the workspace root via resolveCatalogDir, rejecting `../` and absolute escapes before any filesystem access. - Allocation size overflow (High): route make() size/capacity hints that add two lengths through a new collections.SafeAdd helper that saturates at math.MaxInt instead of wrapping (fuzzy matrix, model candidates, and the prompt-entity merge helpers). - Shell command from environment values (Medium): build the git command in vite.config.ts with execFileSync + an argv array so no shell is spawned. - Workflow permissions (Medium): add a least-privilege top-level `permissions: contents: read` block to the Test workflow. Adds a resolveCatalogDir unit test covering the traversal cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186pQzdDXNY1jsbozM7Ed3B
WalkthroughChangesAllocation safety
Security hardening
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
adityathebe
enabled auto-merge (squash)
July 21, 2026 08:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/collections/alloc.go (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd boundary tests for
SafeAdd.Please cover normal addition,
math.MaxInt + 1, and negative operands so the saturation contract remains protected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/collections/alloc.go` around lines 5 - 16, Add boundary-focused tests for SafeAdd covering ordinary addition, saturation when adding 1 to math.MaxInt, and either negative operand; assert each result matches the documented saturation contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cli/permission_catalog.go`:
- Around line 55-57: Update the path validation condition to append the path
separator to base only when base does not already end with one, while preserving
the existing target != base check and workspace-escape rejection. Apply this in
the validation logic surrounding the target and base variables so root paths
such as / and C:\ correctly allow valid subdirectories.
In `@pkg/collections/fuzzy.go`:
- Around line 42-44: Update the inner loop in the matrix initialization to use
SafeAdd(len(s2), 1) as its range bound, matching the allocation of each matrix
row. Keep the existing initialization behavior unchanged while ensuring the loop
cannot overflow independently of the allocated row size.
---
Nitpick comments:
In `@pkg/collections/alloc.go`:
- Around line 5-16: Add boundary-focused tests for SafeAdd covering ordinary
addition, saturation when adding 1 to math.MaxInt, and either negative operand;
assert each result matches the documented saturation contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f60c42e-bcaf-4e44-87af-57edc075f7bc
📒 Files selected for processing (8)
.github/workflows/test.ymlpkg/api/model.gopkg/cli/permission_catalog.gopkg/cli/permission_catalog_test.gopkg/cli/prompt_entity.gopkg/cli/webapp/vite.config.tspkg/collections/alloc.gopkg/collections/fuzzy.go
Comment on lines
+55
to
+57
| if target != base && !strings.HasPrefix(target, base+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("dir %q escapes workspace root", dir) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix path validation when baseCwd is a filesystem root.
If base is a root directory (e.g., / on Unix or C:\ on Windows), filepath.Clean(base) retains the trailing slash. Appending string(os.PathSeparator) results in a double slash (e.g., // or C:\\), which causes strings.HasPrefix to falsely reject valid subdirectories (e.g., /foo).
Ensure the separator is only appended if base does not already end with one.
🛠️ Proposed fix
- if target != base && !strings.HasPrefix(target, base+string(os.PathSeparator)) {
+ prefix := base
+ if !strings.HasSuffix(prefix, string(os.PathSeparator)) {
+ prefix += string(os.PathSeparator)
+ }
+ if target != base && !strings.HasPrefix(target, prefix) {
return "", fmt.Errorf("dir %q escapes workspace root", dir)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if target != base && !strings.HasPrefix(target, base+string(os.PathSeparator)) { | |
| return "", fmt.Errorf("dir %q escapes workspace root", dir) | |
| } | |
| prefix := base | |
| if !strings.HasSuffix(prefix, string(os.PathSeparator)) { | |
| prefix += string(os.PathSeparator) | |
| } | |
| if target != base && !strings.HasPrefix(target, prefix) { | |
| return "", fmt.Errorf("dir %q escapes workspace root", dir) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cli/permission_catalog.go` around lines 55 - 57, Update the path
validation condition to append the path separator to base only when base does
not already end with one, while preserving the existing target != base check and
workspace-escape rejection. Apply this in the validation logic surrounding the
target and base variables so root paths such as / and C:\ correctly allow valid
subdirectories.
Comment on lines
+42
to
+44
| matrix := make([][]int, SafeAdd(len(s1), 1)) | ||
| for i := range matrix { | ||
| matrix[i] = make([]int, len(s2)+1) | ||
| matrix[i] = make([]int, SafeAdd(len(s2), 1)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== fuzzy.go outline ==\n'
ast-grep outline pkg/collections/fuzzy.go --view expanded || true
printf '\n== fuzzy.go relevant lines ==\n'
cat -n pkg/collections/fuzzy.go | sed -n '1,140p'
printf '\n== SafeAdd definitions/usages ==\n'
rg -n "func SafeAdd|SafeAdd\(" -S .
printf '\n== integer type context ==\n'
sed -n '1,120p' pkg/collections/fuzzy.go | cat -nRepository: flanksource/captain
Length of output: 4496
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('pkg/collections/fuzzy.go')
text = p.read_text()
print(text)
PY
printf '\n== SafeAdd definition(s) ==\n'
rg -n "func SafeAdd|type .*SafeAdd|SafeAdd =" -S pkg . || trueRepository: flanksource/captain
Length of output: 1542
🏁 Script executed:
set -euo pipefail
printf '--- fuzzy.go ---\n'
nl -ba pkg/collections/fuzzy.go | sed -n '1,120p'
printf '\n--- SafeAdd references ---\n'
rg -n "SafeAdd" pkg -S
printf '\n--- go.mod ---\n'
cat -n go.mod | sed -n '1,120p'Repository: flanksource/captain
Length of output: 248
🏁 Script executed:
set -euo pipefail
nl -ba pkg/collections/alloc.go | sed -n '1,120p'
printf '\n== SafeAdd callers in collections ==\n'
rg -n "SafeAdd\(" pkg/collections -S
printf '\n== Any tests for fuzzy/alloc ==\n'
rg -n "Levenshtein|SafeAdd" -S . --glob '*_test.go'Repository: flanksource/captain
Length of output: 197
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('pkg/collections/alloc.go')
print(p.read_text())
PYRepository: flanksource/captain
Length of output: 692
🏁 Script executed:
set -euo pipefail
cat -n pkg/collections/alloc.go | sed -n '1,120p'Repository: flanksource/captain
Length of output: 803
Use SafeAdd for the inner loop bound too.
matrix[i] is sized with SafeAdd(len(s2), 1), but for j := range len(s2) + 1 can still wrap at MaxInt and leave the first-row initialization out of sync with the allocated row. Use the same helper here.
Proposed fix
- for j := range len(s2) + 1 {
+ for j := range SafeAdd(len(s2), 1) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/collections/fuzzy.go` around lines 42 - 44, Update the inner loop in the
matrix initialization to use SafeAdd(len(s2), 1) as its range bound, matching
the allocation of each matrix row. Keep the existing initialization behavior
unchanged while ensuring the loop cannot overflow independently of the allocated
row size.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the open CodeQL code-scanning alerts on
mainacross four categories.Path traversal — "Uncontrolled data used in path expression" (High) —
pkg/cli/permission_catalog.goThe
dir/cwdquery parameter of the permission-catalog HTTP handler flowed straight intoos.ReadFile/os.Stat/os.ReadDir. An attacker could passdir=../../etc(or an absolute path) to enumerate arbitrary directories. AddedresolveCatalogDir, which resolves the value against the workspace root and rejects anything that escapes it (using a cleaned-path prefix containment check) before any filesystem access. The handler now returns400for invalid input.Integer overflow — "Size computation for allocation may overflow" (High)
make()size/capacity hints computed by adding two lengths (len(a)+len(b),len(x)+1) were flagged as potentially overflowing. Added a smallcollections.SafeAddhelper that saturates atmath.MaxIntunder a proper overflow guard, and routed all flagged sites through it — preserving the original capacity hints:pkg/collections/fuzzy.go(Levenshtein matrix)pkg/api/model.go(Candidates)pkg/cli/prompt_entity.go(mergeStringMaps,mergeToolModes,mergePresets)Shell injection — "Shell command built from environment values" (Medium) —
pkg/cli/webapp/vite.config.tsThe build-time
git()helper interpolated a filesystem-derived path into a shell command string viaexecSync. Switched toexecFileSyncwith an argv array so no shell is spawned.Workflow hardening — "Workflow does not contain permissions" (Medium) —
.github/workflows/test.ymlAdded a least-privilege top-level
permissions: contents: readblock.Testing
go build/go testpass for the affected packages (collections,api,cli).TestResolveCatalogDircovering the allowed and rejected (traversal / absolute) cases.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Security