Skip to content

Disable keydown on native cells in untrusted notebooks #12914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 13, 2020
Merged

Disable keydown on native cells in untrusted notebooks #12914

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1d2e8f
Disable jupyter shortcuts for untrusted notebooks
joyceerhl Jul 13, 2020
03ca846
Whitelist some jupyter keystrokes
joyceerhl Jul 13, 2020
dfb3fab
Don's feedback
joyceerhl Jul 13, 2020
dead27a
Fixes
joyceerhl Jul 13, 2020
4559a2e
Oops
joyceerhl Jul 13, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/datascience-ui/native-editor/nativeCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,9 @@ export class NativeCell extends React.Component<INativeCellProps> {

// tslint:disable-next-line: cyclomatic-complexity max-func-body-length
private keyDownInput = (cellId: string, e: IKeyboardEvent) => {
if (!this.isNotebookTrusted() && isNotWhitelistedCommand(e)) {
Copy link

@DonJayamanne DonJayamanne Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we need a better property. I think what we're after is readonly.
I.e. if a cell is readonly, then some keyboard shortcuts cannot be handled & the like.

Cuz the requirement is when a notebook is not trusted, then treat it as readonly, right now, when a notebook is not trusted, then ignore some keys, etc.
I know its laet for this change to be made, hence I wouldn't change it now, as it needs to be done across the UI.
Copy link
Member

@IanMatthewHuff IanMatthewHuff Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd agree with this. I think that it's going to be easy to accidentally add a new key / command in the future and possibly enable something for an untrusted notebook that we don't want enable.
return;
}
const isFocusedWhenNotSuggesting = this.isFocused() && e.editorInfo && !e.editorInfo.isSuggesting;
switch (e.code) {
case 'ArrowUp':
Expand Down Expand Up @@ -886,3 +889,14 @@ export class NativeCell extends React.Component<INativeCellProps> {
export function getConnectedNativeCell() {
return connect(null, actionCreators)(NativeCell);
}

function isNotWhitelistedCommand(e: IKeyboardEvent) {
Copy link

@DonJayamanne DonJayamanne Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what context is a whiteListedCommand used? it mean be anything.
is it a list of commands excluded when a notebook is running, disabled, kernel is busy, no kernel, etc.
Copy link
Author

@joyceerhl joyceerhl Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12914 (comment)
Copy link

@DonJayamanne DonJayamanne Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks but when looking at the code it is not obvious, i'd like a better name.
E.g. isCellNavigationKeyboardEvent
Copy link
Author

@joyceerhl joyceerhl Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DonJayamanne how about isNotebookEditingCommand or isNotNotebookNavigationCommand?
Copy link
Author

@joyceerhl joyceerhl Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry missed your suggestion, isCellNavigationKeyboardEvent works
Copy link
Author

@joyceerhl joyceerhl Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should actually be isNotCellNavigationKeyboardEvent, i.e. if the notebook is untrusted and the command is not just for navigating the notebook, we shouldn't allow it
return (
e.code !== 'Enter' &&
e.code !== 'NumpadEnter' &&
e.code !== 'ArrowUp' &&
e.code !== 'ArrowDown' &&
e.code !== 'j' &&
e.code !== 'Escape'
Copy link
Author

@joyceerhl joyceerhl Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These keystrokes are used to navigate and not edit a notebook, so I think we should propagate these, but open to suggestions
Copy link

@DonJayamanne DonJayamanne Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its better to check if something is a cell navigation shortcut or a cell edit shortcut.
This method is a little confusing for me cuz there are two negations, each if condition and then a negation at the method name level.

Also i know ArrowUp or k means select the previous cell. I.e. ArrowUp is a cell navigation, what k is treated as a non-cell navigation key. That doesn't seem right.

I personally think it would be more readable to have a function call ~isCellNavigationKeyboardEvent than doing the negation in the function and naming it as such.
Copy link
Author

@joyceerhl joyceerhl Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch, k is supposed to be there and has now been added. Also renamed the function!
);
}