chore: update workflow permissions and code scanning reports#2984
chore: update workflow permissions and code scanning reports#2984yashmehrotra wants to merge 2 commits intomainfrom
Conversation
WalkthroughAdds explicit GitHub Actions workflow- and job-level permission scopes and pins an action; updates a runtime dependency version; adds a new Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
88-89: Consider adding explicit permissions to thee2ejob.The
e2ejob has nopermissionsblock, so it inherits the top-levelcontents: read(line 3-4). That is sufficient for current steps (checkout + tests), but for consistency with the hardening applied totest/release.ymljobs, consider declaring it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 88 - 89, Add an explicit permissions block to the e2e job so it doesn't rely on top-level defaults: in the e2e job definition (job name "e2e") add a permissions section granting at minimum contents: read (to match the hardening used in the other jobs such as "test" and "release.yml"); ensure the new permissions block is properly indented under the e2e job so actions/checkout and test steps retain read-only access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/dir.go`:
- Around line 9-14: SafeJoin currently allows names like "." or ".." through
because filepath.Base returns them; update SafeJoin to sanitize/reject those
cases by computing b := filepath.Base(name) (after filepath.Clean) and if b ==
"." || b == ".." || strings.ContainsRune(b, os.PathSeparator) then replace b
with a safe fallback (e.g. a constant like "_" or an escaped form) or return the
pathPrefix-only result; ensure you use filepath.Clean and os.PathSeparator
checks and keep the function signature (SafeJoin) unchanged so callers continue
to receive a safe, non-traversing joined path.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 88-89: Add an explicit permissions block to the e2e job so it
doesn't rely on top-level defaults: in the e2e job definition (job name "e2e")
add a permissions section granting at minimum contents: read (to match the
hardening used in the other jobs such as "test" and "release.yml"); ensure the
new permissions block is properly indented under the e2e job so actions/checkout
and test steps retain read-only access.
🪄 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: 627735e2-0f37-46be-b388-d617d3f6a4e1
⛔ Files ignored due to path filters (1)
report/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/auto-approve.yml.github/workflows/release.yml.github/workflows/test.ymlreport/package.jsonsnapshot/writer.goutils/dir.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ata used in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/dir.go (1)
26-26: Simplify the traversal check withstrings.HasPrefix.The current expression relies on Go's operator precedence (
&&binds tighter than||) and manual slicing, which is easy to misread. Sincefilepath.Relreturns a cleaned path,strings.HasPrefix(rel, "..")combined with a boundary check is both clearer and equivalent.- if rel == ".." || len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator) { + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return "", fmt.Errorf("invalid path: outside base directory") }Requires adding
"strings"to the import block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/dir.go` at line 26, The traversal-check using `if rel == ".." || len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator)` is hard to read and fragile; replace it with a clearer prefix check using strings.HasPrefix(rel, "..") while preserving the boundary behavior (since filepath.Rel returns cleaned paths) and update the surrounding logic in the same function that calls `filepath.Rel`/uses `rel`; also add the "strings" import to the import block. Ensure you reference and modify the exact check expression so behavior remains equivalent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/dir.go`:
- Around line 11-31: SafeJoin currently compares lexical paths only and can be
bypassed via symlinks; modify SafeJoin to resolve symlinks on both the base and
the target before performing the filepath.Rel check by using
filepath.EvalSymlinks (or if the full target does not yet exist, walk up to the
deepest existing ancestor of targetAbs and EvalSymlinks that ancestor) so the
rel computation is done on real paths; ensure errors from EvalSymlinks are
handled and included in returned errors and keep the same return values from
SafeJoin.
---
Nitpick comments:
In `@utils/dir.go`:
- Line 26: The traversal-check using `if rel == ".." || len(rel) >= 3 && rel[:3]
== ".."+string(filepath.Separator)` is hard to read and fragile; replace it with
a clearer prefix check using strings.HasPrefix(rel, "..") while preserving the
boundary behavior (since filepath.Rel returns cleaned paths) and update the
surrounding logic in the same function that calls `filepath.Rel`/uses `rel`;
also add the "strings" import to the import block. Ensure you reference and
modify the exact check expression so behavior remains equivalent.
🪄 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: 05b504a0-599b-4699-8977-e58800be83ac
⛔ Files ignored due to path filters (1)
report/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/auto-approve.yml.github/workflows/release.yml.github/workflows/test.ymlreport/package.jsonsnapshot/writer.goutils/dir.go
✅ Files skipped from review due to trivial changes (3)
- report/package.json
- .github/workflows/auto-approve.yml
- .github/workflows/release.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test.yml
- snapshot/writer.go
| func SafeJoin(pathPrefix, name string) (string, error) { | ||
| baseAbs, err := filepath.Abs(filepath.Clean(pathPrefix)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to resolve base path: %w", err) | ||
| } | ||
|
|
||
| targetAbs, err := filepath.Abs(filepath.Join(baseAbs, name)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to resolve target path: %w", err) | ||
| } | ||
|
|
||
| rel, err := filepath.Rel(baseAbs, targetAbs) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to verify target path: %w", err) | ||
| } | ||
| if rel == ".." || len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator) { | ||
| return "", fmt.Errorf("invalid path: outside base directory") | ||
| } | ||
|
|
||
| return targetAbs, nil | ||
| } |
There was a problem hiding this comment.
SafeJoin does not resolve symlinks, so traversal via symlinks is still possible.
Unlike safeReadFile in playbook/actions/ai.go (which uses filepath.EvalSymlinks before filepath.Rel), SafeJoin compares lexical paths only. If pathPrefix (or any component under it) contains a symlink that points outside the base directory, a name that traverses through it will pass the filepath.Rel check and return an absolute path that actually resolves outside pathPrefix.
For the current snapshot-writer callers this is not exploitable (names are constructed internally), but the doc comment advertises this as a guard against user-controlled input, and the relevant snippets flag views/render_facet.go and playbook/actions/ai.go as candidate call-sites where attacker-influenced inputs flow in. Consider resolving symlinks before the Rel check, or documenting explicitly that the helper assumes a symlink-free base.
🛡️ Suggested hardening
func SafeJoin(pathPrefix, name string) (string, error) {
- baseAbs, err := filepath.Abs(filepath.Clean(pathPrefix))
+ baseAbs, err := filepath.Abs(pathPrefix)
if err != nil {
return "", fmt.Errorf("failed to resolve base path: %w", err)
}
+ if resolved, err := filepath.EvalSymlinks(baseAbs); err == nil {
+ baseAbs = resolved
+ }
- targetAbs, err := filepath.Abs(filepath.Join(baseAbs, name))
+ targetAbs, err := filepath.Abs(filepath.Join(baseAbs, name))
if err != nil {
return "", fmt.Errorf("failed to resolve target path: %w", err)
}
+ if resolved, err := filepath.EvalSymlinks(targetAbs); err == nil {
+ targetAbs = resolved
+ }
rel, err := filepath.Rel(baseAbs, targetAbs)
if err != nil {
return "", fmt.Errorf("failed to verify target path: %w", err)
}
- if rel == ".." || len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator) {
+ if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
return "", fmt.Errorf("invalid path: outside base directory")
}Note: EvalSymlinks fails if the target does not yet exist (common when the helper is used to compute an output path). In that case you typically want to resolve the deepest existing ancestor instead of the full targetAbs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/dir.go` around lines 11 - 31, SafeJoin currently compares lexical paths
only and can be bypassed via symlinks; modify SafeJoin to resolve symlinks on
both the base and the target before performing the filepath.Rel check by using
filepath.EvalSymlinks (or if the full target does not yet exist, walk up to the
deepest existing ancestor of targetAbs and EvalSymlinks that ancestor) so the
rel computation is done on real paths; ensure errors from EvalSymlinks are
handled and included in returned errors and keep the same return values from
SafeJoin.
2 participants
Summary by CodeRabbit
Refactor
Chores