SONARKT-569 Add support for WebViews to S4830

[antonioaversa]

SONARKT-569

[egon-okerman-sonarsource]

Educational question: does this cover all branching?

[antonioaversa]

Yeah, good question - the answer is: all the ones I could think of :-)
I find unlikely that somebody will find a way to have branching, have proceed and not have any cancel, in other ways... but I would not underestimate people creativity :-)
Anyway, unless I am missing some common scenarios, the risk for FP seems close to zero to me.
If we are concerned about this heuristic we can always remove it go for the Option 3 we discussed (only simple proceed in body), but that seems too much on the side of safety, at the expense of reach.
WDYT?

[leveretka]

Nice work! I see we can have a few improvements here.

[leveretka]

Let's move matchers to the file level. Otherwise, we create them every time AndroidWebViewCandidateVisitor is instantiated

[antonioaversa]

I wanted to keep them local to the context where they are relevant. I should have probably created a private companion object, but maybe it's too much here. So I have moved them to the top of the file, as suggested.

[leveretka]

here we always try to match, both proceed and cancel and always override the foundProceedCall and foundCancelCall.

It seems to me we can inline

Suggested change

	val isProceedCall = androidSslErrorHandlerProceedFunMatcher.matches(expression)
	val isCancelCall = androidSslErrorHandlerCancelFunMatcher.matches(expression)
	foundProceedCall = foundProceedCall || isProceedCall
	foundCancelCall = foundCancelCall || isCancelCall
	foundProceedCall = foundProceedCall || androidSslErrorHandlerProceedFunMatcher.matches(expression)
	foundCancelCall = foundCancelCall || androidSslErrorHandlerCancelFunMatcher.matches(expression)

[leveretka]

Also since these calls can't be matched together, we can refactor to only match one. I'll let you decide how is better to refactor it.

[leveretka]

That's FYI, Looks like for this rule we implemented recursive visit function by ourselves, however, there's a KtTreeVisitorVoid, which is recursive. No need to refactor this check, just FYI for the future rules.

[antonioaversa]

I have changed the code to useKtTreeVisitorVoid everywhere, so that I could remove the logic to traverse descendants. Except for the call to super, it was a drop-in replacement.

[leveretka]

Here we match function to both FunMatchers regardless of the result. I'd suggest extracting refactoring the code so we don't match function a second time if we know it was already matched to something else. As an idea it can look like this:

when {
            javaCryptographyExtensionFunMatchers.any { it.matches(function) } ->
                if (!function.callsCheckTrusted() && !function.throwsCertificateExceptionWithoutCatching())
                    kotlinFileContext.reportIssue(function.nameIdentifier ?: function,
                        "Enable server certificate validation on this SSL/TLS connection.")
            androidWebViewFunMatchers.any { it.matches(function) } ->
                if (function.callsProceedUnconditionally()) {
                    kotlinFileContext.reportIssue(function.nameIdentifier ?: function,
                        "Enable server certificate validation on this SSL/TLS connection.")
                }
        }

[antonioaversa]

Indeed, that's unnecessary computation.
I came up with something slightly different to avoid repeating reporting logic twice:

    override fun visitNamedFunction(function: KtNamedFunction, kotlinFileContext: KotlinFileContext) {
        val match = when {
            javaCryptographyExtensionFunMatchers.any { it.matches(function) } ->
                !function.callsCheckTrusted() && !function.throwsCertificateExceptionWithoutCatching()
            androidWebViewFunMatchers.any { it.matches(function) } ->
                function.callsProceedUnconditionally()
            else -> false
        }
        if (match) {
            kotlinFileContext.reportIssue(function.nameIdentifier ?: function,
                "Enable server certificate validation on this SSL/TLS connection.")
        }
    }

[leveretka]

That's more a matter of taste, what do you think about this way:

with(visitor) { foundProceedCall && !foundCancelCall && !potentialBranching }

[antonioaversa]

Yes, nice! I used it on the other visitor as well, for consistency.

[leveretka]

Let's maybe keep this Kdoc

[antonioaversa]

Sure, I removed it because it seemed redundant - it basically says the same thing as the name of the function, without any further detail.

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube