-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
git push --no-verify should be configurable #9673
Comments
Background: However, I find the argument in the original post here to be stronger and more general, just figured I'd put this here because if this doesn't get fixed, it's impossible to run Renovate in an LFS repo without adding official full-blown LFS support to Renovate. Linking #6842 for visibility. |
In our use-case we publish |
I see some big security concerns allowing git hooks in renovate. It would allow a bad user to execute malicious code in renovate context. I would prefer to move git operations to a separate docker sidecar container first. We should only allow hooks in self-hosted mode until we have a safe method to allow hooks, so they can't attack us. This would be same reason why we don't allow npm / renovate postupdate / ... scripts in hosted app. |
@viceice maybe I'm missing something but running If that was true, it would be a security flaw in git itself 😅. If you're running Renovate in a context where you installed hooks yourself (e.g. in a Dockerfile, or on a dev machine) it is obviously your responsibility to make sure those hooks are okay. |
I know by default there are no local hooks after git checkout, until you call a script to installl them, which can be eg So we currently cant be always sure that no script is addind hooks, so we disabled them to be more safe. I see we can use Maybe @rarkins can give some feedback too. |
One possibility (naming is flexible of course) could be this:
If both are true at runtime then the no verify option can be dropped. For the app, we would keep it to |
BTW there also needs to be a way to surface these errors to the repository, e.g. via the Config warning issue. Otherwise errors might go unnoticed |
Is the security implication here that someone might install a hook, that gets run for others' repos (in the hosted version)? Because otherwise I'm assuming that code that runs in e.g. an npm script is equally dangerous to code that runs in a hook it installs (if it's just in the same repo)? Either way, what @rarkins said sounds fine to me. What are "admin" options vs. "user-configurable" options? |
That why npm scripts are also disabled on hosted renovate app and also for self-hosted by default (can be allowed by
renovate/lib/config/definitions.ts Lines 475 to 482 in 572749a
Those admin options can only be set by bot admin. You can easily search the code for |
Alright makes sense. So the purpose of disabling hooks from running is in case someone finds a way to manage to install hooks? But if someone finds a way to install hooks, can't you argue that they can already execute arbitrary code at that point? E.g. if they can install hooks, they can probably put scripts in a lot of different places (and run various things).
Hmm okay, thanks. |
🎉 This issue has been resolved in version 25.23.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What would you like Renovate to be able to do?
By default, since #6521, renovate runs
git push
with the option--no-verify
. This skips any pre-push hooks, as well as hooks on the receiving end. It shouldn't be default to do this, since people might have important checks that maintain their repo integrity. If people want to bypass their own checks, fine, makes sense, however, that should be a flag you turn on in Renovate. E.g. you deliberately say "skip my hooks". I'm fairly certain that it's a minority that need to skip their hooks, which is why it makes sense to default the behavior to just running things like usual.Did you already have any implementation ideas?
Add a new config option on a repo-level, something like
verifyPush
(please suggest a better name if you know one lol) that decides whether you add--no-verify
to thegit push
commands.I'm fine with pushing a PR to do this. It's technically a breaking change however, last time around it was not counted as a breaking change 🤷
The text was updated successfully, but these errors were encountered: