Decode at-mark and backslash ( #1307 ) #1588

apple502j · 2018-09-16T01:06:19Z

Resolves

Proposed Changes

Decode \\@ and \\\\ using regex and replace. The issue didn't contain the backslash one, but I also found that, so it contains fix for that.

Reason for Changes

Not to change custom block name.

Test Coverage

https://scratch.mit.edu/projects/246543955/
This project has 2 custom blocks named @atmark@ and \backslash\

Tested using above with Windows 7, Firefox 62 and Chrome 68

joker314 · 2018-09-16T10:35:28Z

This is a nice pull request, but it doesn't account for % being encoded.

Take note of the backslash before the final percent sign

Take note of the absence of a backslash before the final percent sign

I haven't tested your PR locally yet, I'm just assuming % won't work from looking through the code. I may be mistaken however.

Note that, per my issuecomment, only backslash, at, and percent are likely to be affected.

apple502j · 2018-09-16T12:34:36Z

OK, I will decode percent too. Thanks @joker314

joker314 · 2018-09-16T13:03:36Z

In Scratch 2.0, the algorithm for escaping the data is not the mirror image of the algorithm for unescaping that data.

This pull request, however, takes the mirror image of the escape function.

This is absolutely fine; and the only time this will not work is if there's 'hacked' JSON.

Somebody may modify their string like so:

- ask user how they are doing
+ \a\s\k\ \u\s\e\r\ \h\o\w\ \t\h\e\y\ \a\r\e\ \d\o\i\n\g

Upon performing such a modification, their project would behave identically. There's no reason to make such a change as a JSON hacker (and so there's no reason to worry about it in this PR, maybe), however, if such a modification has been made, this pull request would leave the backslashes in.

I'd say that this is absolutely fine and we shouldn't worry about it.

Regarding the chaining of .replace()s:

.replace(/\\([%@\\])/g, '$1')

might be shorter. I'm not sure if this is more readable? I also don't know how this may impact efficiency -- it's a style choice that you may wish to consider.

This could be modified to resolve the above issue about hacked JSONs like so:

.replace(/\\(.)/g, '$1')

Though, as stated earlier, the hacked JSON issue seems like a non-issue.

apple502j · 2018-09-16T13:10:05Z

I added percent decoding, but it uses full-width percent to avoid scratchfoundation/scratch-blocks#1368

joker314 · 2018-09-16T14:56:00Z

Hmm. Maybe I was wrong about us needing to decode \% as %. If Scratch 3.0 still considers it special, leave it as-is. Note that in my picture, only one of the percents was keeping the backslash: the one which didn't have a character after it. I'm investigating when Scratch 2.0 escapes it and when it doesn't.

EDIT: Scratch 2.0 will always encode percent symbols with a preceding backslash. Hence, Scratch 3.0 is assigning some percent signs special meanings -- but others not. Likely, if there's a character after the percent sign, it'll become a special token.

EDIT: I can only seem to get the first percent sign to be treated as something that can be escaped.

joker314 · 2018-09-16T16:26:26Z

Okay, some things to note:

It looks like the first escaped percent sign to appear in the string, will be stripped of its backslash. The rest won't. This leads me to believe that there may be a forgotten g flag on a regular expression, elsewhere, and would be out of the scope of this pull request. Perhaps not, though.
This isn't strictly related to this PR, but it is related to the stuff it's addressing:
https://github.com/LLK/scratch-vm/blob/64a1d3e02b6aaaac9d6ec5e7299fc13e8199e136/src/serialization/sb2.js#L60
This looks like it would not account for an escaped backslash before a percent sign (i.e., \\%), because it'd detect there was a \% present and incorrectly ignore it.

Should an issue be filed?

apple502j · 2018-09-16T22:33:04Z

@joker314 It's already help-wanted, scratchfoundation/scratch-blocks#1368

joker314 · 2018-09-16T22:39:49Z

@apple502j When you made the change to the full-width percent sign, did you actually manage to get a working example of a 2.0 project that would cause scratchfoundation/scratch-blocks#1368 to happen?

apple502j · 2018-09-16T22:52:46Z

@joker314 Oops, 1368 was not related at all.

