Skip to content

Diff variable needs to be unquoted when passed to the application #167

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 19, 2022
Merged

Diff variable needs to be unquoted when passed to the application #167

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,6 @@ if [[ "$DIFF" != "" ]];then
echo "${DIFF}"
fi

/action/main.js "${DIFF}"
# This variable is deliberately unquoted so that it can be properly processed on the other side.
# shellcheck disable=SC2086
/action/main.js ${DIFF}
Copy link
Member

Choose a reason for hiding this comment

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

That is sooooo odd; normal rule of thumb is to ALWAYS quote arguments in shell scripts. Would love to know why this occurs...

Copy link
Member Author

@internalsystemerror internalsystemerror Dec 20, 2022 •
edited by Xerkus
Loading

Choose a reason for hiding this comment

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

Same. I think the list of files gets read as a single string, but I'm not certain. It should be easy enough to see what's happening around

laminas-ci-matrix-action/src/main.ts

Lines 14 to 22 in 5e50256

/**
* Remove the first two arguments from this process.
* - first index contains the binary which is used to execute JS (nodejs)
* - second argument contains the filename which is being executed
* After the first two arguments, all remaining arguments should contain filenames from a `diff`. Can be empty in case
* the action is not run from a pull-request.
*/
/* eslint-disable-next-line no-magic-numbers */
const filesWithChanges: string[] = process.argv.slice(2).filter((value) => value !== '');
though.

Copy link
Member

Choose a reason for hiding this comment

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

Each file name from diff is expected to be a separate argument and it explicitly relies on shell word splitting behavior using IFS.
Double quoting prevented word splitting and the whole diff ended up passed as a single argument which then failed to match non-multiline regex like \.php$ as it would effectively be matching only the very last file