microsoft / microsoft/vscode

Agent Host: dispatchAction does validate not whether clients can dispatch that action

Open
#332,434 1 comment 1 reaction 2 assignees Claimed by @roblourens View on GitHub
agent-host bug
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

Ref #331796 (changes)

## Summary

The AHP protocol server's `dispatchAction` handler decides whether to dispatch an incoming client action by checking which *family* it belongs to (`isSessionAction`, `isChatAction`, `isTerminalAction`, ...), not whether that specific action *type* is client-dispatchable. Server-owned action types that live inside an otherwise-permitted family pass the guard and get dispatched.

## Where

[The guard](https://github.com/microsoft/vscode/blob/584b2dacffdba8b29df943d8d6f99154c9f967e4/src/vs/platform/agentHost/node/protocolServerHandler.ts#L515) in `src/vs/platform/agentHost/node/protocolServerHandler.ts`:

```ts
} else if (isSessionAction(action) || isChatAction(action) || isTerminalAction(action) || isChangesetAction(action) || isAnnotationsAction(action) || action.type === ActionType.RootConfigChanged) {
this._agentService.dispatchAction(channel, action, client.clientId, msg.params.clientSeq, client.telemetryContext);
}
```

The family guards return `true` for every action in the family. The protocol already distinguishes client-vs server-owned actions per type via the [`IS_CLIENT_DISPATCHABLE` map](https://github.com/microsoft/vscode/blob/584b2dacffdba8b29df943d8d6f99154c9f967e4/src/vs/platform/agentHost/common/state/protocol/action-origin.generated.ts#L323) and the [`isClientDispatchable()` helper](https://github.com/microsoft/vscode/blob/584b2dacffdba8b29df943d8d6f99154c9f967e4/src/vs/platform/agentHost/common/state/protocol/common/reducer-helpers.ts#L29-L32), whose doc comment says "Servers SHOULD call this to validate incoming `dispatchAction` requests." The handler consults neither.

## Impact

A client can dispatch server-owned actions the protocol intends only the server to emit, for example `automation/set` and the server-owned `automationRun/*` lifecycle transitions, plus server-owned types in the session family (`session/ready`, `session/creationFailed`, etc). There's no downstream re-check: `agentService.dispatchAction` routes and dispatches without re-validating client-dispatchability.

## Additional context

Noticed while reviewing #331796 (automations AHP migration). That PR switched this guard to `isClientDispatchable`, the correct per-type check, but doing so pulled synced protocol code into the vscode-specific sync script. Looking at the draft predecessor #330463:

- [`cfb12483`](https://github.com/microsoft/vscode/pull/330463/commits/cfb12483dc91821221f7caad4006c0b7ac3456d9) followed the existing family-guard convention.
- [`23915495`](microsoft/vscode/pull/330463/commits/23915495e319560fc2a50ce20c64ee55982769a7) switched to `isClientDispatchable` as a "review findings" fix.

It looks like we should use `isClientDispatchable(action)` per action type and explicitly reject server-owned actions, instead of checking family membership.

Note: `isClientDispatchable` likely needs its type expanded if brought into `dispatchAction`

🤖 AI Assisted Bug Report

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.