Disable keydown on native cells in untrusted notebooks by joyceerhl · Pull Request #12914 · microsoft/vscode-python

joyceerhl · 2020-07-13T00:23:58Z

For #12912

joyceerhl · 2020-07-13T05:34:36Z

src/datascience-ui/native-editor/nativeCell.tsx

+        e.code !== 'ArrowUp' &&
+        e.code !== 'ArrowDown' &&
+        e.code !== 'j' &&
+        e.code !== 'Escape'


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

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.

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

DonJayamanne · 2020-07-13T15:11:54Z

src/datascience-ui/native-editor/nativeCell.tsx

    return connect(null, actionCreators)(NativeCell);
 }
+
+function isNotWhitelistedCommand(e: IKeyboardEvent) {


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.

#12914 (comment)

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

@DonJayamanne how about isNotebookEditingCommand or isNotNotebookNavigationCommand?

Oh sorry missed your suggestion, isCellNavigationKeyboardEvent works

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

DonJayamanne · 2020-07-13T15:14:11Z

src/datascience-ui/native-editor/nativeCell.tsx


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


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.

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.

DonJayamanne

Blocking based on function name isNotWhiteListedCommand, there's no context hence not sure what it means here.
Also instead of whitelist i'd prefer to use included/excluded or similar.

DonJayamanne · 2020-07-13T15:39:39Z

src/datascience-ui/native-editor/nativeCell.tsx

+        e.code !== 'ArrowUp' &&
+        e.code !== 'ArrowDown' &&
+        e.code !== 'j' &&
+        e.code !== 'Escape'


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.

sonarqubecloud · 2020-07-13T17:41:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

DonJayamanne · 2020-07-13T19:09:04Z

src/datascience-ui/native-editor/nativeCell.tsx


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


~~Shouldn't these be checked only if !this.isFocused()~~
~~Cuz if a cell is focused, then k might do something else..~~

I'm unfamiliar with the intended behavior here, but isn't that check already done in onOuterKeyDown?

Please ignore the comment, thats why i striked it out.

* Fix regressions in trusted notebooks (#12902) * Handle trustAllNotebooks selection * Fix bug where after trusting, UI didn't update * Recover from ENOENT due to missing parent directory when trusting notebook (#12913) * Disable keydown on native cells in untrusted notebooks (#12914)

* Update model.isTrusted on trust change (#12820) (#12823) * Reduce visual complexity of trust prompt (#12839) (#12847) * Port python 2.7 fix to release (#12877) * port color fix on collapse all (#12895) (#12897) * fix a color on collapse all (#12895) * update changelog * Merge fixes into July release (#12889) Co-authored-by: Timothy Ruscica <[email protected]> * Merge more fixes into july release (#12918) Co-authored-by: Joyce Er <[email protected]> * Port trust fixes (#12929) * Fix regressions in trusted notebooks (#12902) * Handle trustAllNotebooks selection * Fix bug where after trusting, UI didn't update * Recover from ENOENT due to missing parent directory when trusting notebook (#12913) * Disable keydown on native cells in untrusted notebooks (#12914) * Hide editor icons when editor is not a notebook (#12934) (#12935) * Check for hideFromUser before activating current terminal (#12942) (#12956) * Check for hideFromUser before activating current terminal * Add tests * Tweak logic * Port final trust fixes for release (#12965) * Only allow Enter / NumpadEnter w/o ctrl/shift/alt (#12939) * Send telemetry for notebook trust prompt selections (#12964) * Fixes for persisting trust (#12950) * Display survey for native notebooks on/after 1st August (#12961) (#12975) Co-authored-by: Joyce Er <[email protected]> Co-authored-by: Joyce Er <[email protected]> * Contains cherry picks, version updates, change log updates (#12983) * Update version and change log * Improve detection when LS is fully loaded for IntelliCode (#12853) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Wait for client ready * Handle async dispose * Fix the date Co-authored-by: Mikhail Arkhipov <[email protected]> * hide the gather button while a cell is executing (#12984) * Update date (#13002) * remove release notes from the start page (#13032) * Cherry pick, version change and change log update (#13079) * Ensure languageServer value is valid, send event during activate (#13064) * Update change log and version * Activate banner prompt for Pylance (#12817) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Remove LS experiments * Frequency + tests * Fix test * Update message to match spec * Open workspace for extension rather than changing setting * Fix localization string * Show banners asynchronously * Add experiments * Formatting * Typo * Put back verifyAll * Remove obsolete experiments, add Pylance * Suppress experiment if Pylance is installed * PR feedback Co-authored-by: Jake Bailey <[email protected]> * Update change log as per comments Co-authored-by: Jake Bailey <[email protected]> Co-authored-by: Mikhail Arkhipov <[email protected]> * Port fix the gather survey (#13086) (#13105) * Fix the gather survey (#13086) * fix the gather survey added 'gather stats' telemetry mention the gather comments to update the python ext * oops * fix tests and address comments * update gather stats when resetting the kernel * set globalstate vars to 0 when we open vs code * fix gather stats telemetry * fix tests * fix tests for real Co-authored-by: Joyce Er <[email protected]> Co-authored-by: Ian Huff <[email protected]> Co-authored-by: David Kutugata <[email protected]> Co-authored-by: Don Jayamanne <[email protected]> Co-authored-by: Timothy Ruscica <[email protected]> Co-authored-by: Mikhail Arkhipov <[email protected]> Co-authored-by: Jake Bailey <[email protected]>

Disable jupyter shortcuts for untrusted notebooks

f1d2e8f

joyceerhl added the no-changelog No news entry required label Jul 13, 2020

joyceerhl requested review from DavidKutu, DonJayamanne, IanMatthewHuff and rchiodo July 13, 2020 00:23

Whitelist some jupyter keystrokes

03ca846

joyceerhl commented Jul 13, 2020

View reviewed changes

DonJayamanne reviewed Jul 13, 2020

View reviewed changes

DonJayamanne requested changes Jul 13, 2020

View reviewed changes

Don's feedback

dfb3fab

DonJayamanne requested changes Jul 13, 2020

View reviewed changes

joyceerhl requested a review from DonJayamanne July 13, 2020 15:45

Fixes

dead27a

rchiodo approved these changes Jul 13, 2020

View reviewed changes

Oops

4559a2e

joyceerhl mentioned this pull request Jul 13, 2020

Port trust fixes #12929

Merged

10 tasks

DonJayamanne reviewed Jul 13, 2020

View reviewed changes

DonJayamanne approved these changes Jul 13, 2020

View reviewed changes

joyceerhl merged commit 68f4bb0 into microsoft:master Jul 13, 2020

joyceerhl added a commit that referenced this pull request Jul 13, 2020

Disable keydown on native cells in untrusted notebooks (#12914)

5126787

Conversation

joyceerhl commented Jul 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 13, 2020

Uh oh!

DonJayamanne Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DonJayamanne left a comment •

edited

Loading

DonJayamanne Jul 13, 2020 •

edited

Loading