[PPSC-836] fix(install): write Copilot CLI hook to correct path#202
Merged
Conversation
The install wizard was writing the hook config to ~/.config/github-copilot/hooks.json (the VS Code extension path), but Copilot CLI reads hooks from ~/.copilot/settings.json. - Fix copilotHooksPath() to target ~/.copilot/settings.json - Route Copilot install/remove through installMergedHook to preserve existing user settings (memory, model, etc.) - Add ~/.copilot detection in agentdetect - Clean up the legacy dead file on re-install
yiftach-armis
force-pushed
the
fix/PPSC-836-copilot-cli-hook-path
branch
from
bec66dd to
77c47e9
Compare
Test Coverage Reporttotal: (statements) 71.7% Coverage by function |
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
Total: 1
View all 1 findings
🟠 HIGH (1)
CWE-22 - Broken Access Control (CWE-22
Location: internal/install/native_hooks.go:245
Improper Limitation of a Pathname to a Restricted Directory (Path Traversal)): The code reads a file using os.ReadFile after only normalising the path with filepath.Clean. The path it reads comes from client.ConfigPath(), which can be overridden by the global hookConfigPathOverrides map. If an attacker can set a value in that map, they can supply any file path they want. Because the code does not check that the path stays inside a safe directory, the attacker‑controlled path reaches the file‑reading call unchanged. This means the program can be tricked into opening and reading files that it should not access. No additional checks or restrictions are applied, so the risky operation is exposed to the attacker’s input.
const armisHookMarker = "armis-appsec"
// readJSONFileAsMapSafe reads a JSON file into a map, returning an error
// if the file exists but cannot be parsed. This prevents silent data loss
// when overwriting a corrupt or JSONC config file.
func readJSONFileAsMapSafe(path string) (map[string]interface{}, error) {
data := make(map[string]interface{})
// armis:ignore cwe:22 reason:path from known config locations (XDG paths, hardcoded editor config dirs)
b, err := os.ReadFile(filepath.Clean(path)) //nolint:gosec // path from known config locations
if err != nil {
if os.IsNotExist(err) {
return data, nil
}
return nil, fmt.Errorf("reading config: %w", err)
}
if len(b) == 0 {
return data, nil
}
if err := json.Unmarshal(b, &data); err != nil {
return nil, fmt.Errorf("could not parse %s — not modifying hooks config.\n Fix the JSON syntax or remove the file.\n Parse error: %w", path, err)
CWEs: CWE-22: Improper Limitation of a Pathname to a Restricted Directory (Path Traversal)
There was a problem hiding this comment.
Pull request overview
This PR fixes Copilot CLI native hook installation by moving the hook config from the old VS Code Copilot path to the Copilot CLI settings file while preserving existing user settings.
Changes:
- Updates Copilot native hook installation/removal to use merged settings at
~/.copilot/settings.json. - Adds best-effort cleanup for the old incorrect Copilot hook file.
- Extends Copilot detection to recognize the Copilot CLI config directory and updates related tests/comments.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/install/native_hooks.go |
Changes Copilot hook path/merge behavior and adds legacy cleanup. |
internal/install/native_hooks_test.go |
Updates Copilot hook install test for settings merge behavior. |
internal/cmd/uninstall.go |
Updates uninstall comment to reference the new Copilot CLI settings path. |
internal/agentdetect/detector.go |
Detects Copilot CLI via ~/.copilot. |
internal/agentdetect/detector_test.go |
Adds detection coverage for the Copilot CLI config directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+146
to
+147
| if client.ID == HookClientCopilot { | ||
| cleanupLegacyCopilotHook() |
| // if it only contains Armis hook entries (left over from the incorrect path). | ||
| // armis:ignore cwe:22 reason:path is hardcoded from homeDir(".config","github-copilot","hooks.json"), not user input | ||
| func cleanupLegacyCopilotHook() { | ||
| legacyPath := homeDir(".config", "github-copilot", "hooks.json") |
…leanup
- Remove global legacyCopilotPathOverride; use t.Setenv("HOME") for
test isolation so cleanup never touches the developer's real filesystem
- Add Windows APPDATA legacy path cleanup so Windows users also get
their stale ~/.config/github-copilot/hooks.json removed on reinstall
- Add tests for legacy cleanup (armis-only file removed, non-armis preserved)
On Windows, Go's os.UserHomeDir() reads USERPROFILE (not HOME). The legacy cleanup tests were only setting HOME, so homeDir() returned the real user profile on Windows CI, not the temp dir.
Comment on lines
+236
to
+242
| name: "copilot CLI config dir exists", | ||
| setup: func(t *testing.T, home string, _ *mockPlatform) { | ||
| mustMkdirAll(t, filepath.Join(home, ".copilot")) | ||
| }, | ||
| expected: true, | ||
| }, | ||
| { |
shb7628
approved these changes
May 27, 2026
Three fixes for the Copilot CLI preToolUse hook: 1. Install native hooks alongside MCP registration for all editors that have a matching HookClient (Copilot, Cursor, Gemini, Cline). Previously only Codex had explicit InstallNativeHook calls. 2. Update matcher to include "shell" and "terminal" tool names that Copilot CLI actually uses, in addition to "bash". 3. Change installMergedHook/installCursorHook to replace existing Armis hook entries instead of skipping, enabling config upgrades on reinstall.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The scanner flags the specific line calling removeLegacyFileIfArmisOnly, not the enclosing if-block. Move the suppression comment directly above the flagged call.
Comment on lines
+236
to
+240
| name: "copilot CLI config dir exists", | ||
| setup: func(t *testing.T, home string, _ *mockPlatform) { | ||
| mustMkdirAll(t, filepath.Join(home, ".copilot")) | ||
| }, | ||
| expected: true, |
2 tasks
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
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.
Related Issue
Type of Change
Problem
The install wizard wrote the Copilot CLI hook to
~/.config/github-copilot/hooks.json— the VS Code Copilot extension's config directory. GitHub Copilot CLI (terminal agent) reads hooks from~/.copilot/settings.json, so the hook was silently dead and commits went through unscanned.Solution
copilotHooksPath()to target~/.copilot/settings.jsoninstallMergedHookto safely preserve existing user settings (memory, model, etc.) without injecting a spurious"version": 1~/.copilotto agent detection inagentdetect~/.config/github-copilot/hooks.jsonon re-install (only if it contains exclusively Armis entries)Testing
Automated Tests
Manual Testing
~/.config/github-copilot/hooks.jsonexists on disk (old incorrect install)~/.copilot/settings.jsonis what Copilot CLI v1.0.54 reads (copilot help configconfirmshookskey in settings)copilot --helpshows--log-dirdefaulting to~/.copilot/logs/Reviewer Notes
buildCopilotHooks()output format (using"bash"key, lowercase"preToolUse") is unchanged — only the file it's written to has changed.Checklist