From f497f009ceb6e2c51b8805dabedb0494a0d65e5b Mon Sep 17 00:00:00 2001 From: boojack Date: Tue, 9 Jun 2026 22:58:10 +0800 Subject: [PATCH] fix(webhook): fail loud on malformed signing secret and add tests Follow-up to #6013. The signing path silently fell back to using the raw secret string as the HMAC key when a whsec_-prefixed secret had invalid base64, producing signatures no receiver could verify with no server-side signal. - Extract resolveSigningKey helper that errors on invalid whsec_ base64 - Post returns that error (logged by the async dispatcher); ValidateSigningSecret rejects it at write time so a bad secret is never stored - Fix stale comment referencing a nonexistent Authorization header - Add Go tests: key derivation, secret validation, end-to-end signature round-trip, and the invariant that the secret never leaks into API responses --- internal/webhook/validate.go | 7 +- internal/webhook/webhook.go | 28 +++-- internal/webhook/webhook_test.go | 133 ++++++++++++++++++++++ server/router/api/v1/user_webhook_test.go | 29 +++++ 4 files changed, 189 insertions(+), 8 deletions(-) create mode 100644 server/router/api/v1/user_webhook_test.go diff --git a/internal/webhook/validate.go b/internal/webhook/validate.go index 37afa9d8..bf9034cf 100644 --- a/internal/webhook/validate.go +++ b/internal/webhook/validate.go @@ -84,7 +84,9 @@ func ValidateURL(rawURL string) error { // ValidateSigningSecret checks that secret is either empty (allowed) or contains // only printable ASCII characters (0x20–0x7E), excluding all control characters -// such as \r and \n, which would break the HTTP Authorization header. +// such as \r and \n, which would corrupt the webhook signature headers. When the +// secret uses the Standard Webhooks "whsec_" serialization, the base64 +// body must decode cleanly so signing cannot silently fall back to the wrong key. func ValidateSigningSecret(secret string) error { if secret == "" { return nil @@ -94,5 +96,8 @@ func ValidateSigningSecret(secret string) error { return status.Errorf(codes.InvalidArgument, "signing secret contains invalid character") } } + if _, err := resolveSigningKey(secret); err != nil { + return status.Errorf(codes.InvalidArgument, "%v", err) + } return nil } diff --git a/internal/webhook/webhook.go b/internal/webhook/webhook.go index 70dfa126..9a905651 100644 --- a/internal/webhook/webhook.go +++ b/internal/webhook/webhook.go @@ -88,6 +88,22 @@ type WebhookRequestPayload struct { SigningSecret string `json:"-"` } +// resolveSigningKey returns the raw HMAC key for a signing secret. Secrets using +// the Standard Webhooks "whsec_" serialization are base64-decoded to their +// raw bytes; any other secret is used as-is. It returns an error when a whsec_-prefixed +// secret is not valid base64, so callers fail loudly instead of silently signing with +// the wrong key (which would make every signature unverifiable by the receiver). +func resolveSigningKey(secret string) ([]byte, error) { + if rest, ok := strings.CutPrefix(secret, "whsec_"); ok { + decoded, err := base64.StdEncoding.DecodeString(rest) + if err != nil { + return nil, errors.Wrap(err, "signing secret has whsec_ prefix but is not valid base64") + } + return decoded, nil + } + return []byte(secret), nil +} + // Post posts the message to webhook endpoint. func Post(requestPayload *WebhookRequestPayload) error { body, err := json.Marshal(requestPayload) @@ -103,16 +119,14 @@ func Post(requestPayload *WebhookRequestPayload) error { req.Header.Set("Content-Type", "application/json") if requestPayload.SigningSecret != "" { + key, err := resolveSigningKey(requestPayload.SigningSecret) + if err != nil { + return errors.Wrapf(err, "failed to derive signing key for webhook to %s", requestPayload.URL) + } + msgID := "msg_" + uuid.New().String() timestamp := strconv.FormatInt(time.Now().Unix(), 10) - key := []byte(requestPayload.SigningSecret) - if strings.HasPrefix(requestPayload.SigningSecret, "whsec_") { - if decoded, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(requestPayload.SigningSecret, "whsec_")); err == nil { - key = decoded - } - } - mac := hmac.New(sha256.New, key) mac.Write([]byte(msgID + "." + timestamp + ".")) mac.Write(body) diff --git a/internal/webhook/webhook_test.go b/internal/webhook/webhook_test.go index 43d17d9c..7d50636c 100644 --- a/internal/webhook/webhook_test.go +++ b/internal/webhook/webhook_test.go @@ -1,6 +1,13 @@ package webhook import ( + "crypto/hmac" + "crypto/sha256" + "encoding/base64" + "io" + "net/http" + "net/http/httptest" + "strings" "testing" "github.com/stretchr/testify/require" @@ -11,3 +18,129 @@ func TestPostAsyncNilPayloadDoesNotPanic(t *testing.T) { PostAsync(nil) }) } + +func TestResolveSigningKey(t *testing.T) { + rawKey := []byte("0123456789abcdef") + whsec := "whsec_" + base64.StdEncoding.EncodeToString(rawKey) + + t.Run("plain secret used as-is", func(t *testing.T) { + key, err := resolveSigningKey("my-plain-secret") + require.NoError(t, err) + require.Equal(t, []byte("my-plain-secret"), key) + }) + + t.Run("whsec_ prefix is base64-decoded", func(t *testing.T) { + key, err := resolveSigningKey(whsec) + require.NoError(t, err) + require.Equal(t, rawKey, key) + }) + + t.Run("whsec_ with invalid base64 fails loudly", func(t *testing.T) { + _, err := resolveSigningKey("whsec_not!valid!base64!") + require.Error(t, err) + }) +} + +func TestValidateSigningSecret(t *testing.T) { + tests := []struct { + name string + secret string + wantErr bool + }{ + {name: "empty is allowed", secret: "", wantErr: false}, + {name: "printable ascii", secret: "abcDEF123!@#", wantErr: false}, + {name: "valid whsec_", secret: "whsec_" + base64.StdEncoding.EncodeToString([]byte("key")), wantErr: false}, + {name: "newline rejected", secret: "abc\ndef", wantErr: true}, + {name: "carriage return rejected", secret: "abc\rdef", wantErr: true}, + {name: "tab rejected", secret: "abc\tdef", wantErr: true}, + {name: "non-ascii rejected", secret: "abc€def", wantErr: true}, + {name: "whsec_ with invalid base64 rejected", secret: "whsec_not!base64", wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := ValidateSigningSecret(tc.secret) + if tc.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +// TestPostSignsRequest verifies the end-to-end Standard Webhooks signature so a +// receiver following the documented verification recipe will accept our requests. +func TestPostSignsRequest(t *testing.T) { + // httptest listens on 127.0.0.1, which the SSRF guard blocks by default. + prev := AllowPrivateIPs + AllowPrivateIPs = true + defer func() { AllowPrivateIPs = prev }() + + rawKey := []byte("0123456789abcdef0123456789abcdef") + cases := []struct { + name string + secret string + key []byte + }{ + {name: "plain secret", secret: "plain-secret-value", key: []byte("plain-secret-value")}, + {name: "whsec_ secret", secret: "whsec_" + base64.StdEncoding.EncodeToString(rawKey), key: rawKey}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var gotID, gotTimestamp, gotSignature string + var gotBody []byte + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotID = r.Header.Get("webhook-id") + gotTimestamp = r.Header.Get("webhook-timestamp") + gotSignature = r.Header.Get("webhook-signature") + gotBody, _ = io.ReadAll(r.Body) + _, _ = w.Write([]byte(`{"code":0}`)) + })) + defer server.Close() + + err := Post(&WebhookRequestPayload{ + URL: server.URL, + ActivityType: "memos.memo.created", + Creator: "users/1", + SigningSecret: tc.secret, + }) + require.NoError(t, err) + + require.True(t, strings.HasPrefix(gotID, "msg_"), "webhook-id should be prefixed with msg_") + require.NotEmpty(t, gotTimestamp) + require.True(t, strings.HasPrefix(gotSignature, "v1,"), "signature should carry the v1 version tag") + + // Recompute the signature the way a receiver would and confirm it matches. + mac := hmac.New(sha256.New, tc.key) + mac.Write([]byte(gotID + "." + gotTimestamp + ".")) + mac.Write(gotBody) + want := base64.StdEncoding.EncodeToString(mac.Sum(nil)) + require.Equal(t, "v1,"+want, gotSignature) + }) + } +} + +// TestPostWithoutSecretSetsNoSignatureHeaders ensures unsigned webhooks stay unsigned. +func TestPostWithoutSecretSetsNoSignatureHeaders(t *testing.T) { + prev := AllowPrivateIPs + AllowPrivateIPs = true + defer func() { AllowPrivateIPs = prev }() + + var hasSignatureHeaders bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hasSignatureHeaders = r.Header.Get("webhook-id") != "" || + r.Header.Get("webhook-timestamp") != "" || + r.Header.Get("webhook-signature") != "" + _, _ = w.Write([]byte(`{"code":0}`)) + })) + defer server.Close() + + err := Post(&WebhookRequestPayload{ + URL: server.URL, + ActivityType: "memos.memo.created", + Creator: "users/1", + }) + require.NoError(t, err) + require.False(t, hasSignatureHeaders, "no signature headers should be set when no secret is configured") +} diff --git a/server/router/api/v1/user_webhook_test.go b/server/router/api/v1/user_webhook_test.go new file mode 100644 index 00000000..1acbd9af --- /dev/null +++ b/server/router/api/v1/user_webhook_test.go @@ -0,0 +1,29 @@ +package v1 + +import ( + "testing" + + "github.com/stretchr/testify/require" + + storepb "github.com/usememos/memos/proto/gen/store" + "github.com/usememos/memos/store" +) + +// TestConvertUserWebhookFromUserSettingOmitsSigningSecret guards the core security +// invariant of the signing-secret feature: the secret is INPUT_ONLY and must never +// be copied into an API response, even though it is persisted in the user setting. +func TestConvertUserWebhookFromUserSettingOmitsSigningSecret(t *testing.T) { + user := &store.User{Username: "alice"} + stored := &storepb.WebhooksUserSetting_Webhook{ + Id: "webhook-id", + Title: "My Webhook", + Url: "https://example.com/postreceive", + SigningSecret: "whsec_super-secret-value", + } + + apiWebhook := convertUserWebhookFromUserSetting(stored, user) + + require.Equal(t, "My Webhook", apiWebhook.DisplayName) + require.Equal(t, "https://example.com/postreceive", apiWebhook.Url) + require.Empty(t, apiWebhook.SigningSecret, "signing secret must never be returned in API responses") +}