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

feat(git): allow to install git-hook functions to local repositories #19685

Merged
merged 5 commits into from
Jan 6, 2023

Conversation

NiasSt90
Copy link
Contributor

@NiasSt90 NiasSt90 commented Jan 5, 2023

Changes

Allow to install git-hook functions to local git repositories.

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

viceice
viceice previously approved these changes Jan 5, 2023
@viceice viceice requested a review from rarkins January 5, 2023 14:47
@NiasSt90 NiasSt90 changed the title feat: allow to install git-hook functions to local repositories feat(git): allow to install git-hook functions to local repositories Jan 6, 2023
@rarkins rarkins merged commit 5b4b646 into renovatebot:main Jan 6, 2023
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 34.88.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@fgreinacher
Copy link
Contributor

@viceice @rarkins Reviewing the changelog we were a bit concerned about the security implications of this first, but then noticed that the functionality is not used in the main codebase. Can you shed some light on the idea here?

/cc @dlouzan @nejch

@rarkins
Copy link
Collaborator

rarkins commented Jan 9, 2023

It's for Gerrit support later

@viceice
Copy link
Member

viceice commented Jan 9, 2023

Updated context in PR description

@fgreinacher
Copy link
Contributor

Thanks for the quick answers! I assume this is the relevant part of #4109, correct?

install the gerrit-commit hook in initRepo
to create a change-id if not already exists in commit-message.

@NiasSt90
Copy link
Contributor Author

NiasSt90 commented Jan 9, 2023

If there are real concern about the hook i can try to avoid it for the Gerrit implementation.
The commit-msg hook needed for Gerrit is only needed to generate a Change-ID by calling this:

random=$( (whoami ; hostname ; date; cat $1 ; echo $RANDOM) | git hash-object --stdin)
($1 == file with the existing commit-msg)

This random value will be added to the commit-msg (a new line) like "Change-Id: I${random}"

A random UUID should be sufficient for this.

@viceice
Copy link
Member

viceice commented Jan 9, 2023

If there are real concern about the hook i can try to avoid it for the Gerrit implementation. The commit-msg hook needed for Gerrit is only needed to generate a Change-ID by calling this:

random=$( (whoami ; hostname ; date; cat $1 ; echo $RANDOM) | git hash-object --stdin) ($1 == file with the existing commit-msg)

This random value will be added to the commit-msg (a new line) like "Change-Id: I${random}"

A random UUID should be sufficient for this.

Sounds good. let's revert this feature then.

@fgreinacher
Copy link
Contributor

Sounds cool! I think Renovate should avoid arbitrary code execution as good as possible.

@NiasSt90
Copy link
Contributor Author

Could be reverted, i've tried and it works with:

/**
 * This function should generate a Gerrit Change-ID analogous to the commit hook. 
 * We avoid the commit hook cause of security concerns.
 * random=$( (whoami ; hostname ; date; cat $1 ; echo $RANDOM) | git hash-object --stdin) prefixed with an 'I'.
 * TODO: Gerrit don't accept longer Change-IDs (sha256), but what happens with this https://git-scm.com/docs/hash-function-transition/ ?
 */
function generateChangeId():string {
  return 'I' + createHash('sha1').update(randomUUID()).digest('hex');
}

I'm only need to check how this works after the git hash-function-transition. Currently Gerrit (3.7.0) wont accept Change-Ids that are longer then sha1.

Sorry for the noise with this PR.

@NiasSt90 NiasSt90 deleted the installHook branch January 10, 2023 07:48
viceice added a commit that referenced this pull request Jan 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants