Limit webhook request body size with http.MaxBytesReader

[nevyangelova]

Summary

Add http.MaxBytesReader to the webhook handler to cap incoming request bodies at 25 MB (matching GitHub's documented maximum webhook payload size). This ensures oversized requests are rejected immediately with HTTP 413 before being read into memory, regardless of any upstream transport-layer protections.

This is a defense-in-depth hardening measure — the Mattermost plugin IPC layer already prevents exploitation, but the code should not rely on that external protection.

Ticket Link

https://mattermost.atlassian.net/browse/MM-68166

[coderabbitai]

📝 Walkthrough

Walkthrough

Added a 25 MB size limit to webhook request bodies in the handler using http.MaxBytesReader. When the request exceeds this limit, the handler now returns a 413 Request Entity Too Large status with an appropriate error message. Corresponding test cases were added to verify both size limit enforcement and normal operation.

Changes

Cohort / File(s)	Summary
Webhook Size Limit Implementation server/plugin/webhook.go	Added maxWebhookPayloadSize constant (25 MB) and wrapped r.Body with http.MaxBytesReader before reading. Enhanced error handling to return 413 status for oversized requests and 400 for other read failures.
Webhook Size Limit Tests server/plugin/webhook_test.go	Added TestHandleWebhookBodySizeLimit test function with two subtests: one verifying rejection of 26 MB payloads with 413 status, and another confirming normal-sized requests proceed to authentication checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

3: Security Review

Suggested reviewers

• avasconcelos114
• ogi-m
• edgarbellot

Poem

🐰 A webhook walks into a size check,
Twenty-five megabytes? No sweat, no wreck!
Too large it comes, with 413's chime,
Safety and limits, now keeping time! 🔐

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and clearly describes the main change: adding http.MaxBytesReader to limit webhook request body size.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch MM-68166-max-bytes-reader-webhook

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

server/plugin/webhook_test.go (1)

33-77: Use the production limit constant and add exact-boundary coverage.

Line 46 hardcodes 26*1024*1024. Prefer maxWebhookPayloadSize+1, and add a subtest for exactly maxWebhookPayloadSize to prevent off-by-one regressions.

💡 Proposed test refinement

 func TestHandleWebhookBodySizeLimit(t *testing.T) {
 	t.Run("rejects oversized request body", func(t *testing.T) {
@@
-		oversizedBody := strings.NewReader(strings.Repeat("x", 26*1024*1024))
+		oversizedBody := strings.NewReader(strings.Repeat("x", maxWebhookPayloadSize+1))
 		req := httptest.NewRequest(http.MethodPost, "/webhook", oversizedBody)
 		req.Header.Set("X-Hub-Signature", "sha1=invalid")
 		w := httptest.NewRecorder()
@@
 		assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code)
 	})
 
+	t.Run("accepts request body at exact limit", func(t *testing.T) {
+		_, mockAPI, _, _, _ := GetTestSetup(t)
+		p := NewPlugin()
+		p.initializeAPI()
+		p.SetAPI(mockAPI)
+		p.client = pluginapi.NewClient(mockAPI, p.Driver)
+		p.setConfiguration(&Configuration{
+			WebhookSecret: webhookSecret,
+		})
+
+		mockAPI.On("LogInfo", "Webhook event received")
+
+		bodyAtLimit := strings.NewReader(strings.Repeat("x", maxWebhookPayloadSize))
+		req := httptest.NewRequest(http.MethodPost, "/webhook", bodyAtLimit)
+		req.Header.Set("X-Hub-Signature", "sha1=invalid")
+		w := httptest.NewRecorder()
+
+		p.handleWebhook(w, req)
+
+		assert.Equal(t, http.StatusUnauthorized, w.Code)
+	})
+
 	t.Run("accepts normal sized request body", func(t *testing.T) {
@@
 	})
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/plugin/webhook_test.go` around lines 33 - 77, The test
TestHandleWebhookBodySizeLimit uses a hardcoded 26*1024*1024 size; replace that
with maxWebhookPayloadSize+1 to use the production constant and avoid drift, and
add an additional subtest that sends a request body of exactly
maxWebhookPayloadSize to ensure p.handleWebhook enforces the boundary correctly
(oversized -> StatusRequestEntityTooLarge, exactly at limit -> accepted/falls
through to signature check which returns StatusUnauthorized in existing tests).
Reference TestHandleWebhookBodySizeLimit, p.handleWebhook, and
maxWebhookPayloadSize when making the changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@server/plugin/webhook_test.go`:
- Around line 33-77: The test TestHandleWebhookBodySizeLimit uses a hardcoded
26*1024*1024 size; replace that with maxWebhookPayloadSize+1 to use the
production constant and avoid drift, and add an additional subtest that sends a
request body of exactly maxWebhookPayloadSize to ensure p.handleWebhook enforces
the boundary correctly (oversized -> StatusRequestEntityTooLarge, exactly at
limit -> accepted/falls through to signature check which returns
StatusUnauthorized in existing tests). Reference TestHandleWebhookBodySizeLimit,
p.handleWebhook, and maxWebhookPayloadSize when making the changes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 584138ee-2151-4d7a-bc5a-c9691b5524df

📥 Commits

Reviewing files that changed from the base of the PR and between c569eaf and a62d0fa.

📒 Files selected for processing (2)

• server/plugin/webhook.go
• server/plugin/webhook_test.go