Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hachimitsu-pichi wants to merge 8 commits into
base: master
Choose a base branch
from
+45 −14
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b2d7a6c
fix(webhook): validate signature and body for DELETE webhook message API
hachimitsu-pichi Jan 21, 2026
cbbd8be
fix(webhook): correct HTTP method for DeleteWebhookMessage test cases
hachimitsu-pichi Jan 21, 2026
a7f5042
fix(webhook): update DELETE webhook message test to use newly created…
hachimitsu-pichi Jan 21, 2026
098ff5f
fix(webhook): restructure webhook routes to include message deletion …
hachimitsu-pichi Jan 21, 2026
3d74556
fix(webhook): update signature validation for DELETE webhook message API
Eraxyso Jan 21, 2026
dd42c14
fix(webhook): correct signature validation for DELETE webhook message…
Eraxyso Jan 21, 2026
e63c352
fix(webhook): correct endpoint path for webhook group to plural form
hachimitsu-pichi Jan 21, 2026
b04d7c9
Merge branch 'master' into fix/webhook-delete-api
Eraxyso Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions router/v3/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,7 @@ func (h *Handlers) Setup(e *echo.Group) {
apiWebhooksWID.GET("/icon", h.GetWebhookIcon, requires(permission.GetWebhook))
apiWebhooksWID.PUT("/icon", h.ChangeWebhookIcon, requires(permission.EditWebhook))
apiWebhooksWID.GET("/messages", h.GetWebhookMessages, requires(permission.GetWebhook))
apiWebhooksWIDMessage := apiWebhooksWID.Group("/messages/:messageID", requires(permission.GetWebhook), retrieve.MessageID(), requiresMessageAccessPerm)
{
apiWebhooksWIDMessage.DELETE("", h.DeleteWebhookMessage, requires(permission.GetMessage))
}

}
}
apiGroups := api.Group("/groups")
Expand Down Expand Up @@ -421,7 +418,14 @@ func (h *Handlers) Setup(e *echo.Group) {
}
apiNoAuth.POST("/login", h.Login, noLogin)
apiNoAuth.POST("/logout", h.Logout)
apiNoAuth.POST("/webhooks/:webhookID", h.PostWebhook, retrieve.WebhookID())
apiWebhooks := apiNoAuth.Group("/webhooks/:webhookID", retrieve.WebhookID())
{
apiWebhooks.POST("", h.PostWebhook)
apiWebhooksMessages := apiWebhooks.Group("/messages/:messageID", retrieve.MessageID())
{
apiWebhooksMessages.DELETE("", h.DeleteWebhookMessage)
}
}
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
apiNoAuth.POST("/qall/webhook", h.LiveKitWebhook)
apiNoAuthPublic := apiNoAuth.Group("/public")
{
Expand Down
11 changes: 11 additions & 0 deletions router/v3/webhooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,17 @@ func (h *Handlers) DeleteWebhookMessage(c echo.Context) error {
botUserID := w.GetBotUserID()
messageUserID := m.GetUserID()

// Webhookシークレット確認
if len(w.GetSecret()) > 0 {
sig, _ := hex.DecodeString(c.Request().Header.Get(consts.HeaderSignature))
if len(sig) == 0 {
return herror.BadRequest("missing X-TRAQ-Signature header")
}
if subtle.ConstantTimeCompare(hmac.SHA1([]byte{}, w.GetSecret()), sig) != 1 {
Copy link
Contributor

@ramdos0207 ramdos0207 Jan 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return herror.BadRequest("X-TRAQ-Signature is wrong")
}
}

if botUserID == messageUserID {
if err := h.Repo.DeleteMessage(messageID); err != nil {
return herror.InternalServerError(err)
Expand Down
34 changes: 25 additions & 9 deletions router/v3/webhooks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,21 +551,36 @@ func TestHandlers_DeleteWebhookMessage(t *testing.T) {
wh := env.CreateWebhook(t, rand, user.GetID(), ch.ID)
wh2 := env.CreateWebhook(t, rand, user.GetID(), ch.ID)
message := env.CreateMessage(t, wh.GetBotUserID(), ch.ID, "test")
s := env.S(t, user.GetID())

t.Run("not logged in", func(t *testing.T) {
calcHMACSHA1 := func(t *testing.T, message, secret string) string {
t.Helper()
mac := hmac.New(sha1.New, []byte(secret))
_, _ = mac.Write([]byte(message))
return hex.EncodeToString(mac.Sum(nil))
}

t.Run("bad request (no signature)", func(t *testing.T) {
t.Parallel()
e := env.R(t)
e.DELETE(path, wh.GetID(), message.GetID()).
Expect().
Status(http.StatusUnauthorized)
Status(http.StatusBadRequest)
Comment on lines 559 to +567
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
})

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)
Comment on lines +573 to +576
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
})
Comment on lines +570 to 577
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

t.Run("not found webhook", func(t *testing.T) {
t.Parallel()
e := env.R(t)
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)
Comment on lines 582 to 585
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
})
Expand All @@ -574,7 +589,7 @@ func TestHandlers_DeleteWebhookMessage(t *testing.T) {
t.Parallel()
e := env.R(t)
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)
Comment on lines 591 to 594
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
})
Expand All @@ -583,20 +598,21 @@ func TestHandlers_DeleteWebhookMessage(t *testing.T) {
t.Parallel()
e := env.R(t)
e.DELETE(path, wh2.GetID(), message.GetID()).
WithCookie(session.CookieName, s).
WithHeader("X-TRAQ-Signature", calcHMACSHA1(t, "", wh2.GetSecret())).
Expect().
Status(http.StatusForbidden)
})

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().
Comment on lines +610 to 612
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Status(http.StatusNoContent)

_, err := env.Repository.GetMessageByID(message.GetID())
_, err := env.Repository.GetMessageByID(message2.GetID())
assert.ErrorIs(t, err, repository.ErrNotFound)
})
}
Comment on lines 606 to 618
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.