Skip to content

fix(core): reject signatures with empty secret - #593

Merged
yuvrxj-afk merged 2 commits into
corsairdev:mainfrom
ShinyHero666:fix/587-hmac-empty-secret
Aug 4, 2026
Merged

fix(core): reject signatures with empty secret#593
yuvrxj-afk merged 2 commits into
corsairdev:mainfrom
ShinyHero666:fix/587-hmac-empty-secret

Conversation

@ShinyHero666

@ShinyHero666 ShinyHero666 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

verifyHmacSha256Signature returned true before checking the signature whenever the shared secret was empty. That made the shared helper, and its Slack alias, fail open for direct callers.

This change:

  • returns false when the HMAC secret is empty
  • keeps valid signature verification, timestamp validation, and caller-level skip behavior unchanged
  • adds focused regression tests for the empty-secret path, a valid signature, and the Slack alias

Closes #587

Checklist

Before submitting your PR, please verify the following:

  • I have run pnpm lint and all checks pass
  • I have run pnpm typecheck and there are no TypeScript errors
  • I have run pnpm build and all packages build successfully
  • I have run pnpm test and all tests pass
  • I have added or updated tests where applicable
  • I have added or updated necessary documentation (not applicable; no public API change)

Screenshots / Demos (if applicable)

Not applicable.

Additional Notes

Regression validation:

  • before the implementation change, the new empty-secret tests failed with Received: true
  • pnpm --filter corsair test -- --runTestsByPath tests/webhook-utils.test.ts --runInBand (3 passed)
  • pnpm --filter corsair test -- --runInBand --testPathIgnorePatterns=postgres-js-database.test.ts (280 passed, 2 skipped)
  • GitHub CI completed repository-wide lint, plugin/docs validation, typecheck, build, and tests successfully

Summary by CodeRabbit

  • Bug Fixes

    • Webhook signature verification now correctly rejects requests when no secret is configured.
    • Slack signature verification now fails closed when the signing secret is missing.
  • Tests

    • Added coverage for missing secrets, valid signatures, and rejected webhook requests.

@vercel

vercel Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

@ShinyHero666 is attempting to deploy a commit to the corsair Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The shared HMAC-SHA256 verifier now rejects missing secrets. Tests cover empty-secret rejection, valid-secret acceptance, and fail-closed behavior through the Slack signature alias.

Changes

Webhook signature verification

Layer / File(s) Summary
Fail-closed verification and tests
packages/corsair/async-core/webhook-utils.ts, packages/corsair/tests/webhook-utils.test.ts
verifyHmacSha256Signature returns false when the secret is missing. Tests add local HMAC signing and cover empty secrets, valid secrets, and the Slack alias.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • corsairdev/corsair#514 — Both changes enforce fail-closed webhook verification for missing secrets, although this PR changes the shared HMAC utility.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation returns false for empty secrets and adds tests for empty secrets, valid signatures, and the Slack alias as required by issue #587.
Out of Scope Changes check ✅ Passed The code and tests directly support fail-closed signature verification and the requirements in issue #587.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: rejecting signatures when the HMAC secret is empty.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the core Changes in packages/corsair label Aug 4, 2026
@ShinyHero666
ShinyHero666 marked this pull request as ready for review August 4, 2026 13:37
@ShinyHero666

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes Slack-style HMAC verification fail closed when passed an empty secret while preserving caller-level skip behavior.

  • Changes verifyHmacSha256Signature to reject empty secrets.
  • Adds regression coverage for empty-secret rejection, valid signatures, and the Slack alias.

Confidence Score: 5/5

The PR appears safe to merge, with current callers preserving their existing caller-level handling for absent secrets.

The changed helper now fails closed as intended, valid HMAC verification remains covered, and current repository callers guard missing secrets before invoking the helper.

Important Files Changed

Filename Overview
packages/corsair/async-core/webhook-utils.ts Changes the empty-secret branch from successful verification to rejection without altering timestamp or HMAC validation.
packages/corsair/tests/webhook-utils.test.ts Adds focused tests covering empty-secret rejection, successful verification, and delegation through the Slack alias.

Reviews (1): Last reviewed commit: "fix(core): reject signatures with empty ..." | Re-trigger Greptile

@ShinyHero666

Copy link
Copy Markdown
Contributor Author

@ambikeesshh This is ready for review. The full CI suite and both automated reviews are green, with no inline findings. Thanks!

@yuvrxj-afk
yuvrxj-afk merged commit 6f29d95 into corsairdev:main Aug 4, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes in packages/corsair

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(core): verifyHmacSha256Signature should fail closed on empty secret

2 participants