Feature 345 summary in reports #2156

zigfridus · 2023-04-10T13:16:02Z

This PR will close #345

Jeeppler

Overall you are on the right track, but you need to build up the data structure for the summary in the HTMLScanResultReportModelBuilder. See comments.

sechub-scan/src/main/resources/templates/report/html/scanresult.html

Jeeppler · 2023-04-13T16:29:39Z

sechub-scan/src/main/resources/templates/report/html/scanresult.html

+						<tr>
+							<td>Red</td>
+							<td>
+								<span th:if="!${redList.isEmpty()}"><a href="#redFindingsBlock" th:text="${#lists.size(redList)}"></a></span>


The id should be: #scanTypeRedFindingsBlock

Jeeppler · 2023-04-13T16:31:44Z

sechub-scan/src/main/resources/templates/report/html/scanresult.html

 			<div th:if="!${redList.isEmpty()}">
-				<h2 class='redFindingHeadline'>Red findings</h2>
+				<h2 class='redFindingHeadline' id="redFindingsBlock">Red findings</h2>
 				<table class='findingsTable redFindingsTable'>


There should be a new table for every scanType + redList.

Jeeppler · 2023-04-13T16:33:32Z

sechub-scan/src/main/resources/templates/report/html/scanresult.html

 			<div th:if="!${redList.isEmpty()}">
-				<h2 class='redFindingHeadline'>Red findings</h2>
+				<h2 class='redFindingHeadline' id="redFindingsBlock">Red findings</h2>


Each finding table needs a separate heading.
For example:

<h3 class='redFindingHeadline' id="codeScanRedFindingsBlock">Code Scan</a>

zigfridus · 2023-04-17T11:07:04Z

@Jeeppler
Some quick questions.

What does Secret Scan do? I haven't met this type of scan in SecHub before.
It seems to me it's neccessary to significantly change the structure of model in HTMLScanResultReportModelBuilder. Do you approve this?

Jeeppler · 2023-04-17T13:51:18Z

@zigfridus

Secret Scan is looking through code or repositories and tries to find hard coded secrets and credentials. This is a common problem. For example, with AWS credentials: https://github.com/ramimac/aws-customer-security-incidents. The secret scan module is fairly new. It was introduced a few weeks ago.
It seems to me it's necessary to significantly change the structure of model in HTMLScanResultReportModelBuilder. Do you approve this? - Yes, go ahead and do the necessary changes.

zigfridus · 2023-04-24T14:37:36Z

@Jeeppler
I would like to ask, may I reorganize the summary table in the way like on this image.
image
This will greatly simplify for me the summary table creation?

Jeeppler · 2023-04-25T13:30:32Z

@zigfridus yes, that is a good idea

Jeeppler · 2023-04-25T17:36:51Z

@zigfridus please add the image of the table from the Google drive into this issue here. I would like to have the documentation of this issue in one place.

zigfridus · 2023-04-25T17:59:47Z

@Jeeppler
Proposition of transposition of the Summary table:

…igfridus/sechub into feature-345-summary-in-reports

zigfridus · 2023-04-27T13:46:31Z

@Jeeppler
Please have a look at my last commit with a draft summary table.
Thanks

Jeeppler

Outstanding work. I love how readable the code is.

I found some smaller issues. Please change those.

Still there are three major things I am missing:

