feat: extract VS Code Copilot agent permissions from settings.json

[AakashVelusamy]

The discovery scan reported GitHub Copilot (VS Code) with rules, MCP servers, and skills — but never its permissions. A user who had turned on Copilot's "auto-approve every tool" mode looked identical to a locked-down one. This closes that gap.

Copilot has no dedicated permissions file — its agent-mode controls live in VS Code's settings.json (JSONC). This reads the security-relevant keys and emits the same backend-ready permission record the Cursor extractor produces, so it routes as tool-level permissions with no backend or frontend change.

Every key verified against the actual VS Code binary

The key set was validated against the VS Code 1.136 configuration registry (workbench bundle + Copilot extension package.json) — not blogs. Five keys that appear in blog/Business-era write-ups but the tool never registers were dropped (github.copilot.chat.agent.autoApproveFileChanges / autoApproveTerminal / terminalCommands.blocklist / agent.enabled, and the pre-rename chat.tools.autoApprove kept only as a legacy alias). Reading them would have over-reported settings the tool ignores.

Extracted (all confirmed real): chat.tools.global.autoApprove, chat.permissions.default, chat.tools.eligibleForAutoApproval, chat.tools.terminal.autoApprove (+ enableAutoApprove), chat.tools.edits.autoApprove, chat.tools.urls.autoApprove, chat.agent.enabled / sandbox.enabled / networkFilter / allowed+deniedNetworkDomains, chat.mcp.access / allowedServers / deniedServers, github.copilot.chat.claudeAgent.enabled.

settings.json	to record
chat.tools.global.autoApprove: true	permission_mode: bypassPermissions
chat.tools.terminal.autoApprove allow/deny	allow_rules / deny_rules (Bash(<cmd> *), regex /…/ stripped)
chat.permissions.default: "autoApprove" / "autopilot"	permission_mode: bypassPermissions
chat.tools.edits.autoApprove with any glob true	permission_mode: acceptEdits
chat.tools.terminal.enableAutoApprove: false	allow patterns dropped — the switch gates them
chat.mcp.* (access / allow / deny servers)	kept verbatim in raw_settings as context
chat.agent.sandbox.enabled: "on"/"off"	sandbox_enabled

Scope: user + every profile, not workspace

Read the user-scope settings.json for the default profile and every named profile (profiles/<id>/settings.json), on both stable Code and Insiders, via the same shared enumerate_vscode_user_files helper the workspace mcp.json discovery uses. All profiles are read and merged to the most-permissive posture — a locked-down default with a YOLO named profile is a real risk.

Workspace .vscode/settings.json is deliberately not read: chat.tools.global.autoApprove is policy/application-scoped ("applies globally across all workspaces") and chat.tools.terminal.autoApprove is a restricted setting VS Code honors only in a trusted workspace — so a workspace file's values are a posture the tool may never apply. User + profile settings are the authoritative surface (documented in the class, like Cursor's extractor documents excluding the cloud admin dashboard).

Per-OS paths (verified against real installs)

OS	User settings dir
macOS	~/Library/Application Support/Code/User
Windows	%APPDATA%\Code\User, resolved per-user from the home — never %APPDATA% of the running token (systemprofile under SYSTEM)
Linux	~/.config/Code/User

Skills

VS Code Copilot skills already flow through the shared _get_copilot_cli_skills extractor — paths web-verified against the official VS Code agent-skills docs: ~/.copilot/skills/ (user) and repo .github / .claude / .agents/skills/ (project). Home/repo-based, not profile-scoped; unchanged here, attached to the canonical VS Code row alongside permissions.

Testing

• 50 unit + e2e tests (tests/test_copilot_vscode_permissions.py): the key→record mapping, the phantom-key guard, profile merge/escalation, JSONC/BOM, and the real macOS extractor over planted default + named-profile fixtures.
• Live-verified on real Windows + Linux VMs and macOS via the real detection path: Copilot detected, permissions attach → bypassPermissions from a YOLO named profile, deny = only the real terminal key, MCP-deny merged from a second profile, skills attach — per-user path correct on each OS.
• No regression: 186 tests across the permission + MCP/Copilot suites green.

🤖 Generated with Claude Code

Greptile Summary

The PR extracts VS Code Copilot agent permissions from user and named-profile settings and attaches them to the canonical Copilot report.

• Adds JSONC parsing and permission normalization across stable and Insiders configuration directories.
• Preserves per-user permission records for elevated multi-user scans.
• Adds OS-specific settings-directory discovery and permission extraction tests.

Confidence Score: 4/5

The PR is not yet safe to merge because a reported stable or Insiders installation can still receive the other channel’s Copilot permission posture.

The extractor independently scans both channels and returns whichever is most permissive, while the attachment path supplies no installation or channel identity, leaving the previously reported channel-attribution failure outstanding.

Files Needing Attention: scripts/coding_discovery_tools/coding_tool_base.py, scripts/coding_discovery_tools/ai_tools_discovery.py

Important Files Changed

Filename	Overview
scripts/coding_discovery_tools/coding_tool_base.py	Adds contained JSONC reads and permission aggregation, but the final cross-channel selection remains detached from the detected installation channel.
scripts/coding_discovery_tools/ai_tools_discovery.py	Integrates Copilot permissions and fixes per-user filtering, while canonical-row attachment still lacks channel identity.
scripts/coding_discovery_tools/coding_tool_factory.py	Registers the OS-specific Copilot settings extractors.
scripts/coding_discovery_tools/mcp_extraction_helpers.py	Extends shared VS Code profile-file enumeration used by the settings extractor.
tests/test_copilot_vscode_permissions.py	Covers parsing, profile merging, containment, and per-user attribution, but does not bind a detected installation row to its matching settings channel.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Scan user homes] --> B[Read stable profiles]
  A --> C[Read Insiders profiles]
  B --> D[Merge profiles within stable]
  C --> E[Merge profiles within Insiders]
  D --> F[Select most-permissive channel]
  E --> F
  F --> G[Attach to canonical VS Code Copilot row]
  G --> H[Filter permission record per user]

Loading

_{Reviews (19): Last reviewed commit: "revert: drop the windows parent-junction..." | Re-trigger Greptile}

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

3 findings — 1 high-confidence, 2 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks, Greptile.

• 🔴 [MEDIUM] Workspace walk follows symlinked .vscode dirs and settings.json leaves — scripts/coding_discovery_tools/coding_tool_base.py:1166

  • What: In _walk_workspace_settings, the .vscode branch runs before the is_symlink_or_junction(item) guard, and the settings.json leaf is only checked with is_file() (which follows links), so a symlinked config dir or leaf is read and reported.
  • Why: Under an elevated/root scan across all user homes, an unprivileged local user plants ~/proj/.vscode -> /root/somewhere (or settings.json -> /root/x.json). The root-owned scan reads it, and if it parses as a JSON dict with at least one SECURITY_RELEVANT_KEYS entry, its values land in raw_settings with the unresolved in-home path in settings_path — content from outside the scan boundary, attributed to the user's workspace. There is also no size cap on read_text, so a link to a very large regular file inflates scan memory. (Regular-file-only is_file() does block FIFO/device tricks.)
  • Fix: Apply the same guards the sibling MCP walker uses — check is_symlink_or_junction(item) before the .vscode branch, and check is_symlink_or_junction(settings) on the leaf before is_file(). Optionally confirm the resolved path stays under user_home.
  • Flagged by: Claude (lead), Greptile (P2, security)

• 🟡 [MEDIUM] MCP server entries are copied verbatim into raw_settings — scripts/coding_discovery_tools/coding_tool_base.py:1200

  • What: _build_record copies the whole value of every security-relevant key, including chat.mcp.allowedServers / deniedServers, into raw_settings with no field filtering.
  • Why: The code's own _mcp_lists handles list items that are objects with url / command keys, so object-shaped entries are expected. MCP server objects commonly carry auth material — bearer tokens in a URL query string, headers, or env. Those would be shipped to the backend and stored in the permission record. ⚠️ Uncertain how often real allowedServers entries are objects rather than plain names — worth checking against a real install before deciding severity.
  • Fix: Whitelist the fields kept from MCP entries (name / id only), or redact headers, env, and URL query strings before writing to raw_settings.
  • Flagged by: Claude (lead)

• 🟡 [LOW] Multi-user scans silently drop every record but the first — scripts/coding_discovery_tools/coding_tool_base.py:1195

  • What: extract_settings() logs a warning and returns records[0] when several users have Copilot settings.
  • Why: This is a security-posture reporting tool. On a shared/multi-user device, a user running chat.tools.global.autoApprove: true (YOLO) is reported as having no permission record at all if another user's record sorts first — a false negative on exactly the condition this PR was written to surface. The kept record is also attached to the canonical row regardless of which user it came from.
  • Fix: Return all per-user records and let the per-user report filter pick the matching one, or key the record by the user home it came from.
  • Flagged by: Claude (lead), Greptile (P1)

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 064cbf82 · 2026-09-04T15:03Z

[cursor]

Stale comment

Agentic security review of the Copilot VS Code settings.json permission extractor. One medium finding: the new workspace walk follows .vscode / settings.json redirects that sibling Copilot MCP extractors already refuse, which can leak another user's Copilot posture into a privileged scan report.

  

