getSlug leaves characters that will be escaped in a URL

[alexandros84]

Prerequisites

• [ X ] Are you running the latest version?
• [ X ] Are you able to consistently reproduce the issue?
• [ X ] Did you search the issue queue for existing issue? Search issues

Issue Description

It seems that the default replace options of either the transliteration https://github.com/reactioncommerce/transliteration or slugify https://github.com/simov/slugify packages Reaction is using, do currently allow characters that will be escaped within a URL to be included in shopSlugs and product handles.

Steps to Reproduce

When for example, a user creates a new shop with name "@sarah's Corner#$%" the resulting shopSlug will be "@sarah's-cornerdollar" which is going to become "%40sarah's-cornerdollar" when inside a URL.

This kind of default behavior can result in plenty of unnecessary confusion and can cost in terms of customer support resources. The same behavior can be seen in the making of product handles as the same function (getSlug) is responsible for this as well.

Possible Solution

getSlug uses whatever definition of slugify is passed on to her from lazyLoadSlugify. In that sense since the getSlug slugify might be either transliteration or slugify, the solution I came up with was to replace the following lines of code from lazyLoadSlugify.

// slugify is exported to modules.default while transliteration is exported to modules.slugify slugify = mod.default || mod.slugify;

with this:

// slugify is exported to modules.default while transliteration is exported to modules.slugify if (mod.default) { slugify = mod.default; slugify.extend({ " ": "-", "$": "-dlr-", "&": "-n-", "": "-opn-qte-", ":": "-cln-", "<": "-l-an-br-", ">": "-r-an-br-",
"[": "-l-sq-br-", "]": "-r-sq-br-", "{": "-l-cu-br-", "}": "-r-cu-br-", '"': "-qts-", "+": "-plus-", "#": "-otrp-",
"%": "-pct-", "@": "-at-", "/": "-sls-", ";": "-scl-", "=": "-eql-", "?": "-qstn-", "\": "-bsl-", "^": "-crt-",
"|": "-or-", "~": "-tld-", ",": "-cma-"
});
} else {
slugify = mod.slugify;
slugify.config({
replace: {
" ": "-", "$": "-dlr-", "&": "-n-", "": "-opn-qte-", ":": "-cln-", "<": "-l-an-br-", ">": "-r-an-br-", "[": "-l-sq-br-", "]": "-r-sq-br-", "{": "-l-cu-br-", "}": "-r-cu-br-", '"': "-qts-", "+": "-plus-", "#": "-otrp-", "%": "-pct-", "@": "-at-", "/": "-sls-", ";": "-scl-", "=": "-eql-", "?": "-qstn-", "\\": "-bsl-", "^": "-crt-", "|": "-or-", "~": "-tld-", ",": "-cma-" } }); }

Like getSlug, lazyLoadSlugify is defined multiple times throughout the latest working version of Reaction. So in order to centralize all necessary replacements I have defined this once as a const just below the latinLangs const in lib/api/helpers:

export const replacementOptions = { " ": "-", "$": "-dlr-", "&": "-n-", "": "-opn-qte-",
":": "-cln-",
"<": "-l-an-br-",
">": "-r-an-br-",
"[": "-l-sq-br-",
"]": "-r-sq-br-",
"{": "-l-cu-br-",
"}": "-r-cu-br-",
'"': "-qts-",
"+": "-plus-",
"#": "-otrp-",
"%": "-pct-",
"@": "-at-",
"/": "-sls-",
";": "-scl-",
"=": "-eql-",
"?": "-qstn-",
"\": "-bsl-",
"^": "-crt-",
"|": "-or-",
"~": "-tld-",
",": "-cma-"
};
`

Versions

While I was not able to test this in the latest Reaction version 1.13 because of this issue #4202 I have tested this on our team's 1.11 backed development site and it works as expected.

[brent-hoover]

@alexandros84 I wonder if a better solution wouldn't be to put some validation on the client-side and not let users input these invalid characters in the first place rather than giving them unexpected results?

[alexandros84]

That is definitely one valid approach to the whole issue. However since Reaction is giving us the option to deal with it effectively while giving the opportunity to sellers to differentiate creatively the name of their shops (for example " #hot? ") I decided to go down that way. What I am not sure of, is whether @nnnnat has already implemented the suggested solution #6 #4266 "Refactor server/api/core/utils.js getSlug method to just import transliteration by default...". That would simplify my suggested solution even more as the slugify replacement option would be rendered immediately redundant!

[brent-hoover]

I believe that all server-side instances were changed to just import transliteration since there is no point in lazy-loading on the server-side.

[alexandros84]

Ok. I ve noticed also that the issue with Promise.await has been resolved #4202 so I will test this and modify my suggestion accordingly, hopefully in the next 48 hours. Thank you!

[alexandros84]

@zenweasel based on our discussion I have created this PR #4381