Skip to content
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.

Sign up for GitHub

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

Fix: Fix for GHSL-2024-277 #36836

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Fix: Fix for GHSL-2024-277 #36836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e63a5b
Fix for GHSL-2024-277
sagar-qa007 Oct 11, 2024
18af836
Comment update
sagar-qa007 Oct 14, 2024
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
14 changes: 8 additions & 6 deletions .github/workflows/ci-test-limited-with-count.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,16 +266,18 @@ jobs:
- name: Set Commit Message
env:
EVENT_COMMITS: ${{ toJson(github.event.commits[0].message) }}
COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }}
run: |
if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then
if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV
if [[ "$EVENT_COMMITS" == "null" ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
else
echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV
COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
fi
Comment on lines +269 to 281
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Let's improve our commit message handling, shall we?

Class, you've made a commendable effort in updating the commit message handling. It's like you're writing a more detailed entry in our class journal. However, we need to make a few corrections:

  1. On lines 271 and 273, we're using -ne for string comparison. In bash, we should use != for comparing strings. It's like using the correct comparison operator in our math class!

  2. We need to put our variables in double quotes to prevent any unexpected word splitting. It's similar to putting quotation marks around a direct quote in our English essays.

Let's fix these issues:

- if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
+ if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then
  echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
- elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
+ elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then
  echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
  if [[ "$EVENT_COMMITS" == "null" ]]; then
-   echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
+   echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> "$GITHUB_ENV"
  else
-   COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
+   COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^"//')
-   echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
+   echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> "$GITHUB_ENV"
  fi
fi

These changes will make our code more robust, just like double-checking our work in class!

📝 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.

Suggested change
COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }}
run: |
if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then
if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV
if [[ "$EVENT_COMMITS" == "null" ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
else
echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV
COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
fi
COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }}
run: |
if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ "$EVENT_COMMITS" == "null" ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> "$GITHUB_ENV"
else
COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^"//')
echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> "$GITHUB_ENV"
fi
🧰 Tools
🪛 actionlint

270-270: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)

(shellcheck)

270-270: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting

(shellcheck)

270-270: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)

(shellcheck)

270-270: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting

(shellcheck)

270-270: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting

(shellcheck)

270-270: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting

(shellcheck)

fi

Expand Down
14 changes: 8 additions & 6 deletions .github/workflows/ci-test-limited.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,16 +256,18 @@ jobs:
- name: Set Commit Message
env:
EVENT_COMMITS: ${{ toJson(github.event.commits[0].message) }}
COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }}
run: |
if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then
if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV
if [[ "$EVENT_COMMITS" == "null" ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
else
echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV
COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
fi
Comment on lines +259 to 271
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Class, let's examine the changes in our "Set Commit Message" step.

I'm pleased to see improvements in how we're handling our commit messages. However, there are a few areas where we can make our code even better:

  1. We've introduced a new environment variable COMMIT_INFO_AUTHOR. This is good practice for capturing the author's name.

  2. The logic for setting the commit message has been improved. We're now handling different scenarios more effectively:

    • For repository dispatch events with a PR number
    • For workflow dispatch events with a PR number
    • For other cases, we're extracting the first line of the commit message

However, there are a few areas where we need to make corrections:

  1. In our comparisons, we're using -ne for numeric comparison. While this works, it's more appropriate to use != for string comparisons in this context. Let's update lines 261 and 263:
-if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
+if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then
-elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
+elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then
  1. We should quote our variables to prevent potential issues with word splitting and globbing. For example, on line 262:
-echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
+echo "COMMIT_INFO_MESSAGE=${COMMIT_INFO_MESSAGE}" >> $GITHUB_ENV
  1. Similarly, let's quote the $EVENT_COMMITS variable on line 266:
-if [[ "$EVENT_COMMITS" == "null" ]]; then
+if [[ "${EVENT_COMMITS}" == "null" ]]; then
  1. For consistency, let's also quote $COMMIT_INFO_AUTHOR on line 267:
-echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
+echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${COMMIT_INFO_AUTHOR}" >> $GITHUB_ENV
  1. Lastly, let's quote $COMMIT_FIRST_LINE on line 270:
-echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
+echo "COMMIT_INFO_MESSAGE=${COMMIT_FIRST_LINE}" >> $GITHUB_ENV

Make these changes, and your code will be much more robust and follow best practices. Any questions, class?

📝 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.

Suggested change
COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }}
run: |
if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then
if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV
if [[ "$EVENT_COMMITS" == "null" ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
else
echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV
COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
fi
COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }}
run: |
if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=${COMMIT_INFO_MESSAGE}" >> $GITHUB_ENV
elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ "${EVENT_COMMITS}" == "null" ]]; then
echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${COMMIT_INFO_AUTHOR}" >> $GITHUB_ENV
else
COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
echo "COMMIT_INFO_MESSAGE=${COMMIT_FIRST_LINE}" >> $GITHUB_ENV
fi
🧰 Tools
🪛 actionlint

260-260: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)

(shellcheck)

260-260: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting

(shellcheck)

260-260: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)

(shellcheck)

260-260: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting

(shellcheck)

260-260: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting

(shellcheck)

260-260: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting

(shellcheck)

fi

Expand Down
Loading