fix: reject empty signingSecret to prevent involuntary signature bypass#2946
Conversation
🦋 Changeset detectedLatest commit: 28d09d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
🧪 Taking a look at failing E2E tests now but the failure seems unrelated to these changes. |
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin Such huge thanks for getting this fix set to release 🏁
I left a comment on the Express receiver implementation but that's covered well in testing so we might decide to follow up with changes. For now let's get this merged 🚢 💨
| }); | ||
| } | ||
| if (signatureVerification === true && signingSecret === undefined) { | ||
| if (signatureVerification === true && !signingSecret) { |
There was a problem hiding this comment.
🌟 praise: Fan of the generic false checks!
| if (typeof signingSecret !== 'function') { | ||
| verifySigningSecret(signingSecret, signatureVerification); | ||
| } |
There was a problem hiding this comment.
🔬 question: Is this alright as a best effort check? I'm wondering if we should follow up with an addition that handles the function case?
🗣️ ramble: I notice the empty string case from "function" inputs is caught with more confidence in buildVerificationBodyParserMiddleware so don't consider this a blocker! I'm hoping to mirror implementations across receivers however with these checks all together near the start of this constructor!
| it('should succeed with a token for single team authorization', async () => { | ||
| const MockApp = importApp(overrides); | ||
| const app = new MockApp({ token: '', signingSecret: '' }); | ||
| const app = new MockApp({ token: '', signingSecret: 'test-signing-secret' }); |
There was a problem hiding this comment.
🧪 praise: More explicit test cases like this are nice!
|
🧪 The failing E2E tests are hoped to resolve on |
Summary
signingSecret('') at initialization across all receivers and in the request verification layer to prevent accidental verification bypassverifySigningSecret()helper that validates the signing secret is a non-empty string when signature verification is enabledApp,HTTPReceiver,ExpressReceiver,AwsLambdaReceiver,verifyRequest, andverifySigningSecretMotivation
Previously, passing an empty string (
'') assigningSecretdid not trigger an error, the value is truthy enough to pass=== undefinedchecks but propagates tocreateHmac("sha256", ""), which produces a valid HMAC keyed with an empty secret. This would bypass the request verification without warning the user.The proper way to disable signature verification is by setting
signatureVerification: falseReproduction
Server with issue (
app.js):Forged request:
With this fix applied, the App constructor now throws an AppInitializationError and receivers throw a ReceiverInitializationError, preventing the server from starting with an empty signing secret.
Requirements