[lint] Remove no-control-regex #1258

dayo09 · 2022-09-06T05:06:47Z

This commit enables 'no-control-regex' rule
and removes the errors accordingly.

ONE-vscode-DCO-1.0-Signed-off-by: dayo09 [email protected]

Remove no-control-regex

Reference

https://stackoverflow.com/questions/14313183/javascript-regex-how-do-i-check-if-the-string-is-ascii-only

Problematic lines
// NOTE ASCII characters have codes ranging from u+0000 to u+007f
function containsNonAscii(str: string): boolean {
  return !/^[\u0000-\u007f]*$/.test(str);
}
The line is required to check if it's an ascii, but how about checking 'printable ascii' aside from those problematic non-printables?

dayo09 · 2022-09-06T05:12:05Z

I think that \n, \r and \t characters are in non-printable ascii and the tests includes them. I guess any password cannot contain them, so we may be able to remove those characters from the tests.

dayo09 · 2022-09-06T05:16:04Z

.eslintrc.json

@@ -24,6 +24,7 @@
      "eqeqeq": "warn",
      "no-throw-literal": "warn",
      "semi": "off",
+      "no-control-regex": "warn",


I put this above 'no-extra-semi' to avoid possible merge conflict

dayo09 · 2022-09-06T05:20:13Z

src/Tests/View/PasswordQuickInput.test.ts


-        const strWithSpecialChar = '\n\t\r!@#$%^&*(){}[]\\';
-        assert.isNotTrue(containsNonAscii(strWithSpecialChar));
+        const strWithSpecialChar = '!@#$%^&*(){}[]\\';


I removed \n\t\r from the test.

Reasoning:

(1) \r \n

They are CR, LR keys which are 'new line' characters based on each os.

