JS: library for recognizing startsWith, includes, endsWith#735
Merged
Conversation
Contributor
Author
|
Evaluation. I'll look into the performance of node and a few others. |
xiemaisi
reviewed
Jan 14, 2019
xiemaisi
left a comment
There was a problem hiding this comment.
LGTM, modulo minor typos and evaluation.
Contributor
Author
|
After another evaluation, perf looks good. |
|
Conflicts. |
1cd3aa1 to
cf3dfca
Compare
Contributor
Author
|
This should be ready to go. |
Contributor
Author
|
Looks like my rebasing eliminated the inline fixes from the review. Should be fixed now. |
xiemaisi
suggested changes
Jan 25, 2019
xiemaisi
left a comment
There was a problem hiding this comment.
Two more minor documentation niggles, otherwise lgtm.
| /** | ||
| * Gets the polarity if the check. | ||
| * | ||
| * If the polarity is `false` the check returns `true` if the string does not start |
|
|
||
| /** | ||
| * An expression that appears to be part of an `endsWith`-check, that is, | ||
| * roughly equivalent to `A.endsWith(B)` or `!A.endsWith(B)`. |
Contributor
Author
There was a problem hiding this comment.
Ah, good catch. I wrote this before I decided to exclude expressions that aren't strictly equivalent to endsWith, such as A.substr(-B.length) === B. I've updated the comment.
xiemaisi
approved these changes
Jan 25, 2019
cklin
pushed a commit
that referenced
this pull request
May 20, 2022
Add change note announcing generics support
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds the classes
StringOps::StartsWith,StringOps::EndsWith, andStringOps::Includesfor recognizing calls to the corresponding methods and ad-hoc implementations of these.The former two are needed for some improvements to the tainted path query. I don't have an immediately use for the
endsWithvariant, but it's just there for the sake of completeness.There are a few nags to this approach:
indexOfandincludesare also array methods.A.indexOf(B) > 1implies thatA.includes(B), but the false outcome does not imply!A.includes(B). The current implementation is conservative in this regard and only recognizes instances that are truly equivalent to startsWith/includes/endsWith.EndsWithis hard to implement. For example,A.substr(-B.length) === Bis almost a correctendsWithcheck, except it returnsfalseifBis empty. Some security queries might still want to treat it as an alias forA.endsWith(B), though.One day we might want to generalise this to reason about one-sided tests and "nearly correct" tests, but it might just turn into a time sink. For now, this is at least sufficient for the tainted path query improvements I'm working on.
I'm running an evaluation of all security queries due to change to sanitizers.