Security advisory merge ignores merge strategy and commit message #200266
Replies: 2 comments
-
|
Thank you for your interest in contributing to our community! We currently only accept discussions created through the GitHub UI using our provided discussion templates. Please re-submit your discussion by navigating to the appropriate category and using the template provided. This discussion has been closed because it was not submitted through the expected format. If you believe this was a mistake, please reach out to the maintainers. |
Beta Was this translation helpful? Give feedback.
0 replies
-
|
Posted through the UI: https://github.com/orgs/community/discussions/200267 |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
I maintain
filecoin-project/filecoin-pinand recently published a security advisory (GHSA-m5ph-mmg5-6w5h) using the private fork workflow. When I used Merge pull request(s) in the advisory UI to land the fix on the public default branch, the behavior did not match what our repository settings and release tooling expect. I am filing this to document the gap and suggest two targeted improvements.What I observed
The advisory merge flow always creates a merge commit, even when the repository has
allow_merge_commit: falseand only allows squash or rebase merges. Our repo is configured squash + rebase only; the advisory flow still produced a merge commit on the default branch. (The UI wouldn't let me merge, and I got around this by adding me as a bypass for the ruleset, which then meant it was able to do a standard merge commit.)The merge commit message is fixed to the standard
"Merge pull request #N from owner/repo-ghsa-...:branch"format with no way to customize it. The underlying commits in the private fork (with conventionalfeat:/fix:/docs:messages) are not reflected in that merge commit message.Related observation (not an ask here, but part of the same pattern): this flow also bypasses branch protection rulesets, so merging via the advisory button requires ruleset-bypass permission on the default branch.
Why it matters
Many modern repositories disable merge commits and rely on squash or rebase to keep a linear, conventional-commit-friendly history. Release automation such as release-please, semantic-release, and similar tools often walk history with
git log --first-parent. When the only first-parent commit is a generic merge message, the underlying conventional commit metadata is invisible to those tools.In our case, a
docs:change merged via the advisory flow did not appear in the open release-please CHANGELOG PR. We only noticed after publish when reconciling the release.Asks
Honor repository merge settings in the advisory Merge pull request(s) flow: respect
allow_merge_commit,allow_squash_merge, andallow_rebase_mergethe same way the normal PR merge UI does (defaulting to the repo's preferred strategy when merge commits are disabled).Allow merge commit message customization, or at minimum include the source PR's commit messages in the resulting default-branch commit so
--first-parentrelease tooling can see conventional-commit metadata. Even when a merge commit is unavoidable, a customizable message (or squash-with-retained message) would prevent silent CHANGELOG omissions.Workarounds people use today
BEGIN_COMMIT_OVERRIDEblock in a follow-up PR body to manually inject CHANGELOG entries.Prior art
Concrete example
Happy to provide more detail if useful. Thanks for continuing to invest in the advisory workflow.
Beta Was this translation helpful? Give feedback.
All reactions