Add SSCS as scan type for the triage command (AST-72075) #1022
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
1a51196 to
c22a73a
Compare
| type ScanOverviewHTTPWrapper struct { | ||
| path string | ||
| } | ||
|
|
||
| func NewHTTPScanOverviewWrapper(path string) ScanOverviewWrapper { | ||
| validPath := configurePath(path) |
There was a problem hiding this comment.
The configurePath function is not defined within the provided diff. Ensure that the function exists and is properly implemented to handle the path parameter. If it's defined in another file, make sure it is correctly imported and accessible in this context.
|
|
||
| // configurePath checks if the path is the default path, if not it writes the default path to the config file | ||
| func configurePath(path string) string { | ||
| if path != defaultPath { |
There was a problem hiding this comment.
The condition if path != defaultPath is misleading because the function always returns defaultPath regardless of the input path. Consider returning the actual path that is set in the configuration.
| func configurePath(path string) string { | ||
| if path != defaultPath { | ||
| viper.Set(commonParams.ScsScanOverviewPathKey, defaultPath) | ||
| configFilePath, err := configuration.GetConfigFilePath() |
There was a problem hiding this comment.
The error from configuration.GetConfigFilePath() is logged but not handled. If the config file path cannot be retrieved, the function should not attempt to write to it and should handle the error appropriately.
| if err != nil { | ||
| logger.PrintfIfVerbose("Error getting config file path: %v", err) | ||
| } | ||
| err = configuration.SafeWriteSingleConfigKeyString(configFilePath, commonParams.ScsScanOverviewPathKey, defaultPath) |
There was a problem hiding this comment.
The error from configuration.SafeWriteSingleConfigKeyString is logged but not handled. Consider what should happen if the configuration cannot be updated. Should the function return an error, or should there be a fallback mechanism?
| func TestWriteSingleConfigKeyStringNonExistingFile_CreatingTheFileAndWritesTheKey_Success(t *testing.T) { | ||
| configFilePath := "non-existing-file" | ||
|
|
||
| file, err := os.Open(configFilePath) |
There was a problem hiding this comment.
The file handle file is not closed after opening. Consider deferring the close operation immediately after checking the error from os.Open to avoid resource leaks.
| asserts.NotNil(t, err) | ||
| asserts.Nil(t, file) | ||
|
|
||
| err = configuration.SafeWriteSingleConfigKeyString(configFilePath, cxScsScanOverviewPath, defaultScsScanOverviewPath) |
There was a problem hiding this comment.
The test function name TestWriteSingleConfigKeyStringNonExistingFile_CreatingTheFileAndWritesTheKey_Success is misleading as it implies success, but the test does not assert any conditions to verify the success of the operation. Consider adding assertions to check that the key was written successfully.
| // setDefaultPath checks if the path is the default path, if not it writes the default path to the config file | ||
| func setDefaultPath(path string) string { | ||
| if path != defaultPath { | ||
| configFilePath, err := configuration.GetConfigFilePath() |
There was a problem hiding this comment.
The viper.Set call has been removed, which means the new default path is not being set in the configuration. Ensure that the configuration is updated with the new default path if this is the intended behavior.
86b550a to
2375f6f
Compare
| const ( | ||
| cxAscaPort = "cx_asca_port" | ||
| cxScsScanOverviewPath = "cx_scs_scan_overview_path" | ||
| defaultScsScanOverviewPath = "api/micro-engines/read/scans/%s/scan-overview" |
There was a problem hiding this comment.
The constant defaultScsScanOverviewPath is defined but not used anywhere in the code. Please ensure that it is utilized appropriately or remove it if it's not needed.
| @@ -58,3 +63,18 @@ func (r *ScanOverviewHTTPWrapper) GetSCSOverviewByScanID(scanID string) ( | |||
| return nil, nil, errors.Errorf("response status code %d", resp.StatusCode) | |||
| } | |||
| } | |||
|
|
|||
| // setDefaultPath checks if the path is the default path, if not it writes the default path to the config file | |||
There was a problem hiding this comment.
The function setDefaultPath does not seem to be related to the SSCS triaging capabilities as described in the PR. Please ensure that this function is necessary for the feature being implemented and clarify its purpose in the context of SSCS triaging.
|
|
||
| // setDefaultPath checks if the path is the default path, if not it writes the default path to the config file | ||
| func setDefaultPath(path string) string { | ||
| if path != defaultPath { |
There was a problem hiding this comment.
The function setDefaultPath always returns defaultPath regardless of the input path. This behavior is not intuitive based on the function name and description. Consider returning the actual path set (either the input path or defaultPath) or renaming the function to reflect its behavior more accurately.
| // setDefaultPath checks if the path is the default path, if not it writes the default path to the config file | ||
| func setDefaultPath(path string) string { | ||
| if path != defaultPath { | ||
| configFilePath, err := configuration.GetConfigFilePath() |
There was a problem hiding this comment.
The check if path != defaultPath should be followed by a return statement if the condition is false, to avoid unnecessary operations when the path is already set to the default.
| if err != nil { | ||
| logger.PrintfIfVerbose("Error getting config file path: %v", err) | ||
| } | ||
| err = configuration.SafeWriteSingleConfigKeyString(configFilePath, commonParams.ScsScanOverviewPathKey, defaultPath) |
There was a problem hiding this comment.
The function configuration.SafeWriteSingleConfigKeyString is called without handling the case where the configFilePath could not be retrieved due to an error. This could lead to unexpected behavior. Ensure that the function is not called if configFilePath cannot be determined.
| } | ||
| err = configuration.SafeWriteSingleConfigKeyString(configFilePath, commonParams.ScsScanOverviewPathKey, defaultPath) | ||
| if err != nil { | ||
| logger.PrintfIfVerbose("Error writing Scan Overview path to config file: %v", err) |
There was a problem hiding this comment.
Logging the error using logger.PrintfIfVerbose might not be sufficient for error handling. Consider propagating the error up to the caller of setDefaultPath so that it can be handled appropriately.
By submitting a PR to this repository, you agree to the terms within the Checkmarx Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
References
Testing
Checklist