Conversation
|
|
||
| class TestSonarExceptionWithoutRaise(BaseIntegrationTest): | ||
| codemod = ExceptionWithoutRaise | ||
| codemod = SonarExceptionWithoutRaise |
There was a problem hiding this comment.
I realized this int test was importing the wrong codemod
| pos_to_match = self.node_position(node) | ||
| if self.results is None: | ||
| # Some codemods must run without results existing. | ||
| return True |
There was a problem hiding this comment.
I documented this because it was strange to return True here (do we filter by result or not filter by result?). Then I realized the logic here is that this method is used for all codemods - those with and those without results. We still want to analyze codemods even if there are no results. Maybe later on we can have a filter_by_result that's more relevant. The one I implemented for jwt sonar returns False here, since there should always be results if we want to run the codemod.
There was a problem hiding this comment.
Yes I encountered this too. We're using None as a sentinel for codemods that do not have detectors, and we do not want to short-circuit those cases. If you don't mind, could you update the comment to say something along those lines?
| same_line(pos, location) and fuzzy_column_match(pos, location) | ||
| for location in result.locations | ||
| ) |
There was a problem hiding this comment.
Did you encounter any examples where fuzzy_column_match is needed? From my observations most nodes reported by sonar would match libcst ones with one exception: Tuples. If you look at the match_location from SonarResult you can see the exception treated there. We should discuss how to treat location match at some point.
There was a problem hiding this comment.
You're going to have to give me an example because adding fuzzy_column_match is the entire point of this PR. As I stated in the PR description, sonar returns index for the keyword=value, while we need jwt.decode(...keyword=value). Unit tests have exact sonar results, too. So inspect those as well.
Or perhaps Im' not understanding your comment?
There was a problem hiding this comment.
In the simplest terms, does anything break if you remove fuzzy_column_match and replace it by exact match?
If so, does it break consistently? (e.g. always with the same type of node)
There was a problem hiding this comment.
Yes, there are no location matches without fuzzy matching.
drdavella
left a comment
drdavella
left a comment
There was a problem hiding this comment.
Looks great! Just a few smallish comments.
| results returned have a start/end column for the verify keyword | ||
| within the `decode` call, not for the entire call like semgrep returns. | ||
| """ | ||
| if self.results is None: |
There was a problem hiding this comment.
I believe this will actually never occur in the Sonar case and you can remove this.
There was a problem hiding this comment.
yep very good point. I was trying to keep this method as similar to its base one, but you're right it's nonsensical in this case.
| pos_to_match = self.node_position(node) | ||
| if self.results is None: | ||
| # Some codemods must run without results existing. | ||
| return True |
There was a problem hiding this comment.
Yes I encountered this too. We're using None as a sentinel for codemods that do not have detectors, and we do not want to short-circuit those cases. If you don't mind, could you update the comment to say something along those lines?
| """ | ||
| if self.results is None: | ||
| return False | ||
| match node: |
There was a problem hiding this comment.
This solution makes sense to me and it works for now. In the longer term, I think we should filter the Sonar/SAST results before they ever reach the transformer. This means we would add some logic to each detector that requires it. This would mean implementing some kind of visitor that validates each of the detected locations before passing the results to the transformer. There's a bit of a performance impact in this case since it effectively means another pass on the file, but it only applies to files where there are already results.
I'm saying this not because I think we need this change right now but because I've encountered similar issues with remediating another SAST tool and I think that updating the filtering logic here for each case is going to become cumbersome.
There was a problem hiding this comment.
Very interesting! The detector for this particular codemod is semgrep so that's also a little wrinkle.
|
3 participants
Overview
Add codemod support for the sonar rule for jwt.decode
Description
decodecall node column. Sonar returns column start/end for....verify=Falseor...."verify_signature": Truewhile the semgrep node we care about isjwt.decodewhich contains these verify keywords. To get the codemod to run, I had to implement a codemod specificmatch_locationthat I calledfuzzy.Additional Details
Closes #298