Avoid use of direct string injection in GitHub Workflow "run" steps. by eyebrowsoffire · Pull Request #185301 · flutter/flutter

eyebrowsoffire · 2026-04-20T19:13:31Z

This PR replaces the use of direct string injections from the GitHub environment (i.e. ${{ env.VARIABLE}}) and uses bash environment variables instead. This mitigates the possibility of code injection attacks when running these workflows.

This PR replaces the use of direct string injections from the GitHub environment (i.e. `${{ env.VARIABLE}}`) and uses bash environment variables instead. This mitigates the possibility of code injection attacks when running these workflows.

gemini-code-assist · 2026-04-20T19:13:39Z

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

reidbaker · 2026-04-20T20:04:41Z

Why does using the env variable section for the script mitigate the injection issues? Does github do special parsing logic there?

eyebrowsoffire · 2026-04-20T20:34:01Z

Why does using the env variable section for the script mitigate the injection issues? Does github do special parsing logic there?

The problem with the ${{ env.VARIABLE }} style of string injection is that it literally just puts the string directly into the bash script that is run, so it can even change scopes by putting quotes, braces, etc. Bash environment variables don't have this issue (special characters like quotes are escaped). And when we assign the variable in the env section, that isn't an arbitrary script being executed, GitHub populates the bash environment variables for the script safely.

agentfu

Starting review...

agentfu

📋 Review Summary

The PR correctly identifies several places where direct string injection from the GitHub environment could lead to code injection attacks and replaces them with bash environment variables. However, the implementation is inconsistent across different workflow files, missing several steps that still use direct injections for gh commands, even when those same variables are moved to env in other steps of the same file.

🔍 General Feedback

Excellent work on identifying and mitigating these security risks.
Consistency across all workflows would make the codebase more robust and maintainable.
Consider moving github.repository and github.event.pull_request.number to environment variables in all steps for a uniform approach, even if they are low-risk.

agentfu · 2026-04-20T21:32:32Z

        id: revert-commit
        working-directory: ./flutter
+        env:
+          PR_NUMBER: ${{ github.event.pull_request.number }}


[MEDIUM] The PR_NUMBER variable is moved to env in this step, but github.repository is still directly injected in the gh commands below. For consistency and to fully adhere to the PR's goal, consider moving github.repository to the env block as well (or use the default GITHUB_REPOSITORY environment variable).

Suggested change

PR_NUMBER: ${{ github.event.pull_request.number }}

env:

PR_NUMBER: ${{ github.event.pull_request.number }}

REPOSITORY: ${{ github.repository }}

run: |

if git revert $MERGE_COMMIT --no-edit; then

echo "success=true" >> $GITHUB_OUTPUT

else

echo "success=false" >> $GITHUB_OUTPUT

gh pr comment $PR_NUMBER -R $REPOSITORY -b "Failed to revert commit $MERGE_COMMIT cleanly. Please resolve conflicts manually."

gh pr edit $PR_NUMBER -R $REPOSITORY --remove-label "revert_wf"

exit 1

fi

agentfu · 2026-04-20T21:32:32Z

-          git push origin ${{ env.BRANCH_NAME }}
+          git push origin $BRANCH_NAME

          PR_LINK="https://github.com/flutter/flutter/pull/${{ github.event.pull_request.number }}"


[LOW] While github.event.pull_request.number is an integer and not a direct injection risk, it should be moved to the env block for consistency with the rest of the PR's changes.

Suggested change

PR_LINK="https://github.com/flutter/flutter/pull/${{ github.event.pull_request.number }}"

PR_LINK="https://github.com/flutter/flutter/pull/$PR_NUMBER"

(Note: Ensure PR_NUMBER is added to the env block of this step as well).

I am a human - I don't think this is needed?

agentfu · 2026-04-20T21:32:32Z

-            --head flutteractionsbot:${{ env.BRANCH_NAME }})
+            --head flutteractionsbot:$BRANCH_NAME)

          gh pr comment ${{ github.event.pull_request.number }} -R flutter/flutter -b "Successfully created revert PR: $NEW_PR_URL"


[LOW] Missed direct injection for PR_NUMBER and REPOSITORY.

Suggested change

gh pr comment ${{ github.event.pull_request.number }} -R flutter/flutter -b "Successfully created revert PR: $NEW_PR_URL"

gh pr comment $PR_NUMBER -R $REPOSITORY -b "Successfully created revert PR: $NEW_PR_URL"

gh pr edit $PR_NUMBER -R $REPOSITORY --remove-label "revert_wf"

jtmcdole · 2026-04-20T19:24:10Z

        run: |
-          if git ls-remote --exit-code --heads origin ${{ env.BRANCH_NAME }}; then
-            echo "Error: Branch ${{ env.BRANCH_NAME }} already exists on remote."
+          if git ls-remote --exit-code --heads origin $BRANCH_NAME; then


I'm guessing "BRANCH_NAME" is a unique github variable that doesn't need "env:" given the previous usage was "env.BRANCH_NAME"

I found out that things that are in env are automatically added to the bash environment, so this just works as long as it was added to the environment in a previous step. For other things like ${{ github.event.XYZ }} and stuff like that, you have to specify them in the env section in order to actually put them in bash environment variables.

jtmcdole · 2026-04-20T21:34:37Z

-          git push origin ${{ env.BRANCH_NAME }}
+          git push origin $BRANCH_NAME

          PR_LINK="https://github.com/flutter/flutter/pull/${{ github.event.pull_request.number }}"


I am a human - I don't think this is needed?

jtmcdole · 2026-04-20T22:45:18Z

Too bad the bot wouldn't create this review locally so I could not have it post lower quality comments..

eyebrowsoffire · 2026-04-20T23:59:37Z

Too bad the bot wouldn't create this review locally so I could not have it post lower quality comments..

They actually aren't low quality, IMO, but they are nits and probably not worth going back to do a whole new PR over. Maybe I'll try to clean these up later the next time I touch these files.

flutter/flutter@2844af6...3d0e822 2026-04-21 [email protected] Reland "Unpin google_mobile_ads" (flutter/flutter#180838) 2026-04-21 [email protected] fix: correct LicenseRegistry docs to reference NOTICES instead of LICENSE (flutter/flutter#184572) 2026-04-21 [email protected] Roll Skia from f8637ade3d92 to a234f0ed7245 (2 revisions) (flutter/flutter#185334) 2026-04-21 [email protected] Roll Skia from 3b338913f623 to f8637ade3d92 (9 revisions) (flutter/flutter#185331) 2026-04-21 [email protected] Fix non-minimal relative imports in flutter_tools (flutter/flutter#183971) 2026-04-21 [email protected] Reapply "Unpin sdk package dependencies" (flutter/flutter#185268) 2026-04-21 [email protected] Remove unused private header (flutter/flutter#185260) 2026-04-20 [email protected] [iOS] Improve LaunchEngine implementation/API/docs (flutter/flutter#185200) 2026-04-20 [email protected] [ios][pv] Reland platform view hitTest approach (again) (flutter/flutter#185126) 2026-04-20 [email protected] Roll Skia from 75c2791c6274 to 3b338913f623 (3 revisions) (flutter/flutter#185304) 2026-04-20 [email protected] ignore avoid_type_to_string lint rule (flutter/flutter#184765) 2026-04-20 [email protected] Fix race condition in modifying release manifest. (flutter/flutter#185185) 2026-04-20 [email protected] In the dev/bots/analyze.dart script, obtain the relevant set of paths from Git instead of crawling the filesystem (flutter/flutter#185058) 2026-04-20 [email protected] [wimp] Implement images for wimp. (flutter/flutter#183913) 2026-04-20 [email protected] add the next batch for VPAT assessment (flutter/flutter#185053) 2026-04-20 [email protected] Roll Packages from c2e3d1f to 01c505f (21 revisions) (flutter/flutter#185287) 2026-04-20 [email protected] Avoid use of direct string injection in GitHub Workflow "run" steps. (flutter/flutter#185301) 2026-04-20 [email protected] Regenerate pubspec.lock (flutter/flutter#185290) 2026-04-20 [email protected] Report an error if the git ls-tree command fails in the content_aware_hash script (flutter/flutter#185170) 2026-04-20 [email protected] Roll Skia from d8415c5d7b91 to 75c2791c6274 (40 revisions) (flutter/flutter#185284) 2026-04-20 [email protected] Move widget_preview_scaffold into pub workspace (flutter/flutter#185176) 2026-04-20 [email protected] [record_use] Run build hooks and link hooks in separate targets (flutter/flutter#184880) 2026-04-20 [email protected] feat: add reloadIsRestart to handle hot reload as a restart for web #179448 (flutter/flutter#183233) 2026-04-20 [email protected] Roll Fuchsia Linux SDK from aDbXQm6WA0wFCAUp-... to LPa7NLiXEZP2A7IwZ... (flutter/flutter#185269) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…r#11548) flutter/flutter@2844af6...3d0e822 2026-04-21 [email protected] Reland "Unpin google_mobile_ads" (flutter/flutter#180838) 2026-04-21 [email protected] fix: correct LicenseRegistry docs to reference NOTICES instead of LICENSE (flutter/flutter#184572) 2026-04-21 [email protected] Roll Skia from f8637ade3d92 to a234f0ed7245 (2 revisions) (flutter/flutter#185334) 2026-04-21 [email protected] Roll Skia from 3b338913f623 to f8637ade3d92 (9 revisions) (flutter/flutter#185331) 2026-04-21 [email protected] Fix non-minimal relative imports in flutter_tools (flutter/flutter#183971) 2026-04-21 [email protected] Reapply "Unpin sdk package dependencies" (flutter/flutter#185268) 2026-04-21 [email protected] Remove unused private header (flutter/flutter#185260) 2026-04-20 [email protected] [iOS] Improve LaunchEngine implementation/API/docs (flutter/flutter#185200) 2026-04-20 [email protected] [ios][pv] Reland platform view hitTest approach (again) (flutter/flutter#185126) 2026-04-20 [email protected] Roll Skia from 75c2791c6274 to 3b338913f623 (3 revisions) (flutter/flutter#185304) 2026-04-20 [email protected] ignore avoid_type_to_string lint rule (flutter/flutter#184765) 2026-04-20 [email protected] Fix race condition in modifying release manifest. (flutter/flutter#185185) 2026-04-20 [email protected] In the dev/bots/analyze.dart script, obtain the relevant set of paths from Git instead of crawling the filesystem (flutter/flutter#185058) 2026-04-20 [email protected] [wimp] Implement images for wimp. (flutter/flutter#183913) 2026-04-20 [email protected] add the next batch for VPAT assessment (flutter/flutter#185053) 2026-04-20 [email protected] Roll Packages from c2e3d1f to 01c505f (21 revisions) (flutter/flutter#185287) 2026-04-20 [email protected] Avoid use of direct string injection in GitHub Workflow "run" steps. (flutter/flutter#185301) 2026-04-20 [email protected] Regenerate pubspec.lock (flutter/flutter#185290) 2026-04-20 [email protected] Report an error if the git ls-tree command fails in the content_aware_hash script (flutter/flutter#185170) 2026-04-20 [email protected] Roll Skia from d8415c5d7b91 to 75c2791c6274 (40 revisions) (flutter/flutter#185284) 2026-04-20 [email protected] Move widget_preview_scaffold into pub workspace (flutter/flutter#185176) 2026-04-20 [email protected] [record_use] Run build hooks and link hooks in separate targets (flutter/flutter#184880) 2026-04-20 [email protected] feat: add reloadIsRestart to handle hot reload as a restart for web #179448 (flutter/flutter#183233) 2026-04-20 [email protected] Roll Fuchsia Linux SDK from aDbXQm6WA0wFCAUp-... to LPa7NLiXEZP2A7IwZ... (flutter/flutter#185269) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

eyebrowsoffire added the CICD Run CI/CD label Apr 20, 2026

eyebrowsoffire requested a review from jtmcdole April 20, 2026 19:14

A little more cleanup of unnecessary string injection.

0cef0e1

github-actions Bot removed the CICD Run CI/CD label Apr 20, 2026

eyebrowsoffire added the CICD Run CI/CD label Apr 20, 2026

This comment was marked as off-topic.

Sign in to view

Merge branch 'master' into mitigate_injection

bab7708

github-actions Bot removed the CICD Run CI/CD label Apr 20, 2026

eyebrowsoffire added the CICD Run CI/CD label Apr 20, 2026

reidbaker approved these changes Apr 20, 2026

View reviewed changes

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2026

auto-submit Bot added this pull request to the merge queue Apr 20, 2026

agentfu reviewed Apr 20, 2026

View reviewed changes

Merged via the queue into flutter:master with commit c459527 Apr 20, 2026
17 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2026

jtmcdole reviewed Apr 20, 2026

View reviewed changes

engine-flutter-autoroll mentioned this pull request Apr 21, 2026

Roll Flutter from 2844af6a7134 to 3d0e8228240f (24 revisions) flutter/packages#11548

Merged

-          PR_NUMBER: ${{ github.event.pull_request.number }}
+        env:
+          PR_NUMBER: ${{ github.event.pull_request.number }}
+          REPOSITORY: ${{ github.repository }}
+        run: |
+          if git revert $MERGE_COMMIT --no-edit; then
+            echo "success=true" >> $GITHUB_OUTPUT
+          else
+            echo "success=false" >> $GITHUB_OUTPUT
+            gh pr comment $PR_NUMBER -R $REPOSITORY -b "Failed to revert commit $MERGE_COMMIT cleanly. Please resolve conflicts manually."
+            gh pr edit $PR_NUMBER -R $REPOSITORY --remove-label "revert_wf"
+            exit 1
+          fi

	PR_LINK="https://github.com/flutter/flutter/pull/${{ github.event.pull_request.number }}"
	PR_LINK="https://github.com/flutter/flutter/pull/$PR_NUMBER"

	gh pr comment ${{ github.event.pull_request.number }} -R flutter/flutter -b "Successfully created revert PR: $NEW_PR_URL"
	gh pr comment $PR_NUMBER -R $REPOSITORY -b "Successfully created revert PR: $NEW_PR_URL"
	gh pr edit $PR_NUMBER -R $REPOSITORY --remove-label "revert_wf"

Uh oh!

Conversation

eyebrowsoffire commented Apr 20, 2026

Uh oh!

gemini-code-assist Bot commented Apr 20, 2026

Uh oh!

This comment was marked as off-topic.

reidbaker commented Apr 20, 2026

Uh oh!

eyebrowsoffire commented Apr 20, 2026

Uh oh!

agentfu left a comment

Choose a reason for hiding this comment

Uh oh!

agentfu left a comment

Choose a reason for hiding this comment

📋 Review Summary

🔍 General Feedback

Uh oh!

agentfu Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

agentfu Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

jtmcdole Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

agentfu Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jtmcdole Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

eyebrowsoffire Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

jtmcdole Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

jtmcdole commented Apr 20, 2026

Uh oh!

eyebrowsoffire commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants