microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.17k stars 29.28k forks source link

Dangerous probing for `List` in `editorCommandsContext` #224050

Open bpasero opened 3 months ago

bpasero commented 3 months ago

This check here:

https://github.com/microsoft/vscode/blob/5b5e760015c063cd44e987e4f2e5316a8ff412e2/src/vs/workbench/browser/parts/editor/editorCommandsContext.ts#L64

Potentially leaves out Tree instances. As you can see, elsewhere we also check for other kinds of widgets:

https://github.com/microsoft/vscode/blob/5b5e760015c063cd44e987e4f2e5316a8ff412e2/src/vs/workbench/contrib/files/browser/files.ts#L73-L83

And besides, it might wrongfully assume the context is a list when the active editor focus is in a list widget as it is the case in notebooks (https://github.com/microsoft/vscode/issues/221582).

bpasero commented 3 months ago

Added this as workaround to address https://github.com/microsoft/vscode/issues/221582 but I think its not the best fix:

https://github.com/microsoft/vscode/blob/a2d507672ff2d59607fc1129c6a1e3b2075d7d17/src/vs/workbench/browser/parts/editor/editorCommandsContext.ts#L151-L158

benibenj commented 2 months ago

I assume only a List instance check was introduced here because it only accepts context from Open Editor View currently. Similar file commands also only check for List and Async Data Tree instead of checking for all types such as Object Tree, Paged Lists and Tables which could in theory also use file commands, but do not in practice.

Ideally we would not have instance of checks but the command would include it's source (Open Editor View, Tabs, Command, ...). This would remove false positives we currently have.

Thoughts?

bpasero commented 2 months ago

But a command can always be executed from anywhere, e.g. a keybinding with no specific context, so how would that help?

benibenj commented 2 months ago

If we know the source then we would exactly know how to extract the context.

However, in the case of keyboard shortcuts we probably would still need to probe the list and group model to figure out the context. So this approach would probably reduce false positives (including https://github.com/microsoft/vscode/issues/221582) but not fully solve the problem :/