search

atom / settings-view

🔧 Edit Atom settings
MIT License
272 stars 276 forks source link

Escape CSON correctly #1109

Closed Aerijo closed 5 years ago

Aerijo commented 5 years ago

Requirements

Description of the Change

Escapes quotes and backslashes better in the keybinding-to-string function.

In addition to linked issue, this is a keybinding I found on my own Atom that needs quote escaping

'atom-text-editor[data-grammar~=\'css\']':
  'ctrl-alt-c': 'css-declaration-sorter:sort'

Without this PR, it wouldn't escape the value quotes.

Also tidied up the composition part, which was really hard to read IMO. Now it's a clean single line with well defined newlines and spacing.

Alternate Designs

Considered CSON.stringify(...), but (1) it does a lot of extra work for little benefit, (2) it returns with double quotes, and (3) it's wrong; trying that with the raw text \\" (escaped backslash + ") ironically gives \", which is an escaped ". (Actually might not be wrong, as \\" would already be escaped by JSON.stringify to \\\". But I don't want to add a new dependency just for one thing)

Could also run JSON.stringify and then swap quote escapes + replace single with double quotes, but I don't think it's necessary.

Benefits

Correctly escape ', ", and \ in strings

Possible Drawbacks

none

Applicable Issues

closes https://github.com/atom/settings-view/issues/1110

rsese commented 5 years ago

Almost forget to ask, can you add a test for this? Whoever ends up reviewing will ask :smile:

Aerijo commented 5 years ago

@rsese added specs

rafeca commented 5 years ago

Thanks for the contribution! 😍

Since the escape logic of CSON matches the one in JSON (with the only difference of having to escape single quotes instead of double quotes when using single quote notation), it will be easier to just use JSON.stringify() and change the quoting escaping.

Your escapeCSON function would look like:

const escapeCSON = (input) => {
  return JSON.stringify(input)
    .slice(1, -1) // Remove wrapping double quotes
    .replace(/\\"/g, '"') // Unescape double quotes
    .replace(/'/g, '\\\''); // Escape single quotes
}

This will also help escaping other reserved characters like \n, etc.

What do you think?

Aerijo commented 5 years ago

Yeah, it feels safer to leave the majority of the escaping to the builtin JSON.

@rafeca Done, and the failed spec seems completely unrelated.