fix(security): use HMAC with SECRET_KEY for webhook hash

[onovy]

WARNING: This is breaking change and old tokens will stop to work!

[redimp]

Thanks for submitting this PR! I'm questioning whether a strong secret is truly needed here, given the limited attack surface. Perhaps I'm missing something?

More importantly, this PR appears incomplete; it only covers the update of the hook validation and is missing to update to the generation of the hash in the admin interface.

[onovy]

An attacker who knows (or guesses) the remote URL can compute the hash and trigger spurious pulls — but pulls only come from the already-configured remote, so the damage is limited to triggering unnecessary git operations.
Attack surface is small/limited, but this can be used to:

• DoS Otterwiki instance (git pull is not cheap)
• Reflection DDoS, when many public Otter wiki instances can DDoS same git source (for example github.com)

Ad generation in admin: That's thing I was missing, "where is it generated for user?" :). Sorry for mistake, working on fix now.

[redimp]

I thought about the DoS aspect of the pull webhook and discussed it briefly with @deseven.

I'm not as much concerned about an attacker abusing the webhook than accidental calls to the webhook. So this could be improved by only allowing a webhook pull every 60s and returning a 429 else.

[onovy]

in my POV rate limiting is just workaround. Problem is still there: (Almost) anyone, without authentication can trigger "git pull". Webhook secret (by definition and it's name :) should be secret. Current implementation is just security by obscurity, hash check is useless now.

[onovy]

ad rate limit: This could break it more. For example two commits done to external repository in short period = two web hook trigger, second one fails => inconsistency.

Another idea, let's make it backward compatible:

• generate only new tokens
• accept old and new tokens
• add config option which disables accepting of old tokens - default false

[redimp]

I would assume anything triggering the webhook url has a proper retry mechanism .. and a 429 is exactly made for this. It tells the client to retry.

[deseven]

Another idea, let's make it backward compatible:

* generate only new tokens

* accept old and new tokens

* add config option which disables accepting of old tokens - default false

Could be handled more gracefully:

• add /-/api/v2/pull/<string:secure_webhook_hash> route, keep the v1 one intact with current logic
• generate new v2 links in the admin panel using the new logic

The generated link in the admin panel will no longer match the link used in previously configured setups, but the old links will still work.

UPD: Ah, well, in that case it will be still possible to use the old route :D Maybe we just need to add a special property to the remotes that are using the new logic? Like GIT_REMOTE_PULL_URL_SECURE=true which would be false/undefined on existing setups and would be automatically set to true if user changes the preferences in the admin panel. We can also write a warning there that the webhook URL will be changed on save. Still feels a bit better than some env param that everyone will forget to change.

[onovy]

Now it's completely backward compatible.

[deseven]

Not a fan of this personally, maybe better to hide the field completely and instead write a nicely looking note? We can also write here some warning when GIT_REMOTE_PULL_URL_SECURE is not true so the user will be aware that their current configuration needs to be updated.

[onovy]

right, fixed in last commit

[redimp]

Hey @onovy, thanks for the effort. Will review and check later this week.