Repository navigation
fix(webhook): trim webhook URL and enforce http/https scheme in schema validation - #2757
Open
Gabrieljr42 wants to merge 3 commits into
Open
Gabrieljr42 wants to merge 3 commits into
Gabrieljr42 wants to merge 3 commits into
Conversation
…a validation Persist a trimmed webhook URL and reject values that are not http or https or that contain whitespace. Verified with npx tsx src/api/integrations/event/webhook/webhook.url.test.ts, npm run lint, and npm run build. Signed-off-by: Gabrieljr42 <gabrieljunior42566@gmail.com>
Contributor
Reviewer's GuideThe PR centralizes webhook URL handling with a shared trim helper and schema regex, applies trimming across both persistence paths, and adds a standalone regression test covering whitespace rejection, HTTP(S) enforcement, valid HTTPS input, and stored-value normalization. Sequence diagram for webhook URL validation and persistencesequenceDiagram
participant Client
participant webhookSchema
participant WebhookController
participant persistedWebhookUrl
participant Storage
Client->>webhookSchema: validate(webhookData)
alt valid http or https URL without whitespace
webhookSchema-->>WebhookController: validation succeeds
WebhookController->>persistedWebhookUrl: persistedWebhookUrl(url)
persistedWebhookUrl-->>WebhookController: trimmed URL
WebhookController->>Storage: persist trimmed URL
else invalid scheme or whitespace
webhookSchema-->>Client: validation fails
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/api/integrations/event/webhook/webhook.schema.ts" line_range="36" />
<code_context>
- url: { type: 'string' },
+ url: {
+ type: 'string',
+ pattern: WEBHOOK_HTTP_URL_PATTERN,
+ description: 'The "url" must be an http or https URL without whitespace',
+ },
</code_context>
<issue_to_address>
**Instance webhooks are skipped**
When an instance is created with a nested webhook URL using a non-HTTP(S) scheme, `instanceSchema` does not validate nested `webhook.url`, so `EventManager.setInstance` forwards it to `WebhookController.set`, which persists it; `emit` then skips the non-HTTP(S) destination and the instance sends no webhook events.
Validate the nested URL in `instanceSchema` or apply `webhookSchema` during instance creation before calling `EventManager.setInstance`.
</issue_to_address>
### Comment 2
<location path="src/api/integrations/event/webhook/webhook.url.ts" line_range="2" />
<code_context>
+/** http(s) URL with no whitespace. Anchored so the schema rejects any other scheme or padding. */
+export const WEBHOOK_HTTP_URL_PATTERN = '^https?:\\/\\/\\S+$';
+
+/** Value stored for a webhook URL. Callers persist this, not the raw input. */
</code_context>
<issue_to_address>
**Malformed destinations pass validation**
When a caller submits a non-whitespace value with an HTTP(S) prefix that is not a valid URL, `WEBHOOK_HTTP_URL_PATTERN` checks only the scheme and whitespace, so malformed destinations pass validation and are persisted; Axios then fails to deliver the webhook event.
Validate that the entire value is a valid HTTP(S) URL, including a host, rather than relying on a scheme-and-whitespace pattern.
Also at `src/api/integrations/event/webhook/webhook.schema.ts:36`.
</issue_to_address>
### Comment 3
<location path="src/api/integrations/event/webhook/webhook.url.ts" line_range="2" />
<code_context>
+/** http(s) URL with no whitespace. Anchored so the schema rejects any other scheme or padding. */
+export const WEBHOOK_HTTP_URL_PATTERN = '^https?:\\/\\/\\S+$';
+
+/** Value stored for a webhook URL. Callers persist this, not the raw input. */
</code_context>
<issue_to_address>
**Valid uppercase schemes are rejected**
When a caller supplies a valid URL with an uppercase or mixed-case HTTP(S) scheme, `WEBHOOK_HTTP_URL_PATTERN` is case-sensitive, so validation rejects the destination and callers cannot save the webhook configuration.
Make the scheme check case-insensitive.
Also at `src/api/integrations/event/webhook/webhook.schema.ts:36`.
</issue_to_address>
### Comment 4
<location path="src/api/integrations/event/webhook/webhook.url.ts" line_range="2" />
<code_context>
+/** http(s) URL with no whitespace. Anchored so the schema rejects any other scheme or padding. */
+export const WEBHOOK_HTTP_URL_PATTERN = '^https?:\\/\\/\\S+$';
+
+/** Value stored for a webhook URL. Callers persist this, not the raw input. */
</code_context>
<issue_to_address>
**Trailing newlines pass validation**
When a submitted URL ends in a line terminator, javaScript’s `$` matches before a final line terminator, so `WEBHOOK_HTTP_URL_PATTERN` accepts the value and schema validation does not reject the trailing whitespace.
Change the pattern to require the actual end of the input after the URL.
Also at `src/api/integrations/event/webhook/webhook.schema.ts:36`.
</issue_to_address>
### Comment 5
<location path="src/api/integrations/event/webhook/webhook.url.test.ts" line_range="23-24" />
<code_context>
+assert.equal(httpsResult.valid, true, 'normal https:// URL must be accepted');
+console.log('normal https:// URL accepted');
+
+const persisted = persistedWebhookUrl(' https://example.com/hook ');
+assert.equal(persisted, 'https://example.com/hook');
+console.log('persisted value is trimmed', persisted);
</code_context>
<issue_to_address>
**Controller stores padded URLs**
When a controller persistence call site stops using the helper, if `WebhookController.set` passes `data.webhook.url` directly to Prisma instead of calling `persistedWebhookUrl`, the test still passes: `webhook.url.test.ts` lines 23–24 invoke and assert only the helper, not the value persisted by the controller.
Exercise `WebhookController.set` and assert that the value passed to Prisma is trimmed.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and webhook URL values are now trimmed before persistence, so a wrong normalization can remain in stored configuration after rollback, but it is bounded and can be corrected by updating the webhook URL. The stricter schema may reject previously accepted values; that is a reversible configuration/API behavior change.
Blocking findings: src/api/integrations/event/webhook/webhook.schema.ts:36, src/api/integrations/event/webhook/webhook.url.ts:2, src/api/integrations/event/webhook/webhook.url.ts:2, src/api/integrations/event/webhook/webhook.url.ts:2, src/api/integrations/event/webhook/webhook.url.test.ts:24
This branch has not been deployed
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
1 participant
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.
Description
Webhook configuration now rejects non-HTTP(S) schemes, missing hosts and whitespace in both webhook settings and nested webhook settings supplied during instance creation. Both schemas use a shared URL format backed by the Node URL parser, so malformed hosts and ports are rejected before instance creation. The controller applies the same check before calling Prisma and returns HTTP 400 for invalid destinations.
Valid uppercase and mixed-case HTTP(S) schemes are accepted and stored in lowercase so the existing delivery scheme check can send them. Controller persistence trims surrounding whitespace on both create and update. Schema validation still rejects whitespace in incoming URL values, including final line terminators.
The regression test calls
WebhookController.setwith an injected Prisma stub and asserts the URL passed to both persistence paths. It also checks that invalid destinations never reach Prisma. It does not start a database, Redis or a message broker.Type of Change
Testing
npm run test:webhook, 7 tests passednpm run lint:checknpm run buildThe cases cover normal HTTPS, HTTP on localhost with a port, IPv6, uppercase and mixed-case schemes, missing hosts, invalid ports, invalid IPv6, unsupported schemes, padded values, embedded whitespace and final LF/CRLF. Instance creation without webhook settings remains valid. Supplying webhook settings requires a URL.
The code quality workflow runs
npm run test:webhookbefore the build.Review follow-up
The update addresses the five Sourcery findings by validating nested instance webhook URLs, checking complete destinations before persistence, accepting uppercase schemes, rejecting trailing newlines at the schema boundary, and testing controller persistence rather than only the trim helper.
Compatibility
Malformed webhook destinations that were previously accepted now return a validation error. No database schema or migration changes are required.