Skip to content
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

Fix GHES support #367

Merged
merged 4 commits into from
Feb 16, 2023
Merged

Fix GHES support #367

merged 4 commits into from
Feb 16, 2023

Conversation

frequ
Copy link
Contributor

@frequ frequ commented Feb 10, 2023

Fixes #363

We got little further with the initial GHES support, but it still hits with the verifySignatures:

Warning: PR contains invalid dependabot commit signatures, skipping.

Checklist

@simoneb
Copy link
Collaborator

simoneb commented Feb 10, 2023

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.

@frequ
Copy link
Contributor Author

frequ commented Feb 10, 2023

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 verifyCommit for GitHub Enterprise Server. E.g. check if GITHUB_SERVER_URL is set.

@simoneb
Copy link
Collaborator

simoneb commented Feb 10, 2023

What I would like to understand is why commits cannot be verified in a GHES environment

@frequ
Copy link
Contributor Author

frequ commented Feb 10, 2023

Not sure why dependabot is not a verified committer in GHES, but something similar was done here: dependabot/fetch-metadata#225

@frequ
Copy link
Contributor Author

frequ commented Feb 13, 2023

I was finally able to test this with GHES and can confirm that it fully works now.

@zetaab
Copy link

zetaab commented Feb 13, 2023

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

@simoneb
Copy link
Collaborator

simoneb commented Feb 13, 2023

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.

Copy link

@davideroffo davideroffo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just left a comment not related to your changes but that we can address with this PR.

src/action.js Outdated Show resolved Hide resolved
Copy link

@davideroffo davideroffo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@frequ
Copy link
Contributor Author

frequ commented Feb 16, 2023

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.

Can you fix this behaviour as well?

Good catch! Made a fix and extended tests to cover all the boolean inputs.

Copy link

@davideroffo davideroffo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@simoneb simoneb merged commit 88cd6e7 into fastify:main Feb 16, 2023
@github-actions github-actions bot mentioned this pull request Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GHES support
4 participants