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.

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

Avoid interpolating values into bash #4774

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Avoid interpolating values into bash #4774

merged 1 commit into from
Dec 6, 2024

Conversation

alex
Copy link
Contributor

@alex alex commented Dec 6, 2024

This can lead to code execution. See https://woodruffw.github.io/zizmor/audits/#template-injection for details

@alex alex added the CI-skip-changelog Skip checking changelog entry label Dec 6, 2024
@alex alex requested a review from davidhewitt December 6, 2024 16:23
@LilyFoote LilyFoote added this pull request to the merge queue Dec 6, 2024
Merged via the queue into main with commit 7077ada Dec 6, 2024
47 checks passed
@LilyFoote LilyFoote deleted the alex-patch-1 branch December 6, 2024 21:04
env:
# Don't put these in bash, because we don't want the expansion to
# risk code execution
BASE_REF: "refs/heads/{{ github.event.pull_request.base.ref }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BASE_REF: "refs/heads/{{ github.event.pull_request.base.ref }}"
BASE_REF: "refs/heads/${{ github.event.pull_request.base.ref }}"

I think this one is missing a "$" for expansion? At least it is currently broken in #4768

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sigh, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops! 🙈

davidhewitt pushed a commit that referenced this pull request Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants