fix(webhook): validate signature and body for DELETE webhook message API #2913

hachimitsu-pichi · 2026-01-21T05:39:40Z

… message

…under specific webhook context

Copilot

Pull request overview

This pull request adds signature validation to the DELETE webhook message API endpoint. The change moves the endpoint from an authenticated API to a no-auth API, replacing cookie-based authentication with HMAC signature validation.

Changes:

Moved DELETE webhook message endpoint from authenticated API section to no-auth API section with signature validation
Changed endpoint path from /v3/webhooks/:webhookID/messages/:messageID to /v3/webhook/:webhookID/messages/:messageID (plural to singular)
Updated tests to use signature headers instead of session cookies

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File	Description
router/v3/webhooks.go	Added signature validation logic requiring non-empty body and valid HMAC signature for DELETE requests
router/v3/router.go	Moved DeleteWebhookMessage endpoint from authenticated API to no-auth API section, changed path from "webhooks" to "webhook" (singular)
router/v3/webhooks_test.go	Updated tests to use signature-based authentication instead of session cookies, added signature validation test cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T06:39:08Z

router/v3/webhooks.go

+	if len(body) == 0 {
+		return herror.BadRequest("empty body")
+	}


The DELETE endpoint now requires a non-empty request body, but DELETE requests typically should not have a body according to HTTP semantics. This validation logic at lines 263-265 forces clients to send a body (even if just a dummy value) when making DELETE requests, which is unusual and may cause issues with HTTP clients that don't support bodies in DELETE requests. Consider allowing empty bodies for DELETE requests or documenting this requirement clearly if it's intentional.

Suggested change

if len(body) == 0 {

return herror.BadRequest("empty body")

}

Copilot · 2026-01-21T06:39:08Z

router/v3/webhooks_test.go

 		e := env.R(t)
 		e.DELETE(path, wh2.GetID(), message.GetID()).
-			WithCookie(session.CookieName, s).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).


The signature is calculated using wh.GetSecret() but the request is being made to wh2.GetID(). This means the signature will be validated against wh2's secret, causing a signature mismatch error before the forbidden check can occur. The signature should be calculated with wh2.GetSecret() to properly test the forbidden scenario where the webhook exists and signature is valid, but the message doesn't belong to that webhook.

Suggested change

WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).

WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh2.GetSecret())).

Copilot · 2026-01-21T06:39:08Z

router/v3/webhooks_test.go

 		e.DELETE(path, wh.GetID(), uuid.Must(uuid.NewV4())).
-			WithCookie(session.CookieName, s).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).
 			Expect().
 			Status(http.StatusNotFound)


The test calculates the signature for an empty body but doesn't explicitly send a body with the DELETE request. However, the handler code requires a non-empty body (lines 263-265 in webhooks.go). This test would fail with "empty body" error. Either send a non-empty body in the test (and calculate its signature) or remove the empty body validation from the handler to allow DELETE requests with empty bodies.

Copilot · 2026-01-21T06:39:09Z

router/v3/webhooks_test.go

 		e.DELETE(path, wh2.GetID(), message.GetID()).
-			WithCookie(session.CookieName, s).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).
 			Expect().
 			Status(http.StatusForbidden)


The test calculates the signature for an empty body but doesn't explicitly send a body with the DELETE request. However, the handler code requires a non-empty body (lines 263-265 in webhooks.go). This test would fail with "empty body" error. Either send a non-empty body in the test (and calculate its signature) or remove the empty body validation from the handler to allow DELETE requests with empty bodies.

Copilot · 2026-01-21T06:39:09Z

router/v3/webhooks_test.go

+		e.DELETE(path, wh.GetID(), message2.GetID()).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).
 			Expect().


The test calculates the signature for an empty body but doesn't explicitly send a body with the DELETE request. However, the handler code requires a non-empty body (lines 263-265 in webhooks.go). This test would fail with "empty body" error. Either send a non-empty body in the test (and calculate its signature) or remove the empty body validation from the handler to allow DELETE requests with empty bodies.

Copilot · 2026-01-21T06:39:09Z

router/v3/router.go

+			{
+				apiWebhooksMessages.DELETE("", h.DeleteWebhookMessage)
+			}
+		}


The endpoint path has changed from "/webhooks/:webhookID/messages/:messageID" (under the authenticated API) to "/webhook/:webhookID/messages/:messageID" (under the no-auth API). This is a breaking API change that changes both the authentication mechanism (from cookie-based to signature-based) and the URL structure (webhooks → webhook, plural to singular). This breaking change should be carefully documented and versioned appropriately, as existing clients using the old authenticated endpoint will break.

Suggested change

}

}

// Backward-compatible routes for legacy plural /webhooks path

apiWebhooksLegacy := apiNoAuth.Group("/webhooks/:webhookID", retrieve.WebhookID())

{

apiWebhooksLegacy.POST("", h.PostWebhook)

apiWebhooksLegacyMessages := apiWebhooksLegacy.Group("/messages/:messageID", retrieve.MessageID())

{

apiWebhooksLegacyMessages.DELETE("", h.DeleteWebhookMessage)

}

}

Copilot · 2026-01-21T06:39:09Z

router/v3/webhooks_test.go

 		e.DELETE(path, wh.GetID(), message.GetID()).
 			Expect().
-			Status(http.StatusUnauthorized)
+			Status(http.StatusBadRequest)


The test doesn't send a request body with the DELETE request, but the handler code requires a non-empty body (lines 263-265 in webhooks.go). This test would fail with "empty body" error before reaching the signature validation check. Either send a non-empty body in the test or remove the empty body validation from the handler.

Copilot · 2026-01-21T06:39:10Z

router/v3/webhooks_test.go

+		e.DELETE(path, wh.GetID(), message.GetID()).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "test", wh.GetSecret())).
+			Expect().
+			Status(http.StatusBadRequest)


The test doesn't send a request body with the DELETE request, but the handler code requires a non-empty body (lines 263-265 in webhooks.go). This test would fail with "empty body" error before the signature can be validated. Either send a non-empty body in the test or remove the empty body validation from the handler.

Copilot · 2026-01-21T06:39:10Z

router/v3/webhooks_test.go

 		e.DELETE(path, uuid.Must(uuid.NewV4()), message.GetID()).
-			WithCookie(session.CookieName, s).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).
 			Expect().
 			Status(http.StatusNotFound)


The test calculates the signature for an empty body but doesn't explicitly send a body with the DELETE request. However, the handler code requires a non-empty body (lines 263-265 in webhooks.go). This test would fail with "empty body" error. Either send a non-empty body in the test (and calculate its signature) or remove the empty body validation from the handler to allow DELETE requests with empty bodies.

… API

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T07:03:56Z

router/v3/webhooks_test.go

 	t.Run("success", func(t *testing.T) {
 		t.Parallel()
 		e := env.R(t)
-		e.DELETE(path, wh.GetID(), message.GetID()).
-			WithCookie(session.CookieName, s).
+		message2 := env.CreateMessage(t, wh.GetBotUserID(), ch.ID, "test")
+		e.DELETE(path, wh.GetID(), message2.GetID()).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).
 			Expect().
 			Status(http.StatusNoContent)

-		_, err := env.Repository.GetMessageByID(message.GetID())
+		_, err := env.Repository.GetMessageByID(message2.GetID())
 		assert.ErrorIs(t, err, repository.ErrNotFound)
 	})
 }


Missing test coverage for webhooks without secrets. The signature validation in the handler (webhooks.go line 259) only runs when the webhook has a secret. However, there's no test case that verifies DELETE works correctly for webhooks without secrets. This is an important edge case that should be tested.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T07:16:45Z

router/v3/webhooks_test.go

+	t.Run("bad request (bad signature)", func(t *testing.T) {
+		t.Parallel()
+		e := env.R(t)
+		e.DELETE(path, wh.GetID(), message.GetID()).
+			WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "test", wh.GetSecret())).
+			Expect().
+			Status(http.StatusBadRequest)
 	})


Consider adding test cases that verify signature validation when a body is sent with the DELETE request. For example:

DELETE with a non-empty body and signature computed on that body (should accept if implementation is fixed)

DELETE with a non-empty body but signature computed on empty string (should reject)

This would ensure the signature validation correctly handles the actual request body, even though DELETE requests typically don't include bodies.

ramdos0207 · 2026-01-22T13:49:52Z

router/v3/webhooks.go

+		if len(sig) == 0 {
+			return herror.BadRequest("missing X-TRAQ-Signature header")
+		}
+		if subtle.ConstantTimeCompare(hmac.SHA1([]byte{}, w.GetSecret()), sig) != 1 {


[]byte{}にした理由を聞いておきたいです！（やや奇妙な仕様な気がしています）

fix(webhook): validate signature and body for DELETE webhook message API

b2d7a6c

Eraxyso assigned Eraxyso and hachimitsu-pichi Jan 21, 2026

hachimitsu-pichi added 3 commits January 21, 2026 14:51

fix(webhook): correct HTTP method for DeleteWebhookMessage test cases

cbbd8be

fix(webhook): update DELETE webhook message test to use newly created…

a7f5042

… message

fix(webhook): restructure webhook routes to include message deletion …

098ff5f

…under specific webhook context

Eraxyso requested a review from Copilot January 21, 2026 06:33

Copilot started reviewing on behalf of Eraxyso January 21, 2026 06:34 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

Eraxyso added 2 commits January 21, 2026 06:45

fix(webhook): update signature validation for DELETE webhook message API

3d74556

fix(webhook): correct signature validation for DELETE webhook message…

dd42c14

… API

Eraxyso requested a review from Copilot January 21, 2026 06:58

Copilot started reviewing on behalf of Eraxyso January 21, 2026 06:58 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

fix(webhook): correct endpoint path for webhook group to plural form

e63c352

Eraxyso requested a review from Copilot January 21, 2026 07:12

Copilot started reviewing on behalf of Eraxyso January 21, 2026 07:12 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

hachimitsu-pichi requested a review from Takeno-hito January 21, 2026 08:20

ramdos0207 reviewed Jan 22, 2026

View reviewed changes

Merge branch 'master' into fix/webhook-delete-api

b04d7c9

	WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh.GetSecret())).
	WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh2.GetSecret())).

-		}
+		}
+		// Backward-compatible routes for legacy plural /webhooks path
+		apiWebhooksLegacy := apiNoAuth.Group("/webhooks/:webhookID", retrieve.WebhookID())
+		{
+			apiWebhooksLegacy.POST("", h.PostWebhook)
+			apiWebhooksLegacyMessages := apiWebhooksLegacy.Group("/messages/:messageID", retrieve.MessageID())
+			{
+				apiWebhooksLegacyMessages.DELETE("", h.DeleteWebhookMessage)
+			}
+		}

fix(webhook): validate signature and body for DELETE webhook message API #2913

Are you sure you want to change the base?

fix(webhook): validate signature and body for DELETE webhook message API #2913

Conversation

hachimitsu-pichi commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

ramdos0207 Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hachimitsu-pichi commented Jan 21, 2026 •

edited

Loading

ramdos0207 Jan 22, 2026 •

edited

Loading