chore: New schema change for vulnerabilities test #37204
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow and two scripts related to vulnerability scanning. It removes a step that fetched vulnerability data from a PostgreSQL database and generated output files. The scripts for Scout and Trivy were updated to enhance error handling, streamline functionality, and improve database interactions by handling duplicates more effectively. The overall focus shifted to executing existing vulnerability data scripts without the previous data fetching and file generation processes. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
scripts/scout_vulnerabilities_data.sh (3)
Line range hint
3-12: Enhance security for credential handlingWhile the environment variable checks are good, consider these security improvements:
- Avoid exposing DB credentials in error messages
- Validate credential format/content before use
- Use read-only permissions where possible
# Check required environment variables required_vars=("DB_HOST" "DB_NAME" "DB_USER" "DB_PWD") for var in "${required_vars[@]}"; do if [ -z "${!var}" ] || [[ "${!var}" == "your_${var,,}" ]]; then - echo "Error: Required environment variable $var is missing or not set correctly." + echo "Error: Required environment variable ${var%_PWD*} is missing or not set correctly." exit 1 fi done
Line range hint
28-40: Verify installation script integrity and clean up logsThe installation process should verify script integrity and handle sensitive logs:
while [ $attempts -lt 5 ]; do echo "Attempt $((attempts + 1))..." - curl -fsSL https://raw.githubusercontent.com/docker/scout-cli/main/install.sh -o install-scout.sh + curl -fsSL https://raw.githubusercontent.com/docker/scout-cli/main/install.sh -o install-scout.sh + echo "d41d8cd98f00b204e9800998ecf8427e install-scout.sh" | md5sum -c || { echo "Script integrity check failed"; exit 1; } sh install-scout.sh &> install_scout_log.txt if [ $? -eq 0 ]; then echo "Docker Scout installed successfully." + rm -f install-scout.sh install_scout_log.txt return 0 fi
Line range hint
154-158: Propagate exit codes properlyEnsure script exit codes are properly propagated:
if [ -s "$CSV_OUTPUT_FILE" ]; then insert_vulns_into_db + exit $? else echo "No new vulnerabilities to insert." + exit 0 fi🧰 Tools
🪛 Shellcheck
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/trivy_vulnerabilities_data.sh (2)
34-36: Enhance security by verifying the Trivy download.When downloading and installing Trivy, it's best practice to verify the integrity of the downloaded file to prevent potential security risks.
Consider adding a checksum verification step:
TRIVY_URL="https://github.com/aquasecurity/trivy/releases/download/v$TRIVY_VERSION/trivy_${TRIVY_VERSION}_Linux-64bit.tar.gz" curl -sfL "$TRIVY_URL" | tar -xzf - trivy +TRIVY_CHECKSUM_URL="$TRIVY_URL".sha256sum +curl -sfL "$TRIVY_CHECKSUM_URL" -o trivy_checksums.txt +grep "trivy_${TRIVY_VERSION}_Linux-64bit.tar.gz" trivy_checksums.txt | sha256sum -c -
104-104: Declare and assigncreated_dateseparately to avoid masking return values.Assigning a command substitution directly to a local variable can mask the return status of the command.
Apply this diff:
-local created_date=$(date -u +"%Y-%m-%dT%H:%M:%SZ") +local created_date +created_date=$(date -u +"%Y-%m-%dT%H:%M:%SZ")🧰 Tools
🪛 Shellcheck
[warning] 104-104: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/test-vulnerabilities-data.yml(0 hunks)scripts/scout_vulnerabilities_data.sh(3 hunks)scripts/trivy_vulnerabilities_data.sh(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/test-vulnerabilities-data.yml
🧰 Additional context used
🪛 Shellcheck
scripts/scout_vulnerabilities_data.sh
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/trivy_vulnerabilities_data.sh
[warning] 104-104: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (2)
scripts/trivy_vulnerabilities_data.sh (2)
154-154: Avoid passing database credentials via command line arguments.
Including passwords in command-line arguments can expose them to security risks. It's better to use environment variables or a .pgpass file.
[security_issue]
Consider configuring psql to use a .pgpass file or leveraging environment variables securely.
58-58: Verify that Trivy is available in the PATH after installation.
Ensure that the trivy command is accessible after installation to prevent runtime errors.
Run the following to check Trivy's availability:
✅ Verification successful
Trivy installation and PATH verification is properly handled
The script includes robust Trivy installation logic with:
- Installation with retry mechanism (up to 5 attempts)
- Proper PATH configuration (
$HOME/binadded to PATH) - Explicit verification of Trivy availability after installation
- Appropriate error handling and exit conditions
The implementation in install_trivy_with_retry() function adequately addresses the PATH verification concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Trivy installation
if ! command -v trivy &> /dev/null; then
echo "Trivy is not installed or not in PATH."
exit 1
else
echo "Trivy is installed and available."
fi
Length of output: 114
Script:
#!/bin/bash
# Let's check the trivy installation logic in the script
cat scripts/trivy_vulnerabilities_data.sh
Length of output: 6502
| # Execute the SQL file and rollback on failure | ||
| if psql -e "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" -f "$query_file"; then | ||
| echo "Vulnerabilities successfully inserted into the database." | ||
| else | ||
| echo "Error: Failed to insert vulnerabilities. Please check the database connection or query." | ||
| echo "Error: Failed to insert vulnerabilities. Performing rollback." | ||
| echo "ROLLBACK;" | psql "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" | ||
| exit 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and cleanup
Enhance error handling with detailed logging and cleanup:
if psql -e "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" -f "$query_file"; then
echo "Vulnerabilities successfully inserted into the database."
+ rm -f "$query_file"
else
- echo "Error: Failed to insert vulnerabilities. Performing rollback."
+ echo "Error: Failed to insert vulnerabilities. Check postgresql logs. Performing rollback."
echo "ROLLBACK;" | psql "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME"
+ rm -f "$query_file"
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Execute the SQL file and rollback on failure | |
| if psql -e "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" -f "$query_file"; then | |
| echo "Vulnerabilities successfully inserted into the database." | |
| else | |
| echo "Error: Failed to insert vulnerabilities. Please check the database connection or query." | |
| echo "Error: Failed to insert vulnerabilities. Performing rollback." | |
| echo "ROLLBACK;" | psql "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" | |
| exit 1 | |
| # Execute the SQL file and rollback on failure | |
| if psql -e "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" -f "$query_file"; then | |
| echo "Vulnerabilities successfully inserted into the database." | |
| rm -f "$query_file" | |
| else | |
| echo "Error: Failed to insert vulnerabilities. Check postgresql logs. Performing rollback." | |
| echo "ROLLBACK;" | psql "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" | |
| rm -f "$query_file" | |
| exit 1 |
| if ($2 != "" && $3 ~ /^CVE-/) { | ||
| # Extract severity level, CVE ID and format output correctly | ||
| # Extract severity level, CVE ID, and format output correctly |
There was a problem hiding this comment.
Prevent CSV injection in vulnerability data
The current implementation is vulnerable to CSV injection. Add proper escaping:
- print $3","product_name",""SCOUT"","$2
+ gsub(/[,"]/, "\\\\&", $3)
+ gsub(/[,"]/, "\\\\&", $2)
+ print $3 "," product_name ",\"SCOUT\"," $2Committable suggestion skipped: line range outside the PR's diff.
| # Fetch existing values for this vulnerability ID | ||
| existing_entry=$(psql -t -c "SELECT product, scanner_tool FROM vulnerability_tracking WHERE vurn_id = '$vurn_id'" "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" 2>/dev/null) | ||
|
|
||
| ((count++)) | ||
| done < "scout_vulnerabilities_diff.csv" | ||
| # Process fetched data | ||
| if [[ -z "$existing_entry" ]]; then | ||
| combined_products="$product" | ||
| combined_scanner_tools="$scanner_tool" | ||
| else | ||
| IFS='|' read -r existing_product existing_scanner_tool <<< "$existing_entry" | ||
| combined_products=$(echo "$existing_product,$product" | tr ',' '\n' | sed '/^$/d' | sort -u | tr '\n' ',' | sed 's/^,//; s/,$//') | ||
| combined_scanner_tools=$(echo "$existing_scanner_tool,$scanner_tool" | tr ',' '\n' | sed '/^$/d' | sort -u | tr '\n' ',' | sed 's/^,//; s/,$//') | ||
| fi | ||
|
|
||
| echo "COMMIT;" >> "$query_file" # End the transaction | ||
| # Write the insert query to the SQL file | ||
| echo "INSERT INTO vulnerability_tracking (product, scanner_tool, vurn_id, priority, pr_id, pr_link, github_run_id, created_date, update_date, comments, owner, pod) | ||
| VALUES ('$combined_products', '$combined_scanner_tools', '$vurn_id', '$priority', '$pr_id', '$pr_link', '$GITHUB_RUN_ID', '$created_date', '$created_date', '$comments', '$owner', '$pod') | ||
| ON CONFLICT (vurn_id) | ||
| DO UPDATE SET | ||
| product = '$combined_products', | ||
| scanner_tool = '$combined_scanner_tools', | ||
| priority = EXCLUDED.priority, | ||
| pr_id = EXCLUDED.pr_id, | ||
| pr_link = EXCLUDED.pr_link, | ||
| github_run_id = EXCLUDED.github_run_id, | ||
| update_date = EXCLUDED.update_date, | ||
| comments = EXCLUDED.comments, | ||
| owner = EXCLUDED.owner, | ||
| pod = EXCLUDED.pod;" >> "$query_file" |
There was a problem hiding this comment.
💡 Codebase verification
Critical: SQL Injection vulnerabilities confirmed in vulnerability tracking scripts
The scripts are vulnerable to SQL injection through direct variable interpolation in SQL queries. Key issues:
- Direct variable interpolation in SELECT queries:
WHERE vurn_id = '$vurn_id' - Unescaped variables in INSERT/UPDATE statements
- Basic string cleaning (
sed "s/'/''/g") is insufficient for SQL injection prevention
Fix by using psql's built-in parameterization:
- existing_entry=$(psql -t -c "SELECT product, scanner_tool FROM vulnerability_tracking WHERE vurn_id = '$vurn_id'" ...)
+ existing_entry=$(psql -t -c "SELECT product, scanner_tool FROM vulnerability_tracking WHERE vurn_id = :vurn_id" -v vurn_id="$vurn_id" ...)For the INSERT/UPDATE queries, use a prepared statement file with parameters:
\set ON_ERROR_STOP on
PREPARE vuln_insert AS
INSERT INTO vulnerability_tracking (...) VALUES (:products, :scanner_tools, ...)
ON CONFLICT (vurn_id) DO UPDATE SET ...;🔗 Analysis chain
Critical: Fix SQL injection vulnerabilities
The current implementation is vulnerable to SQL injection attacks. Use prepared statements:
Replace direct variable interpolation with prepared statements:
- existing_entry=$(psql -t -c "SELECT product, scanner_tool FROM vulnerability_tracking WHERE vurn_id = '$vurn_id'" "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" 2>/dev/null)
+ existing_entry=$(psql -t -c "SELECT product, scanner_tool FROM vulnerability_tracking WHERE vurn_id = $1" -v vurn_id="$vurn_id" "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" 2>/dev/null)These commands will help us:
- Find SQL queries with variable interpolation
- Analyze the complete script context
- Identify other database connection patterns
This will give us a better understanding of the SQL injection concerns and database interaction patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify potential SQL injection points
rg -l "psql.*\$[a-zA-Z_]" .
Length of output: 28
Script:
#!/bin/bash
# Search for SQL queries in the script with a more specific pattern
rg -l "psql.*-c.*\$" .
Length of output: 233
Script:
#!/bin/bash
# Get the content of the script to analyze SQL usage
cat scripts/scout_vulnerabilities_data.sh
Length of output: 5818
Script:
#!/bin/bash
# Look for other database interaction patterns
rg "postgresql://" .
Length of output: 5956
| existing_entry=$(psql -t -c "SELECT product, scanner_tool FROM vulnerability_tracking WHERE vurn_id = '$vurn_id'" "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" 2>/dev/null) | ||
|
|
There was a problem hiding this comment.
Enhance security by using parameterized queries with psql.
Directly embedding variables into SQL statements can risk SQL injection attacks. It's safer to use parameterized queries.
Refactor the SQL commands to use psql with variables:
psql "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" <<EOF
INSERT INTO vulnerability_tracking (product, scanner_tool, vuln_id, priority, ...)
VALUES (\$1, \$2, \$3, \$4, ...)
ON CONFLICT (vuln_id)
DO UPDATE SET ...
EOFAdjust the script to pass the variables accordingly.
| trivy image --db-repository public.ecr.aws/aquasecurity/trivy-db --java-db-repository public.ecr.aws/aquasecurity/trivy-java-db --insecure --format json "$IMAGE" > "trivy_vulnerabilities.json" || { | ||
| echo "Error: Trivy scan failed for image: $IMAGE" | ||
| exit 1 |
There was a problem hiding this comment.
Avoid using the --insecure flag with Trivy scans.
The --insecure flag disables SSL certificate validation, which can expose the scan to man-in-the-middle attacks. Unless absolutely necessary, it's recommended to remove this flag.
Apply this diff to enhance security:
trivy image --db-repository public.ecr.aws/aquasecurity/trivy-db --java-db-repository public.ecr.aws/aquasecurity/trivy-java-db --insecure --format json "$IMAGE" > "trivy_vulnerabilities.json" || {
- echo "Error: Trivy scan failed for image: $IMAGE"
+ echo "Error: Trivy scan failed for image: $IMAGE. Ensure the image name is correct and network connectivity is available."
exit 1
}Committable suggestion skipped: line range outside the PR's diff.
| if [[ -z "$vurn_id" || -z "$priority" || -z "$product" || -z "$scanner_tool" ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| # Insert new vulnerabilities into the PostgreSQL database using psql | ||
| insert_vulns_into_db() { | ||
| local count=0 | ||
| local query_file="insert_vulns.sql" | ||
| echo "BEGIN;" > "$query_file" # Start the transaction | ||
|
|
||
| # Create an associative array to hold existing entries from the database | ||
| declare -A existing_entries | ||
|
|
||
| # Fetch existing vulnerabilities from the database to avoid duplicates | ||
| psql -t -c "SELECT vurn_id, product, scanner_tool, priority FROM vulnerability_tracking WHERE scanner_tool = 'TRIVY'" "postgresql://$DB_USER:$DB_PWD@$DB_HOST/$DB_NAME" | while IFS='|' read -r db_vurn_id db_product db_scanner_tool db_priority; do | ||
| existing_entries["$db_product,$db_scanner_tool,$db_vurn_id"]="$db_priority" | ||
| done | ||
|
|
||
| while IFS=, read -r vurn_id product scanner_tool priority; do | ||
| # Skip empty lines | ||
| if [[ -z "$vurn_id" || -z "$priority" || -z "$product" || -z "$scanner_tool" ]]; then | ||
| echo "Skipping empty vulnerability entry" | ||
| continue | ||
| fi | ||
| local pr_id="${GITHUB_PR_ID:-}" | ||
| local pr_link="${GITHUB_PR_LINK:-}" | ||
| local created_date=$(date -u +"%Y-%m-%dT%H:%M:%SZ") | ||
| local comments="Initial vulnerability report" | ||
| local owner="John Doe" | ||
| local pod="Security" | ||
|
|
||
| # Remove spaces and redundant commas, and escape single quotes for SQL | ||
| vurn_id=$(echo "$vurn_id" | sed "s/'/''/g") | ||
| priority=$(echo "$priority" | sed "s/'/''/g") | ||
| product=$(echo "$product" | sed "s/'/''/g" | tr -d ' ' | sed 's/,*$//') | ||
| scanner_tool=$(echo "$scanner_tool" | sed "s/'/''/g" | tr -d ' ' | sed 's/,*$//') |
There was a problem hiding this comment.
Correct typo: Rename vurn_id to vuln_id for clarity.
The variable vurn_id appears to be a typo. Renaming it to vuln_id improves readability and aligns with standard terminology.
Apply this diff to correct the variable name:
while IFS=, read -r vurn_id product scanner_tool priority; do
- if [[ -z "$vurn_id" || -z "$priority" || -z "$product" || -z "$scanner_tool" ]]; then
+ if [[ -z "$vuln_id" || -z "$priority" || -z "$product" || -z "$scanner_tool" ]]; then
continue
fi
local pr_id="${GITHUB_PR_ID:-}"
local pr_link="${GITHUB_PR_LINK:-}"
local created_date=$(date -u +"%Y-%m-%dT%H:%M:%SZ")
local comments="Initial vulnerability report"
local owner="John Doe"
local pod="Security"
# Remove spaces and redundant commas, and escape single quotes for SQL
- vurn_id=$(echo "$vurn_id" | sed "s/'/''/g")
+ vuln_id=$(echo "$vuln_id" | sed "s/'/''/g")
priority=$(echo "$priority" | sed "s/'/''/g")
product=$(echo "$product" | sed "s/'/''/g" | tr -d ' ' | sed 's/,*$//')
scanner_tool=$(echo "$scanner_tool" | sed "s/'/''/g" | tr -d ' ' | sed 's/,*$//')Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck
[warning] 104-104: Declare and assign separately to avoid masking return values.
(SC2155)
sagar-qa007
added
ok-to-test
Description
optimise the view for large number of records
Fixes #
37036Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11677745846
Commit: 083266c
Cypress dashboard.
Tags:
@tag.IDESpec:
Tue, 05 Nov 2024 05:12:55 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor