diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 423e1fe51..621d315fc 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -418,3 +418,4 @@ func TestOpenTelemetry(t *testing.T) { }) } } + diff --git a/internal/destinationmockserver/model.go b/internal/destinationmockserver/model.go index 0cec9b3d7..35d5d55bc 100644 --- a/internal/destinationmockserver/model.go +++ b/internal/destinationmockserver/model.go @@ -36,12 +36,28 @@ type mockStore struct { mu sync.RWMutex destinations map[string]models.Destination events map[string][]Event + + // Built once at construction — shared by every verifySignature call. + sigFormatter destwebhook.SignatureFormatter + headerFormatter destwebhook.HeaderFormatter } func NewMockStore() MockStore { + // The default templates are package constants, so construction cannot + // fail at runtime — a panic here means the constants themselves are broken. + sigFormatter, err := destwebhook.NewSignatureFormatter(destwebhook.DefaultSignatureContentTmpl) + if err != nil { + panic(fmt.Sprintf("destinationmockserver: invalid default signature content template: %v", err)) + } + headerFormatter, err := destwebhook.NewHeaderFormatter(destwebhook.DefaultSignatureHeaderTmpl) + if err != nil { + panic(fmt.Sprintf("destinationmockserver: invalid default signature header template: %v", err)) + } return &mockStore{ - destinations: make(map[string]models.Destination), - events: make(map[string][]Event), + destinations: make(map[string]models.Destination), + events: make(map[string][]Event), + sigFormatter: sigFormatter, + headerFormatter: headerFormatter, } } @@ -120,7 +136,7 @@ func (s *mockStore) ReceiveEvent(ctx context.Context, destinationID string, rawB if signature := metadata["signature"]; signature != "" { // Try current secret if secret := destination.Credentials["secret"]; secret != "" { - event.Verified = verifySignature( + event.Verified = s.verifySignature( secret, rawBody, signature, @@ -136,7 +152,7 @@ func (s *mockStore) ReceiveEvent(ctx context.Context, destinationID string, rawB if invalidAtStr := destination.Credentials["previous_secret_invalid_at"]; invalidAtStr != "" { if invalidAt, err := time.Parse(time.RFC3339, invalidAtStr); err == nil { if time.Now().Before(invalidAt) { - event.Verified = verifySignature( + event.Verified = s.verifySignature( prevSecret, rawBody, signature, @@ -155,7 +171,7 @@ func (s *mockStore) ReceiveEvent(ctx context.Context, destinationID string, rawB } // verifySignature verifies the signature using the provided secret and algorithm -func verifySignature(secret string, payload []byte, signature string, algorithm string, encoding string) bool { +func (s *mockStore) verifySignature(secret string, payload []byte, signature string, algorithm string, encoding string) bool { log.Println("verifySignature", secret, payload, signature, algorithm, encoding) if signature == "" { return false @@ -190,8 +206,8 @@ func verifySignature(secret string, payload []byte, signature string, algorithm secrets, destwebhook.WithEncoder(destwebhook.GetEncoder(encoding)), destwebhook.WithAlgorithm(destwebhook.GetAlgorithm(algorithm)), - destwebhook.WithSignatureFormatter(destwebhook.NewSignatureFormatter(destwebhook.DefaultSignatureContentTmpl)), - destwebhook.WithHeaderFormatter(destwebhook.NewHeaderFormatter(destwebhook.DefaultSignatureHeaderTmpl)), + destwebhook.WithSignatureFormatter(s.sigFormatter), + destwebhook.WithHeaderFormatter(s.headerFormatter), ) for _, sig := range signatures { diff --git a/internal/destregistry/providers/default_test.go b/internal/destregistry/providers/default_test.go new file mode 100644 index 000000000..422057267 --- /dev/null +++ b/internal/destregistry/providers/default_test.go @@ -0,0 +1,46 @@ +package destregistrydefault_test + +import ( + "testing" + + "github.com/hookdeck/outpost/internal/destregistry" + destregistrydefault "github.com/hookdeck/outpost/internal/destregistry/providers" + "github.com/hookdeck/outpost/internal/destregistry/providers/destwebhook" + "github.com/hookdeck/outpost/internal/util/testutil" + "github.com/stretchr/testify/assert" +) + +// Signature template validation lives in destwebhook.New, so registration is +// where a bad template fails startup — and only in default mode. In 'standard' +// mode the templates are fixed by the Standard Webhooks spec, the configured +// ones are never parsed, and registration must succeed regardless of their +// contents. +func TestRegisterDefault_WebhookSignatureTemplates(t *testing.T) { + webhookConfig := func(mode string) *destregistrydefault.DestWebhookConfig { + return &destregistrydefault.DestWebhookConfig{ + Mode: mode, + HeaderPrefix: destwebhook.DefaultHeaderPrefix, + SignatureContentTemplate: destwebhook.DefaultSignatureContentTmpl, + SignatureHeaderTemplate: "v0={{.Body}}", // header templates have no .Body — invalid at render + SignatureEncoding: destwebhook.DefaultEncoding, + SignatureAlgorithm: destwebhook.DefaultAlgorithm, + SigningSecretTemplate: destwebhook.DefaultSigningSecretTmpl, + } + } + + t.Run("default mode rejects an invalid template", func(t *testing.T) { + registry := destregistry.NewRegistry(&destregistry.Config{}, testutil.CreateTestLogger(t)) + err := destregistrydefault.RegisterDefault(registry, destregistrydefault.RegisterDefaultDestinationOptions{ + Webhook: webhookConfig(""), + }) + assert.ErrorContains(t, err, "can't evaluate field Body") + }) + + t.Run("standard mode ignores the configured templates", func(t *testing.T) { + registry := destregistry.NewRegistry(&destregistry.Config{}, testutil.CreateTestLogger(t)) + err := destregistrydefault.RegisterDefault(registry, destregistrydefault.RegisterDefaultDestinationOptions{ + Webhook: webhookConfig("standard"), + }) + assert.NoError(t, err) + }) +} diff --git a/internal/destregistry/providers/destwebhook/destwebhook.go b/internal/destregistry/providers/destwebhook/destwebhook.go index 92a94809c..ef470c0c3 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook.go +++ b/internal/destregistry/providers/destwebhook/destwebhook.go @@ -104,6 +104,15 @@ type WebhookDestination struct { topicHeader headerConfig encoding string algorithm string + // Built once in New() and shared by every publisher: both are derived + // solely from provider config, and a parsed template is safe for parallel + // execution. Building them per destination gave each cached publisher its + // own copy of two sprig function maps (~44 KB retained per publisher). + signatureFormatter SignatureFormatter + headerFormatter HeaderFormatter + encoder SignatureEncoder + signingAlgorithm SigningAlgorithm + rawSigningSecretTemplate string signingSecretTemplate *template.Template maxResponseBodyBytes int @@ -300,6 +309,25 @@ func New(loader metadata.MetadataLoader, basePublisherOpts []destregistry.BasePu seenNames[effective] = h.label } + // Build the signature formatters once — shared by every publisher + destination.signatureFormatter, err = NewSignatureFormatter(destination.signatureContentTemplate) + if err != nil { + return nil, err + } + destination.headerFormatter, err = NewHeaderFormatter(destination.signatureHeaderTemplate) + if err != nil { + return nil, err + } + // Dry-run render both, so a template that parses but references a field + // its payload type doesn't have fails construction even when the provider + // is built outside config validation. + if err := dryRunFormatters(destination.signatureFormatter, destination.headerFormatter, + destination.signatureContentTemplate, destination.signatureHeaderTemplate); err != nil { + return nil, err + } + destination.encoder = GetEncoder(destination.encoding) + destination.signingAlgorithm = GetAlgorithm(destination.algorithm) + // Parse signing secret template — fail on invalid syntax tmpl, err := template.New("signing_secret").Funcs(sprig.TxtFuncMap()).Parse(destination.rawSigningSecretTemplate) if err != nil { @@ -393,10 +421,10 @@ func (d *WebhookDestination) CreatePublisher(ctx context.Context, destination *m sm := NewSignatureManager( secrets, - WithSignatureFormatter(NewSignatureFormatter(d.signatureContentTemplate)), - WithHeaderFormatter(NewHeaderFormatter(d.signatureHeaderTemplate)), - WithEncoder(GetEncoder(d.encoding)), - WithAlgorithm(GetAlgorithm(d.algorithm)), + WithSignatureFormatter(d.signatureFormatter), + WithHeaderFormatter(d.headerFormatter), + WithEncoder(d.encoder), + WithAlgorithm(d.signingAlgorithm), ) var proxyURL *string @@ -762,12 +790,15 @@ func (p *WebhookPublisher) Format(ctx context.Context, event *models.Event) (*ht // Add signature header unless disabled if !p.signatureHeader.disabled { - signatureHeader := p.sm.GenerateSignatureHeader(SignaturePayload{ + signatureHeader, err := p.sm.GenerateSignatureHeader(SignaturePayload{ EventID: event.ID, Topic: event.Topic, Timestamp: now, Body: string(rawBody), }) + if err != nil { + return nil, err + } if signatureHeader != "" { req.Header.Set(resolveHeaderName(p.signatureHeader, p.headerPrefix, "signature"), signatureHeader) } diff --git a/internal/destregistry/providers/destwebhook/destwebhook_config_test.go b/internal/destregistry/providers/destwebhook/destwebhook_config_test.go index 317f20782..8c308a839 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook_config_test.go +++ b/internal/destregistry/providers/destwebhook/destwebhook_config_test.go @@ -229,3 +229,40 @@ func TestWebhookDestination_SignatureOptions(t *testing.T) { }) } } + +// Formatters are built in New() now, so a template that doesn't parse fails +// provider construction rather than the first CreatePublisher call. +func TestWebhookDestination_InvalidSignatureTemplate(t *testing.T) { + baseOpts := []destwebhook.Option{ + destwebhook.WithHeaderPrefix(destwebhook.DefaultHeaderPrefix), + destwebhook.WithSignatureContentTemplate(destwebhook.DefaultSignatureContentTmpl), + destwebhook.WithSignatureHeaderTemplate(destwebhook.DefaultSignatureHeaderTmpl), + destwebhook.WithSignatureEncoding(destwebhook.DefaultEncoding), + destwebhook.WithSignatureAlgorithm(destwebhook.DefaultAlgorithm), + destwebhook.WithSigningSecretTemplate(destwebhook.DefaultSigningSecretTmpl), + } + + tests := []struct { + name string + opt destwebhook.Option + wantErr string + }{ + { + name: "content template", + opt: destwebhook.WithSignatureContentTemplate("{{.Timestamp.{{.Body}}"), + wantErr: "invalid signature content template", + }, + { + name: "header template", + opt: destwebhook.WithSignatureHeaderTemplate("t={{.Timestamp},v0={{.Signatures}"), + wantErr: "invalid signature header template", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := destwebhook.New(testutil.Registry.MetadataLoader(), nil, append(baseOpts, tt.opt)...) + assert.ErrorContains(t, err, tt.wantErr) + }) + } +} diff --git a/internal/destregistry/providers/destwebhook/destwebhook_internal_test.go b/internal/destregistry/providers/destwebhook/destwebhook_internal_test.go new file mode 100644 index 000000000..d3d6a9033 --- /dev/null +++ b/internal/destregistry/providers/destwebhook/destwebhook_internal_test.go @@ -0,0 +1,62 @@ +package destwebhook + +import ( + "context" + "testing" + "time" + + "github.com/hookdeck/outpost/internal/destregistry/metadata" + "github.com/hookdeck/outpost/internal/models" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Formatters are built once in New() and shared by every publisher. This pins +// that sharing: a regression back to per-destination construction (rebuilding +// formatters inside CreatePublisher) makes the identity assertions fail. +// +// White-box on purpose — the formatter fields are unexported, and testutil +// can't be imported here without a cycle, so the destination is built inline. +func TestWebhookDestination_PublishersShareFormatters(t *testing.T) { + t.Parallel() + + provider, err := New(metadata.NewMetadataLoader(""), nil, + WithHeaderPrefix(DefaultHeaderPrefix), + WithSignatureContentTemplate(DefaultSignatureContentTmpl), + WithSignatureHeaderTemplate(DefaultSignatureHeaderTmpl), + WithSignatureEncoding(DefaultEncoding), + WithSignatureAlgorithm(DefaultAlgorithm), + WithSigningSecretTemplate(DefaultSigningSecretTmpl), + ) + require.NoError(t, err) + + now := time.Now() + dest := models.Destination{ + ID: "dest-formatter-sharing", + TenantID: "test-tenant", + Type: "webhook", + Topics: []string{"*"}, + Config: map[string]string{"url": "http://localhost:8080/webhook"}, + Credentials: map[string]string{"secret": "test-secret"}, + CreatedAt: now, + UpdatedAt: now, + } + + pub1, err := provider.CreatePublisher(context.Background(), &dest) + require.NoError(t, err) + defer pub1.Close() + pub2, err := provider.CreatePublisher(context.Background(), &dest) + require.NoError(t, err) + defer pub2.Close() + + wp1, ok := pub1.(*WebhookPublisher) + require.True(t, ok) + wp2, ok := pub2.(*WebhookPublisher) + require.True(t, ok) + + // Both publishers hold the provider's formatter instances, not copies. + assert.Same(t, provider.signatureFormatter, wp1.sm.sigFormatter) + assert.Same(t, provider.headerFormatter, wp1.sm.headerFormatter) + assert.Same(t, wp1.sm.sigFormatter, wp2.sm.sigFormatter) + assert.Same(t, wp1.sm.headerFormatter, wp2.sm.headerFormatter) +} diff --git a/internal/destregistry/providers/destwebhook/destwebhook_publish_test.go b/internal/destregistry/providers/destwebhook/destwebhook_publish_test.go index 104f2b05a..895cc0b05 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook_publish_test.go +++ b/internal/destregistry/providers/destwebhook/destwebhook_publish_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/hookdeck/outpost/internal/destregistry" "github.com/hookdeck/outpost/internal/destregistry/providers/destwebhook" testsuite "github.com/hookdeck/outpost/internal/destregistry/testing" "github.com/hookdeck/outpost/internal/models" @@ -1047,6 +1048,59 @@ func TestWebhookPublisher_PreservesKeyOrder(t *testing.T) { assert.Equal(t, `{"z":1,"a":2,"m":3}`, string(body)) } +// A template that parses but references a field of the other payload type only +// fails when it renders. That must fail the delivery, not the process. +func TestWebhookPublisher_SignatureTemplateRenderFailure(t *testing.T) { + dest := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "http://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "secret": "test-secret", + }), + ) + + // Wrong-field templates now fail provider construction (see + // TestNew_ValidatesSignatureTemplates), so this uses a value-dependent + // template: it survives the construction dry-run, which renders against + // two synthetic signatures, and fails here where the single configured + // secret yields only one. + tests := []struct { + name string + opt destwebhook.Option + }{ + { + name: "header template indexes a signature that doesn't exist", + opt: destwebhook.WithSignatureHeaderTemplate("v0={{index .Signatures 1}}"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + provider := NewTestProvider(t, tt.opt) + + publisher, err := provider.CreatePublisher(context.Background(), &dest) + require.NoError(t, err) + + event := testutil.EventFactory.Any( + testutil.EventFactory.WithDataMap(map[string]interface{}{"hello": "world"}), + ) + + assert.NotPanics(t, func() { + delivery, err := publisher.Publish(context.Background(), &event) + require.Error(t, err) + require.NotNil(t, delivery) + assert.Equal(t, "failed", delivery.Status) + + var publishErr *destregistry.ErrDestinationPublishAttempt + require.ErrorAs(t, err, &publishErr) + assert.Equal(t, "format_failed", publishErr.Data["error"]) + }) + }) + } +} + func TestWebhookPublisher_SignatureTemplates(t *testing.T) { dest := testutil.DestinationFactory.Any( testutil.DestinationFactory.WithType("webhook"), @@ -1158,10 +1212,14 @@ func TestWebhookPublisher_SignatureTemplates(t *testing.T) { if verifyHeaderTemplate == "" { verifyHeaderTemplate = destwebhook.DefaultSignatureHeaderTmpl } + sigFormatter, err := destwebhook.NewSignatureFormatter(verifyContentTemplate) + require.NoError(t, err) + headerFormatter, err := destwebhook.NewHeaderFormatter(verifyHeaderTemplate) + require.NoError(t, err) sm := destwebhook.NewSignatureManager( secrets, - destwebhook.WithSignatureFormatter(destwebhook.NewSignatureFormatter(verifyContentTemplate)), - destwebhook.WithHeaderFormatter(destwebhook.NewHeaderFormatter(verifyHeaderTemplate)), + destwebhook.WithSignatureFormatter(sigFormatter), + destwebhook.WithHeaderFormatter(headerFormatter), ) // Verify signature matches expected content diff --git a/internal/destregistry/providers/destwebhook/signature.go b/internal/destregistry/providers/destwebhook/signature.go index 796efc9ee..4b8c56781 100644 --- a/internal/destregistry/providers/destwebhook/signature.go +++ b/internal/destregistry/providers/destwebhook/signature.go @@ -38,11 +38,11 @@ type SigningAlgorithm interface { } type SignatureFormatter interface { - Format(content SignaturePayload) string + Format(content SignaturePayload) (string, error) } type HeaderFormatter interface { - Format(content HeaderPayload) string + Format(content HeaderPayload) (string, error) } type SignatureEncoder interface { @@ -65,56 +65,60 @@ type SignatureFormatterImpl struct { template *template.Template } -func NewSignatureFormatter(templateStr string) *SignatureFormatterImpl { +func NewSignatureFormatter(templateStr string) (*SignatureFormatterImpl, error) { if templateStr == "" { - panic("signature content template is required — config must provide an explicit value") + return nil, fmt.Errorf("signature content template is required") } tmpl := template.New("signature").Funcs(sprig.TxtFuncMap()) parsed, err := tmpl.Parse(templateStr) if err != nil { - panic(fmt.Sprintf("invalid signature content template %q: %v", templateStr, err)) + return nil, fmt.Errorf("invalid signature content template %q: %w", templateStr, err) } - return &SignatureFormatterImpl{template: parsed} + return &SignatureFormatterImpl{template: parsed}, nil } -func (f *SignatureFormatterImpl) Format(content SignaturePayload) string { +// Format renders the content template. Parsing only validates syntax; field +// references are resolved against the payload at execution, so a template that +// constructs fine can still fail here. The error is returned so the caller can +// fail the delivery instead of taking the process down. +func (f *SignatureFormatterImpl) Format(content SignaturePayload) (string, error) { var buf bytes.Buffer if err := f.template.Execute(&buf, content); err != nil { - // Template was validated at construction time, so execution errors - // indicate a bug (e.g., nil field). Panic to surface it immediately. - panic(fmt.Sprintf("signature content template execution failed: %v", err)) + return "", fmt.Errorf("signature content template execution failed: %w", err) } - return buf.String() + return buf.String(), nil } type HeaderFormatterImpl struct { template *template.Template } -func NewHeaderFormatter(templateStr string) *HeaderFormatterImpl { +func NewHeaderFormatter(templateStr string) (*HeaderFormatterImpl, error) { if templateStr == "" { - panic("signature header template is required — config must provide an explicit value") + return nil, fmt.Errorf("signature header template is required") } tmpl := template.New("header").Funcs(sprig.TxtFuncMap()) parsed, err := tmpl.Parse(templateStr) if err != nil { - panic(fmt.Sprintf("invalid signature header template %q: %v", templateStr, err)) + return nil, fmt.Errorf("invalid signature header template %q: %w", templateStr, err) } - return &HeaderFormatterImpl{template: parsed} + return &HeaderFormatterImpl{template: parsed}, nil } -func (f *HeaderFormatterImpl) Format(content HeaderPayload) string { +// Format renders the header template. See SignatureFormatterImpl.Format for why +// execution errors are returned rather than fatal. +func (f *HeaderFormatterImpl) Format(content HeaderPayload) (string, error) { var buf bytes.Buffer if err := f.template.Execute(&buf, content); err != nil { - panic(fmt.Sprintf("signature header template execution failed: %v", err)) + return "", fmt.Errorf("signature header template execution failed: %w", err) } - return buf.String() + return buf.String(), nil } type HmacAlgo struct { @@ -214,9 +218,9 @@ func NewSignatureManager(secrets []WebhookSecret, opts ...SignatureManagerOption return sm } -func (sm *SignatureManager) GenerateSignatures(content SignaturePayload) []string { +func (sm *SignatureManager) GenerateSignatures(content SignaturePayload) ([]string, error) { if len(sm.secrets) == 0 { - return nil + return nil, nil } // Sort secrets by creation date, newest first @@ -226,7 +230,10 @@ func (sm *SignatureManager) GenerateSignatures(content SignaturePayload) []strin return sortedSecrets[i].CreatedAt.After(sortedSecrets[j].CreatedAt) }) - formattedContent := sm.sigFormatter.Format(content) + formattedContent, err := sm.sigFormatter.Format(content) + if err != nil { + return nil, err + } var signatures []string now := time.Now() @@ -252,13 +259,16 @@ func (sm *SignatureManager) GenerateSignatures(content SignaturePayload) []strin signatures = append(signatures, sm.algorithm.Sign(secret.Key, formattedContent, sm.encoder)) } - return signatures + return signatures, nil } -func (sm *SignatureManager) GenerateSignatureHeader(content SignaturePayload) string { - signatures := sm.GenerateSignatures(content) +func (sm *SignatureManager) GenerateSignatureHeader(content SignaturePayload) (string, error) { + signatures, err := sm.GenerateSignatures(content) + if err != nil { + return "", err + } if len(signatures) == 0 { - return "" + return "", nil } return sm.headerFormatter.Format(HeaderPayload{ EventID: content.EventID, @@ -268,7 +278,12 @@ func (sm *SignatureManager) GenerateSignatureHeader(content SignaturePayload) st }) } +// VerifySignature reports whether signature matches key. It returns false when +// the content template fails to render, since no signature can be verified. func (sm *SignatureManager) VerifySignature(signature, key string, content SignaturePayload) bool { - formattedContent := sm.sigFormatter.Format(content) + formattedContent, err := sm.sigFormatter.Format(content) + if err != nil { + return false + } return sm.algorithm.Verify(key, formattedContent, signature, sm.encoder) } diff --git a/internal/destregistry/providers/destwebhook/signature_test.go b/internal/destregistry/providers/destwebhook/signature_test.go index 86c37b29c..891391784 100644 --- a/internal/destregistry/providers/destwebhook/signature_test.go +++ b/internal/destregistry/providers/destwebhook/signature_test.go @@ -6,15 +6,24 @@ import ( "time" "github.com/hookdeck/outpost/internal/destregistry/providers/destwebhook" + "github.com/hookdeck/outpost/internal/util/testutil" "github.com/stretchr/testify/assert" ) // defaultSignatureManagerOpts provides the standard formatters for tests that // don't care about template behavior — they just need a working manager. func defaultSignatureManagerOpts() []destwebhook.SignatureManagerOption { + sigFormatter, err := destwebhook.NewSignatureFormatter(destwebhook.DefaultSignatureContentTmpl) + if err != nil { + panic(err) + } + headerFormatter, err := destwebhook.NewHeaderFormatter(destwebhook.DefaultSignatureHeaderTmpl) + if err != nil { + panic(err) + } return []destwebhook.SignatureManagerOption{ - destwebhook.WithSignatureFormatter(destwebhook.NewSignatureFormatter(destwebhook.DefaultSignatureContentTmpl)), - destwebhook.WithHeaderFormatter(destwebhook.NewHeaderFormatter(destwebhook.DefaultSignatureHeaderTmpl)), + destwebhook.WithSignatureFormatter(sigFormatter), + destwebhook.WithHeaderFormatter(headerFormatter), } } @@ -88,27 +97,43 @@ func TestSignatureFormatter(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - formatter := destwebhook.NewSignatureFormatter(tt.template) - result := formatter.Format(destwebhook.SignaturePayload{ + formatter, err := destwebhook.NewSignatureFormatter(tt.template) + assert.NoError(t, err) + result, err := formatter.Format(destwebhook.SignaturePayload{ Timestamp: timestamp, Body: body, EventID: "test-id", Topic: "test-topic", }) + assert.NoError(t, err) assert.Equal(t, tt.want, result) }) } } -func TestSignatureFormatter_PanicsOnEmpty(t *testing.T) { - assert.Panics(t, func() { - destwebhook.NewSignatureFormatter("") - }) +func TestSignatureFormatter_ErrorsOnEmpty(t *testing.T) { + _, err := destwebhook.NewSignatureFormatter("") + assert.Error(t, err) +} + +func TestSignatureFormatter_ErrorsOnInvalidSyntax(t *testing.T) { + _, err := destwebhook.NewSignatureFormatter("{{.Timestamp.{{.Body}}") + assert.Error(t, err) } -func TestSignatureFormatter_PanicsOnInvalidSyntax(t *testing.T) { - assert.Panics(t, func() { - destwebhook.NewSignatureFormatter("{{.Timestamp.{{.Body}}") +// A template that parses but references a field the payload type doesn't have +// must fail the render rather than take the process down. +func TestSignatureFormatter_ErrorsOnUnknownField(t *testing.T) { + formatter, err := destwebhook.NewSignatureFormatter(`v1:{{.Timestamp.Unix}}:{{.Signatures | join ","}}`) + assert.NoError(t, err) + + assert.NotPanics(t, func() { + result, err := formatter.Format(destwebhook.SignaturePayload{ + Timestamp: time.Unix(1234567890, 0), + Body: "test", + }) + assert.Error(t, err) + assert.Empty(t, result) }) } @@ -145,30 +170,122 @@ func TestHeaderFormatter(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - formatter := destwebhook.NewHeaderFormatter(tt.template) - result := formatter.Format(destwebhook.HeaderPayload{ + formatter, err := destwebhook.NewHeaderFormatter(tt.template) + assert.NoError(t, err) + result, err := formatter.Format(destwebhook.HeaderPayload{ Timestamp: timestamp, Signatures: signatures, EventID: "test-id", Topic: "test-topic", }) + assert.NoError(t, err) assert.Equal(t, tt.want, result) }) } } -func TestHeaderFormatter_PanicsOnEmpty(t *testing.T) { - assert.Panics(t, func() { - destwebhook.NewHeaderFormatter("") - }) +func TestHeaderFormatter_ErrorsOnEmpty(t *testing.T) { + _, err := destwebhook.NewHeaderFormatter("") + assert.Error(t, err) +} + +func TestHeaderFormatter_ErrorsOnInvalidSyntax(t *testing.T) { + _, err := destwebhook.NewHeaderFormatter("t={{.Timestamp},v0={{.Signatures}") + assert.Error(t, err) } -func TestHeaderFormatter_PanicsOnInvalidSyntax(t *testing.T) { - assert.Panics(t, func() { - destwebhook.NewHeaderFormatter("t={{.Timestamp},v0={{.Signatures}") +func TestHeaderFormatter_ErrorsOnUnknownField(t *testing.T) { + formatter, err := destwebhook.NewHeaderFormatter("v0={{.Body}}") + assert.NoError(t, err) + + assert.NotPanics(t, func() { + result, err := formatter.Format(destwebhook.HeaderPayload{ + Timestamp: time.Unix(1234567890, 0), + Signatures: []string{"abc123"}, + }) + assert.Error(t, err) + assert.Empty(t, result) }) } +// New parses both signature templates and dry-runs them against synthetic +// payloads, so both a template that doesn't parse and one that borrows a field +// from the other payload type fail construction rather than the first +// delivery. +func TestNew_ValidatesSignatureTemplates(t *testing.T) { + tests := []struct { + name string + content string + header string + wantErr string + }{ + { + name: "defaults", + content: destwebhook.DefaultSignatureContentTmpl, + header: destwebhook.DefaultSignatureHeaderTmpl, + }, + { + name: "content template doesn't parse", + content: `{{.Timestamp.Unix}}.{{.Body | join \",\"}}`, + header: destwebhook.DefaultSignatureHeaderTmpl, + wantErr: "invalid signature content template", + }, + { + name: "header template doesn't parse", + content: destwebhook.DefaultSignatureContentTmpl, + header: "t={{.Timestamp},v0={{.Signatures}", + wantErr: "invalid signature header template", + }, + { + name: "content template borrows a header field", + content: `v1:{{.Timestamp.Unix}}:{{.Signatures | join ","}}`, + header: destwebhook.DefaultSignatureHeaderTmpl, + wantErr: "can't evaluate field Signatures", + }, + { + name: "header template borrows a content field", + content: destwebhook.DefaultSignatureContentTmpl, + header: "v0={{.Body}}", + wantErr: "can't evaluate field Body", + }, + { + name: "header template indexes the second signature", + content: destwebhook.DefaultSignatureContentTmpl, + header: "v0={{index .Signatures 0}},v0={{index .Signatures 1}}", + }, + { + name: "empty content template", + content: "", + header: destwebhook.DefaultSignatureHeaderTmpl, + wantErr: "signature content template is required", + }, + { + name: "empty header template", + content: destwebhook.DefaultSignatureContentTmpl, + header: "", + wantErr: "signature header template is required", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := destwebhook.New(testutil.Registry.MetadataLoader(), nil, + destwebhook.WithHeaderPrefix(destwebhook.DefaultHeaderPrefix), + destwebhook.WithSignatureContentTemplate(tt.content), + destwebhook.WithSignatureHeaderTemplate(tt.header), + destwebhook.WithSignatureEncoding(destwebhook.DefaultEncoding), + destwebhook.WithSignatureAlgorithm(destwebhook.DefaultAlgorithm), + destwebhook.WithSigningSecretTemplate(destwebhook.DefaultSigningSecretTmpl), + ) + if tt.wantErr == "" { + assert.NoError(t, err) + return + } + assert.ErrorContains(t, err, tt.wantErr) + }) + } +} + func TestSignatureEncoders(t *testing.T) { tests := []struct { name string @@ -203,13 +320,13 @@ func TestSignatureEncoders(t *testing.T) { func TestSignatureManager(t *testing.T) { t.Run("no secrets", func(t *testing.T) { manager := destwebhook.NewSignatureManager(nil, defaultSignatureManagerOpts()...) - signatures := manager.GenerateSignatures(destwebhook.SignaturePayload{ + signatures, _ := manager.GenerateSignatures(destwebhook.SignaturePayload{ Timestamp: time.Now(), Body: "test", }) assert.Nil(t, signatures) - header := manager.GenerateSignatureHeader(destwebhook.SignaturePayload{ + header, _ := manager.GenerateSignatureHeader(destwebhook.SignaturePayload{ Timestamp: time.Now(), Body: "test", }) @@ -231,7 +348,7 @@ func TestSignatureManager(t *testing.T) { } manager := destwebhook.NewSignatureManager([]destwebhook.WebhookSecret{oldSecret}, defaultSignatureManagerOpts()...) - signatures := manager.GenerateSignatures(payload) + signatures, _ := manager.GenerateSignatures(payload) assert.Len(t, signatures, 1, "should generate signature for single secret regardless of age") // Verify signature is valid with correct key @@ -259,7 +376,7 @@ func TestSignatureManager(t *testing.T) { } manager := destwebhook.NewSignatureManager(secrets, defaultSignatureManagerOpts()...) - signatures := manager.GenerateSignatures(payload) + signatures, _ := manager.GenerateSignatures(payload) assert.Len(t, signatures, 1, "should only use latest secret") // Verify signature is valid with latest key @@ -290,7 +407,7 @@ func TestSignatureManager(t *testing.T) { timestamp := time.Unix(1234567890, 0) body := `{"hello":"world"}` - signatures := manager.GenerateSignatures(destwebhook.SignaturePayload{ + signatures, _ := manager.GenerateSignatures(destwebhook.SignaturePayload{ Timestamp: timestamp, Body: body, EventID: "test-id", @@ -325,7 +442,7 @@ func TestSignatureManager(t *testing.T) { }, ), "signature should be invalid with expired key") - header := manager.GenerateSignatureHeader(destwebhook.SignaturePayload{ + header, _ := manager.GenerateSignatureHeader(destwebhook.SignaturePayload{ Timestamp: timestamp, Body: body, EventID: "test-id", @@ -357,7 +474,7 @@ func TestSignatureManager(t *testing.T) { Topic: "test-topic", } - signatures := manager.GenerateSignatures(payload) + signatures, _ := manager.GenerateSignatures(payload) assert.Len(t, signatures, 3, "should include latest + valid secrets") // Verify each signature is valid with its corresponding key @@ -390,7 +507,7 @@ func TestSignatureManager(t *testing.T) { } manager := destwebhook.NewSignatureManager(secrets, defaultSignatureManagerOpts()...) - signatures := manager.GenerateSignatures(destwebhook.SignaturePayload{ + signatures, _ := manager.GenerateSignatures(destwebhook.SignaturePayload{ Timestamp: time.Unix(1234567890, 0), Body: "test", EventID: "test-id", @@ -407,7 +524,7 @@ func TestSignatureManager(t *testing.T) { } manager := destwebhook.NewSignatureManager(secrets, defaultSignatureManagerOpts()...) - signatures := manager.GenerateSignatures(destwebhook.SignaturePayload{ + signatures, _ := manager.GenerateSignatures(destwebhook.SignaturePayload{ Timestamp: time.Unix(1234567890, 0), Body: "test", EventID: "test-id", diff --git a/internal/destregistry/providers/destwebhook/signature_validation.go b/internal/destregistry/providers/destwebhook/signature_validation.go new file mode 100644 index 000000000..3c1435928 --- /dev/null +++ b/internal/destregistry/providers/destwebhook/signature_validation.go @@ -0,0 +1,46 @@ +package destwebhook + +import ( + "fmt" + "time" +) + +// dryRunFormatters renders both formatters against synthetic payloads, so a +// bad template fails provider construction instead of the first delivery. +// +// Together with the parse step in New, this catches two classes of failure: +// templates that don't parse, and templates that parse but reference a field +// the payload type doesn't have (the content and header templates render +// against different types, so borrowing a field from the wrong one is valid +// syntax). Field resolution depends on the payload's type rather than its +// values, so a synthetic payload settles that class. +// +// It is a smoke test, not a proof: value-dependent failures — `{{index +// .Signatures 2}}` with three secrets configured, or a helper that errors only +// on particular input — can still fail at delivery. Those degrade to a failed +// attempt via the error returned by Format. +func dryRunFormatters(sig SignatureFormatter, header HeaderFormatter, contentTemplate, headerTemplate string) error { + now := time.Now() + + if _, err := sig.Format(SignaturePayload{ + EventID: "evt_validation", + Topic: "validation.topic", + Timestamp: now, + Body: `{"validation":true}`, + }); err != nil { + return fmt.Errorf("invalid signature content template %q: %w", contentTemplate, err) + } + + // Two signatures, since rotation is when .Signatures holds more than one + // element and when a template that mishandles the list shows it. + if _, err := header.Format(HeaderPayload{ + EventID: "evt_validation", + Topic: "validation.topic", + Timestamp: now, + Signatures: []string{"sig_current", "sig_previous"}, + }); err != nil { + return fmt.Errorf("invalid signature header template %q: %w", headerTemplate, err) + } + + return nil +} diff --git a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go index 158f533bb..b677999ee 100644 --- a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go +++ b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go @@ -65,12 +65,23 @@ import ( "github.com/hookdeck/outpost/internal/models" ) +// Signature templates fixed by the Standard Webhooks spec. +const ( + signatureContentTemplate = "{{.EventID}}.{{.Timestamp.Unix}}.{{.Body}}" + signatureHeaderTemplate = "v1,{{index .Signatures 0}}{{range slice .Signatures 1}} v1,{{.}}{{end}}" +) + type StandardWebhookDestination struct { *destregistry.BaseProvider userAgent string proxyURL string headerPrefix string // Prefix for metadata headers (defaults to "webhook-") maxResponseBodyBytes int + + // Standard Webhooks templates are fixed by the spec, so the formatters are + // built once and shared by every publisher rather than per destination. + signatureFormatter destwebhook.SignatureFormatter + headerFormatter destwebhook.HeaderFormatter } type StandardWebhookDestinationConfig struct { @@ -136,6 +147,16 @@ func New(loader metadata.MetadataLoader, basePublisherOpts []destregistry.BasePu // headerPrefix may be empty (after trimming) to disable prefix entirely — that's valid. // But the caller must have explicitly set it via WithHeaderPrefix. // Config is responsible for providing the appropriate default ("webhook-"). + + destination.signatureFormatter, err = destwebhook.NewSignatureFormatter(signatureContentTemplate) + if err != nil { + return nil, err + } + destination.headerFormatter, err = destwebhook.NewHeaderFormatter(signatureHeaderTemplate) + if err != nil { + return nil, err + } + return destination, nil } @@ -220,15 +241,11 @@ func (d *StandardWebhookDestination) CreatePublisher(ctx context.Context, destin }) } - // Create SignatureManager with Standard Webhooks templates + // Create SignatureManager with the shared Standard Webhooks formatters sm := destwebhook.NewSignatureManager( secrets, - destwebhook.WithSignatureFormatter( - destwebhook.NewSignatureFormatter("{{.EventID}}.{{.Timestamp.Unix}}.{{.Body}}"), - ), - destwebhook.WithHeaderFormatter( - destwebhook.NewHeaderFormatter("v1,{{index .Signatures 0}}{{range slice .Signatures 1}} v1,{{.}}{{end}}"), - ), + destwebhook.WithSignatureFormatter(d.signatureFormatter), + destwebhook.WithHeaderFormatter(d.headerFormatter), destwebhook.WithEncoder(destwebhook.GetEncoder("base64")), destwebhook.WithAlgorithm(destwebhook.GetAlgorithm("hmac-sha256")), ) @@ -600,12 +617,15 @@ func (p *StandardWebhookPublisher) Format(ctx context.Context, event *models.Eve req.Header.Set(p.headerPrefix+"timestamp", strconv.FormatInt(now.Unix(), 10)) // Generate and set signature header - signatureHeader := p.sm.GenerateSignatureHeader(destwebhook.SignaturePayload{ + signatureHeader, err := p.sm.GenerateSignatureHeader(destwebhook.SignaturePayload{ EventID: messageID, Topic: event.Topic, Timestamp: now, Body: string(rawBody), }) + if err != nil { + return nil, err + } if signatureHeader != "" { req.Header.Set(p.headerPrefix+"signature", signatureHeader) }