fix(chat): gate response-stream commands behind an allowlist

src/chat-sidebar.tsx

@@ -17,6 +17,11 @@ import * as monaco from 'monaco-editor/esm/vs/editor/editor.api.js';
 
 import { NBIAPI, GitHubCopilotLoginStatus } from './api';
 import { injectTaskTargetNotebook } from './task-target-notebook';
+import {
+  executeResponseStreamCommand,
+  RESPONSE_BUTTON_COMMAND_ALLOWLIST,
+  RUN_UI_COMMAND_ALLOWLIST
+} from './command-ids';
 import {
   formatElapsedSeconds,
   isHeartbeatStale
@@ -640,10 +645,26 @@ function ChatResponse(props: any) {
     setRenderCount(prev => prev + 1);
   };
 
+  // Used by the confirmation and question forms, which pass a command id
+  // written in this file. Deliberately ungated: gating here would refuse
+  // `chat-user-input` and leave the backend waiting forever on a form the
+  // user already answered.
   const runCommand = (commandId: string, args: any) => {
     props.getApp().commands.execute(commandId, args);
   };
 
+  // The one place a command id arrives from the response stream, so the one
+  // place an allowlist belongs (#441). The list is deliberately narrow: the
+  // only ButtonData the backend constructs offers the settings dialog.
+  const runStreamedButtonCommand = (commandId: string, args: any) => {
+    void executeResponseStreamCommand(
+      (id, commandArgs) => props.getApp().commands.execute(id, commandArgs),
+      commandId,
+      args,
+      RESPONSE_BUTTON_COMMAND_ALLOWLIST
+    );
+  };
+
   // group messages by type
   const groupedContents: IChatMessageContent[] = [];
   let lastItemType: ResponseStreamDataType | undefined;
@@ -947,7 +968,10 @@ function ChatResponse(props: any) {
                   <button
                     className="jp-Dialog-button jp-mod-accept jp-mod-styled"
                     onClick={() =>
-                      runCommand(item.content.commandId, item.content.args)
+                      runStreamedButtonCommand(
+                        item.content.commandId,
+                        item.content.args
+                      )
                     }
                   >
                     <div className="jp-Dialog-buttonLabel">
@@ -3202,11 +3226,21 @@ function SidebarComponent(props: any) {
               response.data.args,
               taskTargetNotebookPathRef.current
             );
-            let result = 'void';
+            // `unknown` rather than `string`: a JupyterLab command can
+            // resolve to an object or undefined, and this value only goes
+            // on to `|| 'void'` and JSON. Upstream reached the same runtime
+            // value through `execute`'s `any`.
+            let result: unknown = 'void';
+            // This branch executes with no user interaction, so an id the
+            // backend never sends is refused rather than run (#441). The
+            // refusal travels back on the callback so a caller waiting on
+            // the result gets an answer instead of hanging.
             try {
-              result = await app.commands.execute(
+              result = await executeResponseStreamCommand(
+                (id, commandArgs) => app.commands.execute(id, commandArgs),
                 response.data.commandId,
-                patchedArgs
+                patchedArgs,
+                RUN_UI_COMMAND_ALLOWLIST
               );
             } catch (error) {
               result = `Error executing command: ${error}`;
@@ -3673,11 +3707,17 @@ function SidebarComponent(props: any) {
               response.data.args,
               taskTargetNotebookPathRef.current
             );
-            let result = 'void';
+            let result: unknown = 'void';
+            // Same gate as the other RunUICommand branch above; this one
+            // reaches the app through props (#441).
             try {
-              result = await props
-                .getApp()
-                .commands.execute(response.data.commandId, patchedArgs);
+              result = await executeResponseStreamCommand(
+                (id, commandArgs) =>
+                  props.getApp().commands.execute(id, commandArgs),
+                response.data.commandId,
+                patchedArgs,
+                RUN_UI_COMMAND_ALLOWLIST
+              );
             } catch (error) {
               result = `Error executing command: ${error}`;
             }

src/command-ids.ts

@@ -66,3 +66,106 @@ export namespace CommandIDs {
   export const showTour = 'notebook-intelligence:show-tour';
   export const focusChatInput = 'notebook-intelligence:focus-chat-input';
 }
+
+// Command ids the response stream is allowed to execute.
+//
+// The chat sidebar hands a streamed `commandId` straight to
+// `app.commands.execute`, so without a check anything registered in the
+// application is reachable from response content (#441). Every server-side
+// caller passes a hardcoded literal today, so this is structural hardening
+// rather than a fix for a live exposure: the day a tool derives an id from
+// model output, or a third-party extension streams its own `ButtonData`
+// (`commandId` is a free-form string on a public dataclass), the check is
+// what stands between that and an arbitrary command.
+//
+// Scope limit worth knowing: this gates the id, not the arguments.
+// `runCommandInTerminal` is a legitimate entry whose handler writes its
+// `command` argument into a shell unattended, so an allowlist does not make
+// the response stream safe on its own.
+//
+// Two lists because the legitimate sets differ by an order of magnitude.
+// Keep them beside `CommandIDs` so a new server-driven command is obvious.
+
+/**
+ * Ids the backend drives through `RunUICommand`, which executes with no user
+ * interaction. Includes two JupyterLab built-ins the tools legitimately use,
+ * so this is deliberately not "every value in `CommandIDs`" (17 of those are
+ * frontend-only and never arrive over the wire).
+ */
+export const RUN_UI_COMMAND_ALLOWLIST: ReadonlySet<string> = new Set([
+  CommandIDs.createNewFile,
+  CommandIDs.createNewNotebook,
+  CommandIDs.listAvailableNotebookKernels,
+  CommandIDs.renameNotebook,
+  CommandIDs.addCodeCellToNotebook,
+  CommandIDs.addMarkdownCellToNotebook,
+  CommandIDs.addCodeCellToActiveNotebook,
+  CommandIDs.addMarkdownCellToActiveNotebook,
+  CommandIDs.deleteCellAtIndex,
+  CommandIDs.insertCellAtIndex,
+  CommandIDs.getCellTypeAndSource,
+  CommandIDs.setCellTypeAndSource,
+  CommandIDs.getNumberOfCells,
+  CommandIDs.getCellOutput,
+  CommandIDs.runCellAtIndex,
+  CommandIDs.getCurrentFileContent,
+  CommandIDs.setCurrentFileContent,
+  CommandIDs.openConfigurationDialog,
+  CommandIDs.runCommandInTerminal,
+  // JupyterLab's own commands, driven by the notebook tools.
+  'docmanager:open',
+  'docmanager:save'
+]);
+
+/**
+ * Ids a streamed button may execute on click. Narrower than the tool path:
+ * the only `ButtonData` constructed in the tree offers the settings dialog.
+ */
+export const RESPONSE_BUTTON_COMMAND_ALLOWLIST: ReadonlySet<string> = new Set([
+  CommandIDs.openConfigurationDialog
+]);
+
+/**
+ * Whether `commandId` may be executed from the response stream.
+ *
+ * Unknown ids are refused rather than passed through, which is the opposite
+ * default from the rest of this surface and intentional: a command id that
+ * nothing in the backend sends is either a mistake or someone else's idea.
+ */
+export function isResponseStreamCommandAllowed(
+  commandId: string,
+  allowlist: ReadonlySet<string>
+): boolean {
+  return typeof commandId === 'string' && allowlist.has(commandId);
+}
+
+/**
+ * Run a command that came off the response stream, or refuse it.
+ *
+ * The sinks call through here rather than checking the allowlist themselves.
+ * Testing the predicate and the lists in isolation left the enforcement
+ * unpinned: deleting a sink's check outright kept every test passing,
+ * because nothing asserted the sinks consult the policy at all. Routing the
+ * three of them through one function makes that testable without standing up
+ * a JupyterFrontEnd, which is why this lives here (the module imports no
+ * JupyterLab packages and is already Jest-reachable) rather than in
+ * chat-sidebar.
+ *
+ * Returns the command's own result when it runs. A refusal comes back as an
+ * error string instead of throwing, because both callers report it onward:
+ * the RunUICommand branches send it to the waiting backend caller, so a
+ * refusal has to be an answer rather than a hang.
+ */
+export async function executeResponseStreamCommand(
+  execute: (commandId: string, args: unknown) => Promise<unknown>,
+  commandId: string,
+  args: unknown,
+  allowlist: ReadonlySet<string>
+): Promise<unknown> {
+  if (!isResponseStreamCommandAllowed(commandId, allowlist)) {
+    const refusal = `Error executing command: '${commandId}' is not an allowed UI command`;
+    console.warn(`[NBI] ${refusal}`);
+    return refusal;
+  }
+  return execute(commandId, args);
+}