Skip to content

Commit 38e4ee2

Browse files
varadarajan-twclaude
varadarajan-tw
and
claude
authored
Fix command injection in required-field-check workflow (#3642)
* Fix command injection in required-field-check workflow (CWE-78) Remediate OS command injection vulnerability where PR filenames containing shell metacharacters (e.g., $()) could execute arbitrary commands on the Actions runner. Untrusted filenames from the GitHub API were interpolated unquoted into bash via ${{ }} expressions. Fix: output files as JSON array, pass through env variable (not inline interpolation), parse safely with jq, and build args with proper quoting. Ref: HackerOne #3526875 Co-Authored-By: Claude Opus 4.6 <[email protected]> * Skip required-field warnings for new destinations and new actions New destinations have no existing customers and new actions have no existing configurations, so adding required fields to them is safe. Only warn when required fields are added to actions/settings that already exist on main. Co-Authored-By: Claude Opus 4.6 <[email protected]> * noop * Use printf instead of echo for safer JSON piping to jq * Address Copilot review: pipefail, jq -e, and paginate PR files - set -euo pipefail on both bash steps so pipeline failures from ./bin/run are not masked by jq's exit code - jq -ce to fail on empty/null output - github.paginate() with per_page:100 to fetch all changed files (github.request only returned the first page, default 30 files) Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
1 parent 82ecca5 commit 38e4ee2

1 file changed

Lines changed: 41 additions & 34 deletions

File tree

‎.github/workflows/required-field-check.yml‎

Lines changed: 41 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -33,30 +33,38 @@ jobs:
3333
uses: actions/github-script@v7
3434
with:
3535
script: |
36-
const response = await github.request('GET /repos/{owner}/{repo}/pulls/{pull_number}/files', {
36+
const files = await github.paginate('GET /repos/{owner}/{repo}/pulls/{pull_number}/files', {
3737
owner: context.repo.owner,
3838
repo: context.repo.repo,
39-
pull_number: context.issue.number
40-
})
41-
const files = response.data.map(file => file.filename)
42-
core.setOutput('files', files.join(' '))
39+
pull_number: context.issue.number,
40+
per_page: 100
41+
}, response => response.data.map(file => file.filename))
42+
core.setOutput('files', JSON.stringify(files))
4343
if (files.length === 0) {
4444
core.setOutput('no_files_changed', true)
4545
}
4646
4747
- name: Find required fields on current branch
4848
id: required-fields-curr-branch
4949
if: steps.get_files.outputs.no_files_changed != 'true'
50+
env:
51+
CHANGED_FILES: ${{ steps.get_files.outputs.files }}
5052
run: |
51-
set -e
52-
REQUIRED_FIELDS_CURR_BRANCH=$(./bin/run list-required-fields -p ${{ steps.get_files.outputs.files }} | jq -c .)
53+
set -euo pipefail
54+
ARGS=()
55+
while IFS= read -r file; do
56+
ARGS+=(-p "$file")
57+
done < <(printf '%s' "$CHANGED_FILES" | jq -r '.[]')
58+
REQUIRED_FIELDS_CURR_BRANCH=$(./bin/run list-required-fields "${ARGS[@]}" | jq -ce .)
5359
echo "REQUIRED_FIELDS_CURR_BRANCH=$REQUIRED_FIELDS_CURR_BRANCH" >> $GITHUB_ENV
5460
5561
- name: Check for required fields on main branch
5662
id: required-fields-main-branch
5763
if: steps.get_files.outputs.no_files_changed != 'true'
64+
env:
65+
CHANGED_FILES: ${{ steps.get_files.outputs.files }}
5866
run: |
59-
set -e
67+
set -euo pipefail
6068
git checkout main
6169
# Run install again on main branch to ensure all dependencies are installed
6270
# and the lockfile is up to date
@@ -65,7 +73,11 @@ jobs:
6573
# checked against the correct version of the dependencies
6674
yarn install --frozen-lockfile --silent
6775
# Run the list-required-fields command on the main branch to get the required fields
68-
REQUIRED_FIELDS_MAIN_BRANCH=$(./bin/run list-required-fields -p ${{ steps.get_files.outputs.files }} | jq -c .)
76+
ARGS=()
77+
while IFS= read -r file; do
78+
ARGS+=(-p "$file")
79+
done < <(printf '%s' "$CHANGED_FILES" | jq -r '.[]')
80+
REQUIRED_FIELDS_MAIN_BRANCH=$(./bin/run list-required-fields "${ARGS[@]}" | jq -ce .)
6981
echo "REQUIRED_FIELDS_MAIN_BRANCH=$REQUIRED_FIELDS_MAIN_BRANCH" >> $GITHUB_ENV
7082
7183
- name: Add comment on PR if there is diff in required fields
@@ -82,34 +94,29 @@ jobs:
8294
const requiredFieldsOnMain = JSON.parse(process.env.REQUIRED_FIELDS_MAIN_BRANCH)
8395
8496
Object.keys(requiredFieldsOnBranch).forEach(key => {
85-
// Check if key is present in requiredFieldsOnMain
86-
if(requiredFieldsOnMain[key]) {
87-
const getActionKeys = Object.keys(requiredFieldsOnBranch[key])
88-
for(const actionKey of getActionKeys) {
89-
const branchRequiredFields = requiredFieldsOnBranch[key][actionKey]
90-
const mainRequiredFields = requiredFieldsOnMain[key][actionKey]
91-
const diff = branchRequiredFields.filter(field => !mainRequiredFields?.includes(field))
92-
if(diff.length > 0) {
93-
const isSettingsKey = actionKey === 'settings'
94-
if (isSettingsKey) {
95-
fieldsAdded.push(`- **Destination**: ${key}, **Settings**:${diff.join(',')}`)
96-
} else {
97-
fieldsAdded.push(`- **Destination**: ${key}, Action **Field(s)**:${diff.join(',')}`)
98-
}
99-
}
97+
// Skip new destinations — no existing customers to break
98+
if(!requiredFieldsOnMain[key]) {
99+
return
100+
}
101+
102+
const getActionKeys = Object.keys(requiredFieldsOnBranch[key])
103+
for(const actionKey of getActionKeys) {
104+
// Skip new actions — no existing configurations to break
105+
if(!requiredFieldsOnMain[key][actionKey]) {
106+
continue
100107
}
101-
} else {
102-
103-
// If key is not present in requiredFieldsOnMain, then all fields are added recently
104-
const getActionKeys = Object.keys(requiredFieldsOnBranch[key])
105-
for(const actionKey of getActionKeys) {
106-
const branchRequiredFields = requiredFieldsOnBranch[key][actionKey]
107-
if(actionKey === 'settings') {
108-
fieldsAdded.push(`- **Destination**: ${key}, **Settings**:${branchRequiredFields.join(',')}`)
108+
109+
const branchRequiredFields = requiredFieldsOnBranch[key][actionKey]
110+
const mainRequiredFields = requiredFieldsOnMain[key][actionKey]
111+
const diff = branchRequiredFields.filter(field => !mainRequiredFields.includes(field))
112+
if(diff.length > 0) {
113+
const isSettingsKey = actionKey === 'settings'
114+
if (isSettingsKey) {
115+
fieldsAdded.push(`- **Destination**: ${key}, **Settings**:${diff.join(',')}`)
109116
} else {
110-
fieldsAdded.push(`- **Destination**: ${key}, **Action**:${actionKey}, **Fields**:${branchRequiredFields.join(',')}`)
117+
fieldsAdded.push(`- **Destination**: ${key}, Action **Field(s)**:${diff.join(',')}`)
111118
}
112-
}
119+
}
113120
}
114121
})
115122
}

0 commit comments

Comments
 (0)