From 0102c07446a3cad972f4afcbd0ee4dbc4b6d2d1b Mon Sep 17 00:00:00 2001 From: Jorge <46056498+jorgectf@users.noreply.github.com> Date: Fri, 22 Dec 2023 22:07:32 +0100 Subject: [PATCH] Merge pull request from GHSA-mcph-m25j-8j63 * feat: add `safe_output` input enabled by default * fix: migrate README to safe uses of interpolation * fix: README `uses` typo * fix: README examples to account for newlines * fix: README examples missing `safe_output` * fix: remove sanitization of `'` * fix: also sanitize `|&;` --- README.md | 84 ++++++++++++++++++++++++++++++++------- action.yml | 4 ++ src/changedFilesOutput.ts | 33 ++++++++++----- src/inputs.ts | 3 ++ src/main.ts | 3 +- src/utils.ts | 12 +++++- 6 files changed, 110 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index d32e01a8fb8..431aefb0caf 100644 --- a/README.md +++ b/README.md @@ -123,14 +123,19 @@ jobs: - name: Get changed files id: changed-files uses: tj-actions/changed-files@v40 + with: + safe_output: false # true by default, set to false because we are using an environment variable to store the output and avoid command injection. # To compare changes between the current commit and the last pushed remote commit set `since_last_remote_commit: true`. e.g # with: # since_last_remote_commit: true - name: List all changed files + env: + ALL_CHANGED_FILES: |- + ${{ steps.changed-files.outputs.all_changed_files }} run: | - for file in ${{ steps.changed-files.outputs.all_changed_files }}; do + for file in "$ALL_CHANGED_FILES"; do echo "$file was changed" done @@ -139,14 +144,18 @@ jobs: id: changed-markdown-files uses: tj-actions/changed-files@v40 with: + safe_output: false # true by default, set to false because we are using an environment variable to store the output and avoid command injection. # Avoid using single or double quotes for multiline patterns files: | **.md - name: List all changed files markdown files if: steps.changed-markdown-files.outputs.any_changed == 'true' + env: + ALL_CHANGED_FILES: |- + ${{ steps.changed-markdown-files.outputs.all_changed_files }} run: | - for file in ${{ steps.changed-markdown-files.outputs.all_changed_files }}; do + for file in "$ALL_CHANGED_FILES"; do echo "$file was changed" done @@ -155,6 +164,7 @@ jobs: id: changed-files-yaml uses: tj-actions/changed-files@v40 with: + safe_output: false # true by default, set to false because we are using an environment variable to store the output and avoid command injection. files_yaml: | doc: - '**.md' @@ -170,29 +180,39 @@ jobs: - name: Run step if test file(s) change # NOTE: Ensure all outputs are prefixed by the same key used above e.g. `test_(...)` | `doc_(...)` | `src_(...)` when trying to access the `any_changed` output. if: steps.changed-files-yaml.outputs.test_any_changed == 'true' + env: + TEST_ALL_CHANGED_FILES: |- + ${{ steps.changed-files-yaml.outputs.test_all_changed_files }} run: | echo "One or more test file(s) has changed." - echo "List all the files that have changed: ${{ steps.changed-files-yaml.outputs.test_all_changed_files }}" + echo "List all the files that have changed: $TEST_ALL_CHANGED_FILES" - name: Run step if doc file(s) change if: steps.changed-files-yaml.outputs.doc_any_changed == 'true' + env: + DOC_ALL_CHANGED_FILES: |- + ${{ steps.changed-files-yaml.outputs.doc_all_changed_files }} run: | echo "One or more doc file(s) has changed." - echo "List all the files that have changed: ${{ steps.changed-files-yaml.outputs.doc_all_changed_files }}" + echo "List all the files that have changed: $DOC_ALL_CHANGED_FILES" # Example 3 - name: Get changed files in the docs folder id: changed-files-specific uses: tj-actions/changed-files@v40 with: + safe_output: false # true by default, set to false because we are using an environment variable to store the output and avoid command injection. files: docs/*.{js,html} # Alternatively using: `docs/**` files_ignore: docs/static.js - name: Run step if any file(s) in the docs folder change if: steps.changed-files-specific.outputs.any_changed == 'true' + env: + ALL_CHANGED_FILES: |- + ${{ steps.changed-files-specific.outputs.all_changed_files }} run: | echo "One or more files in the docs folder has changed." - echo "List all the files that have changed: ${{ steps.changed-files-specific.outputs.all_changed_files }}" + echo "List all the files that have changed: $ALL_CHANGED_FILES" ``` #### Using Github's API :octocat: @@ -224,10 +244,15 @@ jobs: - name: Get changed files id: changed-files uses: tj-actions/changed-files@v40 + with: + safe_output: false # true by default, set to false because we are using an environment variable to store the output and avoid command injection. - name: List all changed files + env: + ALL_CHANGED_FILES: |- + ${{ steps.changed-files.outputs.all_changed_files }} run: | - for file in ${{ steps.changed-files.outputs.all_changed_files }}; do + for file in "$ALL_CHANGED_FILES"; do echo "$file was changed" done ``` @@ -265,12 +290,17 @@ jobs: - name: Get changed files id: changed-files uses: tj-actions/changed-files@v40 + with: + safe_output: false # true by default, set to false because we are using an environment variable to store the output and avoid command injection. # NOTE: `since_last_remote_commit: true` is implied by default and falls back to the previous local commit. - name: List all changed files + env: + ALL_CHANGED_FILES: |- + ${{ steps.changed-files.outputs.all_changed_files }} run: | - for file in ${{ steps.changed-files.outputs.all_changed_files }}; do + for file in "$ALL_CHANGED_FILES"; do echo "$file was changed" done ... @@ -715,10 +745,15 @@ See [inputs](#inputs) for more information. - name: Get changed files id: changed-files uses: tj-actions/changed-files@v40 + with: + safe_output: false - name: List all added files + env: + ADDED_FILES: |- + ${{ steps.changed-files.outputs.added_files }} run: | - for file in ${{ steps.changed-files.outputs.added_files }}; do + for file in "$ADDED_FILES"; do echo "$file was added" done ... @@ -736,6 +771,8 @@ See [outputs](#outputs) for a list of all available outputs. - name: Get changed files id: changed-files uses: tj-actions/changed-files@v40 + with: + safe_output: false - name: Run a step if my-file.txt was modified if: contains(steps.changed-files.outputs.modified_files, 'my-file.txt') @@ -756,8 +793,9 @@ See [outputs](#outputs) for a list of all available outputs. - name: Get changed files and write the outputs to a Txt file id: changed-files-write-output-files-txt - uses: ./ + uses: tj-actions/changed-files@v40 with: + safe_output: false write_output_files: true - name: Verify the contents of the .github/outputs/added_files.txt file @@ -775,8 +813,9 @@ See [outputs](#outputs) for a list of all available outputs. ... - name: Get changed files and write the outputs to a JSON file id: changed-files-write-output-files-json - uses: ./ + uses: tj-actions/changed-files@v40 with: + safe_output: false json: true write_output_files: true @@ -820,6 +859,7 @@ See [inputs](#inputs) for more information. id: changed-files-specific uses: tj-actions/changed-files@v40 with: + safe_output: false files: | my-file.txt *.sh @@ -840,15 +880,21 @@ See [inputs](#inputs) for more information. - name: Run step if any of the listed files above is deleted if: steps.changed-files-specific.outputs.any_deleted == 'true' + env: + DELETED_FILES: |- + ${{ steps.changed-files-specific.outputs.deleted_files }} run: | - for file in ${{ steps.changed-files-specific.outputs.deleted_files }}; do + for file in "$DELETED_FILES"; do echo "$file was deleted" done - name: Run step if all listed files above have been deleted if: steps.changed-files-specific.outputs.only_deleted == 'true' + env: + DELETED_FILES: |- + ${{ steps.changed-files-specific.outputs.deleted_files }} run: | - for file in ${{ steps.changed-files-specific.outputs.deleted_files }}; do + for file in "$DELETED_FILES"; do echo "$file was deleted" done ... @@ -958,14 +1004,18 @@ jobs: id: changed-files-specific uses: tj-actions/changed-files@v40 with: + safe_output: false base_sha: ${{ steps.get-base-sha.outputs.base_sha }} files: .github/** - name: Run step if any file(s) in the .github folder change if: steps.changed-files-specific.outputs.any_changed == 'true' + env: + ALL_CHANGED_FILES: |- + ${{ steps.changed-files-specific.outputs.all_changed_files }} run: | echo "One or more files in the .github folder has changed." - echo "List all the files that have changed: ${{ steps.changed-files-specific.outputs.all_changed_files }}" + echo "List all the files that have changed: $ALL_CHANGED_FILES" ... ``` @@ -988,11 +1038,15 @@ See [inputs](#inputs) for more information. id: changed-files-for-dir1 uses: tj-actions/changed-files@v40 with: + safe_output: false path: dir1 - name: List all added files in dir1 + env: + ADDED_FILES: |- + ${{ steps.changed-files-for-dir1.outputs.added_files }} run: | - for file in ${{ steps.changed-files-for-dir1.outputs.added_files }}; do + for file in "$ADDED_FILES"; do echo "$file was added" done ... @@ -1015,7 +1069,7 @@ See [inputs](#inputs) for more information. - name: Run changed-files with quotepath disabled for a specified list of file(s) id: changed-files-quotepath-specific - uses: ./ + uses: tj-actions/changed-files@v40 with: files: test/test-รจ.txt quotepath: "false" diff --git a/action.yml b/action.yml index ecb678102c7..70386574211 100644 --- a/action.yml +++ b/action.yml @@ -134,6 +134,10 @@ inputs: description: "Escape JSON output." required: false default: "true" + safe_output: + description: "Apply sanitization to output filenames before being set as output." + required: false + default: "true" fetch_depth: description: "Depth of additional branch history fetched. NOTE: This can be adjusted to resolve errors with insufficient history." required: false diff --git a/src/changedFilesOutput.ts b/src/changedFilesOutput.ts index 2d4cc2de3d5..89ce6606855 100644 --- a/src/changedFilesOutput.ts +++ b/src/changedFilesOutput.ts @@ -43,7 +43,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ key: getOutputKey('added_files_count', outputPrefix), @@ -64,7 +65,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -86,7 +88,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -108,7 +111,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -130,7 +134,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -152,7 +157,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -174,7 +180,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -199,7 +206,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -226,7 +234,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -314,7 +323,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ @@ -419,7 +429,8 @@ export const setOutputsAndGetModifiedAndChangedFilesStatus = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) await setOutput({ diff --git a/src/inputs.ts b/src/inputs.ts index 5f5f468e60e..ac492d6048c 100644 --- a/src/inputs.ts +++ b/src/inputs.ts @@ -34,6 +34,7 @@ export type Inputs = { dirNamesDeletedFilesIncludeOnlyDeletedDirs: boolean json: boolean escapeJson: boolean + safeOutput: boolean fetchDepth?: number fetchSubmoduleHistory: boolean sinceLastRemoteCommit: boolean @@ -154,6 +155,7 @@ export const getInputs = (): Inputs => { ) const json = core.getBooleanInput('json', {required: false}) const escapeJson = core.getBooleanInput('escape_json', {required: false}) + const safeOutput = core.getBooleanInput('safe_output', {required: false}) const fetchDepth = core.getInput('fetch_depth', {required: false}) const sinceLastRemoteCommit = core.getBooleanInput( 'since_last_remote_commit', @@ -272,6 +274,7 @@ export const getInputs = (): Inputs => { dirNamesIncludeFilesSeparator, json, escapeJson, + safeOutput, writeOutputFiles, outputDir, outputRenamedFilesAsDeletedAndAdded, diff --git a/src/main.ts b/src/main.ts index b3cf2791116..85dc7e552a4 100644 --- a/src/main.ts +++ b/src/main.ts @@ -173,7 +173,8 @@ const getChangedFilesFromLocalGitHistory = async ({ value: allOldNewRenamedFiles.paths, writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, - json: inputs.json + json: inputs.json, + safeOutput: inputs.safeOutput }) await setOutput({ key: 'all_old_new_renamed_files_count', diff --git a/src/utils.ts b/src/utils.ts index 81a04570d23..46e9b4f8b95 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1324,7 +1324,8 @@ export const setArrayOutput = async ({ writeOutputFiles: inputs.writeOutputFiles, outputDir: inputs.outputDir, json: inputs.json, - shouldEscape: inputs.escapeJson + shouldEscape: inputs.escapeJson, + safeOutput: inputs.safeOutput }) } @@ -1334,7 +1335,8 @@ export const setOutput = async ({ writeOutputFiles, outputDir, json = false, - shouldEscape = false + shouldEscape = false, + safeOutput = false }: { key: string value: string | string[] | boolean @@ -1342,6 +1344,7 @@ export const setOutput = async ({ outputDir: string json?: boolean shouldEscape?: boolean + safeOutput?: boolean }): Promise => { let cleanedValue if (json) { @@ -1350,6 +1353,11 @@ export const setOutput = async ({ cleanedValue = value.toString().trim() } + // if safeOutput is true, escape special characters for bash shell + if (safeOutput) { + cleanedValue = cleanedValue.replace(/[$()`|&;]/g, '\\$&') + } + core.setOutput(key, cleanedValue) if (writeOutputFiles) {