Enhance command replacer #38

afilina · 2025-07-17T01:50:18Z

Description

Use functions for easier reasoning about the code.
Compare commands to an allow list; easier to read and scalable.
Check for unsafe tokens.
Add default options so we don't have to repeat them in every COMMAND-OUTPUT.
Replace eval with ${EXECUTABLE_COMMAND[@]} to prevent injections.

Example errors

ls -l
ERROR: refusing to run arbitrary command: ls

phpcs && ls -l
ERROR: refusing unsafe token: &&

PR checklist

I have checked there is no other PR open for the same change.
I have read the Contribution Guidelines.
I grant the project the right to include my changes under the BSD-3-Clause license (and I have the right to grant these rights).
I have verified that my changes comply with the projects coding standards.
[When adding a new Wiki page] I have added the new page to the _Sidebar.md file.
[When adding/updating output examples] I have verified via the GitHub Actions artifact that the pre-processing handles the command correctly.

afilina · 2025-07-17T02:04:18Z

Note: I had to disable the ShellCheck weak warning for || true because that appears to have been a deliberate choice by the original author and I don't want to change the inherent logic.

jrfnl · 2025-07-17T02:07:00Z

@fredden Would you have time to review this ?

jrfnl · 2025-07-17T02:07:57Z

@afilina Thanks for this PR - I have a feeling this will fix issue #35. Is that correct ?

afilina · 2025-07-17T02:16:16Z

@jrfnl Yes, it addresses this issue. I'm not a bash security expert, but it should be a reasonable improvement, in my opinion.

jrfnl · 2025-07-17T02:31:05Z

@jrfnl Yes, it addresses this issue. I'm not a bash security expert, but it should be a reasonable improvement, in my opinion.

In that case, maybe the PR description should contain a "Fixes... " ?

jrfnl

@afilina I've had a quick look, but my bash is abysmal, so I'm hoping @fredden will be able to do a more thorough review.

I've left two small comments inline for now and while looking at this PR, I realized the workflow which runs the script wasn't running for this PR, so I've fixed that via #39.

Could you please rebase the PR so we can make sure the script works as expected in GH Actions ?

jrfnl · 2025-07-17T02:10:33Z

build/wiki-command-replacer.sh

@@ -6,6 +6,46 @@ cd "$(dirname "$0")/.."

 MARKER_START='{{COMMAND-OUTPUT "'
 MARKER_END='"}}'
+ALLOWED_COMMANDS=("phpcs" "phpcbf")
+DEFAULT_OPTIONS=("--parallel=1" "--no-cache" "--no-colors" "--report-width=100" "--report=diff")


The report type should not be a default option.

Suggested change

DEFAULT_OPTIONS=("--parallel=1" "--no-cache" "--no-colors" "--report-width=100" "--report=diff")

DEFAULT_OPTIONS=("--parallel=1" "--no-cache" "--no-colors" "--report-width=100")

I'm also a bit in two minds about the default report width setting at 100, as the help commands at this time use 110 and as these default options are (rightfully) injected between the command and the CLI arguments passed to the command, the options cannot be overruled via the CLI arguments in the markdown files.

That is inherent to how PHPCS reads the CLI arguments, first value seen for an option on the CLI wins.

Also - two of the commands in the markdown files (introduces in #20) should be updated to remove the CLI args which will now be injected by default.

I was going to clean up the other code once this was merged. This PR should break anything there, other than the override issue.

Regarding the default, do you want to make it 110 for now, and we can do some smarter splitting of key/value in the future to avoid adding them if they were already in the USER_COMMAND?

Alternatively, we can exclude default options from this PR and worry about it later.

I have a brain wave about how we can do this in a way to still allow overruling these settings from the markdown: update the default values for these settings ahead of running the phpcs/phpcbf commands found in the markdown files.

In practice, this would mean running the following commands:

phpcs --config-set report_width 100 phpcs --config-set colors 0 phpcs --config-set parallel 1 phpcs --config-set cache 0

PHPCS will use those defaults from then on and the defaults can be overruled for individual commands by passing CLI args.

But there's a down-side to this: when contributors would run the script locally, running the above would mess with the settings for their global PHPCS install.

Now, we could, of course, use the CI environment variable to only run the above commands when running in the context of GH Actions, but then the output someone would generate locally may be different from what they see when the Wiki is deployed for real.

The only way I can currently think of to make this work in a stable manner, both in CI as well as for contributors locally, would probably be as follows:

Run phpcs --config-show

Parse the output of the command.
The first line of the output should look like this if the contributor has a config set up for their global install:
Using config file: path/to/CodeSniffer.conf
If the contributor has no config set up for their global install, it will look like so:
Using config file:

Grab the path to the CodeSniffer.conf file.

Temporarily rename that file. (Not copy, as otherwise additional pre-existing defaults from the contributor may still screw things up).

Run the commands I listed above. That will create a new CodeSniffer.conf file.

Run the script

After all command replacement has finished, delete the "new" CodeSniffer.conf file and rename the original CodeSniffer.conf file back to CodeSniffer.conf.

This would, of course, need to take into account that step 6 may fail/error out, while step 7 should still be run if that's the case.

What do you think ? Is this doable ?

As this would make the script yet more complicated - and it is already unlikely that the current script works cross-OS (i.e. probably won't work on Windows), I wonder if it might make sense to use a PHP script to do the command replacement instead ?

There is no specific reason for the script to be in bash. The choice for bash was mostly from originally trying to get something running directly in the GH Actions workflow and then moving it out to a separate script to make it convenient for running it locally.

This is getting very complicated and I doubt I'll have the time to see it through. Already I need to fix issues caused by the accepted suggested changes, which didn't take SellCheck into account.

I propose these options:

Put default options after the last option and before the first argument.

Exclude default overrides from this iteration and focus just on the security hardening part.

@afilina Sorry, I should have mentioned in my comment that what I suggested is "future scope".

I think option 2 is probably best. It will unblock this PR and allow for merging it.
We can then discuss the default overrides and potentially moving the logic to a PHP script in a separate issue.

I've opened issue #41 about the future scope.

Ok I'll try to find some time today or tomorrow to implement option 2.

fredden · 2025-07-17T06:35:00Z

Thanks for this. Generally this looks good to me. I have a few things I'd like to check when I get to a full-sized computer, which I should be able to do later today.

fredden

These changes look good to me. Please see also the comments from @jrfnl regarding the order of the injected "default options" and then updating existing commands to not duplicate these now-always-used flags.

I did have a concern about the list of forbidden characters not coping with sub-shells, but I've satisfied this with some testing locally.

build/wiki-command-replacer.sh

fredden · 2025-07-17T11:55:07Z

build/wiki-command-replacer.sh

+      # shellcheck disable=SC2310
+      execute_command "${USER_COMMAND}" </dev/null || true


The || true could be moved into the new execute_command function to appease shellcheck.
While we're here, let's add a code comment to explain why it's necessary:

</dev/null is needed because we're in a loop with a file being redirected to STDIN and read in via read. Without this, PHP gets / consumes all following lines in the file.

|| true is to defeat set -e. We want set -e so that the script will stop if there are any errors. However, some phpcs or phpcbf commands have non-zero exit codes, and we don't want to stop for those.

Since I am not a bash expert and the "while we're there" became bigger than the original work intended, could you propose a code change, or we can leave the shellcheck comment for now?

jrfnl · 2025-07-17T12:59:01Z

Oh... one open question for both of you - will the output which is "grabbed" contain STDOUT + STDERR or only STDOUT ?

I think for nearly everything STDOUT only would be sufficient, which should make the example output cleaner as of PHPCS 4.0 (as things like the timing will be send to STDERR, not STDOUT).

The things we may need to think about is whether this needs some sort of flag in the placeholder in the markdown files.
Then again, maybe we only need to start thinking about this if/when there is a command for which we really need STDERR output (or both).

fredden · 2025-07-17T20:51:11Z

will the output which is "grabbed" contain STDOUT + STDERR or only STDOUT ?

STDOUT only with the current implementation. We can change that if needed.

jrfnl · 2025-07-17T20:55:16Z

will the output which is "grabbed" contain STDOUT + STDERR or only STDOUT ?

STDOUT only with the current implementation. We can change that if needed.

I think that's fine for now.

jrfnl · 2025-07-17T20:57:04Z

Re: the build failure - I suspect this has to do with the token being used to post the comment. Also see my previous remarks about this here: #6 (comment)

I will submit a PR to switch back to the PAT.

In the original PR (#6), a Personal Access Token (PAT) was used, but that resulted in the comment being left as if coming from my account, which is misleading, which is why the `repo_token` was initially switched back to `secrets.GITHUB_TOKEN`, with a reminder that a test would need to be done with a PR coming from a fork to verify that the auto-comment would still work. As the first PR from a fork has now come in (#38), it has become clear that the auto-comment does not work with the default GitHub token, so this commit switches the `repo_token` back to the Personal Access Token and updates the message to make it clear I did not leave the comment. We'll still have to test (again) whether using the PAT works for PRs coming from forks. If not, we'll have to revisit this workflow step.

jrfnl · 2025-07-17T21:14:58Z

I will submit a PR to switch back to the PAT.

Submitted and merged PR #40 to (hopefully) fix this. @afilina If you don't mind, it would be great if you could rebase the PR on the current main to test whether the comment creation works now (and to get a passing build 🤞🏻 ).

- Use functions for easier reasoning about the code. - Compare commands to an allow list; easier to read and scalable. - Check for unsafe tokens. - Add default options so we don't have to repeat them in every COMMAND-OUTPUT. - Replace eval with ${EXECUTABLE_COMMAND[@]} to prevent injections. Example errors: `ls -l` ERROR: refusing to run arbitrary command: ls `phpcs && ls -l` ERROR: refusing unsafe token: &&

This was like that when I found it, and I believe it was done on purpose.

Co-authored-by: Juliette <[email protected]>

Co-authored-by: Dan Wallis <[email protected]>

afilina · 2025-07-19T13:08:51Z

I have no idea what I'm doing. I had to sync my own main so I can rebase my branch onto it, then it complained that i need to rebase anyway, so then i had to force-push, so now history looks all wrong and the build still fails. Feel free to open a new PR with just the diff of my changes. I don't need the credit.

jrfnl · 2025-07-19T14:03:29Z

I have no idea what I'm doing. I had to sync my own main so I can rebase my branch onto it, then it complained that i need to rebase anyway, so then i had to force-push, so now history looks all wrong and the build still fails. Feel free to open a new PR with just the diff of my changes. I don't need the credit.

No worries about the force-push and history, the commits in this PR would be squashed on merge anyway.

I've opened a new issue (#42) about the problem with the PR comment, which is the reason the build is failing.

github-actions bot added Type: chores/QA/automation Status: triage Component: Wiki labels Jul 17, 2025

afilina mentioned this pull request Jul 17, 2025

4.0 | Wiki: updates for change of the default standard from PEAR to PSR12 #34

Draft

jrfnl reviewed Jul 17, 2025

View reviewed changes

fredden reviewed Jul 17, 2025

View reviewed changes

jrfnl mentioned this pull request Jul 17, 2025

GH Actions/publish-wiki: use PAT for commenting on PRs #40

Merged

afilina and others added 5 commits July 19, 2025 09:05

Installed ShellCheck and fixed potential errors

ff5ed3b

Disable checking for ||

6d5fd6d

This was like that when I found it, and I believe it was done on purpose.

Update build/wiki-command-replacer.sh

bb21316

Co-authored-by: Juliette <[email protected]>

Update build/wiki-command-replacer.sh

301cc9c

Co-authored-by: Dan Wallis <[email protected]>

afilina force-pushed the wiki/phpcs-4.0/wiki-command-replacement-improvements branch from abc6aeb to 301cc9c Compare July 19, 2025 13:06

jrfnl mentioned this pull request Jul 19, 2025

Command Replacer: explore injecting defaults for a number of CLI options and potentially rewriting the script in PHP #41

Open

	DEFAULT_OPTIONS=("--parallel=1" "--no-cache" "--no-colors" "--report-width=100" "--report=diff")
	DEFAULT_OPTIONS=("--parallel=1" "--no-cache" "--no-colors" "--report-width=100")

		# shellcheck disable=SC2310
		execute_command "${USER_COMMAND}" </dev/null \|\| true

Uh oh!

Enhance command replacer #38

Are you sure you want to change the base?

Enhance command replacer #38

Uh oh!

Conversation

afilina commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example errors

PR checklist

Uh oh!

afilina commented Jul 17, 2025

Uh oh!

jrfnl commented Jul 17, 2025

Uh oh!

jrfnl commented Jul 17, 2025

Uh oh!

afilina commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrfnl commented Jul 17, 2025

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afilina Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredden commented Jul 17, 2025

Uh oh!

fredden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl commented Jul 17, 2025

Uh oh!

fredden commented Jul 17, 2025

Uh oh!

jrfnl commented Jul 17, 2025

Uh oh!

jrfnl commented Jul 17, 2025

Uh oh!

jrfnl commented Jul 17, 2025

Uh oh!

afilina commented Jul 19, 2025

Uh oh!

jrfnl commented Jul 19, 2025

Uh oh!

Uh oh!

afilina commented Jul 17, 2025 •

edited

Loading

afilina commented Jul 17, 2025 •

edited

Loading

afilina Jul 19, 2025 •

edited

Loading