joker314

It's probably okay to remove the replacement to the full-width percent sign, then?

kchadha · 2018-09-25T18:35:07Z

@apple502j, Thanks for making this pull request! We are a little backed up on code reviews at the moment, but we'll take a look at these changes soon.

kchadha

@apple502j, Thank you for the pull request! I have left a comment in the code changes.

src/serialization/sb2.js

@@ -1061,7 +1061,8 @@ const parseBlock = function (sb2block, addBroadcastMsg, getVariableId, extension
            children: [],
            mutation: {
                tagName: 'mutation',
-                proccode: procData[0], // e.g., "abc %n %b %s"
+                proccode: procData[0].replace(/\\%/g, '％').replace(/\\(.)/g, '$1'),


adroitwhiz · 2020-05-14T19:47:12Z

src/serialization/sb2.js

@@ -1061,7 +1061,8 @@ const parseBlock = function (sb2block, addBroadcastMsg, getVariableId, extension
            children: [],
            mutation: {
                tagName: 'mutation',
-                proccode: procData[0], // e.g., "abc %n %b %s"
+                proccode: procData[0].replace(/\\%/g, '％').replace(/\\(.)/g, '$1'),


Note that this is a fullwidth percent sign-- "replace this character with another very similar looking character" strikes me as somewhat janky

apple502j · 2020-05-15T04:37:18Z

Hmm, maybe I should make better one

Decode at-mark and backslash ( scratchfoundation#1307 )

6f5aa37

decode % as well (with todo)

0366a58

Decode all - thanks joker314

971f0cb

joker314 reviewed Sep 16, 2018

View reviewed changes

thisandagain added the pr - needs review label Sep 24, 2018

thisandagain added this to the September 2018 milestone Sep 24, 2018

thisandagain assigned kchadha Sep 24, 2018

kchadha suggested changes Oct 29, 2018

View reviewed changes

kchadha added pr - changes requested and removed pr - needs review labels Oct 29, 2018

kchadha assigned apple502j and unassigned kchadha Oct 29, 2018

thisandagain modified the milestones: September 2018, Backlog Mar 22, 2019

adroitwhiz reviewed May 14, 2020

View reviewed changes

apple502j closed this May 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode at-mark and backslash ( #1307 ) #1588

Decode at-mark and backslash ( #1307 ) #1588

apple502j commented Sep 16, 2018

joker314 commented Sep 16, 2018

apple502j commented Sep 16, 2018

joker314 commented Sep 16, 2018 •

edited

Loading

apple502j commented Sep 16, 2018

joker314 commented Sep 16, 2018 •

edited

Loading

joker314 commented Sep 16, 2018 •

edited

Loading

apple502j commented Sep 16, 2018

joker314 commented Sep 16, 2018

apple502j commented Sep 16, 2018

joker314 left a comment •

edited

Loading

kchadha commented Sep 25, 2018

kchadha left a comment

This comment was marked as abuse.

adroitwhiz May 14, 2020

apple502j commented May 15, 2020

Decode at-mark and backslash ( #1307 ) #1588

Decode at-mark and backslash ( #1307 ) #1588

Conversation

apple502j commented Sep 16, 2018

Resolves

Proposed Changes

Reason for Changes

Test Coverage

joker314 commented Sep 16, 2018

apple502j commented Sep 16, 2018

joker314 commented Sep 16, 2018 • edited Loading

apple502j commented Sep 16, 2018

joker314 commented Sep 16, 2018 • edited Loading

joker314 commented Sep 16, 2018 • edited Loading

apple502j commented Sep 16, 2018

joker314 commented Sep 16, 2018

apple502j commented Sep 16, 2018

joker314 left a comment • edited Loading

Choose a reason for hiding this comment

kchadha commented Sep 25, 2018

kchadha left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

adroitwhiz May 14, 2020

Choose a reason for hiding this comment

apple502j commented May 15, 2020

joker314 commented Sep 16, 2018 •

edited

Loading

joker314 commented Sep 16, 2018 •

edited

Loading

joker314 commented Sep 16, 2018 •

edited

Loading

joker314 left a comment •

edited

Loading