tiennm99/goclaw
1
0 Fork 0
mirror of https://github.com/tiennm99/goclaw.git synced 2026-10-13 04:14:56 +00:00
Code Issues Packages Projects Releases Wiki Activity
Files
dev
goclaw/docs/journals/webhook-agent-triggering-260421-shipped.md
T
Duy /zuey/ ddf8e1099f feat(webhooks): HTTP webhooks to trigger agents with HMAC auth + durable callbacks (#2) 
* feat(webhooks): HTTP webhooks to trigger agents with HMAC auth and durable callbacks

Add multi-tenant HTTP webhook endpoints for agent triggering:
- /v1/webhooks/message: send messages to channels
- /v1/webhooks/llm: sync/async LLM prompts with HMAC-signed callbacks
- HMAC-256 + bearer token authentication
- Rate limiting and tenant isolation
- Durable callback worker with exponential backoff
- PG 000056 + SQLite schema v25 migrations
- Unit + integration tests, P0 tenant isolation invariants
- Channel media capability helpers for attachment routing
- Comprehensive webhook documentation and i18n strings

* fix(webhooks): address post-review findings (K1-K10)

Comprehensive post-merge fixes addressing 10 blocking code review issues
and 2 adversarial re-audit findings in webhook-agent-triggering feature:

K1: Fix auth middleware tenant context lookup sequencing — move
    tenant context injection before authenticate() call to prevent
    unscoped secret lookups.

K2: Canonicalize JSON payload format for jsonb compatibility across
    PostgreSQL and SQLite — ensure consistent serialization without
    whitespace variance to prevent hash mismatches.

K3: Add fail-closed JSON parsing in body hash extraction with explicit
    error handling for malformed payloads before HMAC verification.

K4: Fix worker queue wedge by properly draining slot reservations
    when delivery succeeds, preventing permanent slot occupancy.

K5: Implement lease-token optimistic concurrency control to prevent
    duplicate webhook delivery under high concurrency or retry storms.

K6: Add AES-256-GCM encrypted secret storage at rest with fail-fast
    skip-mount when GOCLAW_ENCRYPTION_KEY environment variable unset.

K7: Implement IP allowlist enforcement supporting both CIDR ranges
    and exact IP matching with proper X-Forwarded-For parsing.

K8: Add HMAC replay nonce cache (5min expiry, non-blocking async flush)
    to prevent request replay attacks on webhook handler.

K9: Fix invariant test schema selection — replace hardcoded assumption
    with explicit schema name from config to support multi-schema testing.

K10: Consolidate rate limiters into single shared instance to prevent
     per-endpoint limiter starvation and ensure fair rate limiting.

New database migrations:
- 000057: webhook_calls.lease_token for optimistic concurrency
- 000058: webhooks.encrypted_secret_key for AES-256-GCM encryption

New i18n keys: MsgWebhookIPDenied, MsgWebhookEncryptionUnavailable
(with English, Vietnamese, Chinese translations).

New modules:
- internal/http/webhooks_payload.go: JSON canonicalization + body hash
- internal/http/webhooks_nonce.go: Replay nonce cache implementation
- internal/http/webhooks_idempotency_test.go: Integration tests

Documentation updates:
- docs/webhooks.md: §13-14 security sections, encryption flow
- docs/00-architecture-overview.md: webhook subsystem security overview
- docs/codebase-summary.md: webhook security patterns
- docs/project-changelog.md: webhook fixes changelog

Test coverage: 53 webhook tests + 4 P0 invariant tests all passing.
No tenant isolation violations. All security gates enforced.

* docs(journals): webhook feature ship + fix cycle entries

* fix(webhooks): address Claude review findings

- webhooks_llm.go: remove misleading ptr() helper; use &completedAt
  pattern for error-path audit rows (matches success path)
- webhooks_auth.go: wrap TouchLastUsed context in WithoutCancel so
  background DB update isn't cancelled when HTTP response completes
- store GetByIDUnscoped (PG+SQLite): add NOT revoked / revoked = 0
  filter for defense-in-depth parity with GetByHashUnscoped
- webhooks/sign.go: fix package doc — HMAC key is raw plaintext
  secret bytes, not hex-decoded SHA-256
- webhooks_admin.go: check auth before encKey guard to avoid leaking
  config state to unauthenticated callers
- webhooks_ratelimit.go: two-phase Load→LoadOrStore to avoid per-call
  entry allocation on the hot path

* docs(webhooks): fix Sign() function doc to match actual key input

Function-level comment still referenced hex-decoded SecretHash after
the package-level doc was corrected. Align with actual caller usage
([]byte(rawSecret)).

* fix(webhooks): use WithoutCancel for worker execute DB updates

Terminal status writes in execute() ran through the worker main-loop
ctx, which is cancelled on graceful shutdown. If the outbound send
completed but the status update raced with shutdown, the row stayed
in 'running' and got re-delivered via reclaimStale. WithoutCancel
lets the DB write survive worker cancellation while preserving
propagated values (tenant ID, etc.).

* fix(webhooks): move tctx init before panic defer in worker execute

Panic recovery called updateRetry with raw ctx (no tenant ID), making
requireTenantID fail and the reset-to-retry DB write silently drop.
Row stayed 'running' until reclaimStale (~90s delay). Init tctx first
so defer closure captures tenant-scoped non-cancellable context.

* fix(webhooks): pass tenant-scoped tctx to invokeAgent in worker

execute() was passing the raw worker-loop ctx (no tenant ID) to
invokeAgent → router.Get → PGAgentStore.GetByID. GetByID reads
TenantIDFromContext which returned uuid.Nil, making every lookup
return 'agent not found'. Async LLM webhook calls silently failed
all retries. Pass tctx (already tenant-scoped + WithoutCancel) so
the router resolves the agent correctly.

* fix(tests): resolve integration test compile errors

- Remove duplicate contains() in mcp_grant_revoke_test.go (already
  defined in tts_gemini_live_test.go)
- Update webhooks_admin_test.go RotateSecret call to match current
  5-arg signature (newSecretHash, newPrefix, newEncryptedSecret)

* fix(webhooks): default nil scopes/ip_allowlist to empty slice in Create

PG columns are NOT NULL DEFAULT '{}'. Explicit NULL from pqStringArray(nil)
violated the constraint, breaking TestWebhookAdminCRUD/TenantIsolation.
Coerce nil slices to empty []string{} so the default applies at the DB layer.

* chore: trigger CI on digitopvn/goclaw fork

* ci: retrigger workflows

* fix(webhooks): renumber migrations to 000059-000061 for merge train
2026-05-11 13:29:24 +07:00

7.0 KiB
Raw Permalink Blame History

Webhook Agent Triggering — Ship Complete

Date: 2026-04-21 23:59 Severity: Medium Component: HTTP webhooks (inbound) + callback worker (outbound) Status: Resolved

What Happened

Shipped HTTP webhook API (POST /v1/webhooks/message + /v1/webhooks/llm) with callback delivery. Feature enables external systems to trigger agents synchronously or asynchronously, with outbound result delivery to a caller-specified callback URL. Dual-database (PostgreSQL Standard + SQLite Lite). 48 files, 9376 insertions. Nine sequential phases. Branch: feat/webhook-agent-triggering, commit 19e0c679.

The Brutal Truth

Red-team review found the plan was unexecutable as written. Two fabricated API methods (Router.Invoke, Manager.SendToChannel media overload), three wrong file anchors, and four unspecified design decisions (media dispatch scope, callback idempotency, tenant concurrency, i18n ordering) meant that handing this to a teammate would have burned 4+ hours on false starts. After rework (2 hours of planner fixes), the plan was sound and execution was linear. The lesson: "trust-but-verify between planner and live code" is not optional — it catches real bugs before implementation wastes cycles.

Technical Details

Shipped contracts

  • POST /v1/webhooks/message: Send text + media to channel. HMAC-SHA256 auth (X-GoClaw-Signature t=,v1=) + bearer token. Rate limit: per-webhook bucket (token refill 10/sec) + per-tenant global bucket (100/sec). Returns {webhook_id, call_id} immediately.
  • POST /v1/webhooks/llm: Sync (wait for response, 30s timeout) or async (return call_id, deliver result to callback_url). Request body capped 1 MB; metadata capped 8 KB. HMAC + tenant-admin auth gate.
  • Callback delivery: exponential backoff [30s, 2m, 10m, 1h, 6h] ±10% jitter, 5 attempts max. Outbound headers carry X-Webhook-Delivery-Id (stable across retries) for receiver-side dedupe. Claim uses FOR UPDATE SKIP LOCKED (PG) / BEGIN IMMEDIATE (SQLite).

Critical decisions

  1. Callback idempotency: delivery_id UUID on webhook_calls.delivery_id stays constant across retries. attempts counter incremented AFTER send completion (not before), so crash-restart never creates duplicates — receiver sees same delivery_id on retry. This invariant required reversing initial design ("increment on claim").

  2. Media dispatch: Phase 05a added channels.SendMediaToChannel() because reused SendToChannel(content string) couldn't carry attachments. Grep found 8 adapters (telegram, discord, whatsapp, feishu, slack, zalo, pancake, facebook) already support bus.OutboundMessage.Media — not a new pattern. Phase 05b gates /message on channels.IsMediaCapable(type) with 501 fallback if unsupported.

  3. Tenant concurrency: Per-tenant semaphore (sync.Map keyed by tenant_id → *semaphore.Weighted) with 5-minute TTL eviction. Prevents single tenant's callbacks from starving others. Non-blocking TryAcquire leaves row unclaimed on failure (no DB busy-loop); next 2s poll retries naturally.

  4. i18n front-loading: All 19 keys × 3 catalogs (en/vi/zh) added upfront in phase 03, before any handler code. Prevents late-discovery "key not found" crashes. Phase 08 verifies the front-load.

What We Tried

  1. Initial plan: Router.Invoke entry point doesn't exist. Real pattern is Router.Get(ctx, agentID) → Agent.Run(ctx, RunRequest), verified at internal/agent/router.go:93 + internal/agent/types.go:18.
  2. Media dispatch design: Planner assumed Manager.SendToChannel could carry attachments. Grep audit found it only took content string. Rework added dedicated SendMediaToChannel(ctx, channelName, chatID, content, []bus.MediaAttachment) method.
  3. Auth helpers location: Plan cited internal/http/auth.go which doesn't define requireTenantAdmin or requireMasterScope. Grep found them at internal/http/tenant_auth_helpers.go:22,71.
  4. Edition gating: Plan referenced nonexistent edition.Current().Standard and .HasChannels() methods. Rework added AllowsChannels() helper at internal/edition/edition.go.

Root Cause Analysis

Why the plan failed initial audit: Planner reused API names from pattern prose without grepping live code. "Reuse Router.Invoke" sounded plausible for an entry point; the actual pattern is two-step (Get + Run). "Manager.SendToChannel carries media" was inferred from method naming, not from examining the struct definition. Edition gating was copy-paste from an older codebase pattern that didn't exist here.

Why we caught it: Red-team enforced CLAUDE Plan Verification Rule #3 ("no fabricated identifiers") and Rule #1 ("verify factual claims against code"). Spot-checks of 15+ claims against grep/line references surfaced every fabrication before implementation.

Why rework was surgical, not rewrite: The architecture (phases, concurrency model, auth gates) was sound. Only the API anchors and medium-sized design decisions needed fixing. Fixes were: (1) cite real entry points, (2) add one new channel method, (3) fix three file paths, (4) resolve four design questions. Execution then followed the reworked plan linearly, no surprises.

Lessons Learned

  1. Trust-but-verify is load-bearing. When a planner says "reuse X", don't delegate without a grep audit. Plausible-sounding APIs are the easiest to hallucinate. A 2-hour red-team pass caught what would have been 8+ hours of teammate confusion and rework.

  2. Crash-restart safety via immutable idempotency tokens is non-negotiable for async work. Original design incremented attempts on claim; rework deferred it to post-send. This single decision eliminates the entire class of duplicate-delivery bugs on worker restart.

  3. Tenant isolation primitives (semaphores, TTL eviction, non-blocking acquire) scale better than ad-hoc limits. Per-tenant semaphore with idle eviction is more complex than a simple global cap, but prevents the single-tenant-starves-others DoS and works at arbitrary scale.

  4. i18n keys as a blocker step, not a chore. Front-loading all keys before handler code prevents runtime "key not found" crashes and makes phase dependencies explicit. Ordering matters more than scope.

  5. Anchoring API references is mechanical, not intuitive. The plan correctly described what needed to be done (webhook auth, callback delivery, rate limiting) but cited wrong files/methods. Grep-by-symbol before writing. "Reuse X" must cite file:line and include a short signature snippet.

Next Steps

  1. Merge branch feat/webhook-agent-triggering → dev when CI green (currently in progress).
  2. Monitor webhook_calls table cardinality and callback latency in first week post-deploy. Alert if p50 delivery time > 1 min (indicates tenant sem contention or stale reclaim pile-up).
  3. v2 scope (deferred): /v1/webhooks/task (trigger workflows with task metadata), admin UI (web + desktop), callback secret rotation with grace window, observability dashboard for webhook metrics.
  4. Document webhook integration pattern in docs/webhooks.md + provide client library examples (curl, Python, Go) for external systems.
Reference in new issue View Git Blame Copy Permalink