fix(webhook): QA review criticals — shutdown drain, readiness token, pending-map leak, tests (#169)

* fix(webhook): address QA review criticals + minimal test coverage

Closes four findings from the post-merge review of PR #161:

C1 — in-flight drawer presses were abandoned on listen shutdown.
h.inFlight counter was incremented/decremented but never read, so
Ctrl-C dropped goroutines mid-press. Replaced with sync.WaitGroup +
shared pressCtx: shutdown now drains in-flight presses up to 30s,
then cancels the shared context so cooperative-cancel goroutines
unwind cleanly before runListen returns.

C3 — waitForReady over-trusted DNS. A stale DNS record pointing the
hostname at some unrelated origin that returns 2xx on /healthz would
pass a naive status-only check. Server now mints a per-process
random readiness token, embeds it in /healthz JSON, and waitForReady
parses + matches it before accepting the tunnel as ready. Closes the
loop through edge DNS → CF → local process rather than just testing
any leg of it. MintReadyToken() exposed for callers (cmd/serve.go)
that run their own HTTP mux.

I1 — pending map was an unbounded leak. Dropped entirely. Replaced
Wait() semantics with an explicit Register()/Deregister() API; Wait
remains as a convenience that handles both. Callers now Register
BEFORE publishing the external URL, eliminating the "event arrives
before Wait" race that pending was there to cover. webhook test
updated to match.

P1 — internal/webhook had no tests. Added:
- TestNewCorrelationID_Uniqueness (10k ids, no dups)
- TestServer_RegisterBeforeReceive (main flow)
- TestServer_WaitContextCancelled (no waiter leak)
- TestServer_HandleWebhook_RejectsSlashInID (path guard)
- TestServer_Healthz_TokenEmbedded (readiness contract)
- TestVerifyAuth for none/basic/header/jwt + env-ref + expired token

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(serve): defer cancelPresses to satisfy lostcancel vet check

Vet flagged a context-leak path: cancelPresses was assigned but not
guaranteed to fire if the readyToken mint failed before the later
shutdown call. Added `defer cancelPresses()` immediately after the
context is created so every return path closes it. Idempotent — the
explicit call in the shutdown drain still works.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: bobakemamian

cmd/serve.go

@@ -91,12 +91,34 @@ func runListen(cmd *cobra.Command, args []string) error {
 	}
 	localURL := fmt.Sprintf("http://%s", ln.Addr().String())
 
+	// Press context: shared across every in-flight drawer press so
+	// shutdown can cancel all of them at once (cooperative cancel
+	// lands way faster than waiting out the 1h press deadline).
+	// Defer the cancel so early-return paths don't leak the context —
+	// vet flags uncalled cancel functions as a leak risk.
+	pressCtx, cancelPresses := context.WithCancel(context.Background())
+	defer cancelPresses()
+
+	// Readiness token: the tunnel verifier pulls /healthz through the
+	// public URL and matches this token. Closes the "stale DNS
+	// pointing elsewhere returns 2xx" gap.
+	readyToken, err := webhook.MintReadyToken()
+	if err != nil {
+		return fmt.Errorf("mint ready token: %w", err)
+	}
+
 	mux := http.NewServeMux()
-	h := &serveHandler{dsvc: dsvc, routes: routes}
+	h := &serveHandler{
+		dsvc:      dsvc,
+		routes:    routes,
+		pressCtx:  pressCtx,
+		cancelAll: cancelPresses,
+	}
 	mux.Handle("/", h)
 	mux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusOK)
-		_, _ = w.Write([]byte(`{"ok":true}`))
+		_, _ = w.Write([]byte(`{"ok":true,"token":"` + readyToken + `"}`))
 	})
 
 	srv := &http.Server{
@@ -119,7 +141,7 @@ func runListen(cmd *cobra.Command, args []string) error {
 			return handleWebhookErr(err)
 		}
 		tunnelCtx, tunnelCancel := context.WithTimeout(ctx, 90*time.Second)
-		tunnel, err = webhook.StartTunnel(tunnelCtx, localURL)
+		tunnel, err = webhook.StartTunnel(tunnelCtx, localURL, readyToken)
 		tunnelCancel()
 		if err != nil {
 			_ = srv.Shutdown(context.Background())
@@ -146,9 +168,40 @@ func runListen(cmd *cobra.Command, args []string) error {
 	case <-ctx.Done():
 	}
 
+	// Orderly shutdown:
+	//
+	//   1. Stop accepting new connections (srv.Shutdown) — 5s grace.
+	//   2. Wait for in-flight presses up to drainTimeout. Shorter than
+	//      the per-press cap on purpose: operators want Ctrl-C to
+	//      feel responsive, and steps that haven't already observed
+	//      cancellation probably won't.
+	//   3. Cancel every in-flight press (cancelPresses) so any
+	//      remaining goroutines unwind via context.Done and we don't
+	//      leave zombies after runListen returns.
+	const drainTimeout = 30 * time.Second
+
 	shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer shutdownCancel()
 	_ = srv.Shutdown(shutdownCtx)
+
+	drained := make(chan struct{})
+	go func() {
+		h.wg.Wait()
+		close(drained)
+	}()
+	select {
+	case <-drained:
+		// All presses finished cleanly.
+	case <-time.After(drainTimeout):
+		fmt.Fprintln(os.Stderr, "draining in-flight presses timed out; cancelling remaining work")
+	}
+	cancelPresses()
+	// Short final wait so cancel-triggered goroutines can clean up
+	// before runListen's caller returns (history flush, etc.).
+	select {
+	case <-drained:
+	case <-time.After(2 * time.Second):
+	}
 	return nil
 }
 
@@ -236,14 +289,18 @@ func printServeBanner(publicBase string, routes []route, tunnel *webhook.Tunnel)
 }
 
 // serveHandler dispatches incoming POSTs to the drawer registered for
