1777787 - Console doesn't show Right-To-Left override chars

Status: VERIFIED FIXED

(DevTools :: Console, defect, P3)

Description

[MeowCKl]

Attachment: Screenshot.png

User Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101 Firefox/102.0

Steps to reproduce:

Open Web Developer Tools.
Paste https://raw.githubusercontent.com/nickboucher/trojan-source/main/JavaScript/stretched-string.js .
(See https://trojansource.codes/)

Actual results:

The code is displayed as `if (accessLevel != "user") // Check if admin'

Expected results:

`if (accessLevel != "user[U+202E] [U+2066]// Check if admin[U+2069] [U+2066]") {'

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

I don't think this could be exploited though.
We have a self-xss mechanism to prevent people to paste things in the console
And if you're a developer, you should be careful executing anything in the console (as what you'd put in your js sources)
But I agree that it would be nice if we'd show those characters in the [U+xxx] form to avoid confusion

Comment 2

[Negritas Sergiu, Desktop QA]

Hello! I have managed to reproduce the issue using firefox 104.0a1(2022-07-20), 103.0 and 102.0.1 on Ubuntu 22.04 and Windows 10, I will mark this issue as NEW and set a component for it in order to get our developers involved.

If it's not the right component please feel free to change it to an appropriate one.

Have a nice day!

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Looks like something we could handle with https://codemirror.net/5/doc/manual.html#option_specialChars

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: characters added to specialChars

it would look like the attached screenshot with the following diff

diff --git a/devtools/client/shared/sourceeditor/editor.js b/devtools/client/shared/sourceeditor/editor.js
--- a/devtools/client/shared/sourceeditor/editor.js
+++ b/devtools/client/shared/sourceeditor/editor.js
@@ -161,6 +161,7 @@ function Editor(config) {
     maxHighlightLength: 1000,
     // Disable codeMirror setTimeout-based cursor blinking (will be replaced by a CSS animation)
     cursorBlinkRate: 0,
+    specialChars: /[\u0000-\u001f\u007f-\u009f\u00ad\u061c\u200b-\u200f\u2028\u2066\u2069\u2029\u202e\ufeff\ufff9-\ufffc]/,
   };

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Itiel, does what's pictured in Comment 4 would be okay? I don't have much experience regarding multi-direction strings, so I'm not sure if it would cause any harm

Comment 6

[Itiel]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)

Itiel, does what's pictured in Comment 4 would be okay? I don't have much experience regarding multi-direction strings, so I'm not sure if it would cause any harm

Thanks for flagging me!
The screenshot in comment 4 is better than nothing, but like VSCode's implementation better.
Some notes:

1. The red dots don't seem to convey much to the user other than "something is there"; VSCode is explicit about what it is
2. If VSCode's implementation can't be forked here somehow, hovering the red dots should at least show a tooltip with the [U+xxx]
3. Regardless of #1 and #2 above, selecting the the code and copy-pasting it elsewhere should be with the markers themselves, without the dots (or the [U+xxx] text, in the VSCode implementation)
4. An optional bonus to #3 above would be to allow selecting and copying the marker (in the VSCode case), only when explicitly selecting that text (as opposed to selecting the whole code in which the marker text should not be included in that selection)

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: print unicode char

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Itiel from comment #6)

The screenshot in comment 4 is better than nothing, but like VSCode's implementation better.
Some notes:

1. The red dots don't seem to convey much to the user other than "something is there"; VSCode is explicit about what it is
2. If VSCode's implementation can't be forked here somehow, hovering the red dots should at least show a tooltip with the [U+xxx]
3. Regardless of #1 and #2 above, selecting the the code and copy-pasting it elsewhere should be with the markers themselves, without the dots (or the [U+xxx] text, in the VSCode implementation)
4. An optional bonus to #3 above would be to allow selecting and copying the marker (in the VSCode case), only when explicitly selecting that text (as opposed to selecting the whole code in which the marker text should not be included in that selection)

We can mimick what's done in VSCode (see Comment 7)
Copying seems to copy the char, not its unicode representation

Comment 9

[Itiel]

Looks great!
#4 above is only optional and a nice-to-have.
Ship it!

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1777787 - [devtools] Display non-printable chars in CodeMirror. r=jdescottes.

This adds some character (e.g. right-to-left override) to CodeMirror default list of
special characters (see https://codemirror.net/5/doc/manual.html#option_specialChar).
We also take this opportunity to show the unicode value for those, instead of the
default red dot CodeMirror provides.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5ed92b74381
[devtools] Display non-printable chars in CodeMirror. r=jdescottes.

Comment 12

[Cristian Tuns]

Backed out for causing mochitest failures on browser_styleeditor_filesave.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_filesave.js | Test timed out -

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

fix attempt: https://treeherder.mozilla.org/jobs?repo=try&revision=6ac704e22758a68c355df34ce88daa8d5ae780ae

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe256e97ac59
[devtools] Display non-printable chars in CodeMirror. r=jdescottes.

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/fe256e97ac59

Comment 16

[Negritas Sergiu, Desktop QA]

Hello! I can confirm that this issue is resolved with firefox 105.0 and 104.0b7 on Ubuntu 22 and Widows 10. I will update the flags accordingly and mark this issue as VERIFIED->FIXED.

Thank you and have a nice day!