fix(plugins): resolve relative executable paths against plugin root

[euxaristia]

Summary

Fixes a High-severity issue where relative command paths defined by plugins (e.g. "command": "./tools/helper.sh") can be hijacked by malicious files placed at that same path inside an untrusted workspace.

When a plugin tool runs, RunWithOptions executes the plugin command within the caller's workspace CWD. If the command is a relative path (e.g. starting with ./ or containing separators), Go's os/exec resolves it relative to the workspace directory.

This PR resolves relative executable paths containing separators to absolute paths inside the plugin's install directory during activation.

Changes

internal/plugins/activate.go

• In expandPluginRootPath, check if the command path is not absolute (!filepath.IsAbs and !isWindowsAbs) and contains path separators. If so, resolve it to an absolute path under the plugin directory using filepath.Join.

internal/plugins/activate_test.go

• Add TestExpandPluginRootPathRelative to assert that relative executable paths are resolved, absolute paths are kept verbatim, and placeholders are correctly expanded.

Test plan

• go test ./internal/plugins/... — ok

Summary by CodeRabbit

• Bug Fixes
  • Fixed plugin executable path resolution for relative values that include directory separators (including / and \).
  • Relative paths now resolve correctly against the plugin directory, while bare executable names and absolute Unix/Windows paths remain unchanged.
  • Enhanced handling for parent-directory traversal and Windows absolute-path detection.
• Tests
  • Added coverage for relative and placeholder-expanded plugin root path cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 886d3189-b785-4485-8bc0-efb86eec245b

📥 Commits

Reviewing files that changed from the base of the PR and between fa7e665 and c7aec84.

📒 Files selected for processing (2)

• internal/plugins/activate.go
• internal/plugins/activate_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/plugins/activate_test.go

---

Walkthrough

expandPluginRootPath now resolves separator-containing relative executable paths against the plugin directory, while toolSafety simplifies permission mapping. Tests cover relative, absolute, bare-name, parent-traversal, and placeholder inputs.

Changes

Plugin activation behavior

Layer / File(s)	Summary
Relative executable path resolution and validation internal/plugins/activate.go, internal/plugins/activate_test.go	Separator-containing relative paths are joined with the plugin directory, while tests verify relative, absolute, bare-name, parent-traversal, and ${AGENT_PLUGIN_ROOT} expansion behavior.
Tool permission mapping internal/plugins/activate.go	toolSafety explicitly maps PermissionAllow and PermissionDeny, with prompt behavior handled by the default branch.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• Gitlawb/zero#185: Introduces the related plugin activation path expansion and tool permission mapping logic.

Suggested reviewers: Vasanthdev2004

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main fix: resolving relative plugin executable paths against the plugin root.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

internal/plugins/activate_test.go (1)

553-558: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider using t.Run for subtest isolation.

Using t.Run per case would produce named subtests, making it easier to identify which input fails without reading the error message. This is a minor ergonomics improvement.

♻️ Optional refactor

 	for _, tc := range cases {
-		got := expandPluginRootPath(tc.input, pluginDir)
-		if got != tc.want {
-			t.Errorf("expandPluginRootPath(%q) = %q, want %q", tc.input, got, tc.want)
-		}
+		t.Run(tc.input, func(t *testing.T) {
+			got := expandPluginRootPath(tc.input, pluginDir)
+			if got != tc.want {
+				t.Errorf("expandPluginRootPath(%q) = %q, want %q", tc.input, got, tc.want)
+			}
+		})
 	}

🤖 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 `@internal/plugins/activate_test.go` around lines 553 - 558, Refactor the
table-driven test around expandPluginRootPath to execute each case within a
named t.Run subtest, using an appropriate case name or index, and keep the
existing assertion inside the subtest for isolated, easily identifiable
failures.

🤖 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.

Nitpick comments:
In `@internal/plugins/activate_test.go`:
- Around line 553-558: Refactor the table-driven test around
expandPluginRootPath to execute each case within a named t.Run subtest, using an
appropriate case name or index, and keep the existing assertion inside the
subtest for isolated, easily identifiable failures.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b95b6b32-d1ad-4b80-bf05-06ef9f5f124e

📥 Commits

Reviewing files that changed from the base of the PR and between aa73a76 and fa7e665.

📒 Files selected for processing (2)

• internal/plugins/activate.go
• internal/plugins/activate_test.go

[kevincodex1]

can you address this @euxaristia

[Vasanthdev2004]

Pulled this into a worktree against current main. The core fix anchors relative command paths (anything with a path separator) to the plugin's install directory via filepath.Join, so a relative executable can't be hijacked by resolving against the caller's CWD; absolute paths and bare binary names pass through unchanged, which is the right call. Build, vet, and the plugins tests all pass, including the new TestExpandPluginRootPathRelative.

One note: ../ escapes that climb out of the plugin root are accepted rather than rejected, but I think that's fine since a manifest author can already specify an arbitrary absolute path and untrusted project-scoped plugins are already dropped by the workspace-trust gate. Approve.

[Vasanthdev2004]

@gnanam1990 when you get a chance — small one: #627 anchors relative plugin exec paths to the plugin install dir so they can't resolve against the caller's CWD. I approved it; the one judgment call is that ../ escapes out of the plugin root are accepted rather than rejected. Worth a second look on the trust angle if you have a minute.

[euxaristia]

Fixed the ineffassign failure on line 623: the initial permission assignment in toolSafety was always overwritten by the switch below it, including a case that just reassigned the same PermissionPrompt value. Removed the redundant initializer and let the switch be exhaustive with a default.