_{Sent by Cursor Security Agent: Security Reviewer}

[cursor]

Stale comment

[AakashVelusamy]

Review disposition — every finding accounted for (as of 883fab8)

Greptile

Finding	Disposition
P1 — Multi-user settings discarded (extract_settings returned records[0])	Fixed. Now returns the most-permissive user's record (with its own settings_path), so under an elevated scan a YOLO user is never hidden behind a benign first user. Regression test proven to fail on records[0].
P1 — Editor channel mismatch (selected stable's first file)	Fixed. Both stable Code and Insiders are read and merged to the most-permissive posture; an Insiders-only install surfaces correctly.
P2 (security) — Workspace links bypass scan boundaries	Fixed by removal. The workspace .vscode walk is gone entirely — permission scope is user + profiles only (the auto-approve keys are policy/application-scoped and restricted = trusted-workspace-only, so workspace files would report a posture the tool never applies).

Consensus / Vignesh security review (head 064cbf8)

Finding	Disposition
🔴 [MEDIUM] Workspace walk follows symlinked .vscode	Fixed by removal (as above — no workspace walk).
🟡 [MEDIUM] MCP server entries copied verbatim into raw_settings (token/headers/env leak)	Fixed. _redact_mcp_secrets keeps only the server name/id in raw_settings; the policy lists already carry just names. Regression test asserts no auth material reaches the record (proven to fail without the redaction).
🟡 [LOW] Multi-user drops all but first	Fixed — same most-permissive-user change as the Greptile P1.
(reviewer note) no size cap on read_text	Addressed. _parse_jsonc skips a >8MB settings file.

Cursor (Bugbot + Security)

No open findings on the latest commit.

Key-set correctness (from the /never-gonna-mess-you-up pass)

Every extracted key was verified against the VS Code 1.136 configuration registry; five blog-era/Business-only phantom keys were removed (they made an acceptEdits path and a blocklist→deny path into dead over-reporting). A guard test blocks their reintroduction.

All fixes carry regression tests proven to fail on the pre-fix code. Live-verified on real Windows + Linux VMs + macOS.

[cursor]

Stale comment

Config/template injection review — Copilot VS Code settings extractor

Verdict: NO_FINDING

The new BaseGitHubCopilotSettingsExtractor does not newly wire CODE DATA into a config object, prompt template, or template variables. It reads local user-scope VS Code settings.json and attaches a filtered permissions record to the discovery report payload only.

Evidence

1. tool_dict["permissions"] is a discovery report field, not a config/template sink

   • Attached at ai_tools_discovery.py:2561-2563 for the canonical VS Code Copilot row.
   • Flows into generate_single_tool_report → tools[0] (ai_tools_discovery.py:2976-2985).
   • Uploaded via send_report_to_backend / S3 (utils.py:834+, s3_uploader.py hashes permissions for dedup only).
   • Local mcp-tools-cache.json refresh uses projects[].mcpServers only — no permissions reads/writes (mcp_tools_cache.py).

2. Parity with Cursor

   • Cursor already emits the same backend record shape including settings_path + filtered raw_settings (coding_tool_base.py:865-869).
   • Copilot mirrors that contract (coding_tool_base.py:1241-1257).

3. raw_settings content

   • Built only from SECURITY_RELEVANT_KEYS (coding_tool_base.py:1126-1136, 1235).
   • Values are permission flags, terminal auto-approve patterns → Bash(...) rules, MCP server names (secrets redacted at 1296-1312), URLs/domains — not source files, prompts, completions, snippets, or stack traces.

4. enumerate_vscode_user_files

   • Filename is a fixed literal ("settings.json" / "mcp.json"); path join + profiles/*/filename glob (mcp_extraction_helpers.py:552-590, call at coding_tool_base.py:1161).
   • No user-controlled glob/template injection surface in this path.

5. Read vs inject

   • This is a local settings read → report serialization path. No write of extracted user code into a config file or prompt/template.

  

_{Sent by Cursor Security Agent: Security Reviewer}

[cursor]

Stale comment

Verdict: CONFIRMED (blast: medium)

The new VS Code Copilot settings.json extractor follows POSIX symlinks / Windows junctions and can spoof per-user permission identity via unresolved settings_path + filter_tool_projects_by_user.

Evidence

1. No symlink/junction guard on user settings leaves

   • enumerate_vscode_user_files never calls is_symlink_or_junction; it accepts leaves via Path.exists() / Path.is_file() (mcp_extraction_helpers.py:576, :585), which follow redirects.
   • _parse_jsonc also skips the guard and uses path.is_file() + path.read_text() (coding_tool_base.py:1217, :1224) — content is whatever the redirect targets.
   • Contrast: workspace Copilot MCP leaves do skip redirects (macos/.../mcp_config_extractor.py:216-217 and OS siblings). User-scope mcp.json via this same helper does not.

2. settings_path is the unresolved in-home path

   • Record stores str(path) from enumeration (coding_tool_base.py:1244), not resolve() / realpath. A planted …/Code/User/settings.json (or profiles/*/settings.json) symlink/junction still looks like the attacker’s home path while content can be out-of-home.

3. filter_tool_projects_by_user treats that path as ownership

   • Keeps permissions when settings_path is under user_home (non-managed) (ai_tools_discovery.py:1847-1855). Extractor comments explicitly rely on this (coding_tool_base.py:1181-1185). Attachment: ai_tools_discovery.py:2558-2563.

4. Attack path (root/admin all-user scan)

   • Local user plants Code/User/settings.json or profiles/<id>/settings.json as symlink/junction → victim’s settings.
   • Elevated scan reads victim content under attacker’s path string → most-permissive merge can promote that posture → filter attributes the record to the attacker.
   • Requires elevated scan (root/admin can follow into the victim tree); non-elevated single-user scans are out of scope for cross-user spoof.

5. NEW vs pre-existing

   • Pre-existing for user-scope mcp.json: same unguarded exists()/is_file() enumeration (this PR generalized enumerate_vscode_mcp_files → enumerate_vscode_user_files).
   • New risk for permissions: expanding that helper to settings.json and wiring the record through settings_path → filter_tool_projects_by_user creates a cross-user identity/attribution channel for Copilot permission posture (bypassPermissions, terminal auto-approve, MCP allow/deny) that did not exist on this surface before.

Blast: medium — integrity of per-user security posture on elevated multi-user scans (misattribution / spoofed YOLO), not RCE. Workspace MCP leaves are already guarded; user settings leaves are not.

  

_{Sent by Cursor Security Agent: Security Reviewer}

[cursor]

Stale comment

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

2 findings — 1 high-confidence, 1 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.

• [MEDIUM] MCP URL fallback leaks credentials into the shipped policy lists — scripts/coding_discovery_tools/coding_tool_base.py (_mcp_lists, in the new BaseGitHubCopilotSettingsExtractor)

  • What: _redact_mcp_secrets sanitizes raw_settings, but _mcp_lists independently re-reads the same raw server objects with item.get("name") or item.get("id") or item.get("url") or item.get("command"), so a server entry with no name/id puts its full URL — query string included — into mcp_tool_allowlist and mcp_policies.
  • Why: A real-world entry like {"url": "https://mcp.example/?token=SECRET123"} (or {"command": "mcp-server --api-key=..."}) is emitted verbatim in the backend-bound permission record. This is the residual path from the already-fixed raw_settings leak, not a re-flag: the regression test only asserts on raw_settings, and its fixture object has a name, so the URL fallback never fires. Your own mapping test asserts the leaky behavior as expected (mcp_tool_allowlist == [..., "https://x"]).
  • Fix: Route the policy lists through the same redaction. Either drop the url/command fallbacks (fall back to "<redacted>" or a stable hash), or strip credentials before emitting — parse the URL and keep only scheme+host+path, discard query/userinfo, and never emit a raw command string. Extend the redaction test with a name-less, token-bearing server object.
  • Flagged by: Claude (lead)

• [MEDIUM] Shared VS Code file enumeration still follows symlinks/junctions — scripts/coding_discovery_tools/mcp_extraction_helpers.py:~576 (enumerate_vscode_user_files)

  • What: default_file.exists() and the profile-glob is_file() both follow POSIX symlinks and Windows junctions, and _parse_jsonc then reads through the link while settings_path keeps the unresolved in-home path.
  • Why: On an elevated multi-user scan, a local user can point their own ~/.config/Code/User/settings.json (or a profiles/<id>/ leaf) at another user's file. The victim's Copilot posture is parsed and attributed to the planter, because filter_tool_projects_by_user keys off the unresolved settings_path. Disclosure is bounded to the security-relevant keys, so this is posture disclosure and misattribution, not secret theft. Note this gap is pre-existing in the helper for mcp.json; this PR touches those lines and extends the helper to settings.json, which is what feeds the permissions identity path. ⚠️ I could not verify from the diff whether any caller applies a link guard upstream — worth checking before acting.
  • Fix: Apply the same is_symlink_or_junction guard the workspace MCP walkers use, on both the default leaf and each profile leaf, and store str(path.resolve()) (or drop the record when the resolved target escapes user_home) so attribution can't be forged.
  • Flagged by: Claude (lead), Cursor Security Reviewer (prior comment on 883fab8)

