Try and deal with CodeQL reports on replace("*", ...)

[jakebailey]

We constantly have to dismiss this warning: https://github.com/microsoft/TypeScript/security/code-scanning/206

But, downstream users are now seeing this if they happen to vendor our code. There must be something we can do to make the code scanner not complain anymore.

For now, I'm moving this out to a helper to make sure CodeQL is still mad, then will experiment later.

Using the replace method directly has tricked CodeQL. Maybe that is enough.

We already bypassed this elsewhere using an arrow function, so I've done that instead.

[fatcerberus]

FYI: the link in the OP 404s.

[RyanCavanaugh]

Context: CodeQL really doesn't like that we only replace the first * in a string, since it looks like a mistake

[fatcerberus]

Can you pass a (non-global) regex instead of a string to get rid of the warning?

[RyanCavanaugh]

Surprisingly, it's slower

[jakebailey]

The analyzer also detects non-global regex replacements, so it's moot.

IndexOf and slicing also would work but any of these are plenty fast as to not matter.

[fatcerberus]

The analyzer also detects non-global regex replacements, so it's moot.

:-\

Like, I get that this is a common mistake people make in JS but it would be nice if they provided a way to say, yes, I really do want the documented behavior here…

This is why I can’t do linters.

[jakebailey]

This is why I can’t do linters.

The thing is that nobody don't lints node_modules or vendored deps, and yet here we are...

[jakebailey]

I ran codeql locally to see what all is left after this PR. The only other remaining bad code is:

stripQuotes(quoted).replace(/'/g, "\\'").replace(/\\"/g, '"')

Where it thinks that we should also be escaping \ in the input, when in actuality we're just swapping one quote for another.

I would also like to fix this one (so our codebase is clean), but this makes me wonder if it'd be better to instead just export stringReplace as a function and use it everywhere instead, at the risk of them eventually detecting the pattern.

I'm also noticing that we bypassed this detection in another way:

/** @internal */
export function escapeSnippetText(text: string): string {
    return text.replace(/\$/gm, () => "\\$");
}

If you use a replacer function, it doesn't care. Introduced in #46716. This would be easier for the one-off quote thing, but I'm curious if anyone has a preference to whether I just use this trick too in the other replacements. @andrewbranch do you have a preference?

[andrewbranch]

I do not have a preference.

[jakebailey]

Well, that didn't work. I guess the arrow thing only works for the backslash test. Mistake for uploading it assuming that would work, bah.

[jakebailey]

Alright, settling on the new function for the star thing, plus the arrow function which mirrors the backslashing error we handle in snippets in the same way. Which is to say the original PR but with the one new arrow function 😄