Skip to content

feat: 增强 Webhook 通知系统(#485)#504

Closed
ding113 wants to merge 1 commit intodevfrom
feature/issue-485-webhook-notifications
Closed

feat: 增强 Webhook 通知系统(#485)#504
ding113 wants to merge 1 commit intodevfrom
feature/issue-485-webhook-notifications

Conversation

@ding113
Copy link
Owner

@ding113 ding113 commented Jan 2, 2026

增强 Webhook 通知系统

Summary

实现 Issue #485 的完整方案:从单一企业微信 Webhook 升级为支持多目标、多平台的通知系统架构,新增钉钉、Telegram、自定义 Webhook 支持,并保持向后兼容。

Fixes #485 - 请求添加自定义WebHook通知
Follow-up to #490 - 在飞书支持基础上进一步扩展为完整的多目标架构


Problem

当前 CCH 通知系统存在的局限性(由 #485#490 识别):

  1. 单一目标限制:每种通知类型只能配置一个 Webhook URL
  2. 平台扩展性差:PR feat(notification): add Feishu webhook support #490 添加飞书支持后,仍无法支持钉钉、Telegram 等其他平台
  3. 缺少模板定制:无法针对不同目标定制消息格式
  4. 代理配置不灵活:无法为不同 Webhook 目标配置专用代理

Solution

采用"目标与绑定分离"的架构重构通知系统:

核心架构

Notification Type (熔断/成本/排行榜)
    ↓ N:N 绑定关系
Webhook Targets (钉钉Bot1, TelegramBot2, 自定义API...)
    ↓ 各自配置
独立的凭证/代理/模板设置

关键设计决策

  1. 向后兼容notification_settings.use_legacy_mode 标志保留旧配置可用
  2. 平台无关抽象:Template → Placeholder → Renderer 三层分离
  3. 降级策略:代理失败可降级为直连(proxy_fallback_to_direct
  4. 模板覆盖:Binding 级别可覆盖 Target 级别的自定义模板

Changes

Database Migration (drizzle/0043_lonely_rick_jones.sql)

新增表结构:

表名 用途 关键字段
webhook_targets Webhook 目标配置 provider_type, webhook_url, dingtalk_secret, telegram_bot_token, proxy_url
notification_target_bindings 通知类型与目标的绑定关系 notification_type, target_id, schedule_cron, template_override

新增枚举类型:

  • notification_type: 'circuit_breaker' | 'daily_leaderboard' | 'cost_alert'
  • webhook_provider_type: 'wechat' | 'feishu' | 'dingtalk' | 'telegram' | 'custom'

兼容性字段:

  • notification_settings.use_legacy_mode (默认 true) - 保留旧版单 Webhook 配置

Backend Changes

New Server Actions (Auto-exposed via /api/actions)

  • src/actions/webhook-targets.ts (+448 lines)

    • getWebhookTargetsAction() - 列出所有 Webhook 目标
    • createWebhookTargetAction() - 创建新目标(支持钉钉签名、Telegram 凭证、自定义模板)
    • updateWebhookTargetAction() - 更新目标配置
    • deleteWebhookTargetAction() - 删除目标
    • testWebhookTargetAction() - 发送测试消息(支持指定通知类型模板)
  • src/actions/notification-bindings.ts (+64 lines)

    • getBindingsForTypeAction() - 获取指定通知类型的绑定配置
    • updateBindingsAction() - 批量更新绑定关系

New Webhook Renderers

  • src/lib/webhook/renderers/dingtalk.ts (+111 lines)

  • src/lib/webhook/renderers/telegram.ts (+117 lines)

  • src/lib/webhook/renderers/custom.ts (+59 lines)

    • 支持自定义 HTTP 请求模板(custom_template JSONB 字段)
    • 允许用户定义请求体结构和自定义 headers

Template System

  • src/lib/webhook/templates/placeholders.ts (+203 lines)

    • 占位符引擎:{{provider_name}}, {{error_message}}, {{total_cost}}
    • 支持 30+ 占位符变量
    • 类型安全的 Placeholder → Value 映射
  • src/lib/webhook/templates/defaults.ts (+45 lines)

    • 为每种通知类型提供默认模板
    • 支持 Binding 级别的 template_override 覆盖

Notification Dispatch Refactor

  • src/lib/notification/notification-queue.ts (~150 lines changed)

    • 从单 Webhook URL 升级为多目标并发发送
    • 支持 Legacy 模式降级(useLegacyMode === true 时使用旧逻辑)
    • 绑定配置查询与调度分离
  • src/lib/notification/notifier.ts (+94 lines)

    • 新增 sendToMultipleTargets() 方法并发发送
    • Target 级别的代理配置与降级逻辑

Frontend Changes

Complete Notification Settings Page Refactor

src/app/[locale]/settings/notifications/page.tsx (-532/+70 lines)

原页面为单体组件,现重构为组合式架构:

新增组件模块:

  1. GlobalSettingsCard - 全局开关与 Legacy 模式切换
  2. NotificationTypeCard - 单个通知类型的绑定管理卡片
  3. BindingSelector - 多选目标 + Cron + 模板覆盖配置
  4. WebhookTargetsSection - Webhook 目标列表管理
  5. WebhookTargetCard - 单个目标的查看/编辑/删除/测试
  6. WebhookTargetDialog - 创建/编辑目标的表单对话框
  7. WebhookTypeForm - 平台类型特定字段表单(钉钉签名/Telegram 凭证/自定义模板)
  8. TemplateEditor - 自定义模板 JSON 编辑器
  9. ProxyConfigSection - 代理 URL + 降级开关
  10. TestWebhookButton - 发送测试消息按钮

状态管理钩子:

  • src/app/[locale]/settings/notifications/_lib/hooks.ts (+345 lines)
    • useNotificationsPageData() - 统一数据获取与状态同步
    • useWebhookTargetForm() - 表单验证与提交逻辑
    • Optimistic UI 更新

Zod Schema 验证:

  • src/app/[locale]/settings/notifications/_lib/schemas.ts (+133 lines)
    • 针对每种 provider_type 的条件验证
    • 钉钉/Telegram 必填字段验证
    • 自定义模板 JSON 格式验证

I18n Updates

更新所有 5 种语言的通知设置文案:

语言 文件 新增/修改
英语 messages/en/settings.json +89/-7
日语 messages/ja/settings.json +88/-6
俄语 messages/ru/settings.json +88/-6
简中 messages/zh-CN/settings.json +89/-7
繁中 messages/zh-TW/settings.json +89/-7

新增翻译键示例:

  • webhookTargets - Webhook 目标管理相关文案
  • providerTypes - 平台类型选择器
  • bindingConfiguration - 绑定配置表单
  • templateEditor - 模板编辑器提示文案

API Route Updates

  • src/app/api/actions/[...route]/route.ts (+241 lines)
    • 注册 webhook-targetsnotification-bindings 到 OpenAPI spec
    • Swagger UI: /api/actions/docs 现包含新端点
    • Scalar UI: /api/actions/scalar 更新

Repository Layer

  • src/repository/webhook-targets.ts (+170 lines) - Webhook 目标 CRUD
  • src/repository/notification-bindings.ts (+259 lines) - 绑定关系 CRUD
  • src/repository/notifications.ts (+80 lines) - 新增 getNotificationBindings() 方法

Type Definitions

  • src/types/fetch-socks.d.ts (+27 lines) - fetch-socks 包的类型声明(支持 SOCKS 代理)

Test Coverage

Unit Tests (New)

文件 覆盖范围
tests/unit/webhook/notifier.test.ts Webhook 发送逻辑、重试机制
tests/unit/webhook/renderers/custom.test.ts 自定义模板渲染
tests/unit/webhook/renderers/dingtalk.test.ts 钉钉消息格式 + 签名算法
tests/unit/webhook/renderers/telegram.test.ts Telegram HTML 格式化
tests/unit/webhook/templates/placeholders.test.ts 占位符替换引擎(30+ 占位符)

Integration Tests (New)

文件 覆盖范围
tests/integration/webhook-targets-crud.test.ts Webhook 目标 CRUD 操作与数据库交互
tests/integration/notification-bindings.test.ts 绑定配置的存储与查询

E2E Tests (New)

文件 覆盖范围
tests/e2e/notification-settings.test.ts 通知设置页面完整操作流程(创建目标 → 配置绑定 → 发送测试消息)

API Integrity Tests (Updated)

  • tests/api/api-actions-integrity.test.ts (+29 lines)
    • 验证新增的 12 个 Server Actions 正确注册到 /api/actions

Breaking Changes

无破坏性变更

潜在影响点 缓解措施
数据库 Schema 变更 迁移使用 IF NOT EXISTS(幂等)+ use_legacy_mode 默认为 true
旧配置兼容性 notification_settings 表保留所有旧字段,Legacy 模式仍使用旧逻辑
前端 UI 重构 Legacy 模式下 UI 显示旧版单 Webhook 配置表单

迁移路径(可选):

  1. 升级后系统默认工作在 Legacy 模式(use_legacy_mode = true
  2. 管理员在 UI 中点击"切换到多目标模式"
  3. 系统自动将旧配置迁移为对应的 Webhook Target + Binding
  4. 迁移后可以添加额外的目标和自定义配置

Manual Testing Checklist

Legacy 模式测试

  • 升级后旧配置正常工作(熔断/成本/排行榜通知发送成功)
  • Legacy 模式下 UI 显示旧版编辑表单

多目标模式测试

  • 创建钉钉 Webhook 目标(需提供 dingtalk_secret
  • 创建 Telegram Bot 目标(需提供 bot_tokenchat_id
  • 创建自定义 Webhook 目标(自定义 JSON 模板)
  • 为"熔断通知"绑定多个目标
  • 触发熔断事件,验证所有绑定目标均收到通知
  • 测试代理配置:设置 proxy_url + proxy_fallback_to_direct = true
  • 测试模板覆盖:在 Binding 级别覆盖默认模板
  • 使用"测试 Webhook"按钮发送测试消息

边界情况

  • 钉钉签名验证失败时的错误提示
  • Telegram API 认证失败时的错误提示
  • 自定义模板 JSON 格式错误时的表单验证
  • 删除被绑定的 Webhook Target 时的级联删除(ON DELETE CASCADE)

Screenshots

暂无 - 需补充多目标配置 UI 和各平台接收到的通知截图


Validation

所有检查项已通过:

✅ bun run lint          # Biome 检查通过
✅ bun run typecheck     # TypeScript 类型检查通过
✅ bun run test          # 单元测试通过
✅ bun run test:integration  # 集成测试通过
✅ bun run test:e2e      # E2E 测试通过
✅ bun run build         # 生产构建成功

Description enhanced by Claude AI

Greptile Summary

This PR successfully implements a comprehensive webhook notification system enhancement that expands beyond WeChat to support DingTalk, Telegram, Feishu, and custom webhooks, addressing Issue #485.

Key Changes:

  • Introduces new webhook_targets and notification_target_bindings tables with idempotent migrations maintaining backward compatibility via useLegacyMode flag
  • Implements multi-target architecture allowing multiple webhook destinations per notification type (circuit breaker, daily leaderboard, cost alerts)
  • Adds provider-specific renderers for DingTalk (markdown), Telegram (HTML), and custom webhooks with template placeholder support
  • Includes SSRF protection blocking internal/private network addresses in webhook URLs (proxies exempt for Telegram use cases)
  • Implements proxy support with automatic fallback to direct connection on proxy failure
  • Adds DingTalk signature authentication using HMAC-SHA256
  • Provides comprehensive i18n updates across 5 locales (en, zh-CN, zh-TW, ja, ru)
  • Includes robust test coverage: unit tests for renderers, integration tests for CRUD operations, and e2e tests for UI workflows

Architecture:

  • Legacy mode: single webhook URL per notification type (backward compatible)
  • New mode: multiple targets per notification type with per-binding configuration
  • Auto-migration: automatically switches to new mode when first webhook target is created
  • Redis-based deduplication prevents duplicate circuit breaker alerts within 5-minute windows

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is production-ready with comprehensive safeguards: idempotent migrations ensure safe deployment, SSRF protection prevents security vulnerabilities, backward compatibility via legacy mode ensures no breaking changes, extensive test coverage validates functionality across unit/integration/e2e levels, and the code follows established patterns with proper error handling and validation throughout
  • No files require special attention

Important Files Changed

Filename Overview
drizzle/0043_lonely_rick_jones.sql Adds webhook_targets and notification_target_bindings tables with proper idempotent migrations, foreign keys, and indexes
src/drizzle/schema.ts Defines new webhook_targets and notification_target_bindings schemas with proper enums, indexes, and foreign key relationships
src/repository/webhook-targets.ts Implements CRUD operations for webhook targets with proper type conversions and error handling
src/repository/notification-bindings.ts Manages notification-to-target bindings with upsert logic, proper joins, and enabled state filtering
src/actions/webhook-targets.ts Server actions with SSRF protection, validation, proxy support, and auto-migration from legacy mode
src/lib/webhook/notifier.ts Core webhook sender with multi-provider support, DingTalk signature, proxy with fallback, and retry logic
src/lib/webhook/renderers/dingtalk.ts DingTalk markdown renderer with proper HTML escaping and structured message formatting
src/lib/webhook/renderers/telegram.ts Telegram HTML renderer with proper escaping and chat_id injection
src/lib/webhook/renderers/custom.ts Custom webhook renderer supporting template placeholders with deep object replacement
src/lib/webhook/templates/placeholders.ts Template variable builder with type-specific placeholders and safe fallback handling
src/lib/notification/notifier.ts Notification dispatcher with legacy/new mode support, Redis deduplication, and multi-target dispatching
src/app/[locale]/settings/notifications/page.tsx Main notifications settings page with global settings, webhook targets section, and notification type cards
tests/unit/webhook/notifier.test.ts Unit tests for webhook notifier covering provider detection, sending, and error handling
tests/integration/webhook-targets-crud.test.ts Integration tests verifying webhook target CRUD operations and test result updates
tests/e2e/notification-settings.test.ts E2E tests validating notification settings UI interactions and workflows

Sequence Diagram

sequenceDiagram
    participant User as User/System
    participant Notifier as Notifier
    participant Queue as NotificationQueue
    participant Binding as BindingRepository
    participant Target as WebhookTarget
    participant Renderer as Renderer
    participant External as External API

    Note over User,External: New Multi-Target Architecture

    User->>Notifier: sendCircuitBreakerAlert(data)
    Notifier->>Notifier: Check if enabled & check Redis cache
    
    alt Legacy Mode
        Notifier->>Queue: addNotificationJob(type, legacyWebhook, data)
        Queue->>Target: Load webhook URL
        Queue->>Renderer: Detect provider & render message
        Renderer->>External: POST webhook (with retry)
        External-->>Queue: Response
    else New Mode (Multi-Target)
        Notifier->>Binding: getEnabledBindingsByType(type)
        Binding-->>Notifier: Return bindings with targets
        loop For each binding
            Notifier->>Queue: addNotificationJobForTarget(type, targetId, bindingId, data)
            Queue->>Target: Load target config
            Queue->>Renderer: createRenderer(providerType, config)
            Renderer->>Renderer: Apply template placeholders
            alt Has proxy
                Renderer->>External: POST via proxy
                alt Proxy fails & fallback enabled
                    Renderer->>External: POST direct
                end
            else No proxy
                Renderer->>External: POST direct
            end
            External-->>Queue: Response
            Queue->>Target: updateTestResult(result)
        end
    end
    
    Note over User,External: Supports WeChat, Feishu, DingTalk, Telegram, Custom
Loading

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

此拉取请求对 Webhook 通知系统进行了重大增强,引入了一个更灵活和强大的多目标管理方法。它从单一 URL 配置转向允许用户定义多个 Webhook 目标(支持钉钉、Telegram 和自定义 Webhook 等多种平台),并可将这些目标绑定到不同的通知类型,提供高级自定义选项,如模板覆盖和代理设置。这些更改涵盖了数据库架构、后端 API、通知调度逻辑以及前端通知设置界面的全面重构,旨在提供更精细和可控的通知体验。

Highlights

  • 数据库架构更新: 新增了 webhook_targetsnotification_target_bindings 表,并在 notification_settings 表中添加了 useLegacyMode 字段,以支持更灵活的通知配置。
  • 后端逻辑增强: 新增了 WebhookTargetBinding 仓库以及相应的服务器动作,并将其注册到 /api/actions,实现了对 Webhook 目标和绑定的管理功能。
  • Webhook 渲染器扩展: 为钉钉、Telegram 和自定义 Webhook 增加了新的渲染器,支持模板占位符、模板覆盖、代理设置以及代理失败回退直连机制。
  • 通知调度与发送: 通知调度和发送链路现在支持多目标绑定,同时保持了对旧版单 URL 通知模式的兼容性。
  • 前端用户界面重构: 重构了通知设置页面,支持多目标管理、绑定配置和模板编辑,旧版 Webhook 在兼容模式下仍可编辑。
  • 国际化支持: 更新了英语、简体中文、繁体中文、日语和俄语的文案,以支持新的通知功能。
  • 全面测试覆盖: 新增了单元测试、集成测试和端到端测试,确保新功能的稳定性和正确性。
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

@github-actions github-actions bot added enhancement New feature or request area:UI area:i18n area:core size/XL Extra Large PR (> 1000 lines) labels Jan 2, 2026
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

这个 PR 实现了一个非常强大的 Webhook 通知系统增强,工作量巨大,完成度非常高,值得称赞。新功能包括:

  • 支持多推送目标和绑定,结构清晰灵活。
  • 新增了对钉钉、Telegram 和自定义 Webhook 的支持,并提供了模板功能。
  • 为 Webhook 添加了代理支持和失败回退直连的机制,增强了可用性。
  • 对通知设置页面进行了彻底重构,新的 UI/UX 非常出色,组件化和状态管理都做得很棒。
  • 增加了完整的单元、集成和 E2E 测试,保证了代码质量。

整体代码质量很高,设计考虑周全。

我的审查发现了一个严重的安全漏洞(SSRF),通过 DNS 解析可以绕过现有的内部网络地址检查。我的评论将主要集中在这个问题上,因为它需要立即修复。

Comment on lines +28 to +78
function isInternalUrl(urlString: string): boolean {
try {
const url = new URL(urlString);
const hostname = url.hostname.toLowerCase();

if (hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1") {
return true;
}

const ipv4Match = hostname.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/);
if (ipv4Match) {
const [, a, b] = ipv4Match.map(Number);
if (a === 127) return true;
if (a === 10) return true;
if (a === 172 && b >= 16 && b <= 31) return true;
if (a === 192 && b === 168) return true;
if (a === 169 && b === 254) return true;
if (a === 0) return true;
}

const ipv6Hostname = hostname.replace(/^\[|\]$/g, "");
if (
ipv6Hostname.startsWith("::ffff:127.") ||
ipv6Hostname.startsWith("::ffff:10.") ||
ipv6Hostname.startsWith("::ffff:192.168.") ||
ipv6Hostname.startsWith("::ffff:0.")
) {
return true;
}
const ipv6MappedMatch = ipv6Hostname.match(/^::ffff:172\.(\d+)\./);
if (ipv6MappedMatch) {
const secondOctet = parseInt(ipv6MappedMatch[1], 10);
if (secondOctet >= 16 && secondOctet <= 31) return true;
}
if (ipv6Hostname.startsWith("fc") || ipv6Hostname.startsWith("fd")) {
return true;
}
if (ipv6Hostname.startsWith("fe80:")) {
return true;
}

const dangerousPorts = [22, 23, 3306, 5432, 27017, 6379, 11211];
if (url.port && dangerousPorts.includes(parseInt(url.port, 10))) {
return true;
}

return false;
} catch {
return true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

严重安全漏洞:可通过 DNS 解析绕过 SSRF 防护

当前的 isInternalUrl 函数在进行 SSRF 防护时,只检查了 URL 中的主机名字符串是否为私有 IP 地址,但没有对域名进行 DNS 解析并检查解析后的 IP 地址。

这会导致一个严重的安全漏洞:攻击者可以设置一个域名(例如 internal.service.attacker.com),并将其解析到内部网络地址(如 192.168.1.10)。当用户配置这个域名作为 Webhook URL 时,isInternalUrl 函数会因为域名本身不是 IP 地址而放行,但实际请求会发送到内部网络,从而绕过了 SSRF 防护。这可能导致攻击者能够扫描内部网络、访问未授权的内部服务。

建议修复方案:

为了修复此漏洞,需要在 isInternalUrl 函数中增加 DNS 解析步骤,并对解析出的所有 IP 地址进行内部地址检查。

  1. isInternalUrl 函数修改为异步函数。
  2. 在函数内部,使用 node:dns/promisesresolve 方法解析主机名。
  3. 遍历所有解析出的 IP 地址,并对每个 IP 地址执行现有的内部 IP 检查逻辑。

由于这会使函数变为异步,调用它的 normalizeTargetInputnormalizeTargetUpdateInput 函数也需要变为 async,并在调用处使用 await

以下是一个简化的实现思路:

import { promises as dns } from "node:dns";
import { logger } from "@/lib/logger";

// ...

// 将 IP 检查逻辑提取到一个辅助函数中
function isIpInternal(ip: string): boolean {
  // ... 将 isInternalUrl 中所有针对 IP 地址的检查逻辑移到这里 ...
  // 例如:
  if (ip === "127.0.0.1" || ip === "::1") return true;
  const ipv4Match = ip.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/);
  if (ipv4Match) {
    // ...
  }
  return false;
}

async function isInternalUrl(urlString: string): Promise<boolean> {
  try {
    const url = new URL(urlString);
    const hostname = url.hostname.toLowerCase();

    // 1. 直接检查主机名是否为 IP 地址
    if (isIpInternal(hostname)) {
      return true;
    }

    // 2. 对域名进行 DNS 解析
    try {
      const ips = await dns.resolve(hostname);
      for (const ip of ips) {
        if (isIpInternal(ip)) {
          // 发现解析到内部 IP,立即阻止
          logger.warn(`SSRF attempt blocked: ${hostname} resolved to internal IP ${ip}`);
          return true;
        }
      }
    } catch (e) {
      // DNS 解析失败,为安全起见,可以阻止或记录日志
      logger.warn(`DNS resolution failed for hostname: ${hostname}`, e);
      return true; // 安全默认:阻止无法解析的域名
    }

    // ... 端口检查等其他逻辑 ...
    const dangerousPorts = [22, 23, 3306, 5432, 27017, 6379, 11211];
    if (url.port && dangerousPorts.includes(parseInt(url.port, 10))) {
      return true;
    }

    return false;
  } catch {
    return true;
  }
}

请注意,这只是一个示例,实际实现时需要将 isInternalUrl 的调用链全部改为异步。这是一个关键的安全修复,建议优先处理。

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

After comprehensive analysis of this XL PR implementing the webhook notification system enhancement, no significant issues were identified that meet the review threshold for reporting.

PR Size: XL

  • Lines changed: 8,863 (8,193 additions + 670 deletions)
  • Files changed: 55

Split Recommendation for Future PRs: Given the XL size, consider splitting features like this into phases:

  1. Core architecture + database migration
  2. Backend renderers + template system
  3. Frontend UI components
  4. Integration tests + documentation

This allows for easier review and reduces risk of regression.

Issues Found

Category Critical High Medium Low
Logic/Bugs 0 0 0 0
Security 0 0 0 0
Error Handling 0 0 0 0
Types 0 0 0 0
Comments/Docs 0 0 0 0
Tests 0 0 0 0
Simplification 0 0 0 0

Review Coverage

  • Logic and correctness - Clean
  • Security (OWASP Top 10) - Clean
    • ✅ SSRF protection implemented (isInternalUrl() at src/actions/webhook-targets.ts:28-78)
    • ✅ Input validation with Zod schemas
    • ✅ DingTalk HMAC signature (src/lib/webhook/notifier.ts:99-113)
    • ✅ HTML/Markdown escaping in renderers
    • ✅ Proxy URL validation
  • Error handling - Clean
    • ✅ Proper error propagation in server actions
    • ✅ Retry logic with exponential backoff
    • ✅ Graceful proxy fallback
    • ✅ Test result persistence for debugging
  • Type safety - Clean
    • ✅ Comprehensive Zod validation
    • ✅ Type-safe provider detection
    • ✅ Proper nullable handling
  • Documentation accuracy - Clean
    • ✅ Clear SSRF comment explaining proxy exemption (line 26)
    • ✅ Template placeholder documentation
  • Test coverage - Adequate
    • ✅ Unit tests for renderers (dingtalk, telegram, custom)
    • ✅ Integration tests for CRUD operations
    • ✅ E2E tests for notification settings workflow
  • Code clarity - Good
    • ✅ Well-structured with clear separation of concerns
    • ✅ Consistent naming conventions
    • ✅ Proper abstraction layers (renderer/template/placeholder)

Notable Implementation Strengths

  1. Security-First Design: Comprehensive SSRF protection covering IPv4, IPv6, link-local addresses, and dangerous ports
  2. Backward Compatibility: Legacy mode flag ensures zero breaking changes
  3. Provider Extensibility: Clean renderer pattern makes adding new providers straightforward
  4. Error Resilience: Multi-layer retry + proxy fallback + proper error context logging
  5. Type Safety: Zod schemas with provider-specific conditional validation
  6. Testing: Complete coverage across unit/integration/e2e levels

Automated review by Claude AI

const { getBindingById } = await import("@/repository/notification-bindings");
const binding = await getBindingById(bindingId);
templateOverride = binding?.templateOverride ?? null;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] [LOGIC-BUG] Disabled/deleted bindings can still send scheduled notifications

src/lib/notification/notification-queue.ts:214

Why this is a problem: When a job has bindingId, the processor only uses it to load templateOverride. If the binding was deleted/disabled after the repeatable job was created (or if jobs weren’t rescheduled), binding becomes null but the job still sends to targetId. This makes “unbind/disable” ineffective and can leak notifications to an unintended webhook.

Problematic code:

if (bindingId) {
  const { getBindingById } = await import("@/repository/notification-bindings");
  const binding = await getBindingById(bindingId);
  templateOverride = binding?.templateOverride ?? null;
}

Suggested fix:

if (bindingId) {
  const { getBindingById } = await import("@/repository/notification-bindings");
  const binding = await getBindingById(bindingId);

  if (!binding || !binding.isEnabled || binding.targetId !== targetId) {
    logger.info({
      action: "notification_binding_missing_or_disabled",
      jobId: job.id,
      type,
      bindingId,
      targetId,
    });
    return { success: true, skipped: true };
  }

  templateOverride = binding.templateOverride ?? null;
}

const settings = await getNotificationSettings();
if (settings.useLegacyMode) {
await updateNotificationSettings({ useLegacyMode: false });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] [LOGIC-BUG] Auto-switching useLegacyMode without re-scheduling leaves Bull repeatable jobs in the wrong mode

src/actions/webhook-targets.ts:337

Why this is a problem: useLegacyMode affects how repeatable jobs are created in scheduleNotifications(), but here you update the DB via updateNotificationSettings() without invoking re-scheduling. That can leave existing repeatable jobs still posting to legacy webhookUrl destinations while other notification paths immediately start using bindings (or skipping when none exist), creating a mixed/incorrect notification state until the next restart or settings update.

Suggested fix (keep the dynamic import pattern to avoid bundling Bull):

if (settings.useLegacyMode) {
  await updateNotificationSettings({ useLegacyMode: false });

  if (process.env.NODE_ENV === "production") {
    const { scheduleNotifications } = await import("@/lib/notification/notification-queue");
    await scheduleNotifications();
  } else {
    logger.warn({
      action: "schedule_notifications_skipped",
      reason: "development_mode",
    });
  }
}

providerType: ProviderTypeSchema,

webhookUrl: z.string().trim().url("Webhook URL 格式不正确").optional().nullable(),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] [SECURITY-VULNERABILITY] webhookUrl accepts non-HTTP(S) schemes (stored invalid config; expands SSRF surface)

src/actions/webhook-targets.ts:105

Why this is a problem: z.string().url() accepts URLs like file:///... / ftp://... / ws://.... Those pass validation here, are not rejected by isInternalUrl(), and get persisted. Later fetch() only supports HTTP(S), so notifications/tests can fail at runtime with confusing errors. Allowing arbitrary schemes also unnecessarily expands the attack surface for SSRF-style abuse.

Problematic code:

webhookUrl: z.string().trim().url("Webhook URL 格式不正确").optional().nullable(),

Suggested fix (restrict protocol explicitly):

webhookUrl: z
  .string()
  .trim()
  .url("Webhook URL 格式不正确")
  .refine((value) => {
    const url = new URL(value);
    return url.protocol === "http:" || url.protocol === "https:";
  }, "Webhook URL 仅支持 http/https")
  .optional()
  .nullable(),

(Optionally also enforce the protocol in normalizeTargetInput()/normalizeTargetUpdateInput() as a defense-in-depth check.)

const validatedBindings = z.array(BindingInputSchema).parse(bindings);

await upsertBindings(validatedType, validatedBindings);
return { ok: true, data: undefined };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] [LOGIC-BUG] Binding updates don’t re-schedule repeatable jobs (new/disabled bindings won’t take effect)

src/actions/notification-bindings.ts:57

Why this is a problem: scheduleNotifications() creates per-binding repeatable jobs for daily_leaderboard / cost_alert. Updating bindings here only changes DB state; existing repeatable jobs remain stale until the next restart or an unrelated settings save. That means new bindings may never receive scheduled notifications, and removed/disabled bindings may keep firing jobs.

Suggested fix: re-run scheduling after binding changes for scheduled types (production-only, same dynamic import approach used elsewhere):

await upsertBindings(validatedType, validatedBindings);

if (
  process.env.NODE_ENV === "production" &&
  (validatedType === "daily_leaderboard" || validatedType === "cost_alert")
) {
  const { scheduleNotifications } = await import("@/lib/notification/notification-queue");
  await scheduleNotifications();
}

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This PR changes the notification system end-to-end (DB schema, scheduling, webhook rendering, and the settings UI). I found multiple correctness issues where scheduled notifications can keep running with stale configuration (or keep sending after disable/unbind), which is risky for both behavior and data leakage.

PR Size: XL

  • Lines changed: 8863
  • Files changed: 55
  • Split suggestion: Consider splitting into 4–5 PRs: (1) DB schema+migrations+repositories, (2) webhook core/renderers+proxy changes, (3) notification queue+scheduling behavior, (4) settings UI+i18n, (5) tests+Vitest configs.

Issues Found

Category Critical High Medium Low
Logic/Bugs 1 2 0 0
Security 0 1 0 0
Error Handling 0 0 0 0
Types 0 0 0 0
Comments/Docs 0 0 0 0
Tests 0 0 0 0
Simplification 0 0 0 0

Critical Issues (Must Fix)

  • [CRITICAL] [LOGIC-BUG] Disabled/deleted bindings can still send scheduled notifications (src/lib/notification/notification-queue.ts), Confidence: 98

High Priority Issues (Should Fix)

  • [HIGH] [LOGIC-BUG] Auto-switching useLegacyMode without re-scheduling leaves repeatable jobs in the wrong mode (src/actions/webhook-targets.ts), Confidence: 88
  • [HIGH] [LOGIC-BUG] Binding updates don’t re-schedule repeatable jobs (src/actions/notification-bindings.ts), Confidence: 88
  • [HIGH] [SECURITY-VULNERABILITY] webhookUrl accepts non-HTTP(S) schemes (src/actions/webhook-targets.ts), Confidence: 85

Review Coverage

  • Logic and correctness
  • Security (OWASP Top 10)
  • Error handling
  • Type safety
  • Documentation accuracy
  • Test coverage
  • Code clarity

Automated review by Codex AI

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Identified open PR #504 and applied the size/XL label.
  • Posted 4 inline review comments on new/changed lines focusing on high-impact issues:
    • src/lib/notification/notification-queue.ts (Critical): stale jobs can still send after a binding is deleted/disabled.
    • src/actions/webhook-targets.ts (High): auto-switching useLegacyMode without re-scheduling can leave repeatable jobs in the wrong mode.
    • src/actions/webhook-targets.ts (High): webhookUrl validation allows non-HTTP(S) schemes.
    • src/actions/notification-bindings.ts (High): binding updates don’t trigger re-scheduling for scheduled notifications.
  • Submitted the required summary review via gh pr review.

@ding113 ding113 closed this Jan 2, 2026
@github-project-automation github-project-automation bot moved this from Backlog to Done in Claude Code Hub Roadmap Jan 2, 2026
@ding113 ding113 deleted the feature/issue-485-webhook-notifications branch January 27, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core area:i18n area:UI enhancement New feature or request size/XL Extra Large PR (> 1000 lines)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant