Skip to content

Add -e (--exclude-tasks) param to gradle-check.sh#6043

Open
cwperks wants to merge 1 commit intoopensearch-project:mainfrom
cwperks:exclude-tasks
Open

Add -e (--exclude-tasks) param to gradle-check.sh#6043
cwperks wants to merge 1 commit intoopensearch-project:mainfrom
cwperks:exclude-tasks

Conversation

@cwperks
Copy link
Member

@cwperks cwperks commented Mar 23, 2026

Description

This PR is part of a series of 3:

This PR adds a new param -e to gradle-check.sh so that OpenSearch core's invocation can pass a list of tasks to exclude: https://github.com/opensearch-project/OpenSearch/blob/main/.github/workflows/gradle-check.yml#L131

Issues Resolved

Related to: opensearch-project/OpenSearch#19378

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit fa527f4.

PathLineSeverityDescription
scripts/gradle/gradle-check.sh66mediumThe EXCLUDE_TASKS value is interpolated directly into a JSON payload string without sanitization. A malicious caller passing shell metacharacters or JSON-injection content via the -e flag could manipulate the payload sent to the Jenkins webhook. This is not clearly intentional malice, but the lack of quoting/escaping for an externally supplied value in a CI trigger context warrants review.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 Security concerns

JSON Injection:
In scripts/gradle/gradle-check.sh, the EXCLUDE_TASKS variable (from user-supplied -e argument) is interpolated directly into a manually constructed JSON string without escaping. A crafted input like task1", "injected_key": "injected_value would produce malformed or manipulated JSON sent to the Jenkins webhook endpoint. This could allow a caller to inject arbitrary fields into the trigger payload.

✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

JSON Injection

The EXCLUDE_TASKS value is inserted directly into a JSON string without sanitization or escaping. If a caller passes a value containing double quotes or other JSON special characters (e.g., -e 'task1", "malicious_key": "value'), it will break the JSON structure of PAYLOAD_JSON and could inject arbitrary fields into the payload sent to Jenkins.

PAYLOAD_JSON="{\"pr_from_sha\": \"$pr_from_sha\", \"pr_from_clone_url\": \"$pr_from_clone_url\", \"pr_to_clone_url\": \"$pr_to_clone_url\", \"pr_title\": \"$PR_TITLE_NEW\", \"pr_number\": \"$pr_number\", \"post_merge_action\": \"$post_merge_action\", \"pr_owner\": \"$pr_owner\", \"exclude_tasks\": \"$EXCLUDE_TASKS\"}"
Uninitialized Variable

In the "Triggered by User or Triggered by Timer" branch, exclude_tasks is used but it is only defined as a generic trigger variable. When the pipeline is triggered manually or by a timer (not via the generic webhook), exclude_tasks may be undefined or empty string, which could cause unexpected behavior depending on how runGradleCheck handles an empty excludeTasks argument. The defaultValue: '' on the trigger variable only applies to webhook triggers.

excludeTasks: "${exclude_tasks}"

@github-actions
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle undefined variable in manual trigger branch

In the "Triggered by User or Triggered by Timer" branch, exclude_tasks comes from
the generic webhook trigger variable, which may not be defined when triggered
manually or by a timer. This could cause a MissingPropertyException or use an
unintended value. Consider using a pipeline parameter or a safe fallback like
params.EXCLUDE_TASKS ?: '' for this branch.

jenkins/gradle/gradle-check.jenkinsfile [152]

-excludeTasks: "${exclude_tasks}"
+excludeTasks: "${params.EXCLUDE_TASKS ?: ''}"
Suggestion importance[1-10]: 6

__

Why: When triggered manually or by a timer, exclude_tasks may not be defined as it comes from the generic webhook trigger. Using params.EXCLUDE_TASKS ?: '' would be safer, though the actual behavior depends on how Jenkins handles undefined webhook variables (often defaults to empty string via defaultValue: '').

Low
Security
Safely construct JSON to prevent injection

The EXCLUDE_TASKS value is interpolated directly into the JSON string without
sanitization, which could break the JSON structure if the value contains special
characters like quotes or backslashes. Consider sanitizing or escaping the value
before embedding it, or use a tool like jq to safely construct the JSON payload.

scripts/gradle/gradle-check.sh [66]

-PAYLOAD_JSON="{\"pr_from_sha\": \"$pr_from_sha\", \"pr_from_clone_url\": \"$pr_from_clone_url\", \"pr_to_clone_url\": \"$pr_to_clone_url\", \"pr_title\": \"$PR_TITLE_NEW\", \"pr_number\": \"$pr_number\", \"post_merge_action\": \"$post_merge_action\", \"pr_owner\": \"$pr_owner\", \"exclude_tasks\": \"$EXCLUDE_TASKS\"}"
+PAYLOAD_JSON=$(jq -n \
+  --arg pr_from_sha "$pr_from_sha" \
+  --arg pr_from_clone_url "$pr_from_clone_url" \
+  --arg pr_to_clone_url "$pr_to_clone_url" \
+  --arg pr_title "$PR_TITLE_NEW" \
+  --arg pr_number "$pr_number" \
+  --arg post_merge_action "$post_merge_action" \
+  --arg pr_owner "$pr_owner" \
+  --arg exclude_tasks "$EXCLUDE_TASKS" \
+  '{pr_from_sha: $pr_from_sha, pr_from_clone_url: $pr_from_clone_url, pr_to_clone_url: $pr_to_clone_url, pr_title: $pr_title, pr_number: $pr_number, post_merge_action: $post_merge_action, pr_owner: $pr_owner, exclude_tasks: $exclude_tasks}')
Suggestion importance[1-10]: 6

__

Why: Direct interpolation of $EXCLUDE_TASKS into the JSON string could break the JSON structure if the value contains special characters. Using jq to construct the payload is a valid security improvement, though the risk is moderate since this is an internal CI script.

Low

@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.58%. Comparing base (657fa6c) to head (fa527f4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6043   +/-   ##
=======================================
  Coverage   96.58%   96.58%           
=======================================
  Files         405      405           
  Lines       18758    18758           
=======================================
  Hits        18118    18118           
  Misses        640      640           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant