Skip to content

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

Open
pjdoland wants to merge 1 commit into
plmbr:mainfrom
pjdoland:fix/441-command-allowlist
Open

pjdoland wants to merge 1 commit into
plmbr:mainfrom
pjdoland:fix/441-command-allowlist

Conversation

@pjdoland

Copy link
Copy Markdown
Collaborator

Closes #441

What this adds

The backend can ask the frontend to run a JupyterLab command by id, through run_ui_command and through the ButtonData a response stream can render. Nothing validated that id, so any command registered in the application was reachable from the response stream.

This adds two allowlists in src/command-ids.ts:

  • RUN_UI_COMMAND_ALLOWLIST: the ids that run_ui_command actually sends, including the bare JupyterLab ids docmanager:open and docmanager:save.
  • RESPONSE_BUTTON_COMMAND_ALLOWLIST: only openConfigurationDialog, since that is the one ButtonData the backend constructs.

executeResponseStreamCommand is a single chokepoint, so the sinks cannot drift into checking the policy inconsistently. A refused id returns an error string on the callback rather than leaving the caller waiting.

Scope limit, stated plainly

notebook-intelligence:run-command-in-terminal stays allowlisted because the backend sends it. Shell execution is therefore exactly as reachable after this patch as before. The gate filters command ids, not arguments. Narrowing that path means validating arguments, which is a separate change.

Tests

  • tests/ts/response-stream-command-allowlist.test.ts: 18 tests over the predicate, both list contents, and the refusal path of the wrapper.
  • tests/test_run_ui_command_allowlist_parity.py: cross-checks the allowlist against the Python call sites in both directions, so a new run_ui_command id that is not allowlisted fails the suite instead of silently breaking a tool in the browser.

Verified: pytest 1834 passed, tsc clean, jest 441 passed, stylelint and eslint clean. Seven mutants, including deleting a wrapper call site, gutting the gate, and widening either list, each fail a named test.

Follow-ups and known limits

  • The parity test parses run_ui_command call sites as text. It is blind to a keyword-argument or variable id shape, so it asserts that every call site it finds is readable and fails loudly if a new shape appears, rather than dropping it in silence.
  • The wrapper-call-site count in the parity test is a smoke test for accidental removal, not a security boundary. It cannot see an adversarial bypass such as assigning the streamed id to a local variable first. Enforcement lives in the wrapper, which the jest tests cover directly.
  • The five chat-user-input callers keep an ungated path on purpose. Gating them would refuse a form the user had already answered and leave the backend future unresolved.

The backend can ask the frontend to run a JupyterLab command by id. No
code checked that id, so any command the application had registered was
reachable from the response stream.

RUN_UI_COMMAND_ALLOWLIST holds the ids run_ui_command actually sends.
RESPONSE_BUTTON_COMMAND_ALLOWLIST is deliberately narrower, because the
only ButtonData the backend constructs offers the settings dialog.

executeResponseStreamCommand is a single chokepoint, so the sinks cannot
drift into checking the policy inconsistently. A refused id returns an
error string on the callback rather than leaving the caller waiting.

The five chat-user-input callers keep an ungated path on purpose. Gating
them would refuse a form the user had already answered and leave the
backend future unresolved.

One limit worth stating plainly: run-command-in-terminal remains
allowlisted because the backend sends it, so shell reachability is
unchanged by this patch. The gate filters command ids, not arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(chat): no allowlist on the command ids the response stream can execute

1 participant