Previously acknowledged (not re-flagged)

• Workspace .vscode symlink walk bypasses scan boundaries — fixed by removing the workspace walk entirely; permission scope is user + profiles only.
• MCP server entries copied verbatim into raw_settings — fixed via _redact_mcp_secrets (see finding 1 for the sink that fix does not cover).
• Multi-user records collapsed to records[0] — changed to return the most-permissive user's record with its own settings_path.
• Stable vs. Insiders channel mismatch — both channels are now read and merged to the most-permissive posture.
• No size cap on read_text — _parse_jsonc now skips files over 8 MB.

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 883fab85 · 2026-09-04T16:13Z

[AakashVelusamy]

Addendum — Greptile 1/5 findings (as of c941fb9)

Finding	Disposition
Linked settings escape the scan boundary (user-scope reads followed symlinks)	Fixed in c941fb9. _resolves_within drops any settings.json / profile file whose realpath escapes the user's home — under a root scan a user can no longer symlink their settings.json to a root-owned file and have it reported as theirs. An in-home stow/chezmoi symlink still resolves inside and is read. Regression test proven to fail without the guard.
Channels merged without preserving attribution	Fixed in c941fb9. The per-user merge now bases on the most-permissive profile/channel record, so settings_path attributes to the actual risk source (e.g. an Insiders YOLO profile), not the first file read.
Multi-user collapses to one record	Accepted architectural limitation, mitigated. The scan builds each tool report once for all users then filters per user (process_single_tool once → filter_tool_projects_by_user per home) — a single permissions field per canonical row. This is the model every sibling settings extractor uses (Cursor/Claude), not new here. The most-permissive pick makes it security-correct for the case that matters: a YOLO user's own report keeps their record (its settings_path is under their home); only benign users omit permissions. Preserving every user's distinct posture (two simultaneous YOLO users) needs a per-user permission structure + filter_tool_projects_by_user/backend changes — out of scope for this PR, and I've flagged it for a follow-up rather than re-architecting the shared attachment path here.

All fixable findings are fixed with prove-fail regression tests. The remaining item is a deliberate, documented tradeoff shared fleet-wide — surfaced for the reviewer's call on the score.

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

3 findings — 1 high-confidence, 2 to triage. Reviewers: consensus reviewer, semgrep (0), gitleaks (0).

• 🔴 [MEDIUM] MCP server URLs bypass _redact_mcp_secrets and reach the report — scripts/coding_discovery_tools/coding_tool_base.py (_mcp_lists, ~L1350)

  • What: _redact_mcp_secrets sanitizes raw_settings, but _mcp_lists runs on the unredacted data and falls back to item.get("url") or item.get("command") when a server object has no name/id — so the full URL or command string lands in mcp_tool_allowlist and mcp_policies.
  • Why: A remote MCP server is commonly configured as {"url": "https://mcp.example.com/sse?key=SECRET"} with no name key. That whole string, API key included, is copied into the permission record and shipped to the backend. Your own test asserts this flow: test_mcp_allow_deny_strings_and_objects expects mcp_tool_allowlist == [..., "https://x"]. The redaction test doesn't catch it because its object has a name, so the URL fallback never fires. This contradicts the disposition note "the policy lists already carry just names".
  • Fix: In names(), when falling back to url, emit only scheme+host (drop query, fragment, and userinfo); drop the command fallback or reduce it to the bare executable name. Add a regression test with a name-less {"url": "...?token=SECRET"} server asserting SECRET appears nowhere in the record.
  • Flagged by: consensus reviewer (provable from diff + existing tests)

• 🟡 [LOW] Check-then-open race on the symlink guard — scripts/coding_discovery_tools/coding_tool_base.py:1245 (_parse_jsonc)

  • What: _resolves_within calls os.realpath during enumeration, then _parse_jsonc independently re-opens the same path with is_file() / stat() / read_text(). The path is re-resolved at open time, not pinned.
  • Why: Narrower than the escape you already fixed in c941fb9. Under a root all-user scan, the settings file sits in a directory the unprivileged user owns, so they can flip the link between the check and the read (a tight swap loop wins often enough to matter) and get an out-of-home file parsed and attributed to them.
  • Fix: Open once and reuse the handle — os.open(path, O_RDONLY | O_NOFOLLOW), then run the containment check against os.readlink('/proc/self/fd/N') (or compare os.fstat to the lstat taken at enumeration) and read from that same descriptor.
  • Flagged by: consensus reviewer

• 🟡 [LOW] raw_settings copies terminal and URL auto-approve values verbatim — scripts/coding_discovery_tools/coding_tool_base.py:~1258 (_build_record)

  • What: Redaction covers only chat.mcp.allowedServers / deniedServers. chat.tools.terminal.autoApprove and chat.tools.urls.autoApprove are copied into raw_settings unchanged.
  • Why: Those values are user-authored command patterns and URLs. An auto-approve entry like a curl invocation with an Authorization header, or an allow-listed URL with a signed query token, is stored and shipped in the same record the MCP redaction was added to protect. ⚠️ Lower confidence than the finding above — plausible from the key semantics, not proven by a fixture in this diff.
  • Fix: Extend the redaction pass to strip query strings from URL entries and mask obvious credential substrings (Bearer , token=, api[_-]?key=) in terminal patterns before they enter raw_settings.
  • Flagged by: consensus reviewer

Previously acknowledged (not re-flagged)

• Workspace .vscode symlink/junction follow (Greptile P2, Cursor MEDIUM ×2) — fixed by removing the workspace walk entirely; permission scope is user + profiles only.
• User-scope settings symlink escaping home (Greptile 1/5) — fixed in c941fb9 via _resolves_within, with a prove-fail regression test.
• Multi-user records collapsed to one — accepted architectural limitation; mitigated by returning the most-permissive user's record, matching every sibling settings extractor; follow-up flagged.
• Stable vs Insiders channel merge — fixed; merge now bases on the most-permissive profile/channel so settings_path attributes to the real risk source.
• MCP server objects copied verbatim into raw_settings — fixed by _redact_mcp_secrets (the residual gap in the policy lists is finding 1 above, which is a different code path).
• No size cap on read_text — fixed; _parse_jsonc skips files over 8 MB.

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head c941fb93 · 2026-09-04T17:22Z

[AakashVelusamy]

@greptile-apps re-review request — on the two remaining P1s, with the consistency + backend context.

These are deliberate, fleet-consistent choices, not defects unique to this PR:

1. "Multi-user settings collapse to one record." This is the established pattern of every settings extractor in this repo, not new here:

• BaseCursorSettingsExtractor.extract_settings() returns settings_list[0] — literally logging "Found Cursor settings for N users, returning first only" (coding_tool_base.py:819-821).
• The scan is once-then-filter: process_single_tool runs once for all users, then filter_tool_projects_by_user trims per user. One permissions field per canonical row is the contract every tool shares.
• The backend requires exactly one permissions dict per user report — _process_permissions(device, installation, permissions_data) in ai-gateway-data rejects a non-dict, and stores per (device, tool_installation, settings_path) where AIToolInstallation is keyed by home_user. So the wire contract is one-dict-per-user-report; a list would be rejected.
• Copilot here improves on the sibling pattern: it returns the most-permissive user's record, so the riskiest user (a YOLO config) is surfaced in their own report rather than an arbitrary first user. A benign user's settings genuinely carry no risk signal.

Full per-user permission attribution (a record for every user simultaneously) is achievable given the backend's per-user AIToolInstallation, but only as a uniform change across all extractors + the shared filter_tool_projects_by_user — making Copilot alone diverge with a tool-specific branch in shared code would be the real inconsistency. I'll track that as a fleet-wide follow-up rather than special-case it here.

2. "Channels merged without matching the detected installation." For a security-posture tool this is intentional: if any installed channel (stable or Insiders) has a YOLO config, the device has a YOLO Copilot, and the most-permissive merge surfaces it. settings_path now attributes to the actual risk source. Reporting only one channel could hide a risk present in the other.

The genuinely-fixable items you raised — the workspace-symlink escape, MCP-secret leakage into raw_settings, channel attribution, and settings size cap — are all fixed with prove-fail regression tests. Requesting a re-review of the score with this context.

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

2 findings — 1 high-confidence, 1 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.

• [MEDIUM] MCP server URL with embedded credentials survives redaction and ships in the policy lists — scripts/coding_discovery_tools/coding_tool_base.py:1342

  • What: _mcp_lists.names() falls back to item.get("url") (then item.get("command")) when an MCP server object has no name/id, and that raw string is written verbatim into mcp_tool_allowlist and mcp_policies.
  • Why: _redact_mcp_secrets only sanitizes raw_settings, and only for the two chat.mcp.* keys. A settings entry such as "chat.mcp.allowedServers": [{"url": "https://mcp.example/?token=SECRET123"}] is redacted to "<redacted>" in raw_settings but is emitted in full as mcp_tool_allowlist: ["https://mcp.example/?token=SECRET123"], so the token leaves the customer machine in the permission record. The existing test test_mcp_allow_deny_strings_and_objects asserts exactly this pass-through ("https://x" in the allowlist); test_mcp_server_secrets_redacted_from_raw_settings only greps raw_settings, so nothing catches it.
  • Note on prior discussion: the disposition table says "the policy lists already carry just names." That holds only when the object has a name or id — the url/command fallback is the gap.
  • Fix: drop the url/command fallback in names() (use <unnamed> or a hash), or strip userinfo and the query string before appending. Extend the redaction test to assert SECRET is absent from json.dumps(rec), not just rec["raw_settings"].
  • Flagged by: this review (provable from the diff and from the committed test expectations)

