Fix panic exit when containers ff could not be fetched (AST-86767) #1069
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
internal/commands/scan.go
Outdated
| @@ -1169,6 +1169,19 @@ func validateScanTypes(cmd *cobra.Command, jwtWrapper wrappers.JWTWrapper, featu | |||
| return nil | |||
| } | |||
|
|
|||
| func isContainersEngineEnabled(featureFlagsWrapper wrappers.FeatureFlagsWrapper) bool { | |||
| runContainerEngineCLI := true | |||
There was a problem hiding this comment.
Is this value necessary if it will be overwritten later anyway, whether there's an error or not?
There was a problem hiding this comment.
i need to define the var outside the if/else scope. but you are right we can initialize it as nil.
internal/commands/scan.go
Outdated
| containerEngineCLIEnabled, err := featureFlagsWrapper.GetSpecificFlag(wrappers.ContainerEngineCLIEnabled) | ||
| if err != nil { | ||
| logger.PrintfIfVerbose("could not get CONTAINER_ENGINE_CLI_ENABLED FF. defaulting to `false` error: %s", err) | ||
| runContainerEngineCLI = false |
There was a problem hiding this comment.
If retrieving the Feature Flag fails (e.g., due to a network issue), the function defaults to false, which could mistakenly disable container scanning.
Should the default be true or false? Defining it clearly would avoid confusion.
or maybe, return an error and let the user decide...
There was a problem hiding this comment.
if we can not determine a FF i would like it to default to false. we also add a log here so the user can understand what happen. do u think it should be defaulted to true?
There was a problem hiding this comment.
Defaulting to false with clear logging makes the most sense. When we can't get the feature flag, we should turn scanning off and tell the user what happened. This way they know scanning is off and why.
We should check if false is really the safer default,
but good logs to user is good idea
By submitting this pull request, you agree to the terms within the Checkmarx Code of Conduct. Please review the contributing guidelines for guidance on creating high-quality pull requests.
Description
Please provide a summary of the changes and the related issue. Include relevant motivation and context.
When creating a scan we check if the containers engine FF is enabled or not. if the FF could not be fetched the object that cli hold is nil. when trying to access
containerEngineCLIEnabled.Statuswe get a panic error and the cli fails.Type of Change
Related Issues
Link any related issues or tickets.
Checklist
Screenshots (if applicable)
Additional Notes
Add any other relevant information.