From 85045016f6c53fb47433c42d3e9bb749fc16b135 Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Aug 2026 13:13:28 +0200 Subject: [PATCH 1/8] fix(webhook): harden Teams and WhatsApp Cloud adapters Two gaps surfaced by the security audit: - Add the pre-auth read-concurrency semaphore (readSem, cap 16) to the Teams and WhatsApp Cloud webhook handlers, matching the GitHub and GitLab siblings. It bounds peak pre-auth read memory and sheds a flood of large POSTs with 503 before the body is buffered, closing the copied-scaffolding drift between the four webhook adapters. - PathEscape the wire-derived media ID before interpolating it into the Graph API URL in the Cloud ResolveAttachmentURL path (defense-in-depth; it cannot escape the pinned graph.facebook.com host regardless), mirroring the existing url.PathEscape on the Teams send path. --- internal/teams/server.go | 18 +++++++++++ internal/teams/server_test.go | 17 +++++++++++ internal/teams/teams.go | 14 ++++++--- internal/whatsapp/cloud/cloud.go | 32 +++++++++++++++++-- internal/whatsapp/cloud/cloud_test.go | 44 ++++++++++++++++++++++++++- 5 files changed, 117 insertions(+), 8 deletions(-) diff --git a/internal/teams/server.go b/internal/teams/server.go index 5e00432..6231e84 100644 --- a/internal/teams/server.go +++ b/internal/teams/server.go @@ -30,6 +30,10 @@ const ( // inbound Activities cannot spawn unbounded work. The handler acquires a slot // before acking and sheds load with 503 (the platform retries) when full. maxConcurrentDispatch = 256 + // maxConcurrentReads bounds concurrent inbound request processing (the body + // read + JSON unmarshal before JWT verification), capping peak pre-auth read + // memory at maxConcurrentReads times maxRequestBytes. Mirrors github/gitlab. + maxConcurrentReads = 16 ) // conversation holds what a reply needs: the serviceUrl to POST to and the bot's @@ -103,6 +107,20 @@ func serve(srv *http.Server, ln net.Listener, done func(error)) { } func (a *adapter) handleMessages(dispatchCtx context.Context, w http.ResponseWriter, r *http.Request, deps core.AdapterDeps) { + // Bound concurrent inbound reads before buffering + unmarshaling the body: a + // flood of large bodies is shed with 503 (the platform retries) rather than + // buffering unbounded memory. readSem is allocated once in newAdapter, so this + // direct read races nothing. Separate from dispatchSem, which bounds post-ack + // dispatch. + select { + case a.readSem <- struct{}{}: + default: + a.log().Warn("teams: inbound concurrency limit reached; shedding with 503", "limit", maxConcurrentReads) + w.WriteHeader(http.StatusServiceUnavailable) + return + } + defer func() { <-a.readSem }() + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxRequestBytes)) if err != nil { w.WriteHeader(http.StatusBadRequest) diff --git a/internal/teams/server_test.go b/internal/teams/server_test.go index 35e4eeb..40c8062 100644 --- a/internal/teams/server_test.go +++ b/internal/teams/server_test.go @@ -269,6 +269,23 @@ func TestHandleMessages_SaturatedDispatchReturns503(t *testing.T) { asserts.Equal(t, w2.Code, http.StatusServiceUnavailable, "saturated dispatch sheds load with 503") } +// A saturated read semaphore must answer 503 before the body is buffered and +// unmarshaled, so a flood of large POSTs cannot exhaust memory ahead of the JWT +// check. Mirrors the github/gitlab sibling test. +func TestHandleMessages_ReadSaturationReturns503(t *testing.T) { + a := testAdapter(t) + a.readSem = make(chan struct{}, 1) + a.readSem <- struct{}{} // occupy the only inbound slot + + var got []*core.Message + body := activityJSON("message", "hi", allowedServiceURL, "user-1", "bot-1", "conv-1") + auth := "Bearer " + mintToken(t, testKID, validClaims(a.cfg.AppID, allowedServiceURL)) + w := post(a, captureDeps(&got, nil), body, auth) + + asserts.Equal(t, w.Code, http.StatusServiceUnavailable, "saturated read gate returns 503") + asserts.Equal(t, len(got), 0, "nothing dispatched") +} + func TestHandleMessages_IgnoresNonMessage(t *testing.T) { a := testAdapter(t) var got []*core.Message diff --git a/internal/teams/teams.go b/internal/teams/teams.go index e55c6b2..688fb4d 100644 --- a/internal/teams/teams.go +++ b/internal/teams/teams.go @@ -88,10 +88,15 @@ type adapter struct { // captured at acquire, so a slot a context-ignoring handler never releases is // confined to that connection rather than leaking for the adapter's lifetime. dispatchSem chan struct{} - convs map[string]conversation // conversationID -> reply routing info - convOrder []string // FIFO insertion order for bounded eviction - token cachedToken - keys map[string]*jwksKey // kid -> signing key + channel endorsements + // readSem bounds concurrent inbound request processing so a flood of large + // bodies can't buffer unbounded memory before JWT verification (see + // maxConcurrentReads); separate from dispatchSem, which bounds dispatch + // goroutines. Allocated once in newAdapter and never reassigned. + readSem chan struct{} + convs map[string]conversation // conversationID -> reply routing info + convOrder []string // FIFO insertion order for bounded eviction + token cachedToken + keys map[string]*jwksKey // kid -> signing key + channel endorsements // keysAt is the last JWKS fetch attempt (rate-limits refreshes); keysFreshAt // is the last successful refresh (drives the jwksMaxAge staleness gate). keysAt time.Time @@ -157,5 +162,6 @@ func newAdapter(cfg Config) (*adapter, error) { tokenURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/token", openIDURL: openIDConfigURL, dispatchSem: make(chan struct{}, maxConcurrentDispatch), + readSem: make(chan struct{}, maxConcurrentReads), }, nil } diff --git a/internal/whatsapp/cloud/cloud.go b/internal/whatsapp/cloud/cloud.go index 58292f3..d6340ae 100644 --- a/internal/whatsapp/cloud/cloud.go +++ b/internal/whatsapp/cloud/cloud.go @@ -26,6 +26,7 @@ import ( "log/slog" "net" "net/http" + "net/url" "strconv" "strings" "sync" @@ -52,6 +53,11 @@ const ( // goroutines. maxConcurrentDispatch = 256 + // maxConcurrentReads bounds concurrent inbound request processing (the body + // read before signature verification), capping peak pre-auth read memory at + // maxConcurrentReads times maxRequestBytes. Mirrors the github/gitlab siblings. + maxConcurrentReads = 16 + // maxErrorBodyBytes caps how much of a non-2xx response body is read into errors. maxErrorBodyBytes = 4 << 10 // 4 KiB @@ -148,6 +154,11 @@ type adapter struct { // handler never releases is confined to that connection rather than leaking for // the adapter's lifetime. dispatchSem chan struct{} + // readSem bounds concurrent inbound request processing so a flood of large + // bodies can't buffer unbounded memory before signature verification (see + // maxConcurrentReads); separate from dispatchSem, which bounds dispatch + // goroutines. Allocated once in newAdapter and never reassigned. + readSem chan struct{} } // log returns the Bot's logger handed over at Connect, or slog.Default() @@ -206,7 +217,7 @@ func newAdapter(cfg Config) (*adapter, error) { if cfg.HTTPClient == nil { cfg.HTTPClient = &http.Client{Timeout: 30 * time.Second} } - return &adapter{cfg: cfg, baseURL: graphBaseURL, http: cfg.HTTPClient, dispatchSem: make(chan struct{}, maxConcurrentDispatch)}, nil + return &adapter{cfg: cfg, baseURL: graphBaseURL, http: cfg.HTTPClient, dispatchSem: make(chan struct{}, maxConcurrentDispatch), readSem: make(chan struct{}, maxConcurrentReads)}, nil } func (a *adapter) Connect(ctx context.Context, deps core.AdapterDeps) error { @@ -288,6 +299,19 @@ func (a *adapter) handleVerify(w http.ResponseWriter, r *http.Request) { } func (a *adapter) handleWebhook(dispatchCtx context.Context, w http.ResponseWriter, r *http.Request, deps core.AdapterDeps) { + // Bound concurrent inbound reads before buffering the body: a flood of large + // bodies is shed with 503 (Meta retries) rather than buffering unbounded + // memory. readSem is allocated once in newAdapter, so this direct read races + // nothing. Separate from dispatchSem, which bounds post-ack dispatch. + select { + case a.readSem <- struct{}{}: + default: + a.log().Warn("whatsapp: inbound concurrency limit reached; shedding with 503", "limit", maxConcurrentReads) + w.WriteHeader(http.StatusServiceUnavailable) + return + } + defer func() { <-a.readSem }() + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxRequestBytes)) if err != nil { w.WriteHeader(http.StatusBadRequest) @@ -500,8 +524,10 @@ func (a *adapter) ResolveAttachmentURL(ctx context.Context, att core.Attachment) return "", nil } - url := fmt.Sprintf("%s/%s/%s", a.baseURL, a.cfg.GraphVersion, media.ID) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + // PathEscape the wire-derived media ID so it cannot alter the request path; + // it stays confined to the pinned graph.facebook.com host either way. + endpoint := fmt.Sprintf("%s/%s/%s", a.baseURL, a.cfg.GraphVersion, url.PathEscape(media.ID)) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return "", err } diff --git a/internal/whatsapp/cloud/cloud_test.go b/internal/whatsapp/cloud/cloud_test.go index b1b5315..6711df3 100644 --- a/internal/whatsapp/cloud/cloud_test.go +++ b/internal/whatsapp/cloud/cloud_test.go @@ -38,7 +38,7 @@ func validConfig() Config { func testAdapter() *adapter { cfg := validConfig() cfg.Path = defaultPath - return &adapter{cfg: cfg, baseURL: graphBaseURL, http: http.DefaultClient, dispatchSem: make(chan struct{}, maxConcurrentDispatch)} + return &adapter{cfg: cfg, baseURL: graphBaseURL, http: http.DefaultClient, dispatchSem: make(chan struct{}, maxConcurrentDispatch), readSem: make(chan struct{}, maxConcurrentReads)} } // sign returns the X-Hub-Signature-256 header value for body under secret. @@ -484,6 +484,25 @@ func TestHandleWebhook_SaturationReturns503(t *testing.T) { a.drainDispatch(ctx) } +// A saturated read semaphore must answer 503 before the (up to maxRequestBytes) +// body is buffered, so a flood of large POSTs cannot exhaust memory ahead of the +// signature check. Mirrors the github/gitlab sibling test. +func TestHandleWebhook_ReadSaturationReturns503(t *testing.T) { + a := testAdapter() + a.readSem = make(chan struct{}, 1) + a.readSem <- struct{}{} // occupy the only inbound slot + + var got []*core.Message + body := []byte(textWebhook) + r := httptest.NewRequest(http.MethodPost, "/webhook", strings.NewReader(textWebhook)) + r.Header.Set(signatureHeader, sign(a.cfg.AppSecret, body)) + w := httptest.NewRecorder() + a.handleWebhook(context.Background(), w, r, captureDeps(&got, nil)) + + asserts.Equal(t, w.Code, http.StatusServiceUnavailable, "saturated read gate returns 503") + asserts.Equal(t, len(got), 0, "nothing dispatched") +} + func TestHandleWebhook_StatusOnlyIgnored(t *testing.T) { a := testAdapter() var got []*core.Message @@ -713,6 +732,29 @@ func TestResolveAttachmentURL(t *testing.T) { asserts.Equal(t, gotAuth, "Bearer tok", "resolve should set the bearer token") } +// A wire-derived media ID with URL-significant characters must be PathEscaped so +// it cannot alter the request path (defense-in-depth; it can't escape the pinned +// host regardless). +func TestResolveAttachmentURL_EscapesMediaID(t *testing.T) { + var gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.EscapedPath() + _, _ = io.WriteString(w, `{"url":"https://lookaside.fbsbx.com/media/blob"}`) + })) + defer srv.Close() + + a := testAdapter() + a.baseURL = srv.URL + a.http = srv.Client() + a.cfg.GraphVersion = "v23.0" + + _, err := a.ResolveAttachmentURL(context.Background(), + core.Attachment{ExtraData: &Media{ID: "a/b?c"}}) + + asserts.NoError(t, err, "resolve should succeed") + asserts.Equal(t, gotPath, "/v23.0/a%2Fb%3Fc", "media id is PathEscaped in the request path") +} + func TestResolveAttachmentURL_NoMediaID(t *testing.T) { a := testAdapter() a.http = nil // a no-op resolve must short-circuit before any HTTP call From 0a36bdf4b6075813b6738fb75e9debfe3c892c7d Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Aug 2026 13:13:33 +0200 Subject: [PATCH 2/8] docs: add security & performance audit report Investigation plan and findings report. Covers the four webhook adapters, Teams serviceUrl SSRF surface, secret/log handling, memory growth, and send/dispatch performance. Nearly every plan item proved a false positive or a documented accepted risk; the two real fixes are shipped in the preceding commit. --- SECURITY_PERF_INVESTIGATION.md | 176 +++++++++++++++++++++++++++++++++ SECURITY_PERF_REPORT.md | 112 +++++++++++++++++++++ 2 files changed, 288 insertions(+) create mode 100644 SECURITY_PERF_INVESTIGATION.md create mode 100644 SECURITY_PERF_REPORT.md diff --git a/SECURITY_PERF_INVESTIGATION.md b/SECURITY_PERF_INVESTIGATION.md new file mode 100644 index 0000000..5672b3d --- /dev/null +++ b/SECURITY_PERF_INVESTIGATION.md @@ -0,0 +1,176 @@ +# Security & Performance Investigation Plan + +Goal: produce a report with a security and performance overview of botbooter and a +prioritized list of improvements. This document is the investigation plan — what will +be examined, how, and with which tools. Findings land in a separate report +(`SECURITY_PERF_REPORT.md`). + +## Scope + +The whole library: `internal/core` (lifecycle + dispatch), all ten adapters +(`internal/{cli,slack,discord,telegram,whatsapp/cloud,whatsapp/whatsmeow,teams,github,gitlab,signal}`), +the public facade packages, and the dependency graph. `_examples` only insofar as it +demonstrates unsafe usage patterns. + +## Method + +1. Static pass: read the code paths listed below, file:line notes per finding. +2. Tool pass: run automated scanners (see Tooling) and triage output. +3. Dynamic pass: race detector + targeted benchmarks on hot paths. +4. Write report: each finding gets severity (high/med/low), evidence, and a concrete fix. + +--- + +## Security investigation + +### 1. Webhook ingress authentication (highest-value target) + +Four adapters run their own HTTP servers exposed to the internet. Verify each one: + +- **GitHub** (`internal/github`): HMAC signature verification — constant-time compare? + Verified before body is trusted? Body size cap before reading? Behavior on missing + signature header. +- **GitLab** (`internal/gitlab`): `X-Gitlab-Token` compare — confirm `subtle.ConstantTimeCompare` + (docs claim constant-time; verify). Confirm auth happens before body read as documented. +- **Teams** (`internal/teams`): JWKS JWT verification — algorithm pinning (alg confusion), + issuer/audience claim checks, JWKS cache poisoning/refresh behavior, clock skew handling. +- **WhatsApp Cloud** (`internal/whatsapp/cloud`): `X-Hub-Signature-256` verification, + verify-token handshake on GET. + +Cross-cutting for all four (the "copied scaffolding" — CLAUDE.md says a bug in one +probably applies to siblings; sweep all): + +- `http.Server` timeouts set? (`ReadHeaderTimeout`, `ReadTimeout`, `WriteTimeout`, + `IdleTimeout`) — missing ReadHeaderTimeout = Slowloris. +- Request body caps (`http.MaxBytesReader` or equivalent) — consistent across adapters? +- Concurrency shedding — can a flood of authentic-looking requests exhaust goroutines/memory? +- TLS story: servers appear to be plain HTTP (reverse-proxy assumption). Is that assumption + documented per adapter? Any option to serve TLS directly? +- Error responses: do 4xx/5xx bodies leak internal details (paths, versions, Go errors)? +- Method/path filtering: non-POST, wrong paths — rejected cheaply and early? + +### 2. Secrets handling + +- The `Secret` builder in core — what does it protect against (logging? String()?)? + Are all Config secret fields routed through it? +- Grep all logging (`slog`/adapters) for token/secret/header leakage, incl. debug paths. +- whatsmeow SQLite store: chmod 0600 claimed — verify at creation *and* that a pre-existing + looser-perm file isn't silently reused. Path traversal on the configured store path? +- Signal adapter: phone number + container URL — anything sensitive logged? + +### 3. Outbound request surfaces (SSRF-adjacent) + +- `ResolveAttachmentURL` / `AttachmentResolver` implementations: are returned URLs + attacker-influenced (from inbound payloads)? A consumer fetching them is the SSRF risk — + is that documented? +- `whatsmeow.Download`: media proto fields come from the wire — validate before fetch? +- Signal adapter dial-out: WebSocket + REST to a configured container URL — scheme + restrictions? Credentials in URL? +- Teams `conversationID→serviceUrl` map: serviceUrl comes from *inbound* Activities. + A forged/compromised activity could redirect replies to an attacker host — how is this + bounded (JWT verification scope covers it? serviceUrl allowlist?). This is the single + most interesting security question in the codebase. + +### 4. Untrusted-input parsing + +- Regex command patterns: consumer-supplied, but matched against *attacker-supplied* text. + Go's RE2 = no catastrophic backtracking, so ReDoS is off the table — confirm no + third-party regex engine snuck in. +- JSON decoding of webhook payloads: decoder limits, unknown-field behavior, deeply + nested payloads. +- GitLab `internalNote` raw-body side-parse: hand-rolled parsing of untrusted JSON — check + for confusion between top-level and nested keys. +- CLI adapter: any-token-that-is-a-file becomes an attachment — documented as trusted-local + only; verify the warning is prominent and consider a constructor-level opt-in. + +### 5. Loop & abuse resistance + +- Self-message filters per platform (documented matrix) — verify each against code; + the known gaps (Slack reactions, GitLab other-bots, Signal, WhatsApp) should appear + in the report as accepted risks with mitigations for consumers (idempotency, rate limits). +- No built-in rate limiting on dispatch: a hostile chat can drive unbounded handler + concurrency? Check goroutine-per-message vs. serialized dispatch per adapter. +- GitHub reaction poller: API budget logic (3000 req/h) — can a malicious repo list + or wildcard expansion blow the budget or amplify requests? + +### 6. Dependency & supply chain + +- `govulncheck ./...` — known CVEs in the graph. +- Review go.mod: versions of `golang-jwt/jwt/v5` (history of CVEs), `gorilla/websocket`, + `whatsmeow`, `modernc.org/sqlite`, `go-github`, GitLab client-go. +- Confirm isolation tests (`isolation_deps_test.go`) actually prevent SDK bleed — they're + a supply-chain blast-radius control. + +--- + +## Performance investigation + +### 1. Dispatch hot path (`internal/core`) + +- Per-message allocations: Message struct copies, middleware closure chain (composed + per-dispatch or once at registration?). +- Regex matching: first-match-wins over N patterns = O(N) regex runs per message — + fine for small N; note scaling behavior, no action unless evidence. +- `recover` wrapper cost: negligible, confirm no defer-in-loop patterns. +- Add micro-benchmarks: `BenchmarkDispatch` with 1/10/100 handlers, with/without + middleware. Currently no benchmarks in repo (verify with grep). + +### 2. Lock contention & lifecycle + +- `b.mu` held across `adapter.Connect` — intentional (documented), but verify no adapter's + Connect does blocking I/O under the lock that would stall concurrent API calls. +- Connection-scoped `sync.Once` teardown — race-detector coverage exists; confirm + `make test-race` clean on this branch. + +### 3. Unbounded memory growth (the real perf risk in a long-running bot) + +- Teams `conversationID→serviceUrl` map: grows per conversation, never evicted? +- GitHub reaction dedup store: in-memory, documented as such — bounded (LRU/TTL) or + unbounded set? +- GitLab/GitHub/Teams/WhatsApp in-flight drain tracking: WaitGroup vs. map — leak on + abnormal paths? +- Flow state (`HandleFlow` multi-step forms): per-user state — TTL/eviction on abandoned + flows? + +### 4. Webhook adapter throughput + +- Goroutine-per-delivery dispatch: shedding thresholds, drain behavior under load. +- Body read: full `io.ReadAll` vs. streaming decode; double-buffering (GitLab reads raw + body for `internalNote` side-parse *and* typed parse — one read reused, or two?). +- Response latency: GitLab must ack within ~10s or delivery counts as failed — is dispatch + fully async from the handler (ack-then-dispatch) in all webhook adapters? + +### 5. Poller efficiency (GitHub reactions) + +- Steady-state cost documented as 1 req/repo/cycle — verify; check per-cycle allocations + and whether comment listing reuses conditional requests (ETags) to cut rate-limit spend. + +### 6. Send path + +- Per-Send client allocations? Connection reuse (`http.Client` shared, keep-alives)? +- Retry behavior: none documented — note as reliability/perf tradeoff, not a defect. + +--- + +## Tooling + +```bash +govulncheck ./... +gosec ./... # triage aggressively; webhook servers are the signal +golangci-lint run # already in make all; check enabled linters cover gosec-lite +go test -race ./... # baseline +go test -bench . -benchmem ./internal/core/... # after adding benchmarks +go vet ./... +``` + +## Deliverable + +`SECURITY_PERF_REPORT.md` with: + +1. Executive overview (posture summary, one paragraph each for security and performance). +2. Findings table: ID, severity, area, file:line, one-line description. +3. Detail per finding: evidence, impact, concrete fix (smallest change that works), + and whether it sweeps to sibling adapters. +4. Accepted risks / non-findings worth stating (RE2 no-ReDoS, documented filter gaps, + deliberate scaffolding duplication). +5. Prioritized improvement list: quick wins vs. structural work. diff --git a/SECURITY_PERF_REPORT.md b/SECURITY_PERF_REPORT.md new file mode 100644 index 0000000..ff0f964 --- /dev/null +++ b/SECURITY_PERF_REPORT.md @@ -0,0 +1,112 @@ +# Security & Performance Report + +Result of the investigation in `SECURITY_PERF_INVESTIGATION.md`. Scope: `internal/core`, +all ten adapters, the public facade, and the dependency graph. Method: static read of +every path in the plan, automated scanners (`govulncheck`, `gosec`, `golangci-lint`, +`go vet`, `go test -race`), and cross-adapter consistency checks. + +## 1. Executive overview + +**Security posture: strong.** The four internet-facing webhook adapters (GitHub, GitLab, +Teams, WhatsApp Cloud) are uniformly hardened: full `http.Server` timeout sets, body-size +caps before `io.ReadAll`, cheap method/path rejection, constant-time secret compares, and +no internal detail in error responses. The highest-risk question — whether a forged inbound +Teams Activity can redirect the bot's outbound replies to an attacker host — is **fully +mitigated**: the reply `serviceUrl` is bound to the RS256-signed `serviceurl` claim *and* +constrained to an https Bot-Framework host allowlist, both enforced before the value is +stored or used. No secret is logged anywhere. Two small real gaps were found and fixed +(below); everything else in the plan proved to be a false positive or a documented, +accepted limitation. + +**Performance posture: strong.** Every adapter reuses one pooled `http.Client` +(keep-alives preserved); no client or transport is allocated per `Send`. The dispatch hot +path composes the middleware chain and compiles every command regex **once** (at +connect/registration), not per message; `Message` is passed by pointer with no per-message +deep copies. No unbounded memory growth: the Teams conversation map is FIFO-capped (10k), +flow state is TTL-swept, and in-flight drain tracking is a symmetric, panic-safe atomic +counter. No benchmarks were added — no hot-path allocation problem exists to measure, so +`BenchmarkDispatch` would be speculative. + +## 2. Findings + +| ID | Severity | Area | Location | Description | Status | +|----|----------|------|----------|-------------|--------| +| S1 | Low | DoS / consistency | `internal/teams/server.go`, `internal/whatsapp/cloud/cloud.go` | Teams & Cloud lacked the pre-auth read-concurrency semaphore (`readSem`, cap 16) that GitHub & GitLab have — the copied-scaffolding drift CLAUDE.md warns to sweep | **Fixed** | +| S2 | Info | Defense-in-depth | `internal/whatsapp/cloud/cloud.go:503` | Wire-derived `media.ID` interpolated into the Graph API URL path unescaped | **Fixed** | +| S3 | Low | Dependencies | `go.mod` (stdlib `crypto/tls`) | `govulncheck` GO-2026-5856 (ECH privacy leak), fixed in go1.26.5 — a toolchain bump | Accepted (see §4) | + +### S1 — pre-auth read-concurrency parity (fixed) + +GitHub and GitLab bound concurrent inbound body reads at `maxConcurrentReads = 16` before +the body is buffered, capping pre-auth read memory at 16 × the body cap. Teams and Cloud +bounded only *post-auth dispatch* (256), so pre-auth read buffering was limited by +connection count × 1 MiB rather than a hard 16 × 1 MiB. Impact is minor (each read is +already capped at 1 MiB and lifetime-bounded by `ReadTimeout`), but it is a genuine +correctness/consistency gap in the deliberately-duplicated webhook scaffolding. + +Fix: added the same `readSem` acquire/`defer` release at the top of `teams.handleMessages` +and `cloud.handleWebhook`, mirroring the siblings exactly. Both shed with 503 (their +platforms retry), matching each adapter's existing dispatch-shed response. Sweeps to: all +four webhook adapters now hold the identical `readSem` + `dispatchSem` shape. + +### S2 — escape wire-derived media ID (fixed) + +`ResolveAttachmentURL` built `{base}/{version}/{media.ID}` with `media.ID` (from the +HMAC-verified webhook body) unescaped. It could not escape the pinned `graph.facebook.com` +host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matching the +`url.PathEscape(channelID)` already used on the Teams send path. + +### Non-findings verified (proved false-positive or accepted) + +- **Webhook Slowloris / error-body leaks / body caps / method filtering** — all four + servers set the full timeout set, cap bodies, reject non-POST cheaply, and write only + `WriteHeader` (errors logged, never returned in the response body). +- **Teams serviceUrl SSRF** — `serviceurl` claim from the signed token is compared to the + body value (`auth.go:108-116`); `isAllowedServiceHost` enforces https + Bot-Framework + host allowlist (`auth.go:277-292`). JWT is alg-pinned to RS256, checks aud/iss/exp with + 5-min leeway, and binds signing keys to the channel. JWKS cache has max-age refresh, + min-refresh-interval (unknown-kid DoS guard) and a stale-fallback ceiling. +- **Secrets & logging** — no config-secret redaction wrapper exists, but no log or format + call anywhere emits a token, secret, signature, private key, phone number or request + body. (`Secret` is a flow-answer marker, not a config wrapper — it never claimed to + protect config.) whatsmeow tightens a pre-existing loose-perm store to 0600 + unconditionally; Signal rejects credentials-in-URL and unwraps parse errors to strip + embedded passwords. +- **Memory growth** — Teams map FIFO-capped at 10k; flow state TTL-swept every minute with + immediate delete on complete/cancel; drain tracking is a deferred atomic counter (runs + during panic unwinding); GitHub reaction dedup set is documented-unbounded but grows one + int64 per *dispatched* reaction at poller rates (accepted). +- **Send / dispatch perf** — shared pooled `http.Client` per adapter; middleware and regex + composed once; no defer-in-loop; webhook dispatch bounded + shed. Signal's dispatch + goroutine is uncapped but serially fed by a single receive socket (accepted). +- **`govulncheck`** — of 5 reported, only `crypto/tls` (S3) is reachable; the other four + (x/text infinite-loop, slack-go `SecretsVerifier`, os symlink, openpgp) are confirmed + **not called** by our code. +- **`gosec`** — 5 issues, all pre-existing `//nolint`'d with correct justifications (OAuth + scope URL, header name, plain-text challenge echo, adapter-owned file paths). No new + issues introduced by the fixes. +- **ReDoS** — Go's RE2 has no catastrophic backtracking; no third-party regex engine in the + graph. Off the table. + +## 3. Accepted risks (stated, not fixed) + +- **GitHub reaction dedup set** unbounded in principle; negligible at poller rates and + documented in-code. Upgrade path: LRU keyed on the connect cutoff, YAGNI today. +- **Flow-secret durable-store exclusion** is aspirational — no `Store` implementation + exists yet, so nothing durably persists secrets; harmless until one is added. +- **Signal dispatch** is uncapped but bounded by a single serial receive socket. +- **Self-message filter gaps** (Slack reactions, GitLab other-bots, Signal, WhatsApp) are + documented platform limitations; consumers must be idempotent and rate-limit. + +## 4. Prioritized improvements + +**Done (this PR):** S1 (readSem parity), S2 (media.ID escape). + +**Quick wins (out of scope for this PR — dependency/toolchain, not code):** +- Bump the build toolchain past go1.26.5 (or confirm the pinned go1.25.x patch carries the + crypto/tls fix) to clear GO-2026-5856. +- Opportunistically bump `slack-go` (`SecretsVerifier` CVE, uncalled) and `x/text` on the + next dependency sweep. + +**Structural (only if evidence demands):** LRU/TTL on the GitHub reaction dedup set; +a `readSem`-style cap on Signal dispatch. Neither is warranted at v1 loads. From 7d7e4651fc757297b7c0375c26b8dcf01abc431e Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Aug 2026 16:10:04 +0200 Subject: [PATCH 3/8] test(webhook): assert shed requests never read the body --- internal/teams/server_test.go | 16 +++++++++++++--- internal/whatsapp/cloud/cloud_test.go | 13 ++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/internal/teams/server_test.go b/internal/teams/server_test.go index 40c8062..eecd6d4 100644 --- a/internal/teams/server_test.go +++ b/internal/teams/server_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "io" "log/slog" "net" "net/http" @@ -269,6 +270,12 @@ func TestHandleMessages_SaturatedDispatchReturns503(t *testing.T) { asserts.Equal(t, w2.Code, http.StatusServiceUnavailable, "saturated dispatch sheds load with 503") } +// readSpy counts Read calls so a test can prove a shed request's body was +// never touched. +type readSpy struct{ reads int } + +func (s *readSpy) Read([]byte) (int, error) { s.reads++; return 0, io.EOF } + // A saturated read semaphore must answer 503 before the body is buffered and // unmarshaled, so a flood of large POSTs cannot exhaust memory ahead of the JWT // check. Mirrors the github/gitlab sibling test. @@ -278,12 +285,15 @@ func TestHandleMessages_ReadSaturationReturns503(t *testing.T) { a.readSem <- struct{}{} // occupy the only inbound slot var got []*core.Message - body := activityJSON("message", "hi", allowedServiceURL, "user-1", "bot-1", "conv-1") - auth := "Bearer " + mintToken(t, testKID, validClaims(a.cfg.AppID, allowedServiceURL)) - w := post(a, captureDeps(&got, nil), body, auth) + spy := &readSpy{} + r := httptest.NewRequest(http.MethodPost, a.cfg.Path, spy) + r.Header.Set("Authorization", "Bearer "+mintToken(t, testKID, validClaims(a.cfg.AppID, allowedServiceURL))) + w := httptest.NewRecorder() + a.handleMessages(context.Background(), w, r, captureDeps(&got, nil)) asserts.Equal(t, w.Code, http.StatusServiceUnavailable, "saturated read gate returns 503") asserts.Equal(t, len(got), 0, "nothing dispatched") + asserts.Equal(t, spy.reads, 0, "shed request's body is never read") } func TestHandleMessages_IgnoresNonMessage(t *testing.T) { diff --git a/internal/whatsapp/cloud/cloud_test.go b/internal/whatsapp/cloud/cloud_test.go index 6711df3..d86c380 100644 --- a/internal/whatsapp/cloud/cloud_test.go +++ b/internal/whatsapp/cloud/cloud_test.go @@ -484,6 +484,12 @@ func TestHandleWebhook_SaturationReturns503(t *testing.T) { a.drainDispatch(ctx) } +// readSpy counts Read calls so a test can prove a shed request's body was +// never touched. +type readSpy struct{ reads int } + +func (s *readSpy) Read([]byte) (int, error) { s.reads++; return 0, io.EOF } + // A saturated read semaphore must answer 503 before the (up to maxRequestBytes) // body is buffered, so a flood of large POSTs cannot exhaust memory ahead of the // signature check. Mirrors the github/gitlab sibling test. @@ -493,14 +499,15 @@ func TestHandleWebhook_ReadSaturationReturns503(t *testing.T) { a.readSem <- struct{}{} // occupy the only inbound slot var got []*core.Message - body := []byte(textWebhook) - r := httptest.NewRequest(http.MethodPost, "/webhook", strings.NewReader(textWebhook)) - r.Header.Set(signatureHeader, sign(a.cfg.AppSecret, body)) + spy := &readSpy{} + r := httptest.NewRequest(http.MethodPost, "/webhook", spy) + r.Header.Set(signatureHeader, sign(a.cfg.AppSecret, []byte(textWebhook))) w := httptest.NewRecorder() a.handleWebhook(context.Background(), w, r, captureDeps(&got, nil)) asserts.Equal(t, w.Code, http.StatusServiceUnavailable, "saturated read gate returns 503") asserts.Equal(t, len(got), 0, "nothing dispatched") + asserts.Equal(t, spy.reads, 0, "shed request's body is never read") } func TestHandleWebhook_StatusOnlyIgnored(t *testing.T) { From 7ece8118c71bf7139fd1828ee32a40837394b373 Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Aug 2026 16:10:04 +0200 Subject: [PATCH 4/8] docs(audit): per-adapter auth detail, exact toolchain versions, precise line refs --- SECURITY_PERF_INVESTIGATION.md | 8 +++++--- SECURITY_PERF_REPORT.md | 35 +++++++++++++++++++++------------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/SECURITY_PERF_INVESTIGATION.md b/SECURITY_PERF_INVESTIGATION.md index 5672b3d..d6883f6 100644 --- a/SECURITY_PERF_INVESTIGATION.md +++ b/SECURITY_PERF_INVESTIGATION.md @@ -85,9 +85,11 @@ probably applies to siblings; sweep all): ### 5. Loop & abuse resistance -- Self-message filters per platform (documented matrix) — verify each against code; - the known gaps (Slack reactions, GitLab other-bots, Signal, WhatsApp) should appear - in the report as accepted risks with mitigations for consumers (idempotency, rate limits). +- Self-message filters per platform (documented matrix) — verify each against code. + Every platform filters its *own* messages; the known gaps are *other-bot* filtering + where the platform carries no bot marker (Slack reactions, GitLab notes, Signal, + WhatsApp) — these should appear in the report as accepted risks with mitigations for + consumers (idempotency, rate limits). - No built-in rate limiting on dispatch: a hostile chat can drive unbounded handler concurrency? Check goroutine-per-message vs. serialized dispatch per adapter. - GitHub reaction poller: API budget logic (3000 req/h) — can a malicious repo list diff --git a/SECURITY_PERF_REPORT.md b/SECURITY_PERF_REPORT.md index ff0f964..ba1d5fd 100644 --- a/SECURITY_PERF_REPORT.md +++ b/SECURITY_PERF_REPORT.md @@ -9,8 +9,13 @@ every path in the plan, automated scanners (`govulncheck`, `gosec`, `golangci-li **Security posture: strong.** The four internet-facing webhook adapters (GitHub, GitLab, Teams, WhatsApp Cloud) are uniformly hardened: full `http.Server` timeout sets, body-size -caps before `io.ReadAll`, cheap method/path rejection, constant-time secret compares, and -no internal detail in error responses. The highest-risk question — whether a forged inbound +caps before `io.ReadAll`, cheap method/path rejection, and no internal detail in error +responses. Authentication is adapter-specific: GitHub verifies the body HMAC +(`X-Hub-Signature-256` via go-github's constant-time `ValidateSignature`); GitLab compares +`X-Gitlab-Token` with `subtle.ConstantTimeCompare` before reading the body; WhatsApp Cloud +verifies the body HMAC with `hmac.Equal` (and its GET verify-token handshake with +`subtle.ConstantTimeCompare`); Teams verifies an RS256-signed JWT against the Bot +Connector JWKS (alg-pinned, aud/iss/exp checked) rather than a shared secret. The highest-risk question — whether a forged inbound Teams Activity can redirect the bot's outbound replies to an attacker host — is **fully mitigated**: the reply `serviceUrl` is bound to the RS256-signed `serviceurl` claim *and* constrained to an https Bot-Framework host allowlist, both enforced before the value is @@ -22,18 +27,20 @@ accepted limitation. (keep-alives preserved); no client or transport is allocated per `Send`. The dispatch hot path composes the middleware chain and compiles every command regex **once** (at connect/registration), not per message; `Message` is passed by pointer with no per-message -deep copies. No unbounded memory growth: the Teams conversation map is FIFO-capped (10k), -flow state is TTL-swept, and in-flight drain tracking is a symmetric, panic-safe atomic -counter. No benchmarks were added — no hot-path allocation problem exists to measure, so -`BenchmarkDispatch` would be speculative. +deep copies. No unbounded memory growth, with one accepted exception: the Teams +conversation map is FIFO-capped (10k), flow state is TTL-swept, in-flight drain tracking +is a symmetric, panic-safe atomic counter, and the GitHub reaction dedup set is unbounded +in principle but negligible at poller rates (accepted, §3). No benchmarks were added — +the static read found no per-message allocation hotspots, but allocation behavior remains +unmeasured; `BenchmarkDispatch` was judged speculative absent evidence of a problem. ## 2. Findings | ID | Severity | Area | Location | Description | Status | |----|----------|------|----------|-------------|--------| -| S1 | Low | DoS / consistency | `internal/teams/server.go`, `internal/whatsapp/cloud/cloud.go` | Teams & Cloud lacked the pre-auth read-concurrency semaphore (`readSem`, cap 16) that GitHub & GitLab have — the copied-scaffolding drift CLAUDE.md warns to sweep | **Fixed** | -| S2 | Info | Defense-in-depth | `internal/whatsapp/cloud/cloud.go:503` | Wire-derived `media.ID` interpolated into the Graph API URL path unescaped | **Fixed** | -| S3 | Low | Dependencies | `go.mod` (stdlib `crypto/tls`) | `govulncheck` GO-2026-5856 (ECH privacy leak), fixed in go1.26.5 — a toolchain bump | Accepted (see §4) | +| S1 | Low | DoS / consistency | `internal/teams/server.go:109-121`, `internal/whatsapp/cloud/cloud.go:302-313` | Teams & Cloud lacked the pre-auth read-concurrency semaphore (`readSem`, cap 16) that GitHub & GitLab have — the copied-scaffolding drift CLAUDE.md warns to sweep (locations are the fix as landed) | **Fixed** | +| S2 | Info | Defense-in-depth | `internal/whatsapp/cloud/cloud.go:527-529` | Wire-derived `media.ID` interpolated into the Graph API URL path unescaped (location is the fix as landed) | **Fixed** | +| S3 | Low | Dependencies | `go.mod:7` (stdlib `crypto/tls`) | `govulncheck` GO-2026-5856 (ECH privacy leak), fixed in go1.25.12 / go1.26.5; pinned toolchain is go1.25.11 — one patch behind | Accepted (see §4) | ### S1 — pre-auth read-concurrency parity (fixed) @@ -95,16 +102,18 @@ host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matchin - **Flow-secret durable-store exclusion** is aspirational — no `Store` implementation exists yet, so nothing durably persists secrets; harmless until one is added. - **Signal dispatch** is uncapped but bounded by a single serial receive socket. -- **Self-message filter gaps** (Slack reactions, GitLab other-bots, Signal, WhatsApp) are - documented platform limitations; consumers must be idempotent and rate-limit. +- **Other-bot filter gaps** — every platform drops the bot's *own* messages (self filters + verified on all ten adapters); what cannot be filtered is *other* bots where the + platform carries no bot marker (Slack `reaction_added`, GitLab notes, Signal, WhatsApp). + Documented platform limitations; consumers must be idempotent and rate-limit. ## 4. Prioritized improvements **Done (this PR):** S1 (readSem parity), S2 (media.ID escape). **Quick wins (out of scope for this PR — dependency/toolchain, not code):** -- Bump the build toolchain past go1.26.5 (or confirm the pinned go1.25.x patch carries the - crypto/tls fix) to clear GO-2026-5856. +- Bump the pinned toolchain go1.25.11 → go1.25.12 (or go1.26.5+) to pick up the + crypto/tls fix and clear GO-2026-5856. - Opportunistically bump `slack-go` (`SecretsVerifier` CVE, uncalled) and `x/text` on the next dependency sweep. From 1b53f8a00d13057d5a626d3cd75cc3a034af2d0c Mon Sep 17 00:00:00 2001 From: "botbooter-test[bot]" <302759166+botbooter-test[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 16:34:33 +0200 Subject: [PATCH 5/8] docs(audit): apply review fixes to security & performance report --- SECURITY_PERF_REPORT.md | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/SECURITY_PERF_REPORT.md b/SECURITY_PERF_REPORT.md index ba1d5fd..83977cf 100644 --- a/SECURITY_PERF_REPORT.md +++ b/SECURITY_PERF_REPORT.md @@ -5,6 +5,12 @@ all ten adapters, the public facade, and the dependency graph. Method: static re every path in the plan, automated scanners (`govulncheck`, `gosec`, `golangci-lint`, `go vet`, `go test -race`), and cross-adapter consistency checks. +> **Point-in-time snapshot (this PR's branch, 2026-08-03).** Scanner output and the +> dependency/toolchain state drift as the tree and its graph move; re-run the tools listed +> in `SECURITY_PERF_INVESTIGATION.md` rather than treating these results as current. Specific +> advisory IDs and toolchain version numbers are deliberately kept out of this report so a +> stale copy can't imply assurance it no longer has. + ## 1. Executive overview **Security posture: strong.** The four internet-facing webhook adapters (GitHub, GitLab, @@ -40,7 +46,6 @@ unmeasured; `BenchmarkDispatch` was judged speculative absent evidence of a prob |----|----------|------|----------|-------------|--------| | S1 | Low | DoS / consistency | `internal/teams/server.go:109-121`, `internal/whatsapp/cloud/cloud.go:302-313` | Teams & Cloud lacked the pre-auth read-concurrency semaphore (`readSem`, cap 16) that GitHub & GitLab have — the copied-scaffolding drift CLAUDE.md warns to sweep (locations are the fix as landed) | **Fixed** | | S2 | Info | Defense-in-depth | `internal/whatsapp/cloud/cloud.go:527-529` | Wire-derived `media.ID` interpolated into the Graph API URL path unescaped (location is the fix as landed) | **Fixed** | -| S3 | Low | Dependencies | `go.mod:7` (stdlib `crypto/tls`) | `govulncheck` GO-2026-5856 (ECH privacy leak), fixed in go1.25.12 / go1.26.5; pinned toolchain is go1.25.11 — one patch behind | Accepted (see §4) | ### S1 — pre-auth read-concurrency parity (fixed) @@ -86,9 +91,9 @@ host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matchin - **Send / dispatch perf** — shared pooled `http.Client` per adapter; middleware and regex composed once; no defer-in-loop; webhook dispatch bounded + shed. Signal's dispatch goroutine is uncapped but serially fed by a single receive socket (accepted). -- **`govulncheck`** — of 5 reported, only `crypto/tls` (S3) is reachable; the other four +- **`govulncheck`** — triaged reachable-vs-not: advisories against uncalled symbols (x/text infinite-loop, slack-go `SecretsVerifier`, os symlink, openpgp) are confirmed - **not called** by our code. + **not called** by our code. Re-run before relying on this; the advisory set moves. - **`gosec`** — 5 issues, all pre-existing `//nolint`'d with correct justifications (OAuth scope URL, header name, plain-text challenge echo, adapter-owned file paths). No new issues introduced by the fixes. @@ -111,11 +116,10 @@ host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matchin **Done (this PR):** S1 (readSem parity), S2 (media.ID escape). -**Quick wins (out of scope for this PR — dependency/toolchain, not code):** -- Bump the pinned toolchain go1.25.11 → go1.25.12 (or go1.26.5+) to pick up the - crypto/tls fix and clear GO-2026-5856. -- Opportunistically bump `slack-go` (`SecretsVerifier` CVE, uncalled) and `x/text` on the - next dependency sweep. +**Quick wins (out of scope for this PR — dependency/toolchain, not code):** run +`govulncheck` and a toolchain/dependency sweep at review time and act on whatever it +reports then. Specific advisory IDs and toolchain versions are intentionally omitted here +so this report can't drift into wrong as the graph moves. **Structural (only if evidence demands):** LRU/TTL on the GitHub reaction dedup set; a `readSem`-style cap on Signal dispatch. Neither is warranted at v1 loads. From f12c72f10a8b7e33727af1fd065beeb5c347bdb4 Mon Sep 17 00:00:00 2001 From: "botbooter-test[bot]" <302759166+botbooter-test[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 16:38:43 +0200 Subject: [PATCH 6/8] fix(webhook): apply review fixes to Teams adapter and audit report --- SECURITY_PERF_REPORT.md | 37 ++++++++++++++++++++++++++----------- internal/teams/server.go | 8 +++++++- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/SECURITY_PERF_REPORT.md b/SECURITY_PERF_REPORT.md index 83977cf..cd0687c 100644 --- a/SECURITY_PERF_REPORT.md +++ b/SECURITY_PERF_REPORT.md @@ -5,15 +5,25 @@ all ten adapters, the public facade, and the dependency graph. Method: static re every path in the plan, automated scanners (`govulncheck`, `gosec`, `golangci-lint`, `go vet`, `go test -race`), and cross-adapter consistency checks. -> **Point-in-time snapshot (this PR's branch, 2026-08-03).** Scanner output and the -> dependency/toolchain state drift as the tree and its graph move; re-run the tools listed -> in `SECURITY_PERF_INVESTIGATION.md` rather than treating these results as current. Specific -> advisory IDs and toolchain version numbers are deliberately kept out of this report so a -> stale copy can't imply assurance it no longer has. +> **Archived point-in-time snapshot (this PR's branch, 2026-08-03) — not a standing +> guarantee.** Scanner output and the dependency/toolchain state drift as the tree and its +> graph move; re-run the tools listed in `SECURITY_PERF_INVESTIGATION.md` rather than treating +> these results as current. The `file:line` references below are illustrative of where a +> concern was observed on this branch and *will* go stale as code moves — treat them as +> pointers, not as authoritative locations. Specific advisory IDs and toolchain version +> numbers are deliberately kept out of this report so a stale copy can't imply assurance it no +> longer has. +> +> **Open action (not resolved by this PR):** `govulncheck` flagged a *reachable* `crypto/tls` +> advisory that requires a Go toolchain bump. It is out of scope for this two-line hardening +> change and is tracked as an open action (see §4), not covered by the "strong posture" +> summary below. ## 1. Executive overview -**Security posture: strong.** The four internet-facing webhook adapters (GitHub, GitLab, +**Security posture: strong for the reviewed application code, with one open dependency +action (the reachable `crypto/tls` advisory above, unresolved here).** The four +internet-facing webhook adapters (GitHub, GitLab, Teams, WhatsApp Cloud) are uniformly hardened: full `http.Server` timeout sets, body-size caps before `io.ReadAll`, cheap method/path rejection, and no internal detail in error responses. Authentication is adapter-specific: GitHub verifies the body HMAC @@ -93,7 +103,9 @@ host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matchin goroutine is uncapped but serially fed by a single receive socket (accepted). - **`govulncheck`** — triaged reachable-vs-not: advisories against uncalled symbols (x/text infinite-loop, slack-go `SecretsVerifier`, os symlink, openpgp) are confirmed - **not called** by our code. Re-run before relying on this; the advisory set moves. + **not called** by our code. Exception: a *reachable* `crypto/tls` advisory requiring a + toolchain bump is **not** mitigated here and is tracked as an open action (§4). Re-run + before relying on this; the advisory set moves. - **`gosec`** — 5 issues, all pre-existing `//nolint`'d with correct justifications (OAuth scope URL, header name, plain-text challenge echo, adapter-owned file paths). No new issues introduced by the fixes. @@ -116,10 +128,13 @@ host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matchin **Done (this PR):** S1 (readSem parity), S2 (media.ID escape). -**Quick wins (out of scope for this PR — dependency/toolchain, not code):** run -`govulncheck` and a toolchain/dependency sweep at review time and act on whatever it -reports then. Specific advisory IDs and toolchain versions are intentionally omitted here -so this report can't drift into wrong as the graph moves. +**Open actions (out of scope for this PR — dependency/toolchain, not code):** +- **Reachable `crypto/tls` advisory (toolchain bump) — unresolved.** `govulncheck` flagged a + reachable advisory in `crypto/tls` that needs a Go toolchain upgrade. This PR does not + address it; it must be tracked and fixed separately rather than assumed clean. +- Run `govulncheck` and a toolchain/dependency sweep at review time and act on whatever else + it reports then. Specific advisory IDs and toolchain versions are intentionally omitted here + so this report can't drift into wrong as the graph moves. **Structural (only if evidence demands):** LRU/TTL on the GitHub reaction dedup set; a `readSem`-style cap on Signal dispatch. Neither is warranted at v1 loads. diff --git a/internal/teams/server.go b/internal/teams/server.go index 6231e84..587abbb 100644 --- a/internal/teams/server.go +++ b/internal/teams/server.go @@ -119,19 +119,25 @@ func (a *adapter) handleMessages(dispatchCtx context.Context, w http.ResponseWri w.WriteHeader(http.StatusServiceUnavailable) return } - defer func() { <-a.readSem }() body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxRequestBytes)) if err != nil { + <-a.readSem w.WriteHeader(http.StatusBadRequest) return } var act inboundActivity if err := json.Unmarshal(body, &act); err != nil { + <-a.readSem w.WriteHeader(http.StatusBadRequest) return } + // Release the read slot as soon as the body is buffered + unmarshaled: unlike + // the GitHub/GitLab siblings, validateInbound below can block on a cold JWKS + // fetch (up to jwksFetchTimeout), so holding readSem across it would gate + // throughput far more aggressively than the fast HMAC path readSem is sized for. + <-a.readSem // Authenticate before trusting the body. Use the request context, not runCtx: // core cancels runCtx before srv.Shutdown drains this handler, so a JWKS From f1ea2fef2853df1ea19f8d53c8c5d90e31bf8708 Mon Sep 17 00:00:00 2001 From: "botbooter-test[bot]" <302759166+botbooter-test[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 16:41:23 +0200 Subject: [PATCH 7/8] docs(audit): apply review fixes to security & performance report --- SECURITY_PERF_REPORT.md | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/SECURITY_PERF_REPORT.md b/SECURITY_PERF_REPORT.md index cd0687c..dcca92f 100644 --- a/SECURITY_PERF_REPORT.md +++ b/SECURITY_PERF_REPORT.md @@ -54,22 +54,26 @@ unmeasured; `BenchmarkDispatch` was judged speculative absent evidence of a prob | ID | Severity | Area | Location | Description | Status | |----|----------|------|----------|-------------|--------| -| S1 | Low | DoS / consistency | `internal/teams/server.go:109-121`, `internal/whatsapp/cloud/cloud.go:302-313` | Teams & Cloud lacked the pre-auth read-concurrency semaphore (`readSem`, cap 16) that GitHub & GitLab have — the copied-scaffolding drift CLAUDE.md warns to sweep (locations are the fix as landed) | **Fixed** | -| S2 | Info | Defense-in-depth | `internal/whatsapp/cloud/cloud.go:527-529` | Wire-derived `media.ID` interpolated into the Graph API URL path unescaped (location is the fix as landed) | **Fixed** | +| S1 | Low | DoS / consistency | `internal/teams/server.go` (`handleMessages`), `internal/whatsapp/cloud/cloud.go` (`handleWebhook`) | Teams & Cloud lacked the pre-auth read-concurrency semaphore (`readSem`, cap 16) that GitHub & GitLab have — the copied-scaffolding drift CLAUDE.md warns to sweep | **Fixed** | +| S2 | Info | Defense-in-depth | `internal/whatsapp/cloud/cloud.go` (`ResolveAttachmentURL`) | Wire-derived `media.ID` interpolated into the Graph API URL path unescaped | **Fixed** | ### S1 — pre-auth read-concurrency parity (fixed) GitHub and GitLab bound concurrent inbound body reads at `maxConcurrentReads = 16` before -the body is buffered, capping pre-auth read memory at 16 × the body cap. Teams and Cloud -bounded only *post-auth dispatch* (256), so pre-auth read buffering was limited by +the body is buffered, capping the *synchronous read* window at 16 × the body cap. Teams and +Cloud bounded only *post-auth dispatch* (256), so pre-auth read buffering was limited by connection count × 1 MiB rather than a hard 16 × 1 MiB. Impact is minor (each read is already capped at 1 MiB and lifetime-bounded by `ReadTimeout`), but it is a genuine correctness/consistency gap in the deliberately-duplicated webhook scaffolding. -Fix: added the same `readSem` acquire/`defer` release at the top of `teams.handleMessages` -and `cloud.handleWebhook`, mirroring the siblings exactly. Both shed with 503 (their -platforms retry), matching each adapter's existing dispatch-shed response. Sweeps to: all -four webhook adapters now hold the identical `readSem` + `dispatchSem` shape. +Fix: added the same `readSem` acquire/release at the top of `teams.handleMessages` and +`cloud.handleWebhook`. Cloud uses the sibling `defer` release, since its synchronous +portion (HMAC + parse) is fast. Teams deliberately diverges: it releases the slot *before* +`validateInbound`, because that call can block on a cold JWKS fetch — so the 16-slot gate +bounds the read itself, **not** any body still referenced during the JWKS window. This +divergence is documented in-code. Both shed with 503 (their platforms retry), matching each +adapter's existing dispatch-shed response. Sweeps to: all four webhook adapters now hold the +`readSem` + `dispatchSem` shape. ### S2 — escape wire-derived media ID (fixed) @@ -84,8 +88,8 @@ host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matchin servers set the full timeout set, cap bodies, reject non-POST cheaply, and write only `WriteHeader` (errors logged, never returned in the response body). - **Teams serviceUrl SSRF** — `serviceurl` claim from the signed token is compared to the - body value (`auth.go:108-116`); `isAllowedServiceHost` enforces https + Bot-Framework - host allowlist (`auth.go:277-292`). JWT is alg-pinned to RS256, checks aud/iss/exp with + body value; `isAllowedServiceHost` (`auth.go`) enforces https + Bot-Framework + host allowlist. JWT is alg-pinned to RS256, checks aud/iss/exp with 5-min leeway, and binds signing keys to the channel. JWKS cache has max-age refresh, min-refresh-interval (unknown-kid DoS guard) and a stale-fallback ceiling. - **Secrets & logging** — no config-secret redaction wrapper exists, but no log or format From abd2186cddf86ddf2b1613bd9dbf11105fd836bd Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Aug 2026 17:12:54 +0200 Subject: [PATCH 8/8] docs: drop audit plan and report from the repo --- SECURITY_PERF_INVESTIGATION.md | 178 --------------------------------- SECURITY_PERF_REPORT.md | 144 -------------------------- 2 files changed, 322 deletions(-) delete mode 100644 SECURITY_PERF_INVESTIGATION.md delete mode 100644 SECURITY_PERF_REPORT.md diff --git a/SECURITY_PERF_INVESTIGATION.md b/SECURITY_PERF_INVESTIGATION.md deleted file mode 100644 index d6883f6..0000000 --- a/SECURITY_PERF_INVESTIGATION.md +++ /dev/null @@ -1,178 +0,0 @@ -# Security & Performance Investigation Plan - -Goal: produce a report with a security and performance overview of botbooter and a -prioritized list of improvements. This document is the investigation plan — what will -be examined, how, and with which tools. Findings land in a separate report -(`SECURITY_PERF_REPORT.md`). - -## Scope - -The whole library: `internal/core` (lifecycle + dispatch), all ten adapters -(`internal/{cli,slack,discord,telegram,whatsapp/cloud,whatsapp/whatsmeow,teams,github,gitlab,signal}`), -the public facade packages, and the dependency graph. `_examples` only insofar as it -demonstrates unsafe usage patterns. - -## Method - -1. Static pass: read the code paths listed below, file:line notes per finding. -2. Tool pass: run automated scanners (see Tooling) and triage output. -3. Dynamic pass: race detector + targeted benchmarks on hot paths. -4. Write report: each finding gets severity (high/med/low), evidence, and a concrete fix. - ---- - -## Security investigation - -### 1. Webhook ingress authentication (highest-value target) - -Four adapters run their own HTTP servers exposed to the internet. Verify each one: - -- **GitHub** (`internal/github`): HMAC signature verification — constant-time compare? - Verified before body is trusted? Body size cap before reading? Behavior on missing - signature header. -- **GitLab** (`internal/gitlab`): `X-Gitlab-Token` compare — confirm `subtle.ConstantTimeCompare` - (docs claim constant-time; verify). Confirm auth happens before body read as documented. -- **Teams** (`internal/teams`): JWKS JWT verification — algorithm pinning (alg confusion), - issuer/audience claim checks, JWKS cache poisoning/refresh behavior, clock skew handling. -- **WhatsApp Cloud** (`internal/whatsapp/cloud`): `X-Hub-Signature-256` verification, - verify-token handshake on GET. - -Cross-cutting for all four (the "copied scaffolding" — CLAUDE.md says a bug in one -probably applies to siblings; sweep all): - -- `http.Server` timeouts set? (`ReadHeaderTimeout`, `ReadTimeout`, `WriteTimeout`, - `IdleTimeout`) — missing ReadHeaderTimeout = Slowloris. -- Request body caps (`http.MaxBytesReader` or equivalent) — consistent across adapters? -- Concurrency shedding — can a flood of authentic-looking requests exhaust goroutines/memory? -- TLS story: servers appear to be plain HTTP (reverse-proxy assumption). Is that assumption - documented per adapter? Any option to serve TLS directly? -- Error responses: do 4xx/5xx bodies leak internal details (paths, versions, Go errors)? -- Method/path filtering: non-POST, wrong paths — rejected cheaply and early? - -### 2. Secrets handling - -- The `Secret` builder in core — what does it protect against (logging? String()?)? - Are all Config secret fields routed through it? -- Grep all logging (`slog`/adapters) for token/secret/header leakage, incl. debug paths. -- whatsmeow SQLite store: chmod 0600 claimed — verify at creation *and* that a pre-existing - looser-perm file isn't silently reused. Path traversal on the configured store path? -- Signal adapter: phone number + container URL — anything sensitive logged? - -### 3. Outbound request surfaces (SSRF-adjacent) - -- `ResolveAttachmentURL` / `AttachmentResolver` implementations: are returned URLs - attacker-influenced (from inbound payloads)? A consumer fetching them is the SSRF risk — - is that documented? -- `whatsmeow.Download`: media proto fields come from the wire — validate before fetch? -- Signal adapter dial-out: WebSocket + REST to a configured container URL — scheme - restrictions? Credentials in URL? -- Teams `conversationID→serviceUrl` map: serviceUrl comes from *inbound* Activities. - A forged/compromised activity could redirect replies to an attacker host — how is this - bounded (JWT verification scope covers it? serviceUrl allowlist?). This is the single - most interesting security question in the codebase. - -### 4. Untrusted-input parsing - -- Regex command patterns: consumer-supplied, but matched against *attacker-supplied* text. - Go's RE2 = no catastrophic backtracking, so ReDoS is off the table — confirm no - third-party regex engine snuck in. -- JSON decoding of webhook payloads: decoder limits, unknown-field behavior, deeply - nested payloads. -- GitLab `internalNote` raw-body side-parse: hand-rolled parsing of untrusted JSON — check - for confusion between top-level and nested keys. -- CLI adapter: any-token-that-is-a-file becomes an attachment — documented as trusted-local - only; verify the warning is prominent and consider a constructor-level opt-in. - -### 5. Loop & abuse resistance - -- Self-message filters per platform (documented matrix) — verify each against code. - Every platform filters its *own* messages; the known gaps are *other-bot* filtering - where the platform carries no bot marker (Slack reactions, GitLab notes, Signal, - WhatsApp) — these should appear in the report as accepted risks with mitigations for - consumers (idempotency, rate limits). -- No built-in rate limiting on dispatch: a hostile chat can drive unbounded handler - concurrency? Check goroutine-per-message vs. serialized dispatch per adapter. -- GitHub reaction poller: API budget logic (3000 req/h) — can a malicious repo list - or wildcard expansion blow the budget or amplify requests? - -### 6. Dependency & supply chain - -- `govulncheck ./...` — known CVEs in the graph. -- Review go.mod: versions of `golang-jwt/jwt/v5` (history of CVEs), `gorilla/websocket`, - `whatsmeow`, `modernc.org/sqlite`, `go-github`, GitLab client-go. -- Confirm isolation tests (`isolation_deps_test.go`) actually prevent SDK bleed — they're - a supply-chain blast-radius control. - ---- - -## Performance investigation - -### 1. Dispatch hot path (`internal/core`) - -- Per-message allocations: Message struct copies, middleware closure chain (composed - per-dispatch or once at registration?). -- Regex matching: first-match-wins over N patterns = O(N) regex runs per message — - fine for small N; note scaling behavior, no action unless evidence. -- `recover` wrapper cost: negligible, confirm no defer-in-loop patterns. -- Add micro-benchmarks: `BenchmarkDispatch` with 1/10/100 handlers, with/without - middleware. Currently no benchmarks in repo (verify with grep). - -### 2. Lock contention & lifecycle - -- `b.mu` held across `adapter.Connect` — intentional (documented), but verify no adapter's - Connect does blocking I/O under the lock that would stall concurrent API calls. -- Connection-scoped `sync.Once` teardown — race-detector coverage exists; confirm - `make test-race` clean on this branch. - -### 3. Unbounded memory growth (the real perf risk in a long-running bot) - -- Teams `conversationID→serviceUrl` map: grows per conversation, never evicted? -- GitHub reaction dedup store: in-memory, documented as such — bounded (LRU/TTL) or - unbounded set? -- GitLab/GitHub/Teams/WhatsApp in-flight drain tracking: WaitGroup vs. map — leak on - abnormal paths? -- Flow state (`HandleFlow` multi-step forms): per-user state — TTL/eviction on abandoned - flows? - -### 4. Webhook adapter throughput - -- Goroutine-per-delivery dispatch: shedding thresholds, drain behavior under load. -- Body read: full `io.ReadAll` vs. streaming decode; double-buffering (GitLab reads raw - body for `internalNote` side-parse *and* typed parse — one read reused, or two?). -- Response latency: GitLab must ack within ~10s or delivery counts as failed — is dispatch - fully async from the handler (ack-then-dispatch) in all webhook adapters? - -### 5. Poller efficiency (GitHub reactions) - -- Steady-state cost documented as 1 req/repo/cycle — verify; check per-cycle allocations - and whether comment listing reuses conditional requests (ETags) to cut rate-limit spend. - -### 6. Send path - -- Per-Send client allocations? Connection reuse (`http.Client` shared, keep-alives)? -- Retry behavior: none documented — note as reliability/perf tradeoff, not a defect. - ---- - -## Tooling - -```bash -govulncheck ./... -gosec ./... # triage aggressively; webhook servers are the signal -golangci-lint run # already in make all; check enabled linters cover gosec-lite -go test -race ./... # baseline -go test -bench . -benchmem ./internal/core/... # after adding benchmarks -go vet ./... -``` - -## Deliverable - -`SECURITY_PERF_REPORT.md` with: - -1. Executive overview (posture summary, one paragraph each for security and performance). -2. Findings table: ID, severity, area, file:line, one-line description. -3. Detail per finding: evidence, impact, concrete fix (smallest change that works), - and whether it sweeps to sibling adapters. -4. Accepted risks / non-findings worth stating (RE2 no-ReDoS, documented filter gaps, - deliberate scaffolding duplication). -5. Prioritized improvement list: quick wins vs. structural work. diff --git a/SECURITY_PERF_REPORT.md b/SECURITY_PERF_REPORT.md deleted file mode 100644 index dcca92f..0000000 --- a/SECURITY_PERF_REPORT.md +++ /dev/null @@ -1,144 +0,0 @@ -# Security & Performance Report - -Result of the investigation in `SECURITY_PERF_INVESTIGATION.md`. Scope: `internal/core`, -all ten adapters, the public facade, and the dependency graph. Method: static read of -every path in the plan, automated scanners (`govulncheck`, `gosec`, `golangci-lint`, -`go vet`, `go test -race`), and cross-adapter consistency checks. - -> **Archived point-in-time snapshot (this PR's branch, 2026-08-03) — not a standing -> guarantee.** Scanner output and the dependency/toolchain state drift as the tree and its -> graph move; re-run the tools listed in `SECURITY_PERF_INVESTIGATION.md` rather than treating -> these results as current. The `file:line` references below are illustrative of where a -> concern was observed on this branch and *will* go stale as code moves — treat them as -> pointers, not as authoritative locations. Specific advisory IDs and toolchain version -> numbers are deliberately kept out of this report so a stale copy can't imply assurance it no -> longer has. -> -> **Open action (not resolved by this PR):** `govulncheck` flagged a *reachable* `crypto/tls` -> advisory that requires a Go toolchain bump. It is out of scope for this two-line hardening -> change and is tracked as an open action (see §4), not covered by the "strong posture" -> summary below. - -## 1. Executive overview - -**Security posture: strong for the reviewed application code, with one open dependency -action (the reachable `crypto/tls` advisory above, unresolved here).** The four -internet-facing webhook adapters (GitHub, GitLab, -Teams, WhatsApp Cloud) are uniformly hardened: full `http.Server` timeout sets, body-size -caps before `io.ReadAll`, cheap method/path rejection, and no internal detail in error -responses. Authentication is adapter-specific: GitHub verifies the body HMAC -(`X-Hub-Signature-256` via go-github's constant-time `ValidateSignature`); GitLab compares -`X-Gitlab-Token` with `subtle.ConstantTimeCompare` before reading the body; WhatsApp Cloud -verifies the body HMAC with `hmac.Equal` (and its GET verify-token handshake with -`subtle.ConstantTimeCompare`); Teams verifies an RS256-signed JWT against the Bot -Connector JWKS (alg-pinned, aud/iss/exp checked) rather than a shared secret. The highest-risk question — whether a forged inbound -Teams Activity can redirect the bot's outbound replies to an attacker host — is **fully -mitigated**: the reply `serviceUrl` is bound to the RS256-signed `serviceurl` claim *and* -constrained to an https Bot-Framework host allowlist, both enforced before the value is -stored or used. No secret is logged anywhere. Two small real gaps were found and fixed -(below); everything else in the plan proved to be a false positive or a documented, -accepted limitation. - -**Performance posture: strong.** Every adapter reuses one pooled `http.Client` -(keep-alives preserved); no client or transport is allocated per `Send`. The dispatch hot -path composes the middleware chain and compiles every command regex **once** (at -connect/registration), not per message; `Message` is passed by pointer with no per-message -deep copies. No unbounded memory growth, with one accepted exception: the Teams -conversation map is FIFO-capped (10k), flow state is TTL-swept, in-flight drain tracking -is a symmetric, panic-safe atomic counter, and the GitHub reaction dedup set is unbounded -in principle but negligible at poller rates (accepted, §3). No benchmarks were added — -the static read found no per-message allocation hotspots, but allocation behavior remains -unmeasured; `BenchmarkDispatch` was judged speculative absent evidence of a problem. - -## 2. Findings - -| ID | Severity | Area | Location | Description | Status | -|----|----------|------|----------|-------------|--------| -| S1 | Low | DoS / consistency | `internal/teams/server.go` (`handleMessages`), `internal/whatsapp/cloud/cloud.go` (`handleWebhook`) | Teams & Cloud lacked the pre-auth read-concurrency semaphore (`readSem`, cap 16) that GitHub & GitLab have — the copied-scaffolding drift CLAUDE.md warns to sweep | **Fixed** | -| S2 | Info | Defense-in-depth | `internal/whatsapp/cloud/cloud.go` (`ResolveAttachmentURL`) | Wire-derived `media.ID` interpolated into the Graph API URL path unescaped | **Fixed** | - -### S1 — pre-auth read-concurrency parity (fixed) - -GitHub and GitLab bound concurrent inbound body reads at `maxConcurrentReads = 16` before -the body is buffered, capping the *synchronous read* window at 16 × the body cap. Teams and -Cloud bounded only *post-auth dispatch* (256), so pre-auth read buffering was limited by -connection count × 1 MiB rather than a hard 16 × 1 MiB. Impact is minor (each read is -already capped at 1 MiB and lifetime-bounded by `ReadTimeout`), but it is a genuine -correctness/consistency gap in the deliberately-duplicated webhook scaffolding. - -Fix: added the same `readSem` acquire/release at the top of `teams.handleMessages` and -`cloud.handleWebhook`. Cloud uses the sibling `defer` release, since its synchronous -portion (HMAC + parse) is fast. Teams deliberately diverges: it releases the slot *before* -`validateInbound`, because that call can block on a cold JWKS fetch — so the 16-slot gate -bounds the read itself, **not** any body still referenced during the JWKS window. This -divergence is documented in-code. Both shed with 503 (their platforms retry), matching each -adapter's existing dispatch-shed response. Sweeps to: all four webhook adapters now hold the -`readSem` + `dispatchSem` shape. - -### S2 — escape wire-derived media ID (fixed) - -`ResolveAttachmentURL` built `{base}/{version}/{media.ID}` with `media.ID` (from the -HMAC-verified webhook body) unescaped. It could not escape the pinned `graph.facebook.com` -host, so this is defense-in-depth only. Fix: `url.PathEscape(media.ID)`, matching the -`url.PathEscape(channelID)` already used on the Teams send path. - -### Non-findings verified (proved false-positive or accepted) - -- **Webhook Slowloris / error-body leaks / body caps / method filtering** — all four - servers set the full timeout set, cap bodies, reject non-POST cheaply, and write only - `WriteHeader` (errors logged, never returned in the response body). -- **Teams serviceUrl SSRF** — `serviceurl` claim from the signed token is compared to the - body value; `isAllowedServiceHost` (`auth.go`) enforces https + Bot-Framework - host allowlist. JWT is alg-pinned to RS256, checks aud/iss/exp with - 5-min leeway, and binds signing keys to the channel. JWKS cache has max-age refresh, - min-refresh-interval (unknown-kid DoS guard) and a stale-fallback ceiling. -- **Secrets & logging** — no config-secret redaction wrapper exists, but no log or format - call anywhere emits a token, secret, signature, private key, phone number or request - body. (`Secret` is a flow-answer marker, not a config wrapper — it never claimed to - protect config.) whatsmeow tightens a pre-existing loose-perm store to 0600 - unconditionally; Signal rejects credentials-in-URL and unwraps parse errors to strip - embedded passwords. -- **Memory growth** — Teams map FIFO-capped at 10k; flow state TTL-swept every minute with - immediate delete on complete/cancel; drain tracking is a deferred atomic counter (runs - during panic unwinding); GitHub reaction dedup set is documented-unbounded but grows one - int64 per *dispatched* reaction at poller rates (accepted). -- **Send / dispatch perf** — shared pooled `http.Client` per adapter; middleware and regex - composed once; no defer-in-loop; webhook dispatch bounded + shed. Signal's dispatch - goroutine is uncapped but serially fed by a single receive socket (accepted). -- **`govulncheck`** — triaged reachable-vs-not: advisories against uncalled symbols - (x/text infinite-loop, slack-go `SecretsVerifier`, os symlink, openpgp) are confirmed - **not called** by our code. Exception: a *reachable* `crypto/tls` advisory requiring a - toolchain bump is **not** mitigated here and is tracked as an open action (§4). Re-run - before relying on this; the advisory set moves. -- **`gosec`** — 5 issues, all pre-existing `//nolint`'d with correct justifications (OAuth - scope URL, header name, plain-text challenge echo, adapter-owned file paths). No new - issues introduced by the fixes. -- **ReDoS** — Go's RE2 has no catastrophic backtracking; no third-party regex engine in the - graph. Off the table. - -## 3. Accepted risks (stated, not fixed) - -- **GitHub reaction dedup set** unbounded in principle; negligible at poller rates and - documented in-code. Upgrade path: LRU keyed on the connect cutoff, YAGNI today. -- **Flow-secret durable-store exclusion** is aspirational — no `Store` implementation - exists yet, so nothing durably persists secrets; harmless until one is added. -- **Signal dispatch** is uncapped but bounded by a single serial receive socket. -- **Other-bot filter gaps** — every platform drops the bot's *own* messages (self filters - verified on all ten adapters); what cannot be filtered is *other* bots where the - platform carries no bot marker (Slack `reaction_added`, GitLab notes, Signal, WhatsApp). - Documented platform limitations; consumers must be idempotent and rate-limit. - -## 4. Prioritized improvements - -**Done (this PR):** S1 (readSem parity), S2 (media.ID escape). - -**Open actions (out of scope for this PR — dependency/toolchain, not code):** -- **Reachable `crypto/tls` advisory (toolchain bump) — unresolved.** `govulncheck` flagged a - reachable advisory in `crypto/tls` that needs a Go toolchain upgrade. This PR does not - address it; it must be tracked and fixed separately rather than assumed clean. -- Run `govulncheck` and a toolchain/dependency sweep at review time and act on whatever else - it reports then. Specific advisory IDs and toolchain versions are intentionally omitted here - so this report can't drift into wrong as the graph moves. - -**Structural (only if evidence demands):** LRU/TTL on the GitHub reaction dedup set; -a `readSem`-style cap on Signal dispatch. Neither is warranted at v1 loads.