Conversation
|
Thanks for the follow up. We need to be careful with this further changes as they open up surface for a potential attack, which is why we put in place commit verification. Therefore, before going on with this change I think it's worth taking some time to understand why you need this in the first place. |
Well, we are using this action in our public github for automerging depdendabot PRs and would like to use the same action in GHES as well. I do understand the potential security issues. I wonder if we could only allow skipping |
|
What I would like to understand is why commits cannot be verified in a GHES environment |
|
Not sure why dependabot is not a verified committer in GHES, but something similar was done here: dependabot/fetch-metadata#225 |
|
I was finally able to test this with GHES and can confirm that it fully works now. |
|
as customer for github enterprise server: we cannot fix how github is handling this in GHES. If github is allowing such PRs to their own organisation (dependabot dependabot/fetch-metadata#225) I think they do understand that the problem is not easy to solve in GHES side |
Thanks for this clarification, this indeed gives us more confidence that we can do this change. Please allow some time for somebody from the team to review. |
There was a problem hiding this comment.
We have noticed that the SKIP_COMMIT_VERIFICATION is parsed as a String instead of a boolean. For this reason, the verifyCommits will always be skipped.
You can try it easily by logging this in your branch:
if (typeof SKIP_COMMIT_VERIFICATION === 'boolean') {
console.log('skip-commit-verification is a BOOLEAN!')
} else if (typeof SKIP_COMMIT_VERIFICATION === 'string') {
console.log('skip-commit-verification is a STRING!')
}Running the action you will see that the printed line will be:
skip-commit-verification is a STRING!
Can you fix this behaviour as well?
Good catch! Made a fix and extended tests to cover all the boolean inputs. |
Fixes #363
We got little further with the initial GHES support, but it still hits with the verifySignatures:
Checklist
npm run testandnpm run benchmarkand the Code of conduct