fix(gitlab): fail closed when the webhook secret is missing - #609
Conversation
verifyGitlabWebhookSignature returned { valid: true } whenever no secret
was configured, short-circuiting before it ever read the X-Gitlab-Token
header. Any unsigned request was therefore accepted outright.
Return an error instead, matching the fail-closed shape already adopted
by the Spotify, Zoom and Slack verifiers (corsairdev#519, corsairdev#520, corsairdev#514).
Fixes corsairdev#594
Adds packages/gitlab/webhooks/types.test.ts: missing secret, the both-missing case, missing token header, a token that does not match the secret, and the matching-token round-trip. Fixes corsairdev#594
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesGitLab webhook verification
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR changes GitLab webhook verification to fail closed when no secret is configured and adds focused regression coverage.
Confidence Score: 5/5The PR appears safe to merge and correctly closes the missing-secret webhook authentication path. The changed verifier rejects requests before token comparison when its configured secret is absent, while preserving the existing missing-header, mismatch, and successful-match behavior with focused tests. Important Files Changed
Reviews (1): Last reviewed commit: "test(gitlab): cover webhook token fail-c..." | Re-trigger Greptile |
|
LGTM |
Description
Fixes #594.
verifyGitlabWebhookSignaturereturned{ valid: true }whenever no secret was configured. As the issue notes, this one is worse than the Jira/Twitter variants: it short-circuits unconditionally, before it ever reads theX-Gitlab-Tokenheader, so any unsigned request was accepted outright.One line, matching the fail-closed shape and the exact
'Missing webhook secret'string already used by the Spotify verifier after the same family was fixed there (#519, #520, #514).Adds
packages/gitlab/webhooks/types.test.ts:Missing webhook secret— the bugvalid: falseMissing X-Gitlab-Token headerX-Gitlab-Token does not match configured secretvalid: trueAll three acceptance criteria in the issue are covered.
Checklist
pnpm lintand all checks pass —biome checkclean on both changed files; the repo's lint-staged hook (biome check --write) ran clean on both commitspnpm typecheckand there are no TypeScript errors — the repo's pre-push typecheck hook passedpnpm buildand all packages build successfullypnpm testand all tests pass — green in CI; see note below for local runsOn the test box. My new suite passes 5/5. The package has two pre-existing failing suites that fail identically on a clean checkout of
main, so this branch changes nothing about them:The two are
api.test.ts(builds an auth header from an unset API key) andintegration.test.ts(Cannot find module 'corsair/orm'— needs a workspace build). Both unrelated; I left them alone.Screenshots / Demos (if applicable)
On R4 in
PLUGIN_PR_RULES.md: that rule exists because "neither CI nor review bots can call the real third-party API." This change calls no GitLab endpoint and needs no credentials (the issue says so too) — it's a pure local comparison, so the unit tests are the verification and they run in CI:Working proof: the CI run for this branch, where the suite executes and passes —
CI Checks.To be straight about it: that is a link to a CI job, not a screen recording. I can't record one, and for this change there is nothing to film — no UI, no CLI output, no third-party call. If a recording is required regardless, say so and I'll arrange it.
Additional Notes
packages/gitlab/**only (R1). Companion to fix(jira): fail closed when webhook secret or signature is missing #608 (the Jira half of this family) — kept separate to honour one plugin per PR.token !== secret, a plain string comparison rather than a timing-safe one. Unlike the HMAC verifiers this compares the shared secret directly, so it is theoretically distinguishable by timing. I left it alone to keep this PR to the one-line fix the issue asks for — happy to send it separately if you want it.