CR(\r), LF(\n)](https://m.blog.naver.com/taeil34/221325864981)

When I hit the 'enter' key, the password quick pick returns, so they are not expected to be included in passwords.

(2) \t

When I hit the 'tab' key, the password quick pick disappears, so it is not expected to be included in passwords.

Is there any possibility that the key value will be entered through copy and paste?

Oh.. sure. I tried a random password with tab(0x09, control) and space(0x20, printable), and it distinguished those two characters, allowing login by only copy&paste.

(For lint's sake) Allowiing \t \n by the rule

It seems \t \n are allows by the eslint's rule even though they are control characters.

https://eslint.org/docs/latest/rules/no-control-regex

The following elements of regular expression patterns are considered possible errors in typing and are therefore disallowed by this rule:

Hexadecimal character escapes from \x00 to \x1F.
Unicode character escapes from \u0000 to \u001F.
Unicode code point escapes from \u{0} to \u{1F}.
Unescaped raw characters from U+0000 to U+001F.
Control escapes such as \t and \n are allowed by this rule.

(For security's sake) More to consider

It seems 0x7f is a special character(delete) too.

https://www.geeksforgeeks.org/control-characters/

So let me add \n, \r and \t to the regex and see if they are allowed. The other control characters look not good to allow from security's view, IMO.

❗

I tried with ascii 0x1,0x2 and even 0x18(Cancel) and linux os still accept all those control ascii characters.

0x1 Start of Heading 0x2 Start of Text 0x18 Cancel

I tried with 0xff(latin y) 0x80(euro sign), which are extended ascii. They are accepted as a linux passwd but I couldn't login to vscode remote with those passwords

dayo09 · 2022-09-06T05:24:22Z

src/View/PasswordQuickInput.ts

-function containsNonAscii(str: string): boolean {
-  return !/^[\u0000-\u007f]*$/.test(str);
+// NOTE Printable ASCII characters have codes ranging from u+0000 to u+007f
+function containsOnlyPrintableAscii(str: string): boolean {


From clean code's view,
I changed negation from the function to avoid 'double negation'

https://refactoring.com/catalog/removeDoubleNegative.html

src/View/PasswordQuickInput.ts

This commit enables 'no-control-regex' rule and removes the errors accordingly. ONE-vscode-DCO-1.0-Signed-off-by: dayo09 <[email protected]>

ONE-vscode-DCO-1.0-Signed-off-by: dayo09 <[email protected]>

dayo09 · 2022-09-07T04:55:43Z

src/View/PasswordQuickInput.ts

+// NOTE
+//
+//   u+0000-u+001f: control ascii codes,    ascii (ALLOWED)
+//   u+0020-u+007f: printable ascii codes,  ascii (ALLOWED)
+//   u+0080-u+00ff: extended ascii codes,   extended ascii (NOT ALLOWED)
+//
+// By experience, Linux os takes all extended ascii characters(u+0000-u+00ff) for passwords, even
+// controll asciis. whereas 'eslint' recommends not using regex including control ascii codes.
+// Therefore, we allow users to put control ascii codes.
+//
+// By experience, the passwords including extended ascii characters are not working when loging in
+// to remote vscode. Therefore, here we allow only 'ascii characters'(0x00-0x7f).


@jyoungyun I tried several tests under what you've mentioned, I summarized here what I've found out. It seems that leaving the codes as-is is enough for now. PTAL :-D

src/View/PasswordQuickInput.ts

ONE-vscode-DCO-1.0-Signed-off-by: dayo09 <[email protected]>

jyoungyun

LGTM Thank you!

dayo09 · 2022-09-08T04:38:41Z

@Samsung/one-vscode_committers it's ready. PTAL :-D

dayo09 requested a review from jyoungyun September 6, 2022 05:06

dayo09 force-pushed the 0906-control branch from b3b45d4 to 86c0a26 Compare September 6, 2022 05:15

dayo09 commented Sep 6, 2022

View reviewed changes

dayo09 marked this pull request as ready for review September 6, 2022 05:24

dayo09 added the 2 approvals 2 approvals required to be merged label Sep 6, 2022

dayo09 requested a review from a team September 6, 2022 05:24

dayo09 mentioned this pull request Sep 6, 2022

[eslint] Add recommended rules #1253

Closed

dayo09 force-pushed the 0906-control branch from 86c0a26 to be81c6a Compare September 7, 2022 04:39

dayo09 commented Sep 7, 2022

View reviewed changes

src/View/PasswordQuickInput.ts Outdated Show resolved Hide resolved

dayo09 added 2 commits September 7, 2022 13:40

[lint] Remove no-control-regex

9f70e91

This commit enables 'no-control-regex' rule and removes the errors accordingly. ONE-vscode-DCO-1.0-Signed-off-by: dayo09 <[email protected]>

Add comments

4fb3617

ONE-vscode-DCO-1.0-Signed-off-by: dayo09 <[email protected]>

dayo09 force-pushed the 0906-control branch from 520836d to 4fb3617 Compare September 7, 2022 04:53

dayo09 commented Sep 7, 2022

View reviewed changes

src/View/PasswordQuickInput.ts Outdated Show resolved Hide resolved

dayo09 added 2 commits September 7, 2022 13:57

Update src/View/PasswordQuickInput.ts

b37f253

format

e72d35f

ONE-vscode-DCO-1.0-Signed-off-by: dayo09 <[email protected]>

jyoungyun approved these changes Sep 8, 2022

View reviewed changes

dayo09 requested a review from a team September 8, 2022 04:38

llFreetimell approved these changes Sep 8, 2022

View reviewed changes

llFreetimell merged commit 5fd09de into Samsung:main Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lint] Remove no-control-regex #1258

[lint] Remove no-control-regex #1258

dayo09 commented Sep 6, 2022 •

edited

Loading

dayo09 commented Sep 6, 2022

dayo09 Sep 6, 2022

dayo09 Sep 6, 2022 •

edited

Loading

jyoungyun Sep 6, 2022

dayo09 Sep 6, 2022 •

edited

Loading

dayo09 Sep 6, 2022 •

edited

Loading

dayo09 Sep 6, 2022 •

edited

Loading

dayo09 Sep 7, 2022

dayo09 Sep 7, 2022

dayo09 Sep 6, 2022

dayo09 Sep 7, 2022

jyoungyun left a comment

dayo09 commented Sep 8, 2022 •

edited

Loading

[lint] Remove no-control-regex #1258

[lint] Remove no-control-regex #1258

Conversation

dayo09 commented Sep 6, 2022 • edited Loading

Remove no-control-regex

Reference

Problematic lines

dayo09 commented Sep 6, 2022

dayo09 Sep 6, 2022

Choose a reason for hiding this comment

dayo09 Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

(1) \r \n

(2) \t

jyoungyun Sep 6, 2022

Choose a reason for hiding this comment

dayo09 Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

dayo09 Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

(For lint's sake) Allowiing \t \n by the rule

(For security's sake) More to consider

dayo09 Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

dayo09 Sep 7, 2022

Choose a reason for hiding this comment

dayo09 Sep 7, 2022

Choose a reason for hiding this comment

dayo09 Sep 6, 2022

Choose a reason for hiding this comment

dayo09 Sep 7, 2022

Choose a reason for hiding this comment

jyoungyun left a comment

Choose a reason for hiding this comment

dayo09 commented Sep 8, 2022 • edited Loading

dayo09 commented Sep 6, 2022 •

edited

Loading

dayo09 Sep 6, 2022 •

edited

Loading

dayo09 Sep 6, 2022 •

edited

Loading

dayo09 Sep 6, 2022 •

edited

Loading

dayo09 Sep 6, 2022 •

edited

Loading

dayo09 commented Sep 8, 2022 •

edited

Loading