hack: fix unquoted variables in hack scripts to prevent word-splitting#9018
hack: fix unquoted variables in hack scripts to prevent word-splitting#9018Ankitsinghsisodya wants to merge 5 commits intoknative:mainfrom
Conversation
- generate-yamls.sh: quote YAML_OUTPUT_DIR in rm command - release.sh: quote dirname substitution, REPO_ROOT_DIR in cp, and $@ - verify-codegen.sh: quote REPO_ROOT_DIR inside mktemp call Unquoted variable expansions and command substitutions break when paths contain whitespace. Closes knative#8990 Closes knative#8992 Closes knative#9004
knative-prow
bot
added
the
size/S
knative-prow
bot
added
the
needs-ok-to-test
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
knative-prow
bot
added
the
area/test-and-release
There was a problem hiding this comment.
Pull request overview
This PR hardens several hack/ bash scripts by quoting variable expansions/command substitutions to prevent word-splitting when paths contain whitespace, combining the changes from #8990, #8992, and #9004.
Changes:
- Quote
${YAML_OUTPUT_DIR}inhack/generate-yamls.shwhen removing generated YAMLs. - Quote
$(dirname ...)usage,${REPO_ROOT_DIR}copy destination, andmain "$@"invocation inhack/release.sh. - Quote
${REPO_ROOT_DIR}in themktemptemplate path inhack/verify-codegen.sh.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hack/verify-codegen.sh |
Quotes the mktemp directory template to handle repo paths with spaces. |
hack/release.sh |
Quotes script path resolution, repo-root copy destination, and "$@" propagation (but see review comment about cp). |
hack/generate-yamls.sh |
Quotes the output directory in rm while preserving glob expansion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ARTIFACTS_TO_PUBLISH=$(cat "${YAML_LIST}" | tr '\n' ' ') | ||
| if (( ! PUBLISH_RELEASE )); then | ||
| # Copy the generated YAML files to the repo root dir if not publishing. | ||
| cp ${ARTIFACTS_TO_PUBLISH} ${REPO_ROOT_DIR} | ||
| cp "${ARTIFACTS_TO_PUBLISH}" "${REPO_ROOT_DIR}" | ||
| fi |
There was a problem hiding this comment.
Quoting "${ARTIFACTS_TO_PUBLISH}" breaks cp when it contains multiple YAML paths (it becomes a single argument like "file1.yaml file2.yaml ..." and cp will fail). To preserve whitespace-safe paths and multiple files, read the YAML list into an array (e.g., mapfile -t artifacts < "${YAML_LIST}") and then cp "${artifacts[@]}" "${REPO_ROOT_DIR}" (or otherwise iterate line-by-line) instead of building a space-delimited string.
|
@creydr I have raised a seperate PRs and closed those three PRs as suggested by you. |
Using a quoted space-delimited string with cp treats all paths as a single argument and breaks with multiple files. Read YAML_LIST into a local array with mapfile for the cp call, while keeping ARTIFACTS_TO_PUBLISH as a space-separated string that the upstream knative/hack release.sh expects.
creydr
left a comment
creydr
left a comment
There was a problem hiding this comment.
Thanks @Ankitsinghsisodya for combining them. I left one question
| "$(dirname "$0")/generate-yamls.sh" "${REPO_ROOT_DIR}" "${YAML_LIST}" | ||
| mapfile -t artifacts < "${YAML_LIST}" | ||
| ARTIFACTS_TO_PUBLISH="${artifacts[*]}" | ||
| if (( ! PUBLISH_RELEASE )); then | ||
| # Copy the generated YAML files to the repo root dir if not publishing. | ||
| cp ${ARTIFACTS_TO_PUBLISH} ${REPO_ROOT_DIR} | ||
| cp "${artifacts[@]}" "${REPO_ROOT_DIR}" |
There was a problem hiding this comment.
@creydr The original cp ${ARTIFACTS_TO_PUBLISH} ${REPO_ROOT_DIR} had two issues:
-
Word splitting on
$ARTIFACTS_TO_PUBLISH— that variable holds space-separated filenames. If any filename contains spaces or glob characters, the shell splits it incorrectly before passing it tocp. Using"${artifacts[@]}"(a bash array) passes each element as a distinct argument, safely. -
Redundancy —
ARTIFACTS_TO_PUBLISH="${artifacts[*]}"(line 70) collapses the array back into a single space-joined string. The new code skips that intermediary and goes straight from the array tocp, which is both simpler and correct.
There was a problem hiding this comment.
Is ARTIFACTS_TO_PUBLISH needed then? Or you rename artifacts to ARTIFACTS_TO_PUBLISH to make it a bit clearer
There was a problem hiding this comment.
I have Remove it. it was dead code after switching to cp
"${artifacts[@]}". The array flows directly into cp now with no intermediate
variable needed.
|
/ok-to-test |
knative-prow
bot
added
ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9018 +/- ##
==========================================
+ Coverage 51.02% 51.07% +0.05%
==========================================
Files 409 409
Lines 22001 21996 -5
==========================================
+ Hits 11225 11234 +9
+ Misses 9914 9899 -15
- Partials 862 863 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Combines #8990, #8992, and #9004 into a single PR as requested by @creydr.
Also fixes
main $@→main "$@"inrelease.shspotted during review.Changes
generate-yamls.sh: Quote${YAML_OUTPUT_DIR}inrmcommandrelease.sh: Quote$(dirname $0)invocation,${REPO_ROOT_DIR}incp, and$@inmaincallverify-codegen.sh: Quote${REPO_ROOT_DIR}insidemktempcallUnquoted variable expansions and command substitutions break when paths contain whitespace.
Closes #8990
Closes #8992
Closes #9004