• [MEDIUM] Containment check is not enforced at read time (check-then-read race) — scripts/coding_discovery_tools/coding_tool_base.py:1164

  • What: _iter_user_settings_files calls _resolves_within on a path, then _parse_jsonc re-opens that same pathname with is_file() / stat() / read_text(), none of which re-check where the name now points.
  • Why: during a root/admin all-users scan, the scanned user owns the directory and can replace settings.json (or a parent component) with a symlink after the check passes. The privileged process then reads the new target. Practical impact is bounded — the target must still parse as a JSON object and contain chat.* Copilot keys — so this leaks another user's VS Code posture rather than arbitrary files. ⚠️ Exploitability depends on winning a short race against a scan the attacker does not schedule; I could not confirm a reliable trigger from the diff alone.
  • Fix: open once and validate the descriptor rather than the name — os.open(path, O_RDONLY | O_NOFOLLOW), then os.fstat and compare against the home-directory containment rule, and read from that fd. This closes the race and also removes the need for the separate realpath pass.
  • Flagged by: this review; Greptile (P1, 2026-09-04T18:29:58Z on c941fb9) — no maintainer response yet

Previously acknowledged (not re-flagged)

• Workspace .vscode symlink/junction walk follows links — maintainer removed the workspace walk entirely in 883fab8; permission scope is user + profiles only.
• MCP server objects copied verbatim into raw_settings — fixed by _redact_mcp_secrets with a regression test. (The sibling policy-list path is a separate code path and is reported above.)
• User settings symlink escaping the home directory — fixed in c941fb9 via _resolves_within, with a regression test.
• Stable vs Insiders channel attribution mismatch — fixed in c941fb9; the merge now bases on the most-permissive record so settings_path points at the real risk source.
• Multi-user scan collapses to one permission record — maintainer accepted as an architectural limitation shared with the Cursor/Claude extractors, mitigated by returning the most-permissive user's record.
• No size cap on read_text — addressed; _parse_jsonc skips files over 8 MB.

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head b262a1f8 · 2026-09-04T19:02Z

[cursor]

Stale comment

Agentic security review of the Copilot VS Code permissions extractor on the current head. One medium finding remains: the new home-containment check is not bound to the later file open, so a privileged all-user scan can still follow a raced redirect.

  

_{Sent by Cursor Security Agent: Security Reviewer}

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

3 findings — 1 high-confidence, 2 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.

• [MEDIUM] 🔴 os.open() happens before any type or containment check — scripts/coding_discovery_tools/coding_tool_base.py:1163

  • What: _read_contained opens the attacker-reachable path first (os.open(str(path), os.O_RDONLY)) and only checks S_ISREG and home-containment afterwards, on the already-open descriptor.
  • Why: opening a FIFO for read blocks until a writer appears. Under a root/admin all-user scan, any local user replaces their Code/User/settings.json with mkfifo and the whole discovery scan hangs indefinitely — the S_ISREG guard never runs because the open never returns. The same open-first order means a symlink to a character device is genuinely opened (open-time side effects) before the containment check rejects it. The old path.is_file() order could not hang, so this is introduced by the new reader.
  • Fix: open with O_NONBLOCK (os.open(str(path), os.O_RDONLY | getattr(os, "O_NONBLOCK", 0))), keep the existing fstat/S_ISREG check, then clear O_NONBLOCK via fcntl before reading. Optionally add os.O_NOFOLLOW on the leaf and re-open in-home symlink targets deliberately.
  • Flagged by: this review (not seen by semgrep/gitleaks or the prior bot threads).

• [MEDIUM] 🟡 Containment check accepts a hard link and never checks ownership — scripts/coding_discovery_tools/coding_tool_base.py:1172

  • What: containment is realpath(path) under realpath(user_home) plus a (st_dev, st_ino) identity match. A hard link has no target path — it is an in-home path — so the check passes, and there is no st_nlink or st_uid test against the scanned user.
  • Why: on a shared host, a local user hard-links another user's settings.json into their own VS Code profile dir. The privileged scan reads the victim's Copilot posture (chat.tools.global.autoApprove, terminal allow/deny, chat.mcp.*) into raw_settings, keeps settings_path under the planter's home, and filter_tool_projects_by_user attributes it to the planter. Greptile notes the repo already has a link-count/ownership safeguard this reader skips.
  • ⚠️ Exploitability is conditional: needs the same filesystem, traverse access into the victim's directory, and on Linux fs.protected_hardlinks=1 (the distro default) additionally requires the attacker to own or have read+write on the target. macOS lacks that sysctl.
  • Fix: after fstat, reject st.st_nlink > 1, and require st.st_uid to match the scanned user's uid (skip the uid check on Windows, where the link-count check still applies to mklink /H).
  • Flagged by: this review + greptile (security, P2/summary).

• [LOW] 🟡 No size cap on the settings file read — scripts/coding_discovery_tools/coding_tool_base.py:1221

  • What: _parse_jsonc reads the whole file with handle.read(), then runs _strip_jsonc_comments and _strip_trailing_commas over it, with no byte limit.
  • Why: the sibling helpers in the same module deliberately cap reads (_VSCODE_MCP_PROVIDER_CACHE_MAX_BYTES = 5 MB, _VSCODE_EXTENSION_STATE_MAX_BYTES = 1 MB); this path has no equivalent. Any local user plants a multi-GB settings.json (or a profile full of them) and the privileged scan holds several copies in memory through the regex passes and json.loads — memory exhaustion or a very long stall in the discovery agent. Values are also copied verbatim into raw_settings and shipped to the backend.
  • Fix: fstat size is already available in _read_contained — bail out above a cap in the same style as the existing constants, and read at most that many bytes.
  • Flagged by: this review.

Previously acknowledged (not re-flagged)

• Multi-user records collapsed to one (max(records)) — greptile P1, raised 4×; the author states the rationale in code at coding_tool_base.py ("the canonical row carries ONE permissions record… return the MOST-PERMISSIVE user's record so a YOLO user is never hidden behind a benign one") and locks it with test_multi_user_scan_surfaces_the_riskiest_user. Accepted design choice.
• Stable and Insiders channels merged into one record — greptile P1; documented in the class docstring and asserted by test_riskiest_channel_wins_and_is_attributed, which requires settings_path to point at the channel carrying the risk. Accepted design choice.
• Workspace .vscode/settings.json symlink/junction follow — cursor MEDIUM (commits 064cbf82, d30333c4); the workspace walk is no longer in the diff, and the PR body documents workspace scope as deliberately excluded.
• Containment TOCTOU between validation and read — greptile P1 + cursor MEDIUM (883fab85, d30333c4); addressed in the current diff by _read_contained, which validates the open descriptor and compares (st_dev, st_ino) against the in-home path.

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 190e99ed · 2026-09-04T20:12Z

[AakashVelusamy]

Review disposition — as of d537ebb

Two of Greptile's findings this round were real vulnerabilities. Both are fixed, each with a regression test proven to fail on the prior commit.

Fixed — security

Finding	Disposition
Containment race during privileged reads	Fixed in 190e99e. The check resolved the path, then the read opened it separately; a user could swap their settings.json for an out-of-home link in that window and an elevated scan would report a file they cannot see, attributed to their own in-home path. Now the file is opened once and the descriptor actually held is validated: regular file, resolved path inside the home, and that path's (st_dev, st_ino) must match the descriptor's — refusing a swap on either side of the open. In-home stow/chezmoi symlinks still work. Guard proven to fail on 3714820.
Foreign hard links bypass containment	Fixed in d537ebb. A hard link keeps its target's owner while its path stays inside the home, so realpath containment is blind to it. Now the descriptor's uid must match the home directory's owner. Proven on the Linux runner with real privilege separation: a root-owned file hard-linked into /home/ubuntu → before: leaked root content; after: refused. (st_uid is 0 for all files on Windows, where the comparison is a no-op.)

Also fixed this round (found by extended testing, not by review)

Finding	Disposition
Object form of chat.tools.terminal.autoApprove silently dropped	Fixed in e55bb0b. VS Code also accepts {"approve": true, "matchCommandLine": true}; only bare booleans were read, so every object-form entry vanished — including an object-form false, which hid a real terminal denial. Verified on real Windows/Linux: deny now contains Bash(curl | sh *).
Unreadable-settings case asserted nothing as root	Fixed in 3714820 — root bypasses chmod, so the case is skipped when elevated (the shape an MDM scan actually runs in).

Held — documented, fleet-wide behaviour

