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

Add Support to the New Scanners Config #412

Merged
merged 8 commits into from
Oct 5, 2023

Conversation

talarian1
Copy link
Contributor

  • All tests passed. If this feature is not already covered by the tests, I added new tests.

Add support for the new project config file.
Depends on jfrog/ide-plugins-common#134

@talarian1 talarian1 added the ignore for release Automatically generated release notes label Sep 10, 2023
Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

Looks like there is a missing directory:
scan/data/applications

I'd like to review it again after handling my comments and adding the missing directory.

}

protected List<JFrogSecurityWarning> execute(ScanConfig.Builder inputFileBuilder, List<String> args, Runnable checkCanceled, boolean createInputFile, File executionDir) throws IOException, InterruptedException {
protected List<JFrogSecurityWarning> execute(ScanConfig.Builder inputFileBuilder, List<String> args, Runnable checkCanceled, boolean newConfigFormat) throws IOException, InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

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

This method became very long and not readable. Please consider refactoring by splitting it into submethods in this or another PR.

@@ -288,7 +287,7 @@ protected void downloadBinary() throws IOException {
}
}

Path createTempRunInputFile(ScansConfig scanInput) throws IOException {
Path createTempRunInputFile(Object scanInput) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Deserializing an object is considered insecure. See https://owasp.org/www-project-top-ten/2017/A8_2017-Insecure_Deserialization.
Let's deserialize only ScanConfig or NewScanConfig. You can use method overloading for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are internal input objects that should be unified in the near future (when all the scanners will support the new config file). Hence we are only deserializing objects we are creating, I see no security concern here.

JFrogApplicationsConfig projectConfig = parseJFrogApplicationsConfig();

if (projectConfig != null) {
for (ModuleConfig moduleConfig : projectConfig.getModules()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we run them in tasks? Running each scanner serially may take a lot of time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should do it in a separate PR in the near future.

@talarian1 talarian1 added the safe to test Approve running integration tests on a pull request label Oct 3, 2023
@talarian1 talarian1 added safe to test Approve running integration tests on a pull request and removed safe to test Approve running integration tests on a pull request labels Oct 3, 2023
@talarian1 talarian1 requested a review from yahavi October 3, 2023 09:31
Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

Looks better! Please consider the below changes.

@talarian1 talarian1 added improvement Automatically generated release notes safe to test Approve running integration tests on a pull request and removed ignore for release Automatically generated release notes safe to test Approve running integration tests on a pull request labels Oct 5, 2023
@talarian1 talarian1 merged commit bd1a42f into jfrog:master Oct 5, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Automatically generated release notes safe to test Approve running integration tests on a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants