Rescan #169

vishalhcl-5960 · 2024-08-20T18:46:17Z

Rescan implementation for the SAST scan
Note: Validation of the scanId is pending from our side & will add a new method in the serviceUtil class for it.

CLAassistant · 2024-08-20T18:46:23Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ vishalhcl-5960
❌ Kripajoy Melitpalathingal

Kripajoy Melitpalathingal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

mattmurp · 2024-08-20T19:18:55Z

src/main/java/com/hcl/appscan/sdk/scan/CloudScanServiceProvider.java

@@ -96,7 +96,56 @@ public String createAndExecuteScan(String type, Map<String, String> params) {
        }
        return null;
 	  }
-
+    @Override


This method is an exact copy of createAndExecuteScan() with just a few minor changes (the url and the constant used for getting the id). This should be refactored to put all of the common logic into a private method, so the 2 public methods can simply call that private method, passing in the minor differences.

Sure, We have created a new private common method executeScan() which has been called from both create&ExecuteScan() & rescan() method.

mattmurp · 2024-08-20T19:19:40Z

src/main/java/com/hcl/appscan/sdk/scan/CloudScanServiceProvider.java

+            JSONObject json = (JSONObject) response.getResponseBodyAsJSON();
+
+            if (status == HttpsURLConnection.HTTP_CREATED || status == HttpsURLConnection.HTTP_OK) {
+                String scanId = json.getString(SCAN_ID);


What's the reason for getting the scan id and returning it? Since this is a rescan, the scan id is already known. I think it makes more sense to return the execution id.

Sure, Now we are fetching the executionId if the user go for rescanning.

mattmurp · 2024-08-20T19:20:48Z

src/main/java/com/hcl/appscan/sdk/scanners/sast/SASTScan.java

-        	submitScan();
+
+		if (params.containsKey(CoreConstants.SCAN_ID)) {
+                    params.put(CoreConstants.FILE_ID, fileId);


Both the "if" and "else" branches make the same call to add the file id to the params. That should be done once outside of the conditional rather than repeating the code.

Keys are different. For scan, the key is ApplicationFileId, while for a rescan, it's FileId.

mattmurp · 2024-08-20T19:26:04Z

src/main/java/com/hcl/appscan/sdk/scanners/sast/SASTScan.java

 		if(getScanId() == null)
 			throw new ScannerException(Messages.getMessage(ERROR_SUBMITTING_IRX));
 	}
+


This same method will be needed for DAST rescans, as well. Assuming it will be the same implementation, this should be moved to the ASoCScan class, so the logic can be reused.

mattmurp · 2024-08-20T19:29:25Z

src/main/java/com/hcl/appscan/sdk/scanners/sast/SASTScan.java

-
-        	submitScan();
+
+		if (params.containsKey(CoreConstants.SCAN_ID)) {


Since other scanners will need to do this same check, I suggest doing this in the ASoCScan constructor. It can set a boolean member variable in that class (e.g. m_isRescan) that can also have a getter (e.g. isRescan()). You could then call isRescan() to see if you should call the specific scanner's submitScan() method or the general submitRescan() method on the ASoCScan class (see comment below).

We have created a new method called submitRescan() in the ASoCScan class which has been called from the analyzeIR() method of the SASTscan class.
"protected void submitRescan() {
setExecutionId(getServiceProvider().rescan(getScanId(),getProperties()));
}"

Fetching the scan details with execution ID during re-scanning

…-sdk into Rescan

mattmurp · 2024-09-16T17:40:47Z

src/main/java/com/hcl/appscan/sdk/messages.properties

-message.created.scan=Successfully submitted {0} scan for analysis. Scan ID: {1}
-message.scan.overview={0} scan overview: {1}
+message.created.scan=Successfully submitted {0} scan for analysis. Scan ID:
+message.scan.overview={0} scan overview:


What's the reason for removing the {1} at the end of each of these strings? It looks like the calls that use them still provide information for the 2nd parameter.

We are assigning the 2nd value in a common method ("executeScan") used for createAndExecuteScan() & rescan(). So, for new scan the 2nd value will be scanId while for the rescanning it would be executionId.

mattmurp · 2024-09-16T17:42:25Z

src/main/java/com/hcl/appscan/sdk/messages.properties

+message.created.scan=Successfully submitted {0} scan for analysis. Scan ID:
+message.scan.overview={0} scan overview:
+message.rescan= Successfully submitted rescan for analysis. Execution ID:
+message.rescan.overview= Rescan overview:


Are message.rescan and message.rescan.overview used anywhere? I don't see them referenced in this PR.

Yes, We are using them in the rescan() method.
https://github.com/HCL-TECH-SOFTWARE/appscan-sdk/blob/Rescan/src/main/java/com/hcl/appscan/sdk/scan/CloudScanServiceProvider.java#L72
https://github.com/HCL-TECH-SOFTWARE/appscan-sdk/blob/Rescan/src/main/java/com/hcl/appscan/sdk/scan/CloudScanServiceProvider.java#L73

mattmurp · 2024-09-16T17:44:37Z

src/main/java/com/hcl/appscan/sdk/results/CloudResultsProvider.java

@@ -57,6 +58,16 @@ public CloudResultsProvider(String scanId, String type, IScanServiceProvider pro
 		m_reportFormat = DEFAULT_REPORT_FORMAT;
 	}

+	public CloudResultsProvider(String scanId, String executionId, String type, IScanServiceProvider provider, IProgress progress) {


To limit duplication of code, it would be good to have the original constructor that does not take an executionId call this one directly. E.g.:
this(scanId, null, type, provider, progress);

Okay, sure.

mattmurp · 2024-09-16T17:58:25Z

src/main/java/com/hcl/appscan/sdk/scanners/ASoCScan.java

+	}
+
+        public Boolean isRescan() {
+		return m_properties.containsKey(CoreConstants.SCAN_ID);


There is a setter function setRescan(boolean rescan) that sets the member variable m_rescan, but the getter function isRescan() checks for a key in m_properties. These should be made consistent, so they both work with the member variable, which can be set in the constructor based on m_properties.

Added a getter method & assigning the m_rescan value in the constructor based on the m_properties. Also, removed the isRescan() method.

mattmurp · 2024-09-16T17:58:51Z

src/main/java/com/hcl/appscan/sdk/scanners/ASoCScan.java

@@ -25,6 +25,8 @@ public abstract class ASoCScan implements IScan, ScanConstants, Serializable {

 	private String m_target;
 	private String m_scanId;
+        private String m_executionId;
+        private Boolean m_rescan;


Any reason to use the class Boolean as opposed to the primitive type boolean?

Changed the "m_rescan" type to boolean.

mattmurp · 2024-09-20T13:10:50Z

src/main/java/com/hcl/appscan/sdk/http/HttpClient.java

+     */
+    public HttpResponse put(String url, Map<String, String> headerProperties, Map<String, String> parameters)
+            throws IOException, JSONException {
+        JSONObject params = new JSONObject(parameters);


What's the purpose of creating the params JSONObject instead of just working with the parameters Map?

It is because we are putting the boolean value if the key value equals to "true"/"false", Map properties doesn't allow me to put boolean as well as string values in the same map.
For v4 APIs, we can't assign a string value for boolean taking parameters.

I understand the creation of objectMap, but params is never written to. It's only read. So we should be able to just read from the Map parameters, as opposed to creating the JSONObject params from it.

Okay, Made the changes accordingly.

mattmurp · 2024-09-20T16:58:00Z

src/main/java/com/hcl/appscan/sdk/http/HttpClient.java

+     */
+    public HttpResponse put(String url, Map<String, String> headerProperties, Map<String, String> parameters)
+            throws IOException, JSONException {
+        JSONObject params = new JSONObject(parameters);


I understand the creation of objectMap, but params is never written to. It's only read. So we should be able to just read from the Map parameters, as opposed to creating the JSONObject params from it.

mattmurp · 2024-09-20T17:32:24Z

src/main/java/com/hcl/appscan/sdk/http/HttpClient.java

+        for (Object key : params.keySet()) {
+            if (params.get(key) != null){
+                String value = params.get(key).toString();
+                if (value != null) {


Line 175 does a null check on params.get(key). The value is then put into the variable "value" and we do another null check of it on line 177, which is redundent.

Removed the 2nd null check.

Kripajoy Melitpalathingal added 3 commits August 19, 2024 12:28

Rescan

e91bc1b

Params

5a9ce92

Java Doc

b8842db

vishalhcl-5960 requested a review from mattmurp August 20, 2024 18:46

mattmurp reviewed Aug 20, 2024

View reviewed changes

vishalhcl-5960 and others added 12 commits August 21, 2024 18:47

scanId validation method

e08614d

Rewrite console-logs for rescan

a3b336b

scan details with execution ID

1fd4d89

Fetching the scan details with execution ID during re-scanning

Update ServiceUtil.java

5eed66a

Update NonCompliantIssuesResultProvider.java

433f83e

As per the comments

587b030

removed the "new" status from query

c09f338

Merge branch 'Rescan' of https://github.com/HCL-TECH-SOFTWARE/appscan…

3686da4

…-sdk into Rescan

Addressed the PR comments

4b2f2bb

addressing comments

fd46f4e

Merge branch 'master' into Rescan

45994bb

ASA-9456

052f5c9

mattmurp reviewed Sep 16, 2024

View reviewed changes

vishalhcl-5960 added 4 commits September 19, 2024 20:31

ASA-9480 & ASA-9479

c02ad47

As per comments

6628196

ASA-9475

02aedd7

Created a common constant for a status statement

c8062fd

mattmurp reviewed Sep 20, 2024

View reviewed changes

vishalhcl-5960 added 2 commits September 21, 2024 00:12

As per comments

396cd08

Update HttpClient.java

d1649b8

mattmurp approved these changes Sep 20, 2024

View reviewed changes

vishalhcl-5960 merged commit 98db010 into master Sep 25, 2024
1 of 2 checks passed

vishalhcl-5960 deleted the Rescan branch September 25, 2024 11:11

kripajoym restored the Rescan branch October 1, 2024 09:26

kripajoym deleted the Rescan branch October 1, 2024 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rescan #169

Rescan #169

vishalhcl-5960 commented Aug 20, 2024 •

edited

Loading

CLAassistant commented Aug 20, 2024 •

edited

Loading

mattmurp Aug 20, 2024

vishalhcl-5960 Sep 12, 2024 •

edited

Loading

mattmurp Aug 20, 2024

vishalhcl-5960 Sep 12, 2024

mattmurp Aug 20, 2024

kripajoym Aug 27, 2024

mattmurp Aug 20, 2024

mattmurp Aug 20, 2024

vishalhcl-5960 Sep 12, 2024 •

edited

Loading

mattmurp Sep 16, 2024

vishalhcl-5960 Sep 19, 2024

mattmurp Sep 16, 2024

vishalhcl-5960 Sep 19, 2024

mattmurp Sep 16, 2024

vishalhcl-5960 Sep 19, 2024

mattmurp Sep 16, 2024

vishalhcl-5960 Sep 19, 2024

mattmurp Sep 16, 2024

vishalhcl-5960 Sep 19, 2024

mattmurp Sep 20, 2024

vishalhcl-5960 Sep 20, 2024

mattmurp Sep 20, 2024

vishalhcl-5960 Sep 20, 2024

mattmurp Sep 20, 2024

mattmurp Sep 20, 2024

vishalhcl-5960 Sep 20, 2024


		submitScan();

		if (params.containsKey(CoreConstants.SCAN_ID)) {

Rescan #169

Rescan #169

Conversation

vishalhcl-5960 commented Aug 20, 2024 • edited Loading

CLAassistant commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

vishalhcl-5960 Sep 12, 2024 • edited Loading

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

Choose a reason for hiding this comment

vishalhcl-5960 Sep 12, 2024 • edited Loading

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

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

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

vishalhcl-5960 commented Aug 20, 2024 •

edited

Loading

CLAassistant commented Aug 20, 2024 •

edited

Loading

vishalhcl-5960 Sep 12, 2024 •

edited

Loading

vishalhcl-5960 Sep 12, 2024 •

edited

Loading