-
Notifications
You must be signed in to change notification settings - Fork 86
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
Code samples usage #2438
Code samples usage #2438
Conversation
This reverts commit 74284fe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is much needed!
I'm just paranoid and I'm wondering if we can harden this a bit, just to be sure - but I really like this.
{ | ||
$pattern = null === $codeSampleFilePath ? '= include_file' : $codeSampleFilePath; | ||
$command = "grep '$pattern' -Rl docs | sort"; | ||
exec($command, $rawIncludingFileList, $commandResultCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there is an issue here - but just to be sure that there's no chance of command injection with some crazy filenames - could you use the Process component (https://symfony.com/doc/current/components/process.html)?
I'd also limit the scope of the token to the minimum (https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token#defining-access-for-the-github_token-permissions) for this job, just for safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command injection
I'd like to stay simple and not have to wrap all this into a Symfony application.
I'm using escapeshellarg
around the file path since ebc14f5.
I tried args like php tools/code_samples/code_samples_usage.php "code_samples/api/product_catalog/src/Command/AttributeCommand.php'; echo TEST; :"
to see if I could pass something to the exec
. I didn't manage to. But hackers are imaginative.
Job permissions
I restricted permissions to their minimum:
permissions:
# Needed to manage the comment
pull-requests: write
Another test in #2485 |
May help if someone manage to pass something fancy in the name of new code_samples file
code_samples/ change report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
(cherry picked from commit e89bae1)
Tool to search and preview code samples usage.
It outputs a list of Markdown code blocks using
include_file
, each followed by a preview of its rendering.For examples:
php tools/code_samples/code_samples_usage.php code_samples/api/product_catalog/src/Command/AttributeCommand.php code_samples/api/product_catalog/src/Command/ProductCommand.php
lists usages of the two files passed as arguments (argument count is unlimited).php tools/code_samples/code_samples_usage.php
lists everyinclude_file
usage.⫸
indicates a highlighted line (hl_lines
).The tool is used by a GitHub action for CI on pull request. It compares the usage of modified
code_samples/
files between the target branch and the current branch. If there is changes incode_samples/
, this action posts its report as a comment on the PR. It deletes its previous comment if it exists. (It could have edit and update the previous one but an old comment can become hidden by default and stay hidden even if updated.) Because of GitHub limitation, the comment can be hard to read. A colorized version is available as HTML file in a downloadable archive (due to artefact limitation, it can't be seen online or downloaded unzipped).See the "code_samples/ change report" comment below for an example with two bonus modification.
Bonus:
Two PHP files are modified for demo, they had too long
__construct
lines and new lines are added:A report comment helps to check side effects on their usage.
The page using them doesn't change:
docs/pim/price_api.md has been modified. As the PHP code doesn't change, there is nothing about it in the report comment. Locally, the changes can be checked with
php tools/code_samples/code_samples_usage.php code_samples/api/product_catalog/src/Command/ProductPriceCommand.php
which could be simpler than running mkdocs server.Remove duplicated blank lines before
// ...
:Cases checks
Checklist