js2-mode icon indicating copy to clipboard operation
js2-mode copied to clipboard

js2-mode doesn't recognize LS and PS line endings, leading to incorrect syntax highlighting

Open cpitclaudel opened this issue 1 year ago • 6 comments

Javascript allows LS and PS as line endings, but js2-mode doesn't recognize them and hence produces incorrect highlighting:

Screenshot from 2023-07-22 18-46-47

(This is a JS-mode copy of the bug at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=64792 )

cpitclaudel avatar Jul 22 '23 16:07 cpitclaudel

Hi Clement!

This is probably harder to fix in js-mode, which relies on what font-lock considers a newline.

Here's a quick patch you can try, though there are probably more places that will need updating:

diff --git a/js2-mode.el b/js2-mode.el
index 11ebb37..d53c399 100644
--- a/js2-mode.el
+++ b/js2-mode.el
@@ -5585,16 +5585,17 @@ Increments `js2-ts-lineno' if the return value is a newline char.
 Updates `js2-ts-cursor' to the point after the returned char.
 Returns `js2-EOF_CHAR' if we hit the end of the buffer.
 Also updates `js2-ts-hit-eof' and `js2-ts-line-start' as needed."
+  (defvar js2-eol-chars)
   (let (c)
     ;; check for end of buffer
     (if (>= js2-ts-cursor (point-max))
         (setq js2-ts-hit-eof t
               js2-ts-cursor (1+ js2-ts-cursor)
-              c js2-EOF_CHAR)  ; return value
+              c js2-EOF_CHAR)           ; return value
       ;; otherwise read next char
       (setq c (char-before (cl-incf js2-ts-cursor)))
       ;; if we read a newline, update counters
-      (if (= c ?\n)
+      (if (memq c js2-eol-chars)
           (setq js2-ts-line-start js2-ts-cursor
                 js2-ts-lineno (1+ js2-ts-lineno)))
       ;; TODO:  skip over format characters
@@ -5670,7 +5671,7 @@ See http://es5.github.io/#x7.6"
      ;; TODO:  change this nil to check for Unicode space character
      nil)))
 
-(defconst js2-eol-chars (list js2-EOF_CHAR ?\n ?\r))
+(defconst js2-eol-chars (list js2-EOF_CHAR ?\n ?\r ?\u2028 ?\u2029))
 
 (defun js2-skip-line ()
   "Skip to end of line."

These details aside, do you expect to be using this distinction? Is there JS code in the wild relying on this?

dgutov avatar Jul 23 '23 01:07 dgutov

Thanks!

These details aside, do you expect to be using this distinction? Is there JS code in the wild relying on this?

No, I don't expect to write any such code myself — it's more of a security thing.

cpitclaudel avatar Jul 28 '23 07:07 cpitclaudel

All right.

Then should we go for a more obvious warning via font-lock rather than parsing the code in a different way? This could be done in both js-mode and js2-mode fairly easily: just highlight every such line from the encountered character until eol with the warning face or something.

dgutov avatar Jul 29 '23 19:07 dgutov

Then should we go for a more obvious warning via font-lock rather than parsing the code in a different way?

That would be a reasonably good workaround, I think.

cpitclaudel avatar Aug 12 '23 19:08 cpitclaudel

Considering this class of attacks exists in a bunch of languages, maybe it would be a good idea to report this sort of thing upstream so it can be handled in a consistent way across major modes?

UwUnyaa avatar Aug 13 '23 12:08 UwUnyaa

maybe it would be a good idea to report this sort of thing upstream so it can be handled in a consistent way across major modes?

That could be a good addition to the original report, yes.

dgutov avatar Aug 16 '23 00:08 dgutov