SONARKT-569 Modify rule S4830: add support for WebViews #4673
Conversation
| == Resources | ||
|
|
||
| include::../common/resources/standards.adoc[] | ||
|
|
||
| * https://wiki.sei.cmu.edu/confluence/display/java/MSC61-J.+Do+not+use+insecure+or+weak+cryptographic+algorithms | ||
| * CERT - https://wiki.sei.cmu.edu/confluence/display/java/MSC61-J.+Do+not+use+insecure+or+weak+cryptographic+algorithms | ||
| * Android Documentation - https://developer.android.com/reference/android/webkit/WebViewClient?hl=en#onReceivedSslError(android.webkit.WebView,%20android.webkit.SslErrorHandler,%20android.net.http.SslError)[WebViewClient.onReceivedSslError] method |
There was a problem hiding this comment.
Please also link https://support.google.com/faqs/answer/7071387?hl=en which is Google's official remediation
|
|
||
| === Noncompliant code example | ||
|
|
||
| [source,kotlin] |
There was a problem hiding this comment.
Interestingly, I can see that the Java and Kotlin documentation was never updated to fully support LaYC. You can look at Docker as an example of the recommended structure.
The source tag needs extra information to support the diff view correctly (and we need to go back and fix this in the other parts of S4830 too).
| [source,kotlin] | |
| [source,kotlin,diff-id=101,diff-type=noncompliant] |
(diff-id cannot collide with any of the IDs in other languages, which is why I put 101 here.)
|
|
||
| include::../../common/fix/validation.adoc[] | ||
|
|
||
| ==== Implementing a server certificate validation |
There was a problem hiding this comment.
Like the comment above, to follow the format correctly, the title here should be renamed
| ==== Implementing a server certificate validation | |
| ==== Compliant solution |
and it should be moved above "How to fix it". We generally keep the structure here of:
- Noncompliant code sample + short explanation if necessary
- Compliant code sample + short explanation if necessary
- Longer explanation and possible pitfalls
The text below and code sample are fine otherwise.
|
|
||
| Alternatively, you need to implement a validation of the server certificate received in the `SslErrorHandler` object, calling `proceed` and `cancel` appropriately. | ||
|
|
||
| [source,kotlin] |
There was a problem hiding this comment.
| [source,kotlin] | |
| [source,kotlin,diff-id=101,diff-type=compliant] |
|
|
||
| ==== Implementing a server certificate validation | ||
|
|
||
| Alternatively, you need to implement a validation of the server certificate received in the `SslErrorHandler` object, calling `proceed` and `cancel` appropriately. |
There was a problem hiding this comment.
This sentence probably needs a rewrite considering it will be above the common fix text.
There was a problem hiding this comment.
Yes, I have removed the "Alternatively" and moved above the entire section, so that the two code examples and next to each other, and it makes more sense for them to be in diff with each other.
As suggested, I have adapted the "How to fix section" to have both compliant and non-compliant code examples, to have a coherent experience with other languages such as docker. In order to match that, I have moved the Let me know if that looks better for you. Regarding LaYC: the migration of this rule for Kotlin was done in #2176
The optionality also applies to "How does this work", and any other level 3 section. Given the above, I think that the way Java was done looks good to me: the non-compliant code example doesn't have a corresponding compliant, since all the proposed solutions for Java are ways to not use a custom trust manager at all, so I don't think we should bend the structure to meet a LaYC layout which is defined as optional, and proposed as an example of how things should look. If anything I would remove the |
antonioaversa
force-pushed
the
antonio/SONARKT-569-S4830-add-support-for-webviews
branch
from
d604804 to
b11fd7a
Compare
You're right, my previous comment was wrong. As the recommended fix is omitting a trust manager, it makes sense for there only to be one code sample instead of two in the JCE case. For Android, this is different, so I think your fix makes sense here as well. |
Co-authored-by: Egon Okerman <[email protected]>
Co-authored-by: Egon Okerman <[email protected]>
|
|
SONARKT-569
Review
A dedicated reviewer checked the rule description successfully for: