Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Sep 5, 2019

No description provided.

@ghost ghost added the JS label Sep 5, 2019
@ghost ghost self-requested a review as a code owner September 5, 2019 08:49
@xiemaisi
Copy link

xiemaisi commented Sep 5, 2019

I assume you have verified that this doesn't lose any interesting results? I would have thought that removing quotes via replace is wrong if you can have escaped quotes inside the string.

@ghost
Copy link
Author

ghost commented Sep 5, 2019

I would have thought that removing quotes via replace is wrong

I do not think it is any less wrong than trying to remove other delimiter pairs this way.

The pattern is rare though: 0 results for 236 projects (https://lgtm.com/query/5904476558374725002/).

@xiemaisi
Copy link

xiemaisi commented Sep 5, 2019

I do not think it is any less wrong than trying to remove other delimiter pairs this way.

Braces are less frequently backslash-escaped, no?

@xiemaisi
Copy link

xiemaisi commented Sep 5, 2019

But good to hear that it doesn't happen in practice, so sounds fine to whitelist.

@semmle-qlci semmle-qlci merged commit 16c95d8 into github:master Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants