Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support scan vorpal command (AST-42348) #329

Closed
wants to merge 15 commits into from

Conversation

checkmarx-kobi-hagmi
Copy link
Contributor

No description provided.

@pedrompflopes pedrompflopes requested review from a team, helderfvieira and danielcaldeiracx and removed request for a team May 29, 2024 14:07
Copy link
Contributor

github-actions bot commented May 29, 2024

Logo
Checkmarx One – Scan Summary & Details8ba4bc40-8347-4590-b6cc-2b43bf7382dc

No New Or Fixed Issues Found

@@ -19,6 +20,22 @@ void testScanShow() throws Exception {
Assertions.assertEquals(scanList.get(0).getId(), scan.getId());
}

@Test
void testScanVorpalSuccessfulResponse() throws Exception {
ScanResult scanResult = wrapper.ScanVorpal("my-file.cs", true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this file? Don't you need a real file to send?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to add additional tests after I have real responses anyway. if I add some file now I will not see real results anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion:
will modify tests according to real behavior.

src/test/java/com/checkmarx/ast/ScanTest.java Outdated Show resolved Hide resolved

@Test
void testScanVorpalFailureResponse() throws Exception {
ScanResult scanResult = wrapper.ScanVorpal("my-file.cs", false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test will work for now but with the real vorpal it will fail,
you can send an invalid file path and then the test will return error and you will not need to change it again

@checkmarx-kobi-hagmi checkmarx-kobi-hagmi marked this pull request as draft May 30, 2024 10:43
@JsonDeserialize()
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)
public class ScanResult extends CxBaseObject {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you extending CxBaseObject if you don't use any of his properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the method "parse" that's defined there..
if we really want to get rid of this inheritance we can take "parse" and the method it uses - "isValidJson" to JSON utils, because these two method don't seem related to CxBaseObject logically.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it better idea then have empty base clas fields

@Test
void testScanVorpalFailureResponse() throws Exception {
ScanResult scanResult = wrapper.ScanVorpal("src/test/resources/csharp-file.cs", false);
Assertions.assertEquals("1111", scanResult.getRequestId());
Copy link
Contributor

@OrShamirCM OrShamirCM May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please have a case with a real error as not a good file name.
Please validate the message

@@ -19,6 +23,22 @@ void testScanShow() throws Exception {
Assertions.assertEquals(scanList.get(0).getId(), scan.getId());
}

@Test
void testScanVorpalSuccessfulResponse() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a happy case too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the happy case.. or you mean more assertions?


namespace ast_visual_studio_extension.CxCLI
{
public class CxWrapper
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this file? is this file the test file but it's the wrapper .cs?
maybe we can have a simpler file (or 2 files one. with a real vulnerability, only one, and other one that will return an err?)

arguments.add(CxConstants.VORPAL_LATEST_VERSION);
}

return Execution.executeCommand(withConfigArguments(arguments), logger, ScanResult::fromLine);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you have error, how it handles it? error in the CLI or error that not part of the scan results object error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of an error, the java-wrapper's CxException will be thrown. added test:
void testScanVorpal_WhenMissingFileExtension_ReturnFileExtensionIsRequiredFailure() throws Exception {
// ScanResult scanResult = wrapper.ScanVorpal("src/test/resources/python-vul-file", false);
Assertions.assertThrowsExactly(CxException.class, () -> {
wrapper.ScanVorpal("src/test/resources/python-vul-file", false);
}, "file must have an extension");
}

Copy link
Contributor

@OrShamirCM OrShamirCM Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont want to throw an error and break the IDE.. We want to show the errors in the IDE and the wrapper should return the error message

Assertions.assertNull(scanResult.getScanDetails());
Error error = scanResult.getError();
Assertions.assertNotNull(error);
Assertions.assertEquals("An internal error occurred.", error.description);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please have a test with error and not a scan results model

Copy link
Contributor

@OrShamirCM OrShamirCM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long time since the changes, need updates

@AlvoBen AlvoBen closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants