fix: use consistent URL validation pattern in IncomingWebhook - #2725
Open
dajiaohuang wants to merge 3 commits into
Open
fix: use consistent URL validation pattern in IncomingWebhook#2725dajiaohuang wants to merge 3 commits into
dajiaohuang wants to merge 3 commits into
Conversation
…ponse
The JSON.parse at line 802 was not wrapped in a try-catch, which could
cause an unhandled exception if the response body is not valid JSON.
This is inconsistent with the similar operation at line 811 which is
properly wrapped.
Added try-catch to handle parse failures gracefully, returning
{ ok: false, error: <error message> } instead of throwing.
Before this fix, the code assumed `e` is an Error object and accessed `e.message` directly. If `e` was a primitive value or undefined, this could result in undefined being passed to GenerateInstallUrlError. Now we use the same pattern as line 289 in this file: `e instanceof Error ? e.message : String(e)` This ensures a valid string is always passed to GenerateInstallUrlError.
The URL validation in IncomingWebhook used `if (url === undefined)` which only catches undefined values. WebhookTrigger.ts uses the more robust `if (!url)` which catches undefined, null, and empty string. For consistency and better validation, updated IncomingWebhook to use the same pattern as WebhookTrigger.ts.
|
|
Thanks for the contribution! Before we can merge this, we need @dajiaohuang to sign the Salesforce Inc. Contributor License Agreement. |
Member
|
👋 Hey @dajiaohuang! Before we review this PR I'm wondering if it'd be possible to sign the CLA above? 🏛️ ✨ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The URL validation in
IncomingWebhookusedif (url === undefined)which only catches undefined values. Meanwhile,WebhookTriggerin the same package uses the more robustif (!url)which catches undefined, null, and empty string.Problem
At line 74 in
IncomingWebhook.ts:This check would not catch
nullor empty string""values.Fix
Changed to use the same pattern as
WebhookTrigger.ts:This ensures consistent validation across the webhook package and catches all falsy values.
Testing
npm test --workspace=packages/webhook