Harden Teams & WhatsApp Cloud webhook adapters (security audit) - #48
Conversation
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.
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.
|
✅ No issues found.
|
![botbooter-test[bot]](https://avatars.githubusercontent.com/u/197033?s=60&v=4){kind=small}
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: approve · Grade: 9/10
To reach a higher grade: Drop or relocate the internal audit markdown files and rename the shadowing local url in postMessage; the code fixes themselves are already merge-ready.
📝 Summary of changes
This PR is a security/performance audit deliverable: two new root-level markdown documents (SECURITY_PERF_INVESTIGATION.md, SECURITY_PERF_REPORT.md) plus two small hardening fixes to the Teams and WhatsApp Cloud webhook adapters, with accompanying tests.
Code changes reviewed:
-
readSempre-auth read-concurrency parity (S1). Teams and WhatsApp Cloud gain themaxConcurrentReads = 16counting semaphore that GitHub/GitLab already have, allocated once innewAdapterand acquired/released (viadefer) around the body read in the handler. This matches the sibling adapters' shape and the CLAUDE.md guidance to sweep copied scaffolding. The slot is held only for the synchronous portion of the handler and released before the async dispatch goroutine, so allocating it once (rather than per-connection likedispatchSem) is safe — there's no cross-reconnect leak. A nilreadSemwould send-to-nil-channel and hit thedefault(503) branch, so construction correctness matters; the publicNew→newAdapterpath and the cloudtestAdapterare both updated, and the shed paths are covered by new tests. -
url.PathEscape(media.ID)incloud.ResolveAttachmentURL(S2). Defense-in-depth escaping of the wire-derived media ID; the localurlvariable was correctly renamed toendpointto avoid clashing with the newly-importednet/url. Well-tested with an escaping assertion.
Both fixes are correct, minimal, consistent with existing conventions, and use the in-repo asserts test style. I verified the escaping expectation (a/b?c → a%2Fb%3Fc) and the 503 shed-path logic.
Minor observations (not blocking): The postMessage function still has a local variable named url that now shadows the newly-imported net/url package; it compiles and is harmless today but is a small latent trap if that function ever needs url.*. Separately, whether large internal audit plan/report markdown files belong committed at the repo root is a team judgment call — they are accurate as far as I can corroborate against the retrieved context, but they are meta-documents rather than user-facing docs.
Overall a clean, correct, well-scoped hardening change with matching tests.
✅ No issues found.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change limits pre-authentication webhook reads for Teams and WhatsApp Cloud to 16 concurrent requests. Saturated requests return HTTP 503. WhatsApp Cloud also escapes media IDs in Graph API paths. ChangesWebhook concurrency and path handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookRequest
participant WebhookHandler
participant ReadSemaphore
participant Dispatcher
WebhookRequest->>WebhookHandler: Submit webhook request
WebhookHandler->>ReadSemaphore: Try acquire read slot
alt Slot available
ReadSemaphore-->>WebhookHandler: Grant slot
WebhookHandler->>WebhookHandler: Read and unmarshal body
WebhookHandler->>ReadSemaphore: Release slot
WebhookHandler->>Dispatcher: Authenticate and dispatch message
else Slots saturated
ReadSemaphore-->>WebhookHandler: Reject acquisition
WebhookHandler-->>WebhookRequest: Return HTTP 503
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
internal/teams/server_test.go (1)
272-288: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAssert that the saturated path does not read the request body.
The test checks the 503 response and dispatch count, but it does not observe
r.Body. A regression that buffers and unmarshals the body before checkingreadSemwould still pass. Use a trackingio.ReadCloserand assert that no read occurs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/teams/server_test.go` around lines 272 - 288, Update TestHandleMessages_ReadSaturationReturns503 to replace the request body with a tracking io.ReadCloser that records read attempts, while keeping the read semaphore saturated. After invoking post, assert the response remains 503, nothing is dispatched, and the tracking body reports zero reads.SECURITY_PERF_REPORT.md (1)
27-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDo not infer performance evidence from the absence of benchmarks.
Line 27 says no hot-path allocation problem exists because no benchmark was added. The investigation plan requires
BenchmarkDispatchat Lines 115-116. Add the benchmark, or state that allocation behavior remains unmeasured.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SECURITY_PERF_REPORT.md` around lines 27 - 28, Update SECURITY_PERF_REPORT.md to remove the unsupported conclusion that no hot-path allocation problem exists because benchmarks are absent. Add the required BenchmarkDispatch investigation referenced later in the report, or explicitly state that allocation behavior remains unmeasured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/whatsapp/cloud/cloud_test.go`:
- Around line 487-504: The TestHandleWebhook_ReadSaturationReturns503 test must
verify that the saturated handler never reads the request body. Replace the
request’s strings.Reader body with a read-spy implementing Read, invoke
handleWebhook, then assert the spy recorded zero Read calls while preserving the
existing 503 and no-dispatch assertions.
In `@SECURITY_PERF_INVESTIGATION.md`:
- Around line 88-90: Remove Signal and WhatsApp from the self-message filtering
gaps and accepted-risk lists in SECURITY_PERF_INVESTIGATION.md (lines 88-90) and
SECURITY_PERF_REPORT.md (lines 97-99). Retain only concrete, documented WhatsApp
self-loop issues if one exists; do not classify Signal as a gap, since its
existing source checks and lack of a bot-user concept provide the required
filtering.
In `@SECURITY_PERF_REPORT.md`:
- Around line 25-28: Update the memory-growth conclusion in
SECURITY_PERF_REPORT.md to qualify the “No unbounded memory growth” statement
with the accepted exception that the GitHub reaction deduplication set is
unbounded in principle, while preserving the existing claims about the
FIFO-capped Teams conversation map, TTL-swept flow state, and in-flight drain
tracking.
- Around line 10-17: Update the “Security posture: strong” paragraph in
SECURITY_PERF_REPORT.md to describe authentication controls separately for
GitHub, GitLab, Teams, and WhatsApp Cloud: identify Teams as using RS256/JWKS
JWT verification, and specify the applicable HMAC or token comparison control
for each remaining adapter instead of attributing constant-time secret
comparisons to all four.
- Around line 34-35: Update the S1 and S2 entries in SECURITY_PERF_REPORT.md to
use precise file:line references from the final source files: add line numbers
for both S1 files and correct the S2 cloud.go location to the actual
ResolveAttachmentURL lines. Ensure the locations match the investigation plan’s
required format at Line 171.
- Line 36: Update the relevant Go toolchain references in
SECURITY_PERF_REPORT.md to use exact versions: state that GO-2026-5856 is fixed
in Go 1.25.12 and Go 1.26.5, and identify the pinned toolchain as go1.25.11
rather than an unspecified go1.25.x version.
---
Nitpick comments:
In `@internal/teams/server_test.go`:
- Around line 272-288: Update TestHandleMessages_ReadSaturationReturns503 to
replace the request body with a tracking io.ReadCloser that records read
attempts, while keeping the read semaphore saturated. After invoking post,
assert the response remains 503, nothing is dispatched, and the tracking body
reports zero reads.
In `@SECURITY_PERF_REPORT.md`:
- Around line 27-28: Update SECURITY_PERF_REPORT.md to remove the unsupported
conclusion that no hot-path allocation problem exists because benchmarks are
absent. Add the required BenchmarkDispatch investigation referenced later in the
report, or explicitly state that allocation behavior remains unmeasured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31c117a6-249b-42f6-a7a2-c10909f4621f
📒 Files selected for processing (7)
SECURITY_PERF_INVESTIGATION.mdSECURITY_PERF_REPORT.mdinternal/teams/server.gointernal/teams/server_test.gointernal/teams/teams.gointernal/whatsapp/cloud/cloud.gointernal/whatsapp/cloud/cloud_test.go
|
/loop |
|
🔁 Review-fix loop started — target 9/10, up to 3 fix round(s). |
|
✅ Review posted — 1 finding(s).
|
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: comment · Grade: 8/10
To reach a higher grade: Drop or relocate the aging point-in-time security report doc (keep the two code fixes, which are clean) to reach 9-10.
📝 Summary of changes
This PR is a security-hardening change plus two audit markdown docs. The code changes are small and well-scoped:
- Teams & WhatsApp Cloud
readSem— adds amaxConcurrentReads = 16counting semaphore acquired non-blocking at the top ofhandleMessages/handleWebhook, shedding with 503 before buffering the body. This mirrors the existing GitHub/GitLab scaffolding and correctly addresses the copied-scaffolding drift CLAUDE.md warns about. The semaphore is allocated once innewAdapterand never reassigned, so the unlocked read is safe, and thedeferrelease is synchronous within the handler so there is no leak-across-reconnect concern (unlikedispatchSem). The nil-channelselect/defaultbehavior fails safe (would 503) but all constructors set the field. url.PathEscape(media.ID)incloud.ResolveAttachmentURL— escapes the wire-derived media id in the Graph path (defense-in-depth; cannot escape the pinned host). Local var renamed toendpointto avoid shadowing the newnet/urlimport;postMessage's localurlshadow is harmless.
Tests are thorough and match repo conventions (internal/asserts, readSpy proving the shed body is never read, a PathEscape guard test). make all claimed green.
Code is correct and clean. One note not raised as a finding: in Teams the readSem is held for the whole handler, including any cold JWKS network fetch, so it bounds slightly more than the stated "pre-auth read memory" — this is extra protection rather than a defect, but it diverges from the sibling intent of releasing right after the body read.
The main reservation is the two committed audit documents (SECURITY_PERF_INVESTIGATION.md, SECURITY_PERF_REPORT.md): a point-in-time security posture report with specific, aging claims (govulncheck IDs, go1.25.11 → go1.25.12 toolchain deltas, go.mod:7 referencing stdlib crypto/tls which is not in go.mod) that will bit-rot and could give false assurance. That's a maintainability/scope concern, not a code bug.
🤖 AI prompt to fix all 1 finding(s) (review before running)
Fix 1 issue(s) found during code review of lao/botbooter (PR #48).
--- Issue 1 ---
File: SECURITY_PERF_REPORT.md:8 (side RIGHT)
Severity: low
Issue: Point-in-time security posture report committed to the repo will bit-rot and can mislead
Committing `SECURITY_PERF_REPORT.md` (and the companion plan) bakes a snapshot into the repo that asserts "Security posture: strong" alongside specific, time-sensitive claims — govulncheck IDs, `go1.25.11 → go1.25.12`/`go1.26.5` toolchain deltas, and `go.mod:7` for a stdlib `crypto/tls` finding (stdlib is not listed in go.mod). These will silently go stale as code and dependencies move, and a stale report is worse than none because it implies current assurance.
The actual code fixes (S1 `readSem`, S2 PathEscape) are self-documenting via their in-code comments and tests. Consider keeping this narrative in the PR description / an issue / a wiki rather than the repo root, or at minimum pin it to a commit/date and strip the unverifiable toolchain and `go.mod` line references so it can't drift into wrong. Uncertain how the team wants audit artifacts handled — flagging as a maintainability concern, not blocking the code.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
| every path in the plan, automated scanners (`govulncheck`, `gosec`, `golangci-lint`, | ||
| `go vet`, `go test -race`), and cross-adapter consistency checks. | ||
|
|
||
| ## 1. Executive overview |
There was a problem hiding this comment.
[low] Point-in-time security posture report committed to the repo will bit-rot and can mislead
Committing SECURITY_PERF_REPORT.md (and the companion plan) bakes a snapshot into the repo that asserts "Security posture: strong" alongside specific, time-sensitive claims — govulncheck IDs, go1.25.11 → go1.25.12/go1.26.5 toolchain deltas, and go.mod:7 for a stdlib crypto/tls finding (stdlib is not listed in go.mod). These will silently go stale as code and dependencies move, and a stale report is worse than none because it implies current assurance.
The actual code fixes (S1 readSem, S2 PathEscape) are self-documenting via their in-code comments and tests. Consider keeping this narrative in the PR description / an issue / a wiki rather than the repo root, or at minimum pin it to a commit/date and strip the unverifiable toolchain and go.mod line references so it can't drift into wrong. Uncertain how the team wants audit artifacts handled — flagging as a maintainability concern, not blocking the code.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #48).
File: SECURITY_PERF_REPORT.md:8 (side RIGHT)
Severity: low
Issue: Point-in-time security posture report committed to the repo will bit-rot and can mislead
Committing `SECURITY_PERF_REPORT.md` (and the companion plan) bakes a snapshot into the repo that asserts "Security posture: strong" alongside specific, time-sensitive claims — govulncheck IDs, `go1.25.11 → go1.25.12`/`go1.26.5` toolchain deltas, and `go.mod:7` for a stdlib `crypto/tls` finding (stdlib is not listed in go.mod). These will silently go stale as code and dependencies move, and a stale report is worse than none because it implies current assurance.
The actual code fixes (S1 `readSem`, S2 PathEscape) are self-documenting via their in-code comments and tests. Consider keeping this narrative in the PR description / an issue / a wiki rather than the repo root, or at minimum pin it to a commit/date and strip the unverifiable toolchain and `go.mod` line references so it can't drift into wrong. Uncertain how the team wants audit artifacts handled — flagging as a maintainability concern, not blocking the code.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
|
✅ Review posted — 2 finding(s).
|
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: comment · Grade: 8/10
To reach a higher grade: Reconsider committing the stale-prone audit docs into the main tree (or scope them out), and confirm the Teams readSem hold-duration divergence is acceptable.
📝 Summary of changes
The PR adds two small, well-targeted hardening fixes to the webhook adapters plus two markdown audit documents (a plan and a report).
Code changes (clean):
- S1 — adds a pre-auth
readSem(cap 16) tointernal/teamsandinternal/whatsapp/cloudhandleMessages/handleWebhook, matching the existing GitHub/GitLab siblings. The acquire is non-blocking (sheds 503) and releases viadefer;readSemis allocated once innewAdapterand never reassigned, so the lock-free access is safe. No deadlock risk (dispatchSem acquire is also non-blocking). - S2 —
url.PathEscapeon the wire-derivedmedia.IDincloud.ResolveAttachmentURL, matching the Teams send path. Test verifies escaping.
Both fixes are correct, consistent with codebase conventions, and covered by mirror tests (readSpy proving the body is never read on shed, PathEscape guard).
Docs: Two markdown files (SECURITY_PERF_INVESTIGATION.md, SECURITY_PERF_REPORT.md) are added. They make sweeping posture claims and cite specific file:line locations that will drift as the tree moves; the report itself acknowledges staleness. Also notable: the PR description states a reachable crypto/tls advisory requiring a toolchain bump was found but is not addressed here — worth tracking separately.
Minor nuance: in Teams the readSem slot is held through validateInbound (JWT verification + possible cold JWKS fetch, up to ~15s) rather than just a fast HMAC as in GitHub/GitLab, so the reused constant 16 gates throughput more aggressively under a JWKS refresh. Safe (503 retry), but a real divergence in hold duration from the siblings it mirrors.
Overall the code is correct and low-risk; the reservations are about bundling a stale-prone audit report into the tree and the slightly different readSem semantics for Teams.
🤖 AI prompt to fix all 2 finding(s) (review before running)
Fix 2 issue(s) found during code review of lao/botbooter (PR #48).
--- Issue 1 ---
File: internal/teams/server.go:118 (side RIGHT)
Severity: low
Issue: Teams holds readSem through JWT/JWKS validation, unlike the HMAC-fast siblings it copies
The `readSem` slot is acquired at the top of `handleMessages` and released only when the function returns (`defer func() { <-a.readSem }()`), so it is held across `validateInbound` — RS256 verification plus a potential *cold* JWKS fetch bounded by `jwksFetchTimeout` (~15s). In the GitHub/GitLab siblings this constant (`maxConcurrentReads = 16`) gates only a fast HMAC/token compare, so a slot frees almost immediately. In Teams, under a legitimate burst that coincides with a JWKS refresh, up to 16 slots can stay occupied for seconds, shedding the 17th+ authentic request with 503. The behavior is safe (the platform retries) but the reused `16` doesn't account for the much longer hold, so this is a genuine divergence from the scaffolding it claims to mirror. Consider either releasing `readSem` right after the body read/unmarshal (before `validateInbound`) or documenting/sizing the constant for the JWT path. Marking low/uncertain — it only bites during a cold JWKS fetch under concurrent load.
--- Issue 2 ---
File: SECURITY_PERF_REPORT.md:1 (side RIGHT)
Severity: low
Issue: Point-in-time audit report/plan committed to the main tree will drift into misleading assurance
`SECURITY_PERF_REPORT.md` (and the 178-line `SECURITY_PERF_INVESTIGATION.md`) assert a "strong" security posture and cite concrete `file:line` locations (e.g. `internal/teams/server.go:109-121`, `auth.go:108-116`) that will silently go wrong as the code moves — the report itself flags this staleness risk. Committing a dated audit snapshot into the source tree tends to become a maintenance liability and an implied guarantee it can't keep. Consider keeping these under a clearly-dated/archived location (or out of the repo), and separately: the PR description notes a *reachable* `crypto/tls` advisory needing a toolchain bump that this PR does not address — that should be tracked as an open action rather than folded into a 'strong posture' summary. Low severity; not a correctness bug, but a meaningful maintainability/scope concern for a two-line hardening PR.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
| select { | ||
| case a.readSem <- struct{}{}: | ||
| default: | ||
| a.log().Warn("teams: inbound concurrency limit reached; shedding with 503", "limit", maxConcurrentReads) |
There was a problem hiding this comment.
[low] Teams holds readSem through JWT/JWKS validation, unlike the HMAC-fast siblings it copies
The readSem slot is acquired at the top of handleMessages and released only when the function returns (defer func() { <-a.readSem }()), so it is held across validateInbound — RS256 verification plus a potential cold JWKS fetch bounded by jwksFetchTimeout (~15s). In the GitHub/GitLab siblings this constant (maxConcurrentReads = 16) gates only a fast HMAC/token compare, so a slot frees almost immediately. In Teams, under a legitimate burst that coincides with a JWKS refresh, up to 16 slots can stay occupied for seconds, shedding the 17th+ authentic request with 503. The behavior is safe (the platform retries) but the reused 16 doesn't account for the much longer hold, so this is a genuine divergence from the scaffolding it claims to mirror. Consider either releasing readSem right after the body read/unmarshal (before validateInbound) or documenting/sizing the constant for the JWT path. Marking low/uncertain — it only bites during a cold JWKS fetch under concurrent load.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #48).
File: internal/teams/server.go:118 (side RIGHT)
Severity: low
Issue: Teams holds readSem through JWT/JWKS validation, unlike the HMAC-fast siblings it copies
The `readSem` slot is acquired at the top of `handleMessages` and released only when the function returns (`defer func() { <-a.readSem }()`), so it is held across `validateInbound` — RS256 verification plus a potential *cold* JWKS fetch bounded by `jwksFetchTimeout` (~15s). In the GitHub/GitLab siblings this constant (`maxConcurrentReads = 16`) gates only a fast HMAC/token compare, so a slot frees almost immediately. In Teams, under a legitimate burst that coincides with a JWKS refresh, up to 16 slots can stay occupied for seconds, shedding the 17th+ authentic request with 503. The behavior is safe (the platform retries) but the reused `16` doesn't account for the much longer hold, so this is a genuine divergence from the scaffolding it claims to mirror. Consider either releasing `readSem` right after the body read/unmarshal (before `validateInbound`) or documenting/sizing the constant for the JWT path. Marking low/uncertain — it only bites during a cold JWKS fetch under concurrent load.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
| @@ -0,0 +1,125 @@ | |||
| # Security & Performance Report | |||
There was a problem hiding this comment.
[low] Point-in-time audit report/plan committed to the main tree will drift into misleading assurance
SECURITY_PERF_REPORT.md (and the 178-line SECURITY_PERF_INVESTIGATION.md) assert a "strong" security posture and cite concrete file:line locations (e.g. internal/teams/server.go:109-121, auth.go:108-116) that will silently go wrong as the code moves — the report itself flags this staleness risk. Committing a dated audit snapshot into the source tree tends to become a maintenance liability and an implied guarantee it can't keep. Consider keeping these under a clearly-dated/archived location (or out of the repo), and separately: the PR description notes a reachable crypto/tls advisory needing a toolchain bump that this PR does not address — that should be tracked as an open action rather than folded into a 'strong posture' summary. Low severity; not a correctness bug, but a meaningful maintainability/scope concern for a two-line hardening PR.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #48).
File: SECURITY_PERF_REPORT.md:1 (side RIGHT)
Severity: low
Issue: Point-in-time audit report/plan committed to the main tree will drift into misleading assurance
`SECURITY_PERF_REPORT.md` (and the 178-line `SECURITY_PERF_INVESTIGATION.md`) assert a "strong" security posture and cite concrete `file:line` locations (e.g. `internal/teams/server.go:109-121`, `auth.go:108-116`) that will silently go wrong as the code moves — the report itself flags this staleness risk. Committing a dated audit snapshot into the source tree tends to become a maintenance liability and an implied guarantee it can't keep. Consider keeping these under a clearly-dated/archived location (or out of the repo), and separately: the PR description notes a *reachable* `crypto/tls` advisory needing a toolchain bump that this PR does not address — that should be tracked as an open action rather than folded into a 'strong posture' summary. Low severity; not a correctness bug, but a meaningful maintainability/scope concern for a two-line hardening PR.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
|
✅ Review posted — 1 finding(s).
|
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: comment · Grade: 8/10
📝 Summary of changes
This PR is a security/performance audit that lands two small hardening fixes plus two audit documents.
Code changes (clean and correct):
- S1 —
readSemparity: Adds a pre-auth read-concurrency semaphore (cap 16) to the Teams and WhatsApp Cloud webhook handlers, matching the GitHub/GitLab siblings. Teams releases the slot manually on all three exit paths (read error, unmarshal error, success) and deliberately releases beforevalidateInboundbecause that call can block on a cold JWKS fetch — a well-reasoned divergence from the siblings, correctly documented in-code. WhatsApp Cloud usesdefer func(){ <-a.readSem }(), which is safe because the synchronous portion (HMAC + parse) is fast. I traced every acquire/release path; each acquisition is released exactly once, and the shed path returns 503 before touching the body. Both are backed by focusedreadSpytests proving the body is never read on the shed path. - S2 —
url.PathEscape(media.ID): Escapes a wire-derived media id before interpolating it into the Graph API path. Defense-in-depth (the pinned host cannot be escaped), with a matching test.
The code follows the existing per-adapter conventions (fresh dispatchSem per connection; readSem allocated once in newAdapter like the siblings) and adds proportionate tests. No correctness, concurrency, or API-misuse problems found in the code.
Documentation: Two markdown files (SECURITY_PERF_INVESTIGATION.md, SECURITY_PERF_REPORT.md) are added to the repo root. These are point-in-time process artifacts with self-admittedly-stale file:line references and posture claims; my only reservation is about committing them into the library tree (see finding).
Overall a tidy, well-scoped hardening change consistent with the codebase's deliberately-duplicated webhook scaffolding.
🤖 AI prompt to fix all 1 finding(s) (review before running)
Fix 1 issue(s) found during code review of lao/botbooter (PR #48).
--- Issue 1 ---
File: SECURITY_PERF_REPORT.md:38 (side RIGHT)
Severity: low
Issue: Point-in-time audit artifacts committed to the library repo root will rot
`SECURITY_PERF_REPORT.md` (and the companion investigation plan) are process/audit artifacts, not library documentation. They embed hardcoded `file:line` references (e.g. `internal/teams/server.go:109-121`, `internal/whatsapp/cloud/cloud.go:527-529`) and "as landed" location claims that the report *itself* warns "*will* go stale as code moves." Committing a stale-by-design snapshot into the tree root is a maintainability cost: future readers can't tell whether the "strong posture" summary still holds, and the line anchors drift on the next edit.
Concrete suggestion: keep these in the PR description / an issue / a `docs/audits/` folder dated and clearly marked archival, or drop the volatile line-number anchors and keep only the durable findings (S1/S2 rationale, accepted risks). Note too that the report's S1 phrasing ("capping peak pre-auth read memory at maxConcurrentReads times maxRequestBytes") slightly overstates the Teams case — Teams releases `readSem` *before* `validateInbound`, so bodies referenced during the JWKS window are not bounded by the 16-slot gate. The in-code comment is accurate; the report is the thing that overpromises.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
| 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 |
There was a problem hiding this comment.
[low] Point-in-time audit artifacts committed to the library repo root will rot
SECURITY_PERF_REPORT.md (and the companion investigation plan) are process/audit artifacts, not library documentation. They embed hardcoded file:line references (e.g. internal/teams/server.go:109-121, internal/whatsapp/cloud/cloud.go:527-529) and "as landed" location claims that the report itself warns "will go stale as code moves." Committing a stale-by-design snapshot into the tree root is a maintainability cost: future readers can't tell whether the "strong posture" summary still holds, and the line anchors drift on the next edit.
Concrete suggestion: keep these in the PR description / an issue / a docs/audits/ folder dated and clearly marked archival, or drop the volatile line-number anchors and keep only the durable findings (S1/S2 rationale, accepted risks). Note too that the report's S1 phrasing ("capping peak pre-auth read memory at maxConcurrentReads times maxRequestBytes") slightly overstates the Teams case — Teams releases readSem before validateInbound, so bodies referenced during the JWKS window are not bounded by the 16-slot gate. The in-code comment is accurate; the report is the thing that overpromises.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #48).
File: SECURITY_PERF_REPORT.md:38 (side RIGHT)
Severity: low
Issue: Point-in-time audit artifacts committed to the library repo root will rot
`SECURITY_PERF_REPORT.md` (and the companion investigation plan) are process/audit artifacts, not library documentation. They embed hardcoded `file:line` references (e.g. `internal/teams/server.go:109-121`, `internal/whatsapp/cloud/cloud.go:527-529`) and "as landed" location claims that the report *itself* warns "*will* go stale as code moves." Committing a stale-by-design snapshot into the tree root is a maintainability cost: future readers can't tell whether the "strong posture" summary still holds, and the line anchors drift on the next edit.
Concrete suggestion: keep these in the PR description / an issue / a `docs/audits/` folder dated and clearly marked archival, or drop the volatile line-number anchors and keep only the durable findings (S1/S2 rationale, accepted risks). Note too that the report's S1 phrasing ("capping peak pre-auth read memory at maxConcurrentReads times maxRequestBytes") slightly overstates the Teams case — Teams releases `readSem` *before* `validateInbound`, so bodies referenced during the JWKS window are not bounded by the 16-slot gate. The in-code comment is accurate; the report is the thing that overpromises.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
|
✅ Review posted — 1 finding(s).
|
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: comment · Grade: 8/10
To reach a higher grade: Keep the point-in-time audit report/plan out of the committed tree (or move to a wiki/issue) so the codebase doesn't carry self-declared, stale-by-design security assurances.
📝 Summary of changes
This PR does a security/performance audit and lands two small, well-scoped hardening changes plus two documentation files.
Code changes (correct and well-tested):
- Adds a pre-auth
readSem(cap 16) to the Teams and WhatsApp Cloud webhook handlers, bringing them in line with the GitHub/GitLab siblings' scaffolding. Teams releases the slot early (beforevalidateInbound, since a cold JWKS fetch can block) with a documented rationale; Cloud uses adeferrelease across its fast HMAC+parse path. Both shed with 503. Release paths are correct — each Teams return path releases exactly once, and Cloud's singledefercovers all returns. The reuse of a singlereadSemacross reconnects is sound because it is released synchronously inside the handler (never held by a dispatch goroutine). - Adds
url.PathEscapeon the wire-derivedmedia.IDincloud.ResolveAttachmentURL(defense-in-depth; the pinned host is unreachable regardless). - New tests (
readSpy, 503-shed, PathEscape) are focused and use the repo'sassertsconventions.
The code diff is clean; I found no correctness, concurrency, or security defects in it.
Documentation files: two audit artifacts (SECURITY_PERF_INVESTIGATION.md, SECURITY_PERF_REPORT.md) are committed to the repo root. The report is well-caveated as a point-in-time snapshot but still bakes self-assessed "strong posture" claims and deliberately-stale file:line references into the tree, and records an unresolved reachable crypto/tls advisory. That is a maintainability/noise judgment call rather than a code defect.
🤖 AI prompt to fix all 1 finding(s) (review before running)
Fix 1 issue(s) found during code review of lao/botbooter (PR #48).
--- Issue 1 ---
File: SECURITY_PERF_REPORT.md:23 (side RIGHT)
Severity: low
Issue: Committing a point-in-time security audit report into the repo root is a maintainability liability
The report is honestly caveated as an archived snapshot with stale-prone `file:line` references, but it still persists concrete assurances ("Security posture: strong", "fully mitigated", a findings table of "Fixed" items) plus an *unresolved* reachable `crypto/tls` advisory directly into the source tree. Once merged, these claims outlive the code they describe: a future reader lands on "strong posture" without knowing whether it still holds, and the intentionally-stale line references will point at unrelated code as the tree moves.
Consider moving both `SECURITY_PERF_INVESTIGATION.md` and `SECURITY_PERF_REPORT.md` out of the committed tree (a wiki page, a tracking issue, or PR description) and keeping only the actual code hardening in the PR. If the team does want an audit trail in-repo, at minimum the unresolved reachable `crypto/tls` advisory deserves a real tracked issue rather than a prose "open action" that will be forgotten.
This is a judgment/maintainability note, not a code defect — the two code changes themselves are clean.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
| > summary below. | ||
|
|
||
| ## 1. Executive overview | ||
|
|
There was a problem hiding this comment.
[low] Committing a point-in-time security audit report into the repo root is a maintainability liability
The report is honestly caveated as an archived snapshot with stale-prone file:line references, but it still persists concrete assurances ("Security posture: strong", "fully mitigated", a findings table of "Fixed" items) plus an unresolved reachable crypto/tls advisory directly into the source tree. Once merged, these claims outlive the code they describe: a future reader lands on "strong posture" without knowing whether it still holds, and the intentionally-stale line references will point at unrelated code as the tree moves.
Consider moving both SECURITY_PERF_INVESTIGATION.md and SECURITY_PERF_REPORT.md out of the committed tree (a wiki page, a tracking issue, or PR description) and keeping only the actual code hardening in the PR. If the team does want an audit trail in-repo, at minimum the unresolved reachable crypto/tls advisory deserves a real tracked issue rather than a prose "open action" that will be forgotten.
This is a judgment/maintainability note, not a code defect — the two code changes themselves are clean.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #48).
File: SECURITY_PERF_REPORT.md:23 (side RIGHT)
Severity: low
Issue: Committing a point-in-time security audit report into the repo root is a maintainability liability
The report is honestly caveated as an archived snapshot with stale-prone `file:line` references, but it still persists concrete assurances ("Security posture: strong", "fully mitigated", a findings table of "Fixed" items) plus an *unresolved* reachable `crypto/tls` advisory directly into the source tree. Once merged, these claims outlive the code they describe: a future reader lands on "strong posture" without knowing whether it still holds, and the intentionally-stale line references will point at unrelated code as the tree moves.
Consider moving both `SECURITY_PERF_INVESTIGATION.md` and `SECURITY_PERF_REPORT.md` out of the committed tree (a wiki page, a tracking issue, or PR description) and keeping only the actual code hardening in the PR. If the team does want an audit trail in-repo, at minimum the unresolved reachable `crypto/tls` advisory deserves a real tracked issue rather than a prose "open action" that will be forgotten.
This is a judgment/maintainability note, not a code defect — the two code changes themselves are clean.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
|
🔁 Review-fix loop finished — target 9/10.
|
|
/prompt remove files SECURITY_PERF_INVESTIGATION.md and SECURITY_PERF_REPORT.md |
|
ℹ️ Nothing to change — the request needed no edits, or couldn't be done by editing files.
|
|
/review |
|
✅ No issues found.
|
There was a problem hiding this comment.
Verdict: approve · Grade: 9/10
To reach a higher grade: Consider a guarded defer for the Teams readSem release to remove the manual multi-path release fragility, though the current code is correct.
📝 Summary of changes
This PR closes two small, well-scoped hardening gaps identified in a security/perf audit:
- Pre-auth read-concurrency parity (
readSem) — Teams and WhatsApp Cloud webhook handlers now bound concurrent inbound body reads (cap 16) before buffering/unmarshaling, matching the GitHub/GitLab siblings. Saturation sheds with 503, which both platforms retry. - Media ID escaping —
cloud.ResolveAttachmentURLnowurl.PathEscapes the wire-derivedmedia.IDbefore interpolating it into the Graph API path (defense-in-depth; the host is already pinned).
The implementation is careful and internally consistent:
readSemis allocated once innewAdapterand never reassigned, so the lock-free direct access is correct — deliberately unlikedispatchSem, which is recreated perConnectand therefore captured undera.mu.- Release accounting is exact: in Teams each of the three post-acquire paths (read error, unmarshal error, success) releases exactly once, and the 503 path returns before acquiring. WhatsApp uses
defersince it holds the slot for the whole handler. - The early-release comment in Teams (releasing before
validateInbound, which can block on a cold JWKS fetch up tojwksFetchTimeout) reflects a genuine, well-reasoned difference from the fast-HMAC siblings. - Tests cover both new gates (503 shed, body never read via a
readSpy) and the PathEscape behavior.
Minor optional observation (not blocking): the Teams handler releases readSem via three manual <-a.readSem statements rather than a guarded defer, since it releases early. It is correct as written, but slightly more fragile if a future edit inserts an early return between acquire and the final release. This is a style/maintainability nit, not a defect.
No correctness, security, or concurrency issues found. The change matches the codebase's existing conventions well.
✅ No issues found.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
1 participant
Security & performance audit of botbooter per
SECURITY_PERF_INVESTIGATION.md. Full report inSECURITY_PERF_REPORT.md.Outcome
The codebase is already strongly hardened. Across the whole plan — webhook auth, the Teams
serviceUrlSSRF surface, secret/log handling, memory growth, send/dispatch perf, and the dependency graph — nearly every item proved a false positive or a documented accepted risk. Two small real gaps were found and fixed.Fixes
S1 — pre-auth read-concurrency parity (low / DoS). GitHub and GitLab bound concurrent inbound body reads at
readSem(cap 16) before buffering; Teams and WhatsApp Cloud did not — the copied-scaffolding drift CLAUDE.md warns to sweep. Added the identicalreadSemacquire/release to both handlers; they shed with 503 (platforms retry). All four webhook adapters now share the same shape.S2 — escape wire-derived media ID (info / defense-in-depth).
cloud.ResolveAttachmentURLinterpolated the webhook-suppliedmedia.IDinto the Graph API path unescaped. Addedurl.PathEscape, mirroring the Teams send path. Cannot escape the pinned host regardless; hardening only.Verified non-findings (proof in report)
serviceUrlis bound to the RS256-signedserviceurlclaim and an https Bot-Framework host allowlist — a forged Activity cannot redirect replies.http.Client; middleware/regex composed once.govulncheck: 4 of 5 not reachable; the 5th (crypto/tls) is a toolchain bump.gosec: 5 pre-existing justified nolints, no new issues.Tests
make allgreen (fmt + vet + golangci-lint +go test -race+ examples). Added 503-shed tests for both newreadSemgates and a PathEscape guard test.Summary by CodeRabbit