diff --git a/.changeset/webhook-url-mismatch-warning.md b/.changeset/webhook-url-mismatch-warning.md new file mode 100644 index 000000000..56df58305 --- /dev/null +++ b/.changeset/webhook-url-mismatch-warning.md @@ -0,0 +1,7 @@ +--- +"@slack/webhook": minor +--- + +feat: add logger with URL mismatch warning + +Both `IncomingWebhook` and `WebhookTrigger` now accept optional `logger` and `logLevel` options. On construction, each class warns if the provided URL appears to be intended for the other class (e.g. a `/triggers/` URL passed to `IncomingWebhook`), helping catch misconfiguration before it causes duplicate deliveries or hangs. diff --git a/package-lock.json b/package-lock.json index 5377de0bd..8d7a3b3b1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5587,9 +5587,9 @@ "ws": "^8" }, "devDependencies": { - "@types/sinon": "^22.0.0", + "@types/sinon": "^22", "@types/ws": "^8", - "sinon": "^22.1.0" + "sinon": "^22" }, "engines": { "node": ">=18", @@ -5809,6 +5809,7 @@ "version": "8.0.0", "license": "MIT", "dependencies": { + "@slack/logger": "^5.0.0", "@slack/types": "^3.0.0", "@types/node": ">=20", "@types/retry": "0.12.5", diff --git a/packages/webhook/package.json b/packages/webhook/package.json index 1dac5777c..958fe023e 100644 --- a/packages/webhook/package.json +++ b/packages/webhook/package.json @@ -41,6 +41,7 @@ "test:coverage": "npm run build && node --experimental-test-coverage --test-reporter=spec --test-reporter-destination=stdout --test-reporter=lcov --test-reporter-destination=lcov.info --test-reporter=junit --test-reporter-destination=test-results.xml --import tsx --test src/IncomingWebhook.test.ts src/WebhookTrigger.test.ts src/instrument.test.ts src/retry-policies.test.ts" }, "dependencies": { + "@slack/logger": "^5.0.0", "@slack/types": "^3.0.0", "@types/node": ">=20", "@types/retry": "0.12.5", diff --git a/packages/webhook/src/IncomingWebhook.test.ts b/packages/webhook/src/IncomingWebhook.test.ts index 302e94833..3c28e97da 100644 --- a/packages/webhook/src/IncomingWebhook.test.ts +++ b/packages/webhook/src/IncomingWebhook.test.ts @@ -11,6 +11,7 @@ import { } from './errors'; import { type FetchFunction, IncomingWebhook } from './IncomingWebhook'; import { getUserAgent } from './instrument'; +import { LogLevel } from './logger'; import { rapidRetryPolicy } from './retry-policies'; const url = 'https://hooks.slack.com/services/FAKEWEBHOOK'; @@ -49,6 +50,45 @@ describe('IncomingWebhook', () => { await webhook.send('Hello'); assert.ok(fetchCalled); }); + + it('should warn when URL contains /triggers/', () => { + const warnings: string[] = []; + const logger = { + debug() {}, + info() {}, + warn(...msg: string[]) { + warnings.push(msg.join(' ')); + }, + error() {}, + setLevel() {}, + getLevel() { + return LogLevel.WARN; + }, + setName() {}, + }; + new IncomingWebhook('https://hooks.slack.com/triggers/T000/abc', { logger }); + assert.strictEqual(warnings.length, 1); + assert.ok(warnings[0].includes('WebhookTrigger')); + }); + + it('should not warn when URL contains /services/', () => { + const warnings: string[] = []; + const logger = { + debug() {}, + info() {}, + warn(...msg: string[]) { + warnings.push(msg.join(' ')); + }, + error() {}, + setLevel() {}, + getLevel() { + return LogLevel.WARN; + }, + setName() {}, + }; + new IncomingWebhook('https://hooks.slack.com/services/T000/B000/abc', { logger }); + assert.strictEqual(warnings.length, 0); + }); }); describe('send()', () => { diff --git a/packages/webhook/src/IncomingWebhook.ts b/packages/webhook/src/IncomingWebhook.ts index ba19ee2f6..f4bed1575 100644 --- a/packages/webhook/src/IncomingWebhook.ts +++ b/packages/webhook/src/IncomingWebhook.ts @@ -3,6 +3,7 @@ import pRetry, { AbortError } from 'p-retry'; import { IncomingWebhookHTTPError, IncomingWebhookRequestError, SlackWebhookError } from './errors'; import { getUserAgent } from './instrument'; +import { getLogger, type Logger, LogLevel } from './logger'; import type { RetryOptions } from './retry-policies'; export interface FetchHeaders { @@ -65,6 +66,8 @@ export class IncomingWebhook { */ private retryConfig: RetryOptions; + private logger: Logger; + public constructor( url: string, defaults: IncomingWebhookDefaultArguments = { @@ -75,6 +78,8 @@ export class IncomingWebhook { throw new Error('Incoming webhook URL is required'); } + this.logger = getLogger('IncomingWebhook', defaults.logLevel ?? LogLevel.INFO, defaults.logger); + this.url = url; this.fetchFn = defaults.fetch ?? globalThis.fetch; this.timeout = defaults.timeout ?? 0; @@ -83,8 +88,22 @@ export class IncomingWebhook { 'User-Agent': getUserAgent(), }; + if (url.includes('/triggers/')) { + this.logger.warn( + 'This URL looks like a webhook trigger (contains "/triggers/"). ' + + 'Consider using WebhookTrigger instead of IncomingWebhook.', + ); + } + // Remove transport options so they don't leak into payloads - const { fetch: _fetch, timeout: _timeout, retryConfig: _retryConfig, ...messageDefaults } = defaults; + const { + fetch: _fetch, + timeout: _timeout, + retryConfig: _retryConfig, + logger: _logger, + logLevel: _logLevel, + ...messageDefaults + } = defaults; this.defaults = messageDefaults; } @@ -150,15 +169,17 @@ export class IncomingWebhook { */ export interface IncomingWebhookDefaultArguments { - username?: string; + channel?: string; + fetch?: FetchFunction; icon_emoji?: string; icon_url?: string; - channel?: string; - text?: string; link_names?: boolean; - fetch?: FetchFunction; - timeout?: number; + logLevel?: LogLevel; + logger?: Logger; retryConfig?: RetryOptions; + text?: string; + timeout?: number; + username?: string; } export interface IncomingWebhookSendArguments extends IncomingWebhookDefaultArguments { diff --git a/packages/webhook/src/WebhookTrigger.test.ts b/packages/webhook/src/WebhookTrigger.test.ts index 303df61eb..80bbdce39 100644 --- a/packages/webhook/src/WebhookTrigger.test.ts +++ b/packages/webhook/src/WebhookTrigger.test.ts @@ -11,6 +11,7 @@ import { WebhookTriggerRequestError, } from './errors'; import { addAppMetadata } from './instrument'; +import { LogLevel } from './logger'; import { rapidRetryPolicy } from './retry-policies'; import { WebhookTrigger } from './WebhookTrigger'; @@ -45,6 +46,45 @@ describe('WebhookTrigger', () => { assert.throws(() => new WebhookTrigger(undefined as any), /URL is required/); assert.throws(() => new WebhookTrigger(''), /URL is required/); }); + + it('should warn when URL contains /services/', () => { + const warnings: string[] = []; + const logger = { + debug() {}, + info() {}, + warn(...msg: string[]) { + warnings.push(msg.join(' ')); + }, + error() {}, + setLevel() {}, + getLevel() { + return LogLevel.WARN; + }, + setName() {}, + }; + new WebhookTrigger('https://hooks.slack.com/services/T000/B000/abc', { logger }); + assert.strictEqual(warnings.length, 1); + assert.ok(warnings[0].includes('IncomingWebhook')); + }); + + it('should not warn when URL contains /triggers/', () => { + const warnings: string[] = []; + const logger = { + debug() {}, + info() {}, + warn(...msg: string[]) { + warnings.push(msg.join(' ')); + }, + error() {}, + setLevel() {}, + getLevel() { + return LogLevel.WARN; + }, + setName() {}, + }; + new WebhookTrigger('https://hooks.slack.com/triggers/T000/abc', { logger }); + assert.strictEqual(warnings.length, 0); + }); }); describe('send()', () => { diff --git a/packages/webhook/src/WebhookTrigger.ts b/packages/webhook/src/WebhookTrigger.ts index a9b64e08e..360ef04fd 100644 --- a/packages/webhook/src/WebhookTrigger.ts +++ b/packages/webhook/src/WebhookTrigger.ts @@ -3,6 +3,7 @@ import pRetry, { AbortError } from 'p-retry'; import { SlackWebhookError, WebhookTriggerHTTPError, WebhookTriggerRequestError } from './errors'; import type { FetchFunction, FetchResponse } from './IncomingWebhook'; import { getUserAgent } from './instrument'; +import { getLogger, type Logger, LogLevel } from './logger'; import type { RetryOptions } from './retry-policies'; /** @@ -35,6 +36,8 @@ export class WebhookTrigger { */ private retryConfig: RetryOptions; + private logger: Logger; + public constructor( url: string, defaults: WebhookTriggerDefaultArguments = { @@ -45,6 +48,8 @@ export class WebhookTrigger { throw new Error('Webhook trigger URL is required'); } + this.logger = getLogger('WebhookTrigger', defaults.logLevel ?? LogLevel.INFO, defaults.logger); + this.url = url; this.fetchFn = defaults.fetch ?? globalThis.fetch; this.timeout = defaults.timeout ?? 0; @@ -52,6 +57,13 @@ export class WebhookTrigger { this.headers = { 'User-Agent': getUserAgent(), }; + + if (url.includes('/services/')) { + this.logger.warn( + 'This URL looks like an incoming webhook (contains "/services/"). ' + + 'Consider using IncomingWebhook instead of WebhookTrigger.', + ); + } } /** @@ -112,8 +124,10 @@ export class WebhookTrigger { export interface WebhookTriggerDefaultArguments { fetch?: FetchFunction; - timeout?: number; + logLevel?: LogLevel; + logger?: Logger; retryConfig?: RetryOptions; + timeout?: number; } export interface WebhookTriggerSendArguments { diff --git a/packages/webhook/src/index.ts b/packages/webhook/src/index.ts index fccbe0f33..4e7eba589 100644 --- a/packages/webhook/src/index.ts +++ b/packages/webhook/src/index.ts @@ -22,6 +22,8 @@ export { export { addAppMetadata } from './instrument'; +export { Logger, LogLevel } from './logger'; + export { default as retryPolicies, RetryOptions } from './retry-policies'; export { diff --git a/packages/webhook/src/logger.ts b/packages/webhook/src/logger.ts new file mode 100644 index 000000000..0e713507c --- /dev/null +++ b/packages/webhook/src/logger.ts @@ -0,0 +1,10 @@ +import { ConsoleLogger, type Logger, type LogLevel } from '@slack/logger'; + +export { Logger, LogLevel } from '@slack/logger'; + +export function getLogger(name: string, level: LogLevel, existingLogger?: Logger): Logger { + const logger: Logger = existingLogger ?? new ConsoleLogger(); + logger.setName(name); + logger.setLevel(level); + return logger; +}