Skip to content

Fix Dependabot config indentation issue #2140

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

Merged
merged 3 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 95 additions & 24 deletions remediation/dependabot/dependabotconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ package dependabot
import (
"bufio"
"encoding/json"
"errors"
"fmt"
"strings"

dependabot "github.com/paulvollmer/dependabot-config-go"
"gopkg.in/yaml.v2"
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Use yaml.v3 instead of yaml.v2 package
    The yaml.v2 package has been recommended for deprecation and current code can be updated to use yaml.v3. Update import from gopkg.in/yaml.v2 to gopkg.in/yaml.v3 code line:"gopkg.in/yaml.v2"

"gopkg.in/yaml.v3"
)

type UpdateDependabotConfigResponse struct {
Expand All @@ -27,29 +29,79 @@ type UpdateDependabotConfigRequest struct {
Content string
}

// getIndentation returns the indentation level of the first list found in a given YAML string.
// If the YAML string is empty or invalid, or if no list is found, it returns an error.
func getIndentation(dependabotConfig string) (int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Validate the input to getIndentation function
    The input to getIndentation function shouldn't be empty. Add a validation check at the beginning of the getIndentation function to check if dependabotConfig is empty. code line:func getIndentation(dependabotConfig string) (int, error) {

// Initialize an empty YAML node
t := yaml.Node{}

// Unmarshal the YAML string into the node
err := yaml.Unmarshal([]byte(dependabotConfig), &t)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Handle the error returned by yaml.Unmarshal
    yaml.Unmarshal can return an error and the current code isn't handling it. Check err returned by yaml.Unmarshal and handle it accordingly. code line:err := yaml.Unmarshal([]byte(dependabotConfig), &t)

if err != nil {
return 0, fmt.Errorf("unable to parse yaml: %w", err)
}

// Retrieve the top node of the YAML document
topNode := t.Content
if len(topNode) == 0 {
return 0, errors.New("file provided is empty or invalid")
}

// Check for the first list and its indentation level
for _, n := range topNode[0].Content {
if n.Value == "" && n.Tag == "!!seq" {
// Return the column of the first list found
return n.Column, nil
}
}

// Return an error if no list was found
return 0, errors.New("no list found in yaml")
}

// UpdateDependabotConfig is used to update dependabot configuration and returns an UpdateDependabotConfigResponse.
func UpdateDependabotConfig(dependabotConfig string) (*UpdateDependabotConfigResponse, error) {
var updateDependabotConfigRequest UpdateDependabotConfigRequest
json.Unmarshal([]byte(dependabotConfig), &updateDependabotConfigRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [High]Improve error handling in 'UpdateDependabotConfig'
    There is no error handling in code that could lead to panics or other issues. This may be caused by json unmarshalling or yaml unmarshalling errors. Add error handling to the JSON and YAML unmarshalling code line:json.Unmarshal([]byte(dependabotConfig), &updateDependabotConfigRequest)

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [High]Check for errors while unmarshaling JSON
    JSON unmarshaling can fail due to various reasons such as invalid JSON format, non-matching types between json and Go struct fields, and empty strings etc. Handle the error returned by json.Unmarshal code line:json.Unmarshal([]byte(dependabotConfig), &updateDependabotConfigRequest)


// Handle error in json unmarshalling
err := json.Unmarshal([]byte(dependabotConfig), &updateDependabotConfigRequest)
if err != nil {
return nil, fmt.Errorf("failed to unmarshal JSON from dependabotConfig: %v", err)
}

inputConfigFile := []byte(updateDependabotConfigRequest.Content)
configMetadata := dependabot.New()
err := configMetadata.Unmarshal(inputConfigFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Avoid shadowing variable 'err' in 'UpdateDependabotConfig'
    The variable 'err' is shadowed within the loop in 'UpdateDependabotConfig'. This may lead to unexpected behavior and make the code harder to read. Avoid shadowing the 'err' variable. code line:err := configMetadata.Unmarshal(inputConfigFile)

err = configMetadata.Unmarshal(inputConfigFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [High]Improve error handling in 'UpdateDependabotConfig'
    There is no error handling for unmarshalling input yaml data. This may cause the parsing and marshalling to fail silently in case of errors. Add error handling to the JSON and YAML unmarshalling code line:err = configMetadata.Unmarshal(inputConfigFile)

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Check for errors while unmarshaling dependabot configuration
    Unmarshaling dependabot configuration can fail due to reasons such as empty strings, invalid syntax and unsupported properties. Handle the error returned by configMetadata.Unmarshal or return the error to the caller. code line:err = configMetadata.Unmarshal(inputConfigFile)

if err != nil {
return nil, err
return nil, fmt.Errorf("failed to unmarshal dependabot config: %v", err)
}

indentation := 3

response := new(UpdateDependabotConfigResponse)
response.FinalOutput = updateDependabotConfigRequest.Content
response.OriginalInput = updateDependabotConfigRequest.Content
response.IsChanged = false

// Using strings.Builder for efficient string concatenation
var finalOutput strings.Builder
finalOutput.WriteString(response.FinalOutput)

if updateDependabotConfigRequest.Content == "" {
if len(updateDependabotConfigRequest.Ecosystems) == 0 {
return response, nil
}
response.FinalOutput = "version: 2\nupdates:"
finalOutput.WriteString("version: 2\nupdates:")
} else {
response.FinalOutput += "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Use 'strings.Builder' instead of string concatenation
    String concatenation requires the creation of a new string object each time which can be expensive. Using 'strings.Builder' would be a more efficient way of building the response string for the final output. Use 'strings.Builder' instead of string concatenation. code line:response.FinalOutput += "\n"

if !strings.HasSuffix(response.FinalOutput, "\n") {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Append a new line character to the end of 'FinalOutput' if 'FinalOutput' doesn't end with one
    Having no newline character at the end of the 'FinalOutput' could cause issues in some systems like adding the new content to the last word of the previous line in a file. Append a new line character to the end of 'FinalOutput' if 'FinalOutput' doesn't end with one. Example: finalOutput.WriteString("\n") code line:if !strings.HasSuffix(response.FinalOutput, "\n") {

finalOutput.WriteString("\n")
}
indentation, err = getIndentation(string(inputConfigFile))
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Low]Check if getIndentation() returned an error
    This function call might fail and it's not a fatal error but can cause certain issues in calling functions. Check if getIndentation() returned an error and handle or return it if not handled. code line:indentation, err = getIndentation(string(inputConfigFile))

if err != nil {
return nil, fmt.Errorf("failed to get indentation: %v", err)
}
}

for _, Update := range updateDependabotConfigRequest.Ecosystems {
updateAlreadyExist := false
for _, update := range configMetadata.Updates {
Expand All @@ -58,37 +110,56 @@ func UpdateDependabotConfig(dependabotConfig string) (*UpdateDependabotConfigRes
break
}
}
if !updateAlreadyExist {
item := dependabot.Update{}
item.PackageEcosystem = Update.PackageEcosystem
item.Directory = Update.Directory

schedule := dependabot.Schedule{}
schedule.Interval = Update.Interval

item.Schedule = schedule
items := []dependabot.Update{}
items = append(items, item)
if !updateAlreadyExist {
item := dependabot.Update{
PackageEcosystem: Update.PackageEcosystem,
Directory: Update.Directory,
Schedule: dependabot.Schedule{Interval: Update.Interval},
}
items := []dependabot.Update{item}
addedItem, err := yaml.Marshal(items)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Check for errors while marshalling the created update items
    Marshaling can fail due to various reasons such as circular references, non-supported types, ...etc. Handle the error returned by yaml.Marshal or return the error to the caller. code line:addedItem, err := yaml.Marshal(items)

data := string(addedItem)
if err != nil {
return nil, fmt.Errorf("failed to marshal update items: %v", err)
}

data = addIndentation(data)
data, err := addIndentation(string(addedItem), indentation)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to add indentation: %v", err)
}
response.FinalOutput = response.FinalOutput + data
finalOutput.WriteString(data)
response.IsChanged = true
}
}

// Set FinalOutput to the built string
response.FinalOutput = finalOutput.String()

return response, nil
}

func addIndentation(data string) string {
// addIndentation adds a certain number of spaces to the start of each line in the input string.
// It returns a new string with the added indentation.
func addIndentation(data string, indentation int) (string, error) {
scanner := bufio.NewScanner(strings.NewReader(data))
finalData := "\n"
var finalData strings.Builder

// Create the indentation string
spaces := strings.Repeat(" ", indentation-1)

finalData.WriteString("\n")

// Add indentation to each line
for scanner.Scan() {
finalData += " " + scanner.Text() + "\n"
finalData.WriteString(spaces)
finalData.WriteString(scanner.Text())
finalData.WriteString("\n")
}

// Check for scanning errors
if err := scanner.Err(); err != nil {
return "", fmt.Errorf("error during scanning: %w", err)
}
return finalData

return finalData.String(), nil
}
10 changes: 10 additions & 0 deletions remediation/dependabot/dependabotconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ func TestConfigDependabotFile(t *testing.T) {
Ecosystems: []Ecosystem{{"github-actions", "/", "daily"}, {"npm", "/sample", "daily"}},
isChanged: true,
},
{
fileName: "No-Indentation.yml",
Ecosystems: []Ecosystem{{"npm", "/sample", "daily"}},
isChanged: true,
},
{
fileName: "High-Indentation.yml",
Ecosystems: []Ecosystem{{"npm", "/sample", "daily"}},
isChanged: true,
},
}

for _, test := range tests {
Expand Down
6 changes: 6 additions & 0 deletions testfiles/dependabotfiles/input/High-Indentation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
version: 2
updates:
- package-ecosystem: github-actions
directory: "/"
schedule:
interval: daily
6 changes: 6 additions & 0 deletions testfiles/dependabotfiles/input/No-Indentation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
version: 2
updates:
- package-ecosystem: github-actions
directory: "/"
schedule:
interval: daily
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Add a newline at the end of file
    The absence of a newline at the end of a file can cause issues with certain tools and some git actions. Add a newline at the end of the file. code line: interval: daily

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Low]Add newline at the end of the file
    Missing newline at the end of file can cause issues in some text editors and command line tools. Add a newline at the end of the file. code line: interval: daily

Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ updates:
# Files stored in `app` directory
directory: "/app"
schedule:
interval: "daily"
interval: "daily"
11 changes: 11 additions & 0 deletions testfiles/dependabotfiles/output/High-Indentation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
version: 2
updates:
- package-ecosystem: github-actions
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Specify package ecosystem version
    Not specifying version can result in future incompatibility issues. Specify the package ecosystem version in the configuration file. code line: - package-ecosystem: github-actions

directory: "/"
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [High]Avoid using wildcard in directory path
    Using wildcard might include other directories in the directory path in the future. Use specific directory path instead of wildcard. code line: directory: "/"

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Low]Use consistent quotes
    It's best practice to use single quotes or double quotes consistently. Replace double quotes with single quotes as there are no interpolated variables being used. code line: directory: "/"

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [High]Remove the leading forward slash in directory field for 'github-actions' entry
    The leading forward slash in the directory field for the 'github-actions' entry may cause issues with path resolution. Remove the leading forward slash in the directory field for the 'github-actions' entry. code line:directory: "/"

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [High]Avoid using root directory ('/') for package ecosystem update
    The current code updates the package ecosystem in the root directory ('/') of the project. This can cause unintended updates to packages outside the project and can be potentially harmful. Specify the project directory path instead of the root directory for package ecosystem updates. code line:directory: "/"

schedule:
interval: daily

- package-ecosystem: npm
directory: /sample
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Use the full directory path instead of the relative path for the 'npm' entry
    Using a relative path for the directory field for the 'npm' package-ecosystem entry may cause issues with path resolution when the current working directory changes. Use the full directory path instead of the relative path for the 'npm' entry. code line:directory: /sample

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Low]Use a more specific directory path for 'npm' package ecosystem update
    The current code updates the 'npm' package ecosystem in the '/sample' directory of the project. This path may not be specific enough and can cause updates to unintended packages. Specify the project directory path that contains the 'npm' package ecosystem for updates. code line:directory: /sample

schedule:
interval: daily
11 changes: 11 additions & 0 deletions testfiles/dependabotfiles/output/No-Indentation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
version: 2
updates:
- package-ecosystem: github-actions
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Low]Specify the version number in the package ecosystem configuration
    The YAML file includes package ecosystem configuration for GitHub Actions and npm, but it does not specify the version number of the packages to use. Specify the version number or version range in the relevant package ecosystem configuration to ensure consistent and predictable builds and deployments. For example, use the format "@@" in the package version key or tag. code line:- package-ecosystem: github-actions

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Define explicit versions for dependencies
    The code includes two updates with their respective schedules, but they don't define any specific version for the packages being updated. This can later introduce breaking changes when a new version of the package possibly introduces breaking changes. Define explicit versions for the packages being updated or use versions ranges that include non-breaking changes. code line:- package-ecosystem: github-actions

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Low]Remove unnecessary quotes from yaml keys
    The keys for each of the updates include double quotes that are not necessary. Remove quotes from the keys. code line:- package-ecosystem: github-actions

Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Medium]Specify version for package ecosystem
    Currently, a package ecosystem is being used in an unversioned form. This is not encouraged as unversioned dependencies can potentially break the build when the package ecosystem changes. Specify the exact or a compatible version range for the package ecosystem. For example, if a specific version is needed, use something like “[email protected]”. If any version within a range is acceptable, use something like "github-actions@^1.0.0" code line:- package-ecosystem: github-actions

directory: "/"
schedule:
interval: daily
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Low]Use explicit frequency value instead of words
    Both of the schedules use the word "daily" as their respective intervals. While this will work, it's recommended to use an explicit value for the frequency. Use the numeric values such as 1d to represent an interval of 1 day. Here's an example: schedule: { interval: 1d } code line: interval: daily


- package-ecosystem: npm
directory: /sample
schedule:
interval: daily