Sonarpy 668

[andrey-tyukin-sonarsource]

Two differences to the original formulation of the ticket:

• Presense of CSRFProtect in flask applications is not checked across multiple files; For now, it's limited to a single file
• Decorators like @csrf_exempt are detected by their literal name, not by their resolved FQN.

The 'defaultSeverity' was missing in the automatically generated S5792.json, I've set it to 'Major' for now. Also, the tags were changed to lowercase in the same JSON file.

[andrey-tyukin-sonarsource]

In order to make the mapping of issues in the original test projects to the implementing methods a bit easier, here is the list of triples (original.py / extractedTest.py / implementingMethod):

original: django/mysite/views.py:25:@csrf_exempt # Sensitive
test:       django/djangoExempt.py
src:        decoratorCsrfExemptCheck

original: django/mysite/settings.noncompliant.py:50:] # Sensitive: django.middleware.csrf.CsrfViewMiddleware is missing
test:       django/settings.py
src:        djangoMiddlewareArrayCheck

original: flask/globalsensitive2.py:10:app = Flask(__name__) # Sensitive: CSRFProtect is missing
test:       flask/global.py
src:        improperlyConfiguredFlaskApp

original: flask/globalexemptroutesensitive1.py:21:@csrf.exempt # Sensitive
test:       flask/flaskExempt.py
src:        decoratorCsrfExemptCheck
// Note: the `csrf` variable is renamed there to `csrfProtect`, it shouldn't matter.

original: flask/flaskformsensitive1.py:18:        csrf = False # Sensitive
test:       flask/flaskform1.py
src:        metaCheck

original: flask/flaskformsensitive1.py:29:    form = TestForm(request.form, csrf_enabled=False) # Sensitive
test:       flask/flaskform1.py
src:        formInstantiationCheck

original: flask/globalsensitive1.py:14:app.config['WTF_CSRF_ENABLED'] = False # Sensitive: it overrides whatever configuration
test:       flask/wtfCsrfEnabledFalse.py
src:        flaskWtfCsrfEnabledFalseCheck

original: flask/globalexemptblueprintsensitive1.py:17:csrf.exempt(simple_page) # Sensitive
test:       flask/exemptAsFunction.py
src:        functionCsrfExemptCheck

original: flask/globalsensitive3.py:12:app.config['WTF_CSRF_CHECK_DEFAULT'] = False # Sensitive: it overrides whatever configuration
test:       flask/wtfCsrfEnabledFalse.py
src:        flaskWtfCsrfEnabledFalseCheck

[guillaume-dequenne]

Overall, I'd recommend using Tree#is to test the Kind of nodes rather than testing the instance.
The Kind is generally what we base our assumptions on (we sometimes use the instance but it's not the usual way) and is the strongest guarantee we have, as some different kinds might be linked to the same interface. I didn't flag every cast/test instance to avoid the noise, but I believe it might be worth to reconsider their usage nonetheless.

Also, while I wouldn't want to discourage you from using a functional style, I think some parts might be a bit more readable (at least due to consistency with the style used in the rest of the project) if written in a more idiomatic style (cf for instance the snippets I suggest).

[guillaume-dequenne]

You used the Python specific key here. You should be using the top-level RSPEC key instead (S4502). I believe that's why you had missing information in the generated metadata.

[andrey-tyukin-sonarsource]

Indeed, 4502 worked a bit better, but the tags still had to be converted to lowercase manually (otherwise, PythonRuleRepositoryTest complained about "Flask" and "Django")

[guillaume-dequenne]

That would mean it's an issue with the rule as defined in Jira: https://jira.sonarsource.com/browse/RSPEC-5792. I guess in those cases you can ping whoever created the RSPEC to notify the problem and update it (otherwise we would have to manually update each time we update metadata)

[guillaume-dequenne]

I'm not sure we need those as I'd prefer going for a test on Tree#is and then a direct cast rather than testing the possibility of a cast (as different Tree.Kind could have the same underlying interface).

[guillaume-dequenne]

Still a few comments/nitpicks, I'm not convinced we need the checkedCast methods and I think they can be replaced by something simpler. But I'd like your opinion @andrea-guarino-sonarsource on that because maybe they have their place in a util class as well.

[guillaume-dequenne]

Personally I think we shouldn't need those methods.

Why not use something like .filter(x -> x.is(Kind.Y)).map(Y.class::cast) instead of defining these methods? It feels more idiomatic.
At any rate if we're going to use those methods, I believe they should be in a util class where other checks will be able to benefit from them. What do you think @andrea-guarino-sonarsource ?

[andrey-tyukin-sonarsource]

Well... Optional.ofNullable(x).filter(x -> x.is(Kind.YK)).map(Y.class::cast) still looked a bit noisy, especially since it occurred over and over again in several places.

If the method definitions themselves are too distracting, I can inline them for now.

I think there is no need extracting them into an Util class, stuff like that is easy to introduce, but hard to get rid of. If anyone else wants to discuss how one might cut down the noise from the checks and casts, maybe we could collect some proposals at some later point.

[guillaume-dequenne]

Minor nitpick but I believe you can inline this.

[andrey-tyukin-sonarsource]

inlined

[guillaume-dequenne]

Do we need this method? I believe you could pass the predicate directly to isStringSatisfying, knowing the method is called only once.

[andrey-tyukin-sonarsource]

inlined

[guillaume-dequenne]

Minor nitpick but I believe the else is not needed since you return directly from the if anyway.

[guillaume-dequenne]

I think you can use .filter(l -> l.is(Tree.Kind.LIST_LITERAL)) instead.

[andrey-tyukin-sonarsource]

Ahh, true, missed that one.

[guillaume-dequenne]

Here as well I believe we can rely on Kind.

[sonarsource-next]

Kudos, SonarQube Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

99.5% Coverage
0.0% Duplication

[andrea-guarino-sonarsource]

LGTM