Tests (there should be a test for the ScanTypeCount class and extend HTMLScanResultReportModelBuilderTest)
A list of CWEs found (see: Summary in Reports #345 (comment))
The JSON report should contain the same summary (see: Summary in Reports #345 (comment))

You are clearly on the right track. I know the issue is challenging.

sechub-scan/src/main/java/com/mercedesbenz/sechub/domain/scan/ScanTypeCount.java

...scan/src/main/java/com/mercedesbenz/sechub/domain/scan/HTMLScanResultReportModelBuilder.java

- changed model's scanTypeCount List to Set in the HTMLScanResultReportModelBuilder; - added tests in the HTMLScanResultReportModelBuilderTest; - cleaned and added improvements in the ScanTypeCount; - created tests for the ScanTypeCount; - modified scanresult.html because of new set from the HTMLScanResultReportModelBuilder.

…edes-benz#345

zigfridus · 2023-05-26T07:39:13Z

@Jeeppler
Hello
Please have a look at my last commit. I added the neccessary sections into the JSON report.
I didn't create tests yet. I will create them if you are ok with my improvements.

Jeeppler

Looks pretty good. However, I did found some issues. For details have a look at the comments.

Please include a screenshot with your next pull-request review request (only as comment in the pull-request conversation).

sechub-commons-model/src/main/java/com/mercedesbenz/sechub/commons/model/SecHubReportScan.java

...mmons-model/src/main/java/com/mercedesbenz/sechub/commons/model/SecHubReportScanDetails.java

...b-commons-model/src/main/java/com/mercedesbenz/sechub/commons/model/SecHubReportSummary.java

sechub-scan/src/test/java/com/mercedesbenz/sechub/domain/scan/ScanTypeCountTest.java

zigfridus · 2023-05-29T07:54:14Z

@Jeeppler
Could you please explain for me this your sentence:
"Please include a screenshot with your next pull-request review request (only as comment in the pull-request conversation)."
I don't clearly understand it.

Jeeppler · 2023-06-01T13:39:16Z

@zigfridus It would be nice to have a screenshot of the report with the summary once you request a review. The simply reason is, that you are working on the HTML report and it is always a good idea to show case the visual changes.

Simply include the screenshot as part of this conversation.

zigfridus · 2023-07-05T12:51:49Z

Hello @Jeeppler
According to the test data in the example3_coverity.sarif.json the finding with the same CWE ID (for example CWE-352) could have both HIGH and MEDIUM severity at the same time. Is it possible or is it a mistake in the test data?

Jeeppler · 2023-07-05T19:19:19Z

@zigfridus yes, they could both have different severity levels even for the same CWE, depending on how the tool calculates the severity level (e, g. based on a Control Flow Graph). It is certainly an edge case.

In addition, some tools produce findings with a CWE-0. The problem, a CWE-0 does not exist.

In the real world there are a lot of oddities, quirks and bugs. Tests might help to check for those. However, I do expect that over time we will uncover some situations/combinations you/we did not account for. That is fine.

zigfridus · 2023-07-06T06:44:23Z

@Jeeppler Thank you for the explanation.

zigfridus · 2023-07-06T14:51:58Z

@Jeeppler Finally I have some result that I can show you. These are 2 reports, generated by the modified code.

example 1:

example 3:

Please have a look at the html files in the ZIP:
examples.zip

I didn't find at your example a details block: Should it exist in my report too?

Jeeppler · 2023-07-07T09:38:08Z

@zigfridus Thanks, I know it takes time. Overall, it look very nice. Good job. 🥇

The details block should exist in your report as well. I am surprised, that it does not exist in your code.

Regarding the tables. I think if they look this they would be easier to read:

CodeScan

High

CWE         | Name                                                      | Count
-------------------------------------------------------------------------
CWE-543  | Bad choice of lock object                                    | 1
CWE-352  | Cross-site request forgery                                   | 1
CWE-79    | Cross-site scripting                                        | 131
CWE-22    | Filesystem path, filename, or URI manipulation              | 5
…

In addition, it would be nice to use the <details> - HTML tag for the table with CWE numbers.

Closed:

> SecretScan
> CodeScan
…

CodeScan open:

> SecretScan
v CodeScan

High

CWE         | Name                                                      | Count
-------------------------------------------------------------------------
CWE-543  | Bad choice of lock object                                    | 1
CWE-352  | Cross-site request forgery                                   | 1
CWE-79    | Cross-site scripting                                        | 131
CWE-22    | Filesystem path, filename, or URI manipulation              | 5
…

zigfridus · 2023-07-11T14:17:29Z

@Jeeppler
Please have a look at my improvements:
examples.zip

Added the block of details in description.
Designed summary blocks as tables.
Add possibility to collapse summary blocks.

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

zigfridus · 2023-11-02T20:19:53Z

@Jeeppler
I amended my code according to your recommendations. After that it was built successfully. Please have a look.

Jeeppler

Looks good too me 👍

CLA-Mercedes-Benz · 2023-11-14T15:15:31Z

All committers have signed the CLA.

de-jcup

Thank you for your contribution.
I found some parts I want to have been changed. Especially the report for web scans is not correct rendering (JSON and HTML). Please look at the comments for more details.

...s-model/src/main/java/com/mercedesbenz/sechub/commons/model/SecHubReportMetaDataSummary.java

de-jcup · 2023-11-21T09:55:08Z

...b-doc/src/test/java/com/mercedesbenz/sechub/restdoc/ScanReportRestControllerRestDocTest.java

@@ -241,7 +238,8 @@ public void before() throws Exception {
        map.put("yellowList", new ArrayList<>());
        map.put("greenList", new ArrayList<>());
        map.put("isWebDesignMode", false);
-        map.put("metaData", Optional.ofNullable(null));
+        map.put("metaData", null);


When you do a
map.get("metadata") and you have not defined it, it will always be null

Please remove the line.

@de-jcup It seems to me I have already removed it in this commit or am I wrong?

...scan/src/main/java/com/mercedesbenz/sechub/domain/scan/HTMLScanResultReportModelBuilder.java

sechub-scan/src/main/java/com/mercedesbenz/sechub/domain/scan/HTMLWebSecHubFinding.java

...scan/src/main/java/com/mercedesbenz/sechub/domain/scan/HTMLScanResultReportModelBuilder.java

sechub-scan/src/main/java/com/mercedesbenz/sechub/domain/scan/report/ScanSecHubReport.java

sechub-test/src/test/java/com/mercedesbenz/sechub/test/report/ThymeLeafHTMLReportingTest.java

de-jcup · 2023-11-22T09:48:06Z

...com/mercedesbenz/sechub/integrationtest/scenario10/PDSCodeScanSarifJobScenario10IntTest.java

-            hasMetaDataLabel("test-label2", "").
-            hasMetaDataLabel("test-label3_with_html", "<html>HTML is allowed, but must always be escaped in reports!</html>").
-            hasMetaDataLabel("test-label4_with_special_chars", "Line1\nLine2\tLine3").
+            hasMetaDataSummaryCodeScanTotal(32).


You added your new tests - okay (of course)
But you dropped the existing meta data label tests. Why?

We need these tests to ensure that the labels are inside the report - it is an important version.

Please add the former meta data label checks/tests again.

Unfortunately I didn't find any of these labels in the report that should be tested:
{"metaData":{"labels":{},"summary":
Should they exist here?

de-jcup · 2023-11-22T09:48:16Z

...com/mercedesbenz/sechub/integrationtest/scenario10/PDSCodeScanSarifJobScenario10IntTest.java

-            hasMetaDataLabel("test-label2", "").
-            hasMetaDataLabel("test-label3_with_html", "&lt;html&gt;HTML is allowed, but must always be escaped in reports!&lt;/html&gt;").
-            hasMetaDataLabel("test-label4_with_special_chars", "Line1\nLine2\tLine3");
+            hasHTMLString("<td><a href=\"#redCodeScanTable\">28</a></td>").


You added your new tests - okay (of course)
But you dropped the existing meta data label tests. Why?

We need these tests to ensure that the labels are inside the report - it is an important version.

Please add the former meta data label checks/tests again.

Unfortunately I didn't find any of these labels in the report that should be tested:
{"metaData":{"labels":{},"summary":
Should they exist here?

…ctly in the reports. mercedes-benz#345

de-jcup · 2023-11-27T09:02:38Z

@zigfridus : I am happy to see that you have done some new commits/pushes. When your changes are complete, please re-request a review from me by GitHub UI. Thank you.

zigfridus · 2023-11-28T20:45:09Z

@de-jcup
I fixed some issues you found. Please have a look at my comments for unresolved conversations. I would like to have some clarifications. Thank you.

de-jcup · 2023-12-19T12:30:27Z

@zigfridus : Sorry (again) for the late response

With

Provide a community branch and describe it inside contributions #2759

we introduced a "community" branch.

Please do me a favor and change your PR target from develop to community.
After this, I will merge the PR to community branch, do some additions and after this
merge it finally to develop

We try to install this as the default mechanism - to speed up merges, reduce frustration at contributor side, see build failures before merging to develop (main) branch etc. (see #2759 for details).

So... this is like a test balloon, but I hope this works well in future for other contributions/contributors as well.

Thank you for your patience, your contribution and your perseverance regarding this long ongoing PR...

^{Remark: With #2599 I fixed the sorting of the reduced open api file. Please merge the develop branch into your branch and get rid of the merge conflict (community branch is at same level as develop branch)}

zigfridus · 2023-12-19T12:41:49Z

@de-jcup
I changed this branch to community.

Jeeppler · 2023-12-19T12:49:12Z

@zigfridus awesome, thank you.

zigfridus · 2023-12-19T13:14:43Z

@Jeeppler
I changed the branch here. Could you please advice how to change destination branch in my fork?

Jeeppler · 2023-12-21T08:19:22Z

@zigfridus I think the best way to solve the problem is to close this pull-request (2156) and create a new pull-request fork/branch with the target branch mercedes-benz:community.

To not loos all the discussions from this conversation, you can simply link/mention this pull-request in your new pull-request.

zigfridus added 2 commits April 10, 2023 14:28

Add summary table mercedes-benz#345

5100a67

Add possibility to click on results in the summary table mercedes-ben…

1899d25

…z#345

Jeeppler suggested changes Apr 13, 2023

View reviewed changes

zigfridus added 3 commits April 27, 2023 16:05

The draft summary table in a HTML report mercedes-benz#345

d4d1a83

Merge branch 'mercedes-benz:develop' into feature-345-summary-in-reports

e120a14

Merge branch 'feature-345-summary-in-reports' of https://github.com/z…

6b4f0a3

…igfridus/sechub into feature-345-summary-in-reports

Jeeppler suggested changes Apr 27, 2023

View reviewed changes

zigfridus added 5 commits May 16, 2023 14:35

Merge branch 'mercedes-benz:develop' into feature-345-summary-in-reports

8d6f949

Added summary section into report mercedes-benz#345

3b7d863

Merge branch 'mercedes-benz:develop' into feature-345-summary-in-reports

8e73dc2

Added a details section the child of summary section into report merc…

70ea329

…edes-benz#345

zigfridus requested a review from Jeeppler May 26, 2023 07:39

Jeeppler suggested changes May 26, 2023

View reviewed changes

zigfridus and others added 8 commits November 2, 2023 19:55

Update sechub-commons-model/src/test/java/com/mercedesbenz/sechub/com…

8d49cc8

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

Update sechub-commons-model/src/test/java/com/mercedesbenz/sechub/com…

a9bdd22

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

Update sechub-commons-model/src/test/java/com/mercedesbenz/sechub/com…

d1295a9

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

Update sechub-commons-model/src/test/java/com/mercedesbenz/sechub/com…

a9e8bdb

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

Update sechub-commons-model/src/test/java/com/mercedesbenz/sechub/com…

708ee95

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

Update sechub-commons-model/src/test/java/com/mercedesbenz/sechub/com…

98e6464

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

Update sechub-commons-model/src/test/java/com/mercedesbenz/sechub/com…

2888d06

…mons/model/SecHubReportMetaDataSummaryTest.java Co-authored-by: Jeeppler <Jeeppler@users.noreply.github.com>

Minor modifications in some tests mercedes-benz#345

02fdfa5

Jeeppler approved these changes Nov 3, 2023

View reviewed changes

de-jcup mentioned this pull request Nov 17, 2023

Little changes in SecHub report summary #2679

Closed

de-jcup assigned Jeeppler and de-jcup Nov 21, 2023

de-jcup requested changes Nov 22, 2023

View reviewed changes

zigfridus added 3 commits November 22, 2023 18:17

Merge branch 'mercedes-benz:develop' into feature-345-summary-in-reports

aaeeedd

Merge branch 'mercedes-benz:develop' into feature-345-summary-in-reports

98bef78

Fixed bug when critical, info and unclassified findings shown incorre…

6999d6a

…ctly in the reports. mercedes-benz#345

Fixed some inaccuracies. mercedes-benz#345

59f4918

zigfridus requested a review from de-jcup November 28, 2023 20:45

zigfridus changed the base branch from develop to community December 19, 2023 12:39

zigfridus closed this Dec 22, 2023

zigfridus mentioned this pull request Dec 22, 2023

Feature 345 summary in reports (part 2) #2782

Merged

Feature 345 summary in reports #2156

Feature 345 summary in reports #2156

Conversation

zigfridus commented Apr 10, 2023

Jeeppler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigfridus commented Apr 17, 2023

Jeeppler commented Apr 17, 2023 • edited Loading

zigfridus commented Apr 24, 2023

Jeeppler commented Apr 25, 2023

Jeeppler commented Apr 25, 2023

zigfridus commented Apr 25, 2023

zigfridus commented Apr 27, 2023

Jeeppler left a comment

Choose a reason for hiding this comment

zigfridus commented May 26, 2023

Jeeppler left a comment • edited Loading

Choose a reason for hiding this comment

zigfridus commented May 29, 2023

Jeeppler commented Jun 1, 2023

zigfridus commented Jul 5, 2023 • edited Loading

Jeeppler commented Jul 5, 2023

zigfridus commented Jul 6, 2023

zigfridus commented Jul 6, 2023

Jeeppler commented Jul 7, 2023 • edited Loading

zigfridus commented Jul 11, 2023

zigfridus commented Nov 2, 2023

Jeeppler left a comment

Choose a reason for hiding this comment

CLA-Mercedes-Benz commented Nov 14, 2023 • edited Loading

de-jcup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigfridus Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

de-jcup commented Nov 27, 2023

zigfridus commented Nov 28, 2023 • edited Loading

de-jcup commented Dec 19, 2023 • edited Loading

zigfridus commented Dec 19, 2023

Jeeppler commented Dec 19, 2023

zigfridus commented Dec 19, 2023

Jeeppler commented Dec 21, 2023 • edited Loading

Jeeppler commented Apr 17, 2023 •

edited

Loading

Jeeppler left a comment •

edited

Loading

zigfridus commented Jul 5, 2023 •

edited

Loading

Jeeppler commented Jul 7, 2023 •

edited

Loading

CLA-Mercedes-Benz commented Nov 14, 2023 •

edited

Loading

zigfridus Nov 28, 2023 •

edited

Loading

zigfridus commented Nov 28, 2023 •

edited

Loading

de-jcup commented Dec 19, 2023 •

edited

Loading

Jeeppler commented Dec 21, 2023 •

edited

Loading