fix(jira): fail closed when webhook secret or signature is missing - #608
Conversation
verifyJiraWebhookSignature returned { valid: true } for any request with
no x-hub-signature header, as long as no secret was configured. An
attacker only had to omit the signature header to bypass verification
entirely.
Check the secret first and never treat a missing header as valid, so
neither a missing secret nor a missing signature can resolve to valid.
This matches the shape already used by the Spotify verifier after the
same fail-open family was fixed there (corsairdev#519, corsairdev#520, corsairdev#514).
Fixes corsairdev#595
Adds packages/jira/webhooks/types.test.ts, mirroring the Spotify verifier's test layout: missing secret, missing signature header, the both-missing case that was the actual bypass, wrong signature, length mismatch, a correct round-trip, and the array-valued header form. Fixes corsairdev#595
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughJira webhook signature verification now rejects requests without a webhook secret. New tests cover missing inputs, invalid signatures, valid HMAC-SHA256 signatures, length mismatches, and array-valued headers. ChangesJira webhook verification
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR closes Jira webhook verification when either the configured secret or request signature is missing.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(jira): tighten fail-closed webhook s..." | Re-trigger Greptile |
|
@greptile review |
Description
Fixes #595.
verifyJiraWebhookSignaturereturned{ valid: true }for any request with nox-hub-signatureheader, as long as no secret was configured — so omitting the signature header bypassed verification entirely.Now the secret is checked first and a missing header is never valid, so neither a missing secret nor a missing signature can resolve to
valid: true:I deliberately matched the ordering and error strings already used by
verifySpotifyWebhookSignature, since that verifier was fixed for this same fail-open family (#519, #520, #514) — so all the plugins now read the same way.Adds
packages/jira/webhooks/types.test.ts, mirroring the Spotify verifier's test layout:Missing webhook secretvalid: false— this is the actual bypassMissing x-hub-signature headerInvalid signatureSignature length mismatchvalid: truestring[]valid: trueAll four acceptance criteria in the issue are covered.
Checklist
pnpm lintand all checks pass —biome checkis clean on both changed files, and the repo's own lint-staged hook (biome check --write) ran clean on both commitspnpm typecheckand there are no TypeScript errors —tsc --buildexits 0; the repo's pre-push typecheck hook also passedpnpm buildand all packages build successfully —pnpm --filter @corsair-dev/jira buildsucceedspnpm testand all tests pass — green in CI; see note below for local runsTwo notes so the ticks above are not taken on trust:
Tests. My new suite passes 7/7, but the pre-existing
packages/jira/api.test.tsfails 12/12 on a clean checkout ofmaintoo, because it builds an auth header from an unset API key (Buffer.from(apiKey)→TypeError: ... Received undefined). Unrelated to this change; I left it alone. Happy to fix it in a separate PR if useful.Lint. I verified
biome checkon the two files I changed rather than repo-wide, because I'm on a Windows checkout wherecore.autocrlfgives every file CRLF and repo-widebiome check .therefore fails identically on unmodifiedmain. Git normalises to LF on commit, so the diff itself carries no line-ending changes —git diff --staton the branch is+117 / -4across exactly two files.Screenshots / Demos (if applicable)
On R4 in
PLUGIN_PR_RULES.md: the rule exists because "neither CI nor review bots can call the real third-party API." That doesn't apply here — this changes a pure local HMAC comparison, calls no Jira endpoint, and needs no credentials (the issue notes this too). The unit tests are the verification, and they run in CI. Terminal output: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/jira/**only (R1) — no changes toconstants.tsor the lockfile were needed.Summary by CodeRabbit