Finding	Disposition
Multi-user permissions collapse to one record	Not introduced here. transform_settings_to_backend_format states it "selects the highest precedence settings file … No merging is performed", so every tool (Claude, Augment, Copilot CLI, Cursor) collapses to a single permissions dict, which filter_tool_projects_by_user then keeps for one user. This extractor differs only in the tiebreak: siblings pick highest-precedence scope, this picks most-permissive — and since every record here is scope: "user", precedence could not discriminate anyway, so the tiebreak is strictly safer (a YOLO user can never be hidden behind a benign one). Changing it means changing the transform, the per-user filter and the backend record shape for all tools — tracked separately, not smuggled into this PR.
Stable and Insiders merged	Deliberate. For a security-posture tool, if any installed channel carries a YOLO config the device has a YOLO Copilot; settings_path attributes to the channel that carries the risk. Reporting one channel could hide a risk present in the other.

Verification on d537ebb

39 tests. macOS 39 passed · Linux VM 38 passed, 1 skipped · Windows VM 33 passed, 6 skipped. The extended battery added terminal verdict shapes, mode edge cases, parse resilience, channel/profile coverage (Insiders-only, riskiest-channel attribution, 20-profile determinism, stray entries under profiles/), and both containment guards.

[cursor]

Stale comment

Agentic security review for PR 285 (head d537ebb). One medium finding remains: Windows privileged reads of Copilot settings.json omit the repo's st_nlink > 1 hard-link guard while the new uid check is a documented no-op on NT. Prior TOCTOU between realpath and read_text is addressed by the single-open _read_contained descriptor checks.

  

_{Sent by Cursor Security Agent: Security Reviewer}

[AakashVelusamy]

Disposition for the two remaining findings (d537ebb, Greptile 3/5)

Both security findings from the previous round — the containment race and the foreign hard link — are fixed and no longer appear in Greptile's summary. What remains are two reporting-fidelity points. Both are accurate; here is why neither blocks this PR.

1. "Elevated scans omit most users' permissions"

Accurate, and fleet-wide — not introduced here. transform_settings_to_backend_format documents its own contract:

"selects the highest precedence settings file and transforms it to the backend format. No merging is performed — we simply extract and send the settings as-is from the highest precedence source."

So every settings extractor (Cursor, Claude, Augment, Copilot CLI) collapses to a single permissions dict, which filter_tool_projects_by_user then keeps only for the user whose home holds its settings_path. This extractor differs only in the tiebreak: the siblings pick highest-precedence scope, which cannot discriminate here because every record is scope: "user" — effectively arbitrary. This picks the most-permissive record, so a user running auto-approve can never be hidden behind a benign one.

Making every user's record survive means changing the shared transform, the per-user filter, and the backend record shape for all tools at once. That is a deliberate fleet-wide change, not something to smuggle into a single tool's PR.

2. "Editor-channel policies can be attributed to the wrong installation"

Accurate in principle, and rare in practice. The concern needs two settings sources holding different Copilot permission keys. Checked every machine available:

Machine	Stable Code	Insiders
dev Mac	yes	no
win runner (unbound)	yes	no
win runner (unbound.poc-win-runner)	yes	no
linux runner (root)	yes	no

No dual-channel install anywhere, and the one VS Code profile present uses useDefaultFlags — it inherits the default settings and has no settings.json of its own, so it produces no divergent record either. The merge therefore almost never fires.

Where it could fire, the behaviour is deliberate for a security-posture tool: if any installed channel or profile carries an auto-approve config, the device has one, and settings_path attributes to the source that carries the risk rather than to the first file read.

That said, the fleet's "report as-is, never merge" rule is the better long-term shape — reporting one real file unchanged rather than a union. Tracking that separately rather than churning a green PR for a scenario that does not occur on any observed machine.

State on d537ebb

CI green (6/6 matrix + Cursor + Greptile, 0 failures). 39 tests: macOS 39 passed · Linux VM 38 passed, 1 skipped · Windows VM 33 passed, 6 skipped.

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

2 findings — 1 high-confidence, 1 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.

• 🔴 [MEDIUM] Privileged scan can be hung indefinitely by a FIFO planted at settings.json — scripts/coding_discovery_tools/coding_tool_base.py:1180

  • What: _read_contained calls os.open(str(path), os.O_RDONLY) before checking stat.S_ISREG, and without O_NONBLOCK, so a non-regular file is opened first and rejected only afterwards.
  • Why: On POSIX, opening a FIFO read-only blocks until a writer appears. Any unprivileged user can run mkfifo ~/.config/Code/User/settings.json (or in a profiles/<id>/ dir). During a root/all-user scan the extractor blocks forever on that user's home, stalling the rest of the discovery run — the whole scan, not just that user. The old code path (_parse_jsonc without user_home) used path.is_file(), which returns False for a FIFO; this new path removed that pre-open type gate.
  • Fix: Add os.O_NONBLOCK to the open flags on POSIX (getattr(os, "O_NONBLOCK", 0)), which makes a FIFO open return immediately, and keep the existing S_ISREG rejection right after fstat. O_NOFOLLOW is not needed given the realpath + dev/ino checks, but adding O_NONBLOCK is the minimal fix.
  • Flagged by: Claude (lead). Semgrep/Gitleaks: no findings.

• 🟡 [LOW] settings.json is read with no size cap — scripts/coding_discovery_tools/coding_tool_base.py:1198

  • What: handle.read() slurps the whole file, then _strip_jsonc_comments / _strip_trailing_commas / json.loads each build further copies of it.
  • Why: A user can plant a multi-GB settings.json (or one per profile) and drive the privileged scan into memory exhaustion. The sibling VS Code helpers in the same codebase already cap this kind of read — _VSCODE_EXTENSION_STATE_MAX_BYTES (1 MB) and _VSCODE_MCP_PROVIDER_CACHE_MAX_BYTES (5 MB) in mcp_extraction_helpers.py — so this new read is the odd one out.
  • Fix: Reuse the existing pattern: check st.st_size (already available from the fstat) against a new module constant, e.g. 1 MB, and return None with a debug log when it is exceeded.
  • Flagged by: Claude (lead). Semgrep/Gitleaks: no findings.

Previously acknowledged (not re-flagged)

• Symlink / junction following on settings.json and profile dirs (cursor MEDIUM ×4, greptile P1 security) — fixed in this diff. _read_contained now opens the file once, then validates the descriptor with fstat + realpath containment + owner-uid check + dev/ino identity comparison, with tests covering the escaping-symlink, escaping-profile-dir, and hard-link-owner cases.
• Containment check races the file read (TOCTOU) (greptile P1 security, cursor MEDIUM) — fixed in this diff. The dev/ino comparison between fstat(fd) and stat(realpath(path)) rejects a file swapped on either side of the open; TestContainmentRace exercises exactly this swap.
• Multi-user permission records collapsed to one (greptile P1 ×4) — author's documented design: the canonical VS Code row carries one permissions record, and max(records, key=_permissiveness) deliberately surfaces the riskiest user rather than the first. Reporting-coverage tradeoff, not a security defect.
• Stable Code and Insiders settings merged into one posture (greptile P1) — author's documented design: all channels and profiles are merged to the most-permissive posture, with settings_path attributed to the channel that carries the risk (test_riskiest_channel_wins_and_is_attributed).

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head d537ebb8 · 2026-09-04T21:22Z

[anonpran]

Checked this against the VS Code 1.136 registry and the current docs. Key set, per-OS paths, profile enumeration and the skills paths all hold up, and the object form is right (microsoft/vscode#270641 confirms approve is valid both ways). The containment work is ahead of anything else we have in this repo.

Six things I'd want in before merge:

1. chat.tools.terminal.enableAutoApprove is in SECURITY_RELEVANT_KEYS but _terminal_rules never checks it. With the switch off we still emit allow_rules, and the backend deriver then scores those users for command execution. The registry confirms it gates the whole feature (default true, policy ChatToolsTerminalEnableAutoApprove, min 1.104). This is the one that puts wrong data in front of a customer.
2. chat.permissions.default isn't read. That's what the permissions picker writes, so someone parked on Bypass Approvals reads as default today.
3. chat.tools.edits.autoApprove does exist (ChatConfiguration.AutoApproveEdits, scope APPLICATION), so the "no file-edit auto-approve setting" line in the description isn't right. {"**/*": true} means edits to .env without asking. It's also why the acceptEdits rank in _permissiveness is never assigned.
4. _read_contained opens before the S_ISREG check. A FIFO planted at settings.json hangs the scan (reproduced locally, killed at 10s). A hung scan is worse for us than a crash because of the orphan LIVE lock. O_NONBLOCK on the open plus an is_file() in the default-profile branch of enumerate_vscode_user_files.
5. The uid check drops root-written settings.json silently at debug level, which is the MDM-provisioned case. Keep the hard-link defence, but tag the record instead of dropping it.
6. unittest.main() sits mid-file, so python tests/test_copilot_vscode_permissions.py runs 18 of 39 and skips the containment, ownership and profile suites. pytest sees all 39.

Description also still promises the chat.mcp.allowedServers -> mcp_tool_allowlist mapping that af2550f removed.

On Greptile's multi-user finding: it's real, but I'd ticket it rather than block on it. One correction to the defence though. Claude Code, Cursor CLI, Copilot CLI and Augment all return List[Dict] and keep every user. Only Cursor IDE collapses, and it does it worse (first-wins). So it's a known weakness with one precedent, not the convention.

I checked gateway-data and unbound-fe: no coordinated change needed. _process_permissions is tool agnostic, the risk deriver is deterministic over allow/deny/mode/sandbox, the permissions page is data driven, and ToolIcon already matches "github copilot". It shows up on merge.

Worth separate tickets: managed policy lives outside settings.json (HKLM\SOFTWARE\Policies\GitHubCopilot, com.github.copilot prefs, /etc/github-copilot/managed-settings.json) and overrides user settings, the same way our Claude Code extractor already reads its managed plist; terminal rules do apply from workspace settings in a trusted workspace; remote and WSL machine settings; and CI has no Linux leg even though we ship a Linux extractor.

[AakashVelusamy]

Review disposition — as of a4c22ce

Six issues from @anonpran's review, plus the two bot findings still open on d537ebb. Every one was checked against the VS Code 1.136 configuration registry before being acted on, and each fix has a regression test proven to fail on d537ebb.

Fixed — wrong data in front of a customer

Finding	Disposition
chat.tools.terminal.enableAutoApprove never checked — the key sat in SECURITY_RELEVANT_KEYS but _terminal_rules ignored it, so a user with the master switch off still shipped allow_rules, and the backend deriver scored them for command execution.	Fixed. Registry confirms it: {restricted: true, type: "boolean", default: true, policy: ChatToolsTerminalEnableAutoApprove, minimumVersion: "1.104"}, and the analyzer gates on it twice — S = getValue(…) === true; S && a ? … : a = false. With it off, the allow patterns are inert, so they are dropped. Denials are kept — nothing is auto-approved, so they still hold. TestTerminalMasterSwitch.
chat.permissions.default not read — someone parked on Bypass Approvals read as default.	Fixed. Registry: {type: "string", enum: ["default","autoApprove","autopilot"], default: "default"}, read by getDefaultPermissionLevel(). Both elevated levels now map to bypassPermissions. TestPermissionLevelAndEdits.
chat.tools.edits.autoApprove claimed not to exist — which is why the acceptEdits rank in _permissiveness was unreachable.	Fixed. It exists (type: "object", additionalProperties: boolean, scope APPLICATION). Now read and mapped: any glob set true → acceptEdits, with global auto-approve still outranking it.

One correction to the finding, though. {"**/*": true} is not a .env bypass — it is the shipped default. VS Code's default value is {"**/*": true} plus a deny-list that already covers .env, package.json, lockfiles, .git/**, project files and more. So the risky edit posture is a user re-enabling one of the denied patterns, not the wildcard itself. The exact globs go to the backend verbatim in raw_settings, so that distinction is visible to an analyst rather than flattened by us.

Fixed — security

Finding	Disposition
FIFO at settings.json hangs the scan (@anonpran #4, and consensus 🔴 MEDIUM) — os.open() ran before the S_ISREG check, and a read-only FIFO open blocks until a writer appears. Not one user's scan: the whole run, holding the LIVE lock.	Fixed. O_NONBLOCK on the open (a no-op for the regular files we keep), and enumerate_vscode_user_files takes is_file() instead of exists() in the default-profile branch, matching the profiles branch beside it. This also removes the same hang from the mcp.json path. Reproduced: on d537ebb the test thread was still alive at the 15 s join; now it returns None immediately. TestNonRegularFiles.
Hard-link defence is a no-op on Windows (Cursor MEDIUM, open on d537ebb) — it rested on st_uid, which is 0 for every file there.	Fixed. st_nlink > 1 now carries it on every platform, which is exactly what the sibling privileged readers do (utils.py:1379, plugin_extraction_helpers.py:732, _is_foreign_hardlink). Reproduced with a real hard link: on d537ebb the linked out-of-home content came back in the record ("chat.mcp.access": "OUT-OF-HOME"); now it is refused. TestHardLinkContainment.

Fixed — test hygiene

Finding	Disposition
unittest.main() mid-file — python tests/test_copilot_vscode_permissions.py ran 18 of 39 and silently skipped the containment, ownership and profile suites.	Fixed. Moved to the end. Direct run now executes all 50; pytest agrees.

Accepted limitation

Finding

Disposition

Root-owned settings.json is dropped (@anonpran #5) — the uid check refuses it.

Kept, but no longer silent (debug → info, so it appears in the scan log). Two reasons for keeping the drop: it is the fleet pattern — utils.py:1382 applies the same uid-equality rule in the same situation — and real managed policy does not live at this path anyway. It lives in HKLM\SOFTWARE\Policies, com.github.copilot prefs and /etc/github-copilot/managed-settings.json, which is a separate extraction surface and now its own ticket. A root-owned file at the user path is a provisioning script that forgot to chown.

Greptile — still 3/5, both points stand

Finding	Disposition
Multi-user records collapse to one	Accepted, ticketed. One correction to my earlier defence in this thread, which overstated it: Copilot CLI, Augment and Claude Code extractors all return List[Dict] and keep every user; only Cursor IDE collapses. So it is a known weakness with one precedent, not the convention. What is fleet-wide is the layer below — transform_settings_to_backend_format "selects the highest precedence settings file… No merging is performed", so even a List[Dict] becomes one record in the payload. Fixing this properly means per-user records end to end, which is a backend change, not a rename here. max(records, key=_permissiveness) means the collapse surfaces the riskiest user rather than the first.
Stable and Insiders merged into one posture	Accepted. Merged to the most-permissive posture with settings_path attributed to the channel carrying the risk (test_riskiest_channel_wins_and_is_attributed). No machine observed in the fleet has both channels installed.

Description

Corrected: the chat.mcp.allowedServers → mcp_tool_allowlist row promised a mapping af2550f removed (those keys ride in raw_settings as context), the file-edit line is gone, and the mapping table and test count now match the code.

---

🤖 Generated with Claude Code

---

Addendum — consensus round on a4c22ce

anonpran approved. CI is green on all six matrix legs plus both bots. The consensus reviewer returned 3 findings, 0 high-confidence, all to triage (the MEDIUM is now fixed), and confirmed the four security fixes (symlink escape, hard link, TOCTOU, FIFO) as closed. Dispositions:

Finding	Disposition
[MEDIUM] No size cap on the settings.json read — a local user can plant a multi-GB file in their own home and OOM a privileged scan.	Fixed in b41d2cc. Capped at 5 MB, matching the largest sibling VS Code read cap (_VSCODE_MCP_PROVIDER_CACHE_MAX_BYTES), checked against the st_size already in hand from the fstat. That is ~1000x a real settings.json, so it cannot truncate anything a customer wrote — a test proves a 2 MB file still parses. Measured on a 3 GB sparse file: uncapped the read peaked at ~12 GB RSS over 84 s (the strip and parse passes each hold another copy); capped it returns immediately.
[LOW] _parse_jsonc(user_home=None) is an insecure default	Refuted. The unguarded branch is more guarded than the fleet, not less: the sibling _parse_jsonc in macos/augment/ and macos/copilot_cli/ take a bare path and read it with no containment at all, unconditionally. The sole production caller here always passes user_home. Making it required would force the parse-resilience fixtures to live inside the real user home, which is a worse trade than the footgun it removes.
[LOW] Terminal rule patterns are copied verbatim into the record	Accepted as intended. This is the same call already made on MCP server URLs in this PR: extractors report configuration as written, and every sibling ships raw_settings verbatim. Redacting here would make the Copilot record the only one in the fleet that silently alters what the customer configured, and a rule we rewrite is a rule an analyst cannot match against what is on disk.

Greptile holds at 3/5 on a4c22ce for the same two reporting-fidelity points — both accepted above and ticketed. No new security findings this round.

---

Addendum — Greptile round on 8badd39

Finding	Disposition
P1 security — "Windows containment remains pathname-based" (a parent junction swapped after the open)	Fixed in 8badd39, and the scenario did not reproduce. On the real Windows runner as SYSTEM, with the guard disabled, a genuine mklink /J parent leaked nothing — ntpath.realpath resolves junctions on 3.8+ and containment already refused it. The residual is realpath's unresolved-path fallback, which I could not reach deterministically. The fix makes the refusal independent of realpath by checking the parent chain directly on Windows; POSIX is untouched so stow/chezmoi keeps working. Handle-anchored containment via GetFinalPathNameByHandleW was not added: it needs ctypes, no sibling reader does it (utils.py:1370 is pathname-based in the same situation), and it belongs to a repo-wide decision. Full thread
Score dropped 3/5 → 1/5	No new evidence behind it. The two reporting-fidelity points are unchanged and ticketed; the third is the junction item above, now guarded. CI is green on 8badd39 across all six matrix legs plus both bots.

VM battery for this head is here — including the Windows hard-link leak reproduced and fixed, which is the one that was genuinely exploitable.

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

3 findings — 0 high-confidence, 3 to triage. Reviewers: claude-opus-5 (lead), semgrep (0 findings), gitleaks (0 secrets).

• [MEDIUM] Unbounded read of an attacker-influenceable settings.json — scripts/coding_discovery_tools/coding_tool_base.py:_read_contained / _parse_jsonc

  • What: The new read path has no size cap — handle.read() pulls the whole file, then _strip_jsonc_comments / _strip_trailing_commas / json.loads each hold another copy.
  • Why: Under a root/admin all-user scan, any local user can plant a multi-GB regular file at ~/.config/Code/User/settings.json (or in any profiles/<id>/) inside their own home. It passes every containment, ownership and nlink check because it genuinely is their own file, so the privileged scanner reads it and can OOM. That kills the scan for every other user on the host. The same file in mcp_extraction_helpers.py already establishes the opposite pattern: _VSCODE_MCP_PROVIDER_CACHE_MAX_BYTES = 5 * 1024 * 1024 and _VSCODE_EXTENSION_STATE_MAX_BYTES = 1024 * 1024.
  • Fix: Cap the read in _read_contained — check st.st_size against a constant (a few hundred KB is generous for settings.json) right after the fstat, and return None with a log line when it is over. Reuse the constant style already in mcp_extraction_helpers.py.
  • Flagged by: claude-opus-5

• [LOW] _parse_jsonc defaults to skipping every containment check — scripts/coding_discovery_tools/coding_tool_base.py:_parse_jsonc

  • What: user_home defaults to None, and that branch reads with plain path.is_file() / path.read_text() — no descriptor check, no realpath containment, no ownership check, no hard-link check.
  • Why: Not exploitable today; the only production caller (_extract_for_user) always passes user_home, and the None branch is exercised only by tests. It is an insecure default on a @classmethod that sits in a shared base class, so the next caller silently gets the unguarded read that this PR spent most of its review cycles closing.
  • Fix: Make user_home a required positional argument and update the tests to pass a temp home, or invert the default so the unguarded branch has to be requested explicitly (e.g. allow_uncontained=False).
  • Flagged by: claude-opus-5

• [LOW] User-authored command strings are copied verbatim into the transmitted record — scripts/coding_discovery_tools/coding_tool_base.py:_terminal_rules / _build_record

  • What: Every key of chat.tools.terminal.autoApprove becomes a Bash(<pattern> *) rule and is also kept whole in raw_settings, which then ships to the backend on the canonical Copilot row.
  • Why: With matchCommandLine, those keys are full command lines a developer wrote themselves — an auto-approved curl -H "Authorization: Bearer …" or a command with an inline token ends up in the discovery report and anywhere that report is stored or rendered. The fixed 17-key allowlist correctly prevents arbitrary settings from leaking; these two fields are the one place free-form user text still flows through.
  • Fix: Either truncate/redact rule patterns above a short length, or run the pattern through a simple secret-shaped regex (Bearer\s+\S+, sk-[A-Za-z0-9]{20,}, --password[= ]\S+) and replace the match before it reaches allow_rules / deny_rules / raw_settings.
  • Flagged by: claude-opus-5

Previously acknowledged (not re-flagged)

• Symlink / junction escape on the settings leaf and the profile dir (cursor, greptile) — fixed in the current tree: _read_contained opens the descriptor, requires S_ISREG, and refuses anything whose realpath falls outside the scanned home.
• Hard link to another user's settings file (cursor) — fixed: st_nlink > 1 refusal, backed by TestHardLinkContainment.
• Containment check races the file read (TOCTOU) (greptile, cursor) — fixed: the (st_dev, st_ino) comparison between the held descriptor and the re-stat rejects a file swapped inside the window. Covered by TestContainmentRace.
• FIFO planted at settings.json hangs the scan — fixed: O_NONBLOCK on open plus the is_file() change in enumerate_vscode_user_files. Covered by TestNonRegularFiles.
• Multi-user records collapsed to one; stable and Insiders merged without channel context (greptile P1, repeated) — reporting-completeness issues, not security ones, and the author states the intended behaviour in code (max(records, key=self._permissiveness), "the canonical row carries one permissions record"). Out of scope for this review; still worth a maintainer decision on the product question.

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head a4c22ce6 · 2026-09-05T08:12Z

[cursor]

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

[cursor]

Stale comment

Agentic security review of this Copilot VS Code permissions extractor found one remaining medium issue on the Windows privileged read path. Other modules reported no qualifying findings.

  

_{Sent by Cursor Security Agent: Security Reviewer}

[AakashVelusamy]

VM battery — Stage 1 + Stage 2 on 8badd39

PR is open against staging, so this is the pre-merge gate: local macOS, then the real Azure runners. Nothing ran against a live environment.

What ran where

Leg	Environment	Result
Unit suite	macOS (local)	53 passed, 2 skipped (skips are the Windows-junction cases)
Unit suite	sentinel-agent-runner, as root	52 passed, 3 skipped
Unit suite	poc-win-runner, as NT AUTHORITY\SYSTEM	47 passed, 8 skipped — the two junction tests ran and passed here
CI	GitHub matrix	6/6 + Cursor + Greptile, all green

Multi-user privileged scan — real accounts, real separation

Three synthetic users with distinct UIDs on Linux, three profile trees on Windows. Both runners agreed:

Check	Linux (root)	Windows (SYSTEM)
FIFO planted at settings.json	no hang, 0.0 s	n/a (POSIX)
Riskiest user wins, attributed to their own file	✓ …/cdtu2/…/profiles/yolo/settings.json	✓ …\winu2\…\profiles\yolo\settings.json
Object-form {"approve": true} allow read	✓ Bash(npm run *)	✓
Hard link to a file the user cannot read	not leaked (root-owned)	not leaked (SYSTEM-owned)
3 GB planted settings.json	not read	not read
Master switch off → allows dropped, denies kept	✓ curl gone, rm + rm -rf kept	✓ same
mcp.json enumeration after the is_file() change	✓ still found	✓ still found
Per-user %APPDATA% resolved from each home	n/a	✓ never systemprofile

Picker level and edit auto-approval verified on Linux against a real tree: chat.permissions.default: autopilot → bypassPermissions, chat.tools.edits.autoApprove alone → acceptEdits, and the pre-rename chat.tools.autoApprove → bypassPermissions (backward compatibility for installs written before the key was renamed).

End to end through the real CLI

python -m scripts.coding_discovery_tools.ai_tools_discovery --api-key … --domain … on the Linux runner as root, against a local capture stub — a real Copilot extension footprint planted first, no traffic left the host.

exit=0          captured POSTs: 37
path          : /api/v1/ai-tools/report/
tool          : GitHub Copilot Chat (VS Code)
mode          : bypassPermissions
settings_path : /home/ubuntu/.config/Code/User/profiles/e2e_yolo/settings.json
allow         : ['Bash(git status *)']
deny          : ['Bash(curl | sh *)', 'Bash(rm *)']
source/scope  : user / user
raw keys      : chat.permissions.default, chat.tools.edits.autoApprove,
                chat.tools.global.autoApprove, chat.tools.terminal.autoApprove

settings_source: user satisfies the ['user','project','managed'] validation in _process_permissions, and both new keys ride through to the backend in raw_settings.

Cross-repo consumer, executed not assumed

Ran ai-gateway-data's real permissions_risk_deriver against the records this PR now emits:

Record	Derived autonomy
bypassPermissions + scoped allows	full_auto (risk score 7)
acceptEdits — newly reachable	auto_edit
default, no rules	ask_first
default + Bash(git status *)	ask_first

acceptEdits lands as auto_edit exactly as the deriver intends, and permission_mode is a CharField(max_length=50) that already documents the value — no backend or migration change needed.

Guards proven to fail without the fix

Both on the real Windows runner, by disabling only the guard under test:

Guard	Without it	With it
st_nlink > 1 (hard link)	leaked a SYSTEM-only file as the user's posture — st_uid = 0, so the uid check is a no-op there	refused
Parent-chain junction check	did not leak — realpath already resolved the junction	refused

The second row is recorded honestly: the junction scenario did not reproduce, so 8badd39 is defence-in-depth against realpath's unresolved-path fallback rather than a fix for a demonstrated leak. Detail in the thread reply.

Residual risk and hygiene

• Every fixture, synthetic user and planted file was torn down on both runners; residue checks came back clean. The Linux runner keeps a ~/.unbound/discovery-cache.json written by the real run — the tool's own state on a dedicated test host.
• Not tested: Greptile's remaining two points (multi-user collapse, channel merge) are unchanged by design and ticketed; live staging is out of scope until this merges.

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

1 finding — 0 high-confidence, 1 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.

• [LOW] Symlink and hard-link guards apply only through extract_settings(), not to direct _parse_jsonc(path) calls — scripts/coding_discovery_tools/coding_tool_base.py:1225
  • What: _parse_jsonc only runs the containment and ownership checks when a caller passes user_home; with user_home=None it falls back to plain path.is_file() / path.read_text(), which follow symlinks and junctions.
  • Why: The extractor path always passes user_home, so this is not exploitable today. It is a footgun: any future caller (or subclass) that reuses this helper without the second argument silently loses every protection this PR added, including the size cap and the FIFO guard.
  • Fix: Make user_home required, or route the unguarded branch through _read_contained with the file's own resolved parent, so the safe path is the only path.
  • Flagged by: Claude (lead)

Previously acknowledged (not re-flagged)

• Workspace .vscode/settings.json symlink/junction follow (cursor, greptile, multiple commits) — the PR no longer reads workspace settings at all; the author documented user + profile scope as deliberate, and the diff contains no workspace walk.
• Containment check races the file read (TOCTOU) (greptile P1, cursor MEDIUM on coding_tool_base.py:1240) — addressed in the reviewed diff: _read_contained now opens one descriptor with O_RDONLY|O_NONBLOCK, checks fstat for regular-file, size, st_nlink, and st_uid, and re-compares (st_dev, st_ino) against the realpath target before reading, so a swap after the open is refused. Test TestContainmentRace covers it.
• Windows junction in the parent chain (cursor, greptile) — addressed by _parents_are_direct, which walks parents to the home and refuses any reparse point on Windows.
• Unresolved settings_path used as ownership identity (cursor CONFIRMED, coding_tool_base.py:1312) — the underlying read is now containment- and uid-checked, so an out-of-home target can no longer be attributed to the planter.
• Multi-user records collapsed to one / most-permissive max() (greptile P1, raised four times) — the author's stated design: the canonical VS Code row carries exactly one permission record, and max() by permissiveness is a deliberate choice so the riskiest posture is never hidden. Correctness/coverage concern, not a security vulnerability introduced by this diff.
• Stable and Insiders channels merged into one posture (greptile P1) — same stated design: all channels and profiles merge to the most-permissive posture, documented in the PR body and in the class docstring.

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 8badd390 · 2026-09-05T09:14Z

[AakashVelusamy]

Both remaining Greptile P1s fixed in ad52f7d

These were raised every round and defended every round. On re-examination they were right, so they are fixed rather than argued again — and my earlier defence of the first one was wrong on the facts, which is corrected below.

1. Multi-user records collapsed to one

Valid. An elevated all-users scan reduced every user to one record, and filter_tool_projects_by_user then deleted permissions from every report the record did not belong to. A host with one locked-down user and one running auto-approve reported the second posture once, and nothing at all for the first.

extract_settings_by_user() now returns one record per user, riskiest first. The orchestrator attaches them as _permissions_by_user — an internal key, dropped by the existing _-prefix strip at ai_tools_discovery.py:2976 — and the per-user filter selects the record whose settings_path lives in that user's home.

Correcting my earlier reply on this thread: I argued the collapse was every settings extractor's pattern. It isn't. Copilot CLI, Augment and Claude Code all return List[Dict] and keep every user; only Cursor IDE collapses, and it does so first-wins. One precedent, not a convention.

Verified on the Linux runner as root with three real UIDs:

records kept: 2 of 3 homes      (the third has only a planted FIFO -> correctly none)
  cdtu1  -> default            /tmp/nvm-homes/cdtu1/.config/Code/User/settings.json
  cdtu2  -> bypassPermissions  /tmp/nvm-homes/cdtu2/.config/Code/User/profiles/yolo/settings.json
--- per-user reports ---
  cdtu1  -> default            | internal key gone: True
  cdtu2  -> bypassPermissions  | internal key gone: True
  cdtu3  -> None               | internal key gone: True
  nobody -> None               | internal key gone: True

Before this change cdtu1 received no permissions block at all.

2. Stable and Insiders merged into one posture

Valid. Profiles merged across channels, so a record could carry stable Code's deny rules under an Insiders settings_path — a posture neither installation defines. Profiles now merge within a channel only, and the riskiest channel is reported whole.

On the runner, for a user with settings in both channels:

chosen channel            : stable Code
raw keys                  : chat.tools.global.autoApprove, chat.tools.terminal.autoApprove
insiders-only key present : False

3. Windows containment pathname-based

Guarded in 8badd39; the scenario itself did not reproduce on real Windows. Detail in that thread.

Proven to fail without the fix

Reverting only scripts/ and re-running the three new guards:

FAILED TestChannelsAreNotMerged::test_riskiest_channel_is_reported_whole
  AssertionError: ['Bash(rm *)'] is not None : stable's deny must not appear on the Insiders record
FAILED TestPerUserAttribution::test_every_user_keeps_a_record
FAILED TestPerUserAttribution::test_each_user_report_carries_its_own_posture

Re-run on ad52f7d

Leg	Result
macOS local	56 passed, 2 skipped
sentinel-agent-runner, as root	55 passed, 3 skipped
poc-win-runner, as NT AUTHORITY\SYSTEM	50 passed, 8 skipped
Related suites (962 selected)	no new failures — the only reds are test_copilot_cli_discovery and test_discovery_flow, both failing identically on staging on this machine

Fixtures and synthetic users torn down on both runners.

[vigneshsubbiah16]

🛡️ Automated Security Review (consensus)

4 findings — 1 high-confidence, 3 to triage. Reviewers: Claude (lead), Semgrep, Gitleaks.

• 🟡 [MEDIUM] Privileged os.open() happens before any validation — scripts/coding_discovery_tools/coding_tool_base.py:1172

  • What: _read_contained opens the attacker-reachable path first, then checks file type, containment, ownership and inode. Every guard runs after the descriptor already exists.
  • Why: The enumeration is_file() filter rejects a device node, but the swap window the class already defends against (see TestContainmentRace) also lets a local user replace the leaf with a symlink to a special file between enumeration and read. A root scan then opens it. O_NONBLOCK prevents the hang, but opening some device nodes has side effects at open time (the classic case is /dev/watchdog, where a root open arms a reboot timer). No post-open check can undo that.
  • Fix: Open the leaf with os.O_NOFOLLOW and do a realpath containment check before the open, keeping the existing fd re-checks as the anti-race layer. O_NOFOLLOW still permits the supported in-home stow/chezmoi case if you resolve and re-open the target explicitly.
  • Flagged by: Claude (lead) only — distinct from the earlier bot reports, which concerned checks after the open.

• 🔴 [LOW] The 5 MB read cap is advisory and can be exceeded — scripts/coding_discovery_tools/coding_tool_base.py:1198

  • What: The size cap is enforced against st.st_size from os.fstat, but the read is an unbounded handle.read().
  • Why: A local user can plant a small settings.json, let the fstat pass, then append to the same inode before read() returns. The scan pulls the whole file into memory, so the cap does not actually bound memory during a privileged all-user scan.
  • Fix: Read a bounded amount and reject anything longer: raw = handle.read(_VSCODE_SETTINGS_MAX_BYTES + 1), then return None if len(raw) > _VSCODE_SETTINGS_MAX_BYTES.
  • Flagged by: Claude (lead) — provable directly from the diff.

• 🟡 [LOW] _parse_jsonc has an unguarded mode that skips every protection — scripts/coding_discovery_tools/coding_tool_base.py:1252

  • What: When user_home is None, the method falls back to path.is_file() + path.read_text() — no containment, no ownership check, no size cap.
  • Why: Not reachable from today's production path (_extract_for_user always passes user_home), so this is hardening, not a live bug. But it is a protected-by-default API where the unsafe mode is the shorter call, so a future caller loses all four guards silently.
  • Fix: Make user_home required, or raise/return None when it is missing and update the tests to pass a home.
  • Flagged by: Claude (lead) only.

• 🟡 [LOW] Verbatim terminal-command patterns are shipped to the backend — scripts/coding_discovery_tools/coding_tool_base.py:1276

  • What: raw_settings copies the allowlisted keys verbatim, and allow_rules / deny_rules embed the chat.tools.terminal.autoApprove keys as-is.
  • Why: Those keys are user-authored command patterns, and VS Code's matchCommandLine mode encourages full command lines. A pattern like curl -H "Authorization: Bearer …" would be copied into the discovery report and sent upstream. The key allowlist correctly keeps the rest of settings.json out, so the exposure is limited to this one value shape.
  • Fix: Redact patterns matching common secret shapes (bearer tokens, sk-/ghp_ prefixes, --password/--token flags) before they enter raw_settings and the rule strings, reusing whatever redaction the Cursor extractor path already applies if one exists. ⚠️ Whether users actually put secrets in these patterns is unverified — this is a plausible class, not an observed instance.
  • Flagged by: Claude (lead) only.

Previously acknowledged (not re-flagged)

• Workspace .vscode/settings.json symlink/junction walk (cursor, coding_tool_base.py:1163 / :1181) — the workspace walk was removed; the author documents user + profile scope as the authoritative surface because these keys are application-scoped and restricted.
• Profiles merged to the most-permissive posture (implied over-report) — stated design choice: a locked-down default with a YOLO named profile is a real risk the report must surface.
• Multi-user records collapsed to one (greptile P1, repeated) — resolved in this diff by _permissions_by_user plus the per-user filter in ai_tools_discovery.py.
• Stable/Insiders channel policies merged (greptile P1) — resolved: _extract_for_user now merges only within a channel and picks one channel whole.
• Symlink/junction follow and unresolved settings_path attribution (cursor, multiple) — resolved by _read_contained's realpath containment, st_nlink, uid, and post-open dev/ino re-check. The remaining gap is the pre-validation open above, not the checks these comments asked for.

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 5f291676 · 2026-09-05T10:29Z