-// the request path. Holds a sync.Mutex around the in-flight count so a
-// graceful shutdown can wait for drawer presses rather than dropping
-// them mid-run.
+// the request path. A WaitGroup tracks in-flight drawer presses so
+// graceful shutdown can actually wait for them to finish instead of
+// abandoning goroutines the moment the HTTP listener closes. The
+// press context is derived from pressCtx so Ctrl-C propagates
+// cooperatively — drawer steps honoring ctx cancellation return
+// promptly rather than hanging until the 1h press deadline.
 type serveHandler struct {
-	dsvc    *drawer.Service
-	routes  []route
-	mu      sync.Mutex
-	inFlight int
+	dsvc      *drawer.Service
+	routes    []route
+	wg        sync.WaitGroup
+	pressCtx  context.Context
+	cancelAll context.CancelFunc
 }
 
 func (h *serveHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
@@ -353,19 +410,17 @@ func (h *serveHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// Press asynchronously: respond to the webhook sender right away
 	// with a run id. Most services retry aggressively on slow webhook
 	// responses, and a long-running drawer would block them needlessly.
-	h.mu.Lock()
-	h.inFlight++
-	h.mu.Unlock()
+	//
+	// WaitGroup lets runListen's shutdown path drain in-flight presses
+	// instead of abandoning them.
+	h.wg.Add(1)
 
 	go func() {
-		defer func() {
-			h.mu.Lock()
-			h.inFlight--
-			h.mu.Unlock()
-		}()
-		// Fresh context — not tied to the request (which is about to
-		// close). 1h cap so a hung drawer can't leak forever.
-		ctx, cancel := context.WithTimeout(context.Background(), time.Hour)
+		defer h.wg.Done()
+		// Derive from h.pressCtx (cancelled on shutdown) with a 1h
+		// cap so a wedged drawer can't leak forever. Steps that honor
+		// ctx observe cancellation and return cleanly.
+		ctx, cancel := context.WithTimeout(h.pressCtx, time.Hour)
 		defer cancel()
 
 		exec := drawer.NewExecutor()

cmd/webhook.go

@@ -253,7 +253,7 @@ func runWebhookTest(cmd *cobra.Command, args []string) error {
 	if !jsonOutput {
 		fmt.Fprintf(os.Stderr, "  Starting tunnel (this takes ~15s)…\n")
 	}
-	t, err := webhook.StartTunnel(ctx, srv.LocalURL())
+	t, err := webhook.StartTunnel(ctx, srv.LocalURL(), srv.ReadyToken())
 	if err != nil {
 		return handleWebhookErr(err)
 	}
@@ -263,24 +263,36 @@ func runWebhookTest(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return handleWebhookErr(err)
 	}
+	// Register the waiter BEFORE publishing the URL (spawning the
+	// POST goroutine). Eliminates the "event arrives before Wait
+	// registers" race — no pending-map needed server-side.
+	waiterCh := srv.Register(corr)
+	defer srv.Deregister(corr)
+
 	postURL := fmt.Sprintf("%s/webhook/%s", t.URL, corr)
 
 	if !jsonOutput {
 		fmt.Fprintf(os.Stderr, "  Tunnel up: %s\n", t.URL)
 		fmt.Fprintf(os.Stderr, "  POSTing to %s …\n", postURL)
 	}
 
-	// Fire the test POST in the background; cloudflared sometimes
-	// needs a beat before the first edge request routes, so we retry
-	// with a short backoff.
+	// Fire the test POST in the background. Tunnel readiness is
+	// already verified by StartTunnel's /healthz + token check.
 	delivery := make(chan error, 1)
 	go func() {
 		delivery <- selfPost(ctx, postURL)
 	}()
 
 	waitCtx, waitCancel := context.WithTimeout(ctx, 60*time.Second)
 	defer waitCancel()
-	ev, waitErr := srv.Wait(waitCtx, corr)
+
+	var ev webhook.Event
+	var waitErr error
+	select {
+	case ev = <-waiterCh:
+	case <-waitCtx.Done():
+		waitErr = waitCtx.Err()
+	}
 	if waitErr != nil {
 		postErr := <-delivery
 		return handleWebhookErr(fmt.Errorf("no webhook received within 60s (post err: %v, wait err: %w)", postErr, waitErr))

internal/webhook/auth_test.go

@@ -0,0 +1,167 @@
+package webhook
+
+import (
+	"crypto/hmac"
+	"crypto/sha256"
+	"encoding/base64"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+)
+
+// Guards the four n8n-style auth paths end-to-end. Each test uses a
+// plain httptest request (no tunnel/server) and feeds directly into
+// VerifyAuth so these stay fast and deterministic.
+
+func TestVerifyAuth_NoneAndNil(t *testing.T) {
+	req, _ := http.NewRequest(http.MethodPost, "/x", nil)
+	for _, cfg := range []*TriggerAuthConfig{nil, {Type: ""}, {Type: "none"}} {
+		if res := VerifyAuth(cfg, req); !res.OK {
+			t.Errorf("nil/empty/none cfg should pass, got %+v", res)
+		}
+	}
+}
+
+func TestVerifyAuth_BasicHappyPath(t *testing.T) {
+	cfg := &TriggerAuthConfig{Type: "basic", Username: "u", Password: "p"}
+	req, _ := http.NewRequest(http.MethodPost, "/x", nil)
+	req.SetBasicAuth("u", "p")
+	if res := VerifyAuth(cfg, req); !res.OK {
+		t.Fatalf("basic should pass with matching creds: %+v", res)
+	}
+}
+
+func TestVerifyAuth_BasicRejects(t *testing.T) {
+	cfg := &TriggerAuthConfig{Type: "basic", Username: "u", Password: "p"}
+	cases := []struct {
+		name      string
+		apply     func(*http.Request)
+		wantCode  string
+		wantStatus int
+	}{
+		{"missing", func(r *http.Request) {}, "AUTH_MISSING", http.StatusUnauthorized},
+		{"wrong_user", func(r *http.Request) { r.SetBasicAuth("x", "p") }, "AUTH_INVALID", http.StatusUnauthorized},
+		{"wrong_pass", func(r *http.Request) { r.SetBasicAuth("u", "x") }, "AUTH_INVALID", http.StatusUnauthorized},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			req, _ := http.NewRequest(http.MethodPost, "/x", nil)
+			tc.apply(req)
+			res := VerifyAuth(cfg, req)
+			if res.OK {
+				t.Fatalf("should have failed")
+			}
+			if res.Code != tc.wantCode || res.Status != tc.wantStatus {
+				t.Errorf("got %s/%d, want %s/%d", res.Code, res.Status, tc.wantCode, tc.wantStatus)
+			}
+		})
+	}
+}
+
+func TestVerifyAuth_HeaderHappyAndMismatch(t *testing.T) {
+	cfg := &TriggerAuthConfig{Type: "header", HeaderName: "X-Token", HeaderValue: "s3cret"}
+	req, _ := http.NewRequest(http.MethodPost, "/x", nil)
+	req.Header.Set("X-Token", "s3cret")
+	if res := VerifyAuth(cfg, req); !res.OK {
+		t.Fatalf("should pass with matching header: %+v", res)
+	}
+	req2, _ := http.NewRequest(http.MethodPost, "/x", nil)
+	req2.Header.Set("X-Token", "wrong")
+	if res := VerifyAuth(cfg, req2); res.OK {
+		t.Fatal("should reject on mismatch")
+	}
+	req3, _ := http.NewRequest(http.MethodPost, "/x", nil)
+	// no header at all
+	res := VerifyAuth(cfg, req3)
+	if res.Code != "AUTH_MISSING" {
+		t.Errorf("expected AUTH_MISSING on header absent, got %s", res.Code)
+	}
+}
+
+func TestVerifyAuth_EnvRefResolution(t *testing.T) {
+	t.Setenv("BUTTONS_TEST_SECRET", "env-val")
+	cfg := &TriggerAuthConfig{Type: "header", HeaderName: "X-Token", HeaderValue: "$ENV{BUTTONS_TEST_SECRET}"}
+	req, _ := http.NewRequest(http.MethodPost, "/x", nil)
+	req.Header.Set("X-Token", "env-val")
+	if res := VerifyAuth(cfg, req); !res.OK {
+		t.Fatalf("env-backed header should match its resolved value, got %+v", res)
+	}
+}
+
+// TestVerifyAuth_JWT runs through a full HS256 signing flow against
+// our verifier using only stdlib primitives so the test doesn't
+// depend on a JWT library the production code deliberately avoids.
+func TestVerifyAuth_JWT(t *testing.T) {
+	secret := "test-secret"
+	token := signHS256Token(t, secret, map[string]any{
+		"iss": "test-issuer",
+		"aud": "test-aud",
+		"exp": time.Now().Add(1 * time.Hour).Unix(),
+	})
+
+	cfg := &TriggerAuthConfig{Type: "jwt", JWTSecret: secret, JWTIssuer: "test-issuer", JWTAudience: "test-aud"}
+	req, _ := http.NewRequest(http.MethodPost, "/x", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	if res := VerifyAuth(cfg, req); !res.OK {
+		t.Fatalf("valid JWT should pass: %+v", res)
+	}
+
+	t.Run("wrong_issuer", func(t *testing.T) {
+		cfg2 := &TriggerAuthConfig{Type: "jwt", JWTSecret: secret, JWTIssuer: "other"}
+		if res := VerifyAuth(cfg2, req); res.OK {
+			t.Fatal("should reject issuer mismatch")
+		}
+	})
+	t.Run("bad_signature", func(t *testing.T) {
+		cfg2 := &TriggerAuthConfig{Type: "jwt", JWTSecret: "different-secret"}
+		if res := VerifyAuth(cfg2, req); res.OK {
+			t.Fatal("should reject signature mismatch")
+		}
+	})
+	t.Run("expired", func(t *testing.T) {
+		expired := signHS256Token(t, secret, map[string]any{
+			"exp": time.Now().Add(-2 * time.Hour).Unix(),
+		})
+		req2, _ := http.NewRequest(http.MethodPost, "/x", nil)
+		req2.Header.Set("Authorization", "Bearer "+expired)
+		cfg2 := &TriggerAuthConfig{Type: "jwt", JWTSecret: secret}
+		res := VerifyAuth(cfg2, req2)
+		if res.OK {
+			t.Fatal("should reject expired")
+		}
+		if !strings.Contains(res.Detail, "expired") {
+			t.Errorf("expected 'expired' in detail, got %q", res.Detail)
+		}
+	})
+	t.Run("missing_bearer", func(t *testing.T) {
+		req2, _ := http.NewRequest(http.MethodPost, "/x", nil)
+		cfg2 := &TriggerAuthConfig{Type: "jwt", JWTSecret: secret}
+		if res := VerifyAuth(cfg2, req2); res.Code != "AUTH_MISSING" {
+			t.Errorf("expected AUTH_MISSING, got %s", res.Code)
+		}
+	})
+}
+
+// signHS256Token builds a minimal JWT using only stdlib so our tests
+// don't rely on golang-jwt/jwt — that's a deliberate avoidance in
+// production code we don't want to undo just for tests.
+func signHS256Token(t *testing.T, secret string, claims map[string]any) string {
+	t.Helper()
+	header := map[string]any{"alg": "HS256", "typ": "JWT"}
+	hb, _ := json.Marshal(header)
+	cb, _ := json.Marshal(claims)
+	h := base64.RawURLEncoding.EncodeToString(hb)
+	c := base64.RawURLEncoding.EncodeToString(cb)
+	signingInput := h + "." + c
+	mac := hmac.New(sha256.New, []byte(secret))
+	mac.Write([]byte(signingInput))
+	sig := base64.RawURLEncoding.EncodeToString(mac.Sum(nil))
+	return signingInput + "." + sig
+}
+
+// ensure httptest import used — avoids unused-import break under
+// future refactors that might trim this file.
+var _ = httptest.NewServer