12
\$\begingroup\$

I am building a highlight feature for search results on my website and need to escape the user's input so that I could highlight the matching string within the original text.

The function

function sanitize_for_regex(s){
    var escaped = '';
    for(var i = 0; i < s.length; ++i){
        switch(s[i]){
            case '{':
            case '}':
            case '[':
            case ']':
            case '-':
            case '/':
            case '\\':
            case '(':
            case ')':
            case '*':
            case '+':
            case '?':
            case '.':
            case '^':
            case '$':
            case '|':
                escaped+= '\\';
            default:
                escaped+= s[i];
        }
    }
    return escaped;
}

How it's used

var input_from_user = 'Hey + wi$ll th<is \'" { tex|t / \\ mess w?>ith you?';

var highlighted_text = original_text.replace(new RegExp('('+sanitize_for_regex(input_from_user)+')', 'gi'), '<span style="background-color:#FBFB73;">$1</span>');

My test cases show that it's working quite well but I would like to get some feedback from other professionals.

Does this function look like it will escape all injection "attempts"? (I quoted attempts because the user is usually not aware of attempting to break anything)

Could the performance be improved?

\$\endgroup\$
  • 2
    \$\begingroup\$ Why not look at some prior work? This is what Google uses, and they're notoriously keen on performance... stackoverflow.com/a/18151038/3757232 \$\endgroup\$ – Jared Smith Jan 26 '17 at 22:13
  • \$\begingroup\$ @JaredSmith I've actually seen that answer. Unfortunately I am not a regex expert and as a rule-of-thumb you should never blindly use code which you do not understand. The accepted answer and comments clearly define what it's doing and why. If you could enlighten me as to why the Google one is different then I would consider using it. Feel free to add it as an answer, I will, at minimum, upvote it. Thanks. \$\endgroup\$ – MonkeyZeus Jan 27 '17 at 17:08
  • \$\begingroup\$ Its probably not worthy of an answer: all the first replace is doing is taking the list of regex special characters and adding a double backslash in front of any found in the string (the capturing group match is assigned to the automatic variable $1. There are comments on the answer that explain the second replace. And that's a good rule of thumb, but time spent deciphering regexes is the price you pay for getting good at them. \$\endgroup\$ – Jared Smith Jan 27 '17 at 18:44
  • \$\begingroup\$ @JaredSmith I think I already understood most of that so I should have phrased my question in this way "Why does the Google solution escape more characters than the answer provided by 200_success?" especially considering 200_success's explanation about the square brackets and dashes? In regards to optimization and ignoring the replace(/\x08/g, '\\x08') section, wouldn't the additional and potentially unnecessary regex matching cause a decrease in the performance which Google is so keen on achieving? \$\endgroup\$ – MonkeyZeus Jan 27 '17 at 20:34
  • \$\begingroup\$ Because a general purpose solution is better (more battle-tested) than a bunch of one-off ad hoc ones. Doing a one-off that only escapes the characters you're interested in for a particular use-case is the kind of thing you only do once profiling has shown the general purpose solution to be too slow. \$\endgroup\$ – Jared Smith Feb 2 '17 at 16:17
19
\$\begingroup\$

I'm not a fan of "sanitizing" data. There isn't a clear definition of what "sanitizing" means, other than that it takes untrustworthy input and somehow makes it valid. It might entail discarding the invalid parts of the input — which is not what you are doing here. Escaping is a clearer term to describe what you are doing.

Repeated string concatenation is considered poor practice for performance. Since JavaScript strings are immutable, it would be better to devise a solution that constructs the string "all at once".

The documentation on developer.mozilla.org suggests the following solution for escaping a regular expression:

Escaping user input to be treated as a literal string within a regular expression can be accomplished by simple replacement:

function escapeRegExp(string){
  return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string
}

I strongly recommend replacing your custom solution with the standard recipe (including a citation).

\$\endgroup\$
  • \$\begingroup\$ Oh wow, this is really excellent. I've come across other solutions like this on StackOverflow but for some reason I could not get it to work with my code; probably due to my regex inexperience. My code used to be a larger mess until I narrowed my vision for what I actually need it to accomplish. This works as a drop-in replacement for my function and I feel silly for not seeing the Mozilla docs sooner. Thanks a million! \$\endgroup\$ – MonkeyZeus Jan 26 '17 at 21:44
  • \$\begingroup\$ I think you forgot to add -(hyphen) in the character class. When used in character class, this need to escape or it may select range. \$\endgroup\$ – Tushar Jan 27 '17 at 3:20
  • \$\begingroup\$ @Tushar But character classes are simply not possible if the square brackets are already escaped. \$\endgroup\$ – 200_success Jan 27 '17 at 3:21
  • \$\begingroup\$ Aah. True. Didn't think about it. \$\endgroup\$ – Tushar Jan 27 '17 at 3:22
2
\$\begingroup\$

A couple options as far as escaping goes:

  1. You can use a regular expression that matches any character that needs escaped and replaces it with "\\&$", which is "A backslash, followed by whatever character matched.":

    // The inside regexp just replaces every character with a leading backslash for readability.
    // It can be optimized out just fine.
    
    var pattern = RegExp("[" + "{}[]-/\\()*+?.%$|".replace(RegExp(".", "g"), "\\$&") + "]", "g");
    
    function sanitize_for_regex(val) {
        return val.replace(pattern, "\\$&");
    }
    
  2. You can keep your current code somewhat as-is (but it's relatively inefficient), but checking a string rather than a bunch of case:

    function sanitize_for_regex(s) {
        var escaped;
        for(var i = 0; i < s.length; ++i){
            if("{}[]-/\\()*+?.%$|".indexOf(s[i]) !== -1) {
                escaped += "\\";
            }
            escaped += s[i];
        }
        return escaped;
    }
    

As for what needs to be escaped, you're probably matching far more than you need to -- many of those symbols only have meaning when paired with other symbols that you're already escaping, thus:

  • It's not neccessary to escape - or ^, because you're already escaping [ and ] and thus -- unless you're using the input inside of that part of a pattern, they'll never be treated as special characters.
  • It's technically not necessary to escape ], because the [ that would begin that part of the pattern is already escaped.
  • If you're only using the output inside of a pattern (as opposed to inside of a replacement), I believe the only things that really need escaping are the backslash (\), wildcards and the start of character classes (. and [), quantifiers (?, +, * and {), parenthesis (( and )) and branching expressions (|).
  • So, in short, the list of characters to escape are ()[.?+*{|.

This may vary a bit depending on your usage though.

\$\endgroup\$
  • \$\begingroup\$ Great suggestions, I'm really learning quite a bit! I just facepalmed pretty hard because I did not think of the string and indexOf() approach. Since this is targeted for use upon hitting a key and the search terms should never exceed more than 10 characters I think it will be safe to use the indexOf() approach. Even with 1,000 chars in the search term there is no slow down. Thanks! \$\endgroup\$ – MonkeyZeus Jan 26 '17 at 19:40
  • \$\begingroup\$ @MonkeyZeus The regex approach is likely faster (it's not constructing a string one character at a time), but I listed the other to show one that was closer to your original approach. \$\endgroup\$ – dewin Jan 26 '17 at 19:42
  • \$\begingroup\$ I decided to construct the string outside of the for(){} loop's scope and assign it to a variable named escape_chars. My for(){} loop looks like this on the inside escaped_val+= (escape_chars.indexOf(s[i]) !== -1 ? '\\' : '')+s[i]; I am choosing to avoid regex because I cannot read it very well so readable code trumps uber-optimization in this situation. \$\endgroup\$ – MonkeyZeus Jan 26 '17 at 19:45
1
\$\begingroup\$

This is not a big performance deal, yet you could use a "hashset" instead of going through the cases:

var dispatch = {
    '{':  true,
    '}':  true,
    '[':  true,
    ']':  true,
    '-':  true,
    '/':  true,
    '\\': true,
    '(':  true,
    ')':  true,
    '*':  true,
    '+':  true,
    '?':  true,
    '.':  true,
    '^':  true,
    '$':  true,
    '|':  true
};

function sanitize_for_regex2(val) {
    var escaped = "";

    for (var i = 0; i !== val.length; ++i) {
        if (dispatch[val[i]]) {
            escaped += "\\";
        }

        escaped += val[i];
    }

    return escaped;
}

alert(sanitize_for_regex2("{yeah}"));
alert(sanitize_for_regex("{yeah}"));
\$\endgroup\$
  • \$\begingroup\$ This will certainly clean up my function but I hesitate to put function-specific variables outside of the function's scope. Do you think to would be a huge penalty to declare the hash every time the function is called? Any ideas as to whether or not I've considered all possible regex injection points? \$\endgroup\$ – MonkeyZeus Jan 26 '17 at 18:54
  • \$\begingroup\$ @MonkeyZeus The answer to your 1st question: it would seem the hash will be created every time the function is called. Maybe closures may help. 2nd one: I really don't know (I am not a professional JavaScript developer). :-( \$\endgroup\$ – coderodde Jan 26 '17 at 18:59
  • \$\begingroup\$ I appreciate the insight, this definitely gives me something mull over, thanks! \$\endgroup\$ – MonkeyZeus Jan 26 '17 at 19:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.