From a95b0413f3e41da9e2c638cf64a34f98f9bf8a7c Mon Sep 17 00:00:00 2001 From: Vladimir Babin Date: Mon, 28 Sep 2026 23:42:33 +0800 Subject: [PATCH 1/2] fix(destregistry): include credentials in publisher cache key (#1085) * fix: include credentials in publisher cache key Rotating a destination's credentials (e.g. webhook rotate_secret) had no effect while the destination's publisher was cached, because MakePublisherKey only hashed ID, type and config. Include credentials in the key so a credential change resolves a fresh publisher. Hash map entries in sorted key order so the key is stable across calls. Fixes #1084 * Fold delivery metadata into publisher key; harden eviction log (review) - Include dest.DeliveryMetadata in MakePublisherKey: providers bake delivery metadata into the publisher at creation, so changing delivery_metadata had the same stale-publisher bug as credentials. Add a 'delivery metadata change' subtest (RED without the change: both events hit the pre-change publisher). - onEvict: log only the destination ID prefix, not the full cache key. The key now folds credentials and delivery metadata into an FNV hash, so the suffix is a secret-derived value that should not reach logs. - hashSortedMap: use slices.Sorted(maps.Keys(m)) per review nit. --- internal/destregistry/registry.go | 31 ++++-- internal/destregistry/registry_test.go | 133 +++++++++++++++++++++++-- 2 files changed, 150 insertions(+), 14 deletions(-) diff --git a/internal/destregistry/registry.go b/internal/destregistry/registry.go index 23978a519..a2097988d 100644 --- a/internal/destregistry/registry.go +++ b/internal/destregistry/registry.go @@ -4,8 +4,12 @@ import ( "context" "errors" "fmt" + "hash" "hash/fnv" + "maps" + "slices" "strconv" + "strings" "time" "github.com/hookdeck/outpost/internal/destregistry/metadata" @@ -110,8 +114,12 @@ func NewRegistry(cfg *Config, logger *logging.Logger) Registry { onEvict := func(key string, p Publisher) { if err := p.Close(); err != nil { // TODO: consider how to get context for OTEL logging + // Log only the destination ID prefix, not the full key: the key now + // folds credentials and delivery metadata into an FNV hash, so the + // hash suffix is a secret-derived value that should stay out of logs. + destID, _, _ := strings.Cut(key, ".") logger.Error("failed to close publisher on eviction", - zap.String("key", key), + zap.String("destination_id", destID), zap.Error(err), ) } @@ -282,17 +290,28 @@ func (r *registry) ResolveProvider(destination *models.Destination) (Provider, e return provider, nil } -// MakePublisherKey creates a unique key for a destination that includes type and config +// MakePublisherKey creates a unique key for a destination that includes type, config, +// credentials and delivery metadata. Any of these can be baked into the publisher when +// it is created, so a change in any of them must produce a new cache key. func MakePublisherKey(dest *models.Destination) string { h := fnv.New64a() - for k, v := range dest.Config { - h.Write([]byte(k)) - h.Write([]byte(v)) - } + hashSortedMap(h, dest.Config) + hashSortedMap(h, dest.Credentials) + hashSortedMap(h, dest.DeliveryMetadata) h.Write([]byte(dest.Type)) return dest.ID + "." + strconv.FormatUint(h.Sum64(), 36) } +func hashSortedMap(h hash.Hash64, m map[string]string) { + for _, k := range slices.Sorted(maps.Keys(m)) { + h.Write([]byte(k)) + h.Write([]byte{0}) + h.Write([]byte(m[k])) + h.Write([]byte{0}) + } + h.Write([]byte{0}) +} + func (r *registry) ResolvePublisher(ctx context.Context, destination *models.Destination) (Publisher, error) { key := MakePublisherKey(destination) diff --git a/internal/destregistry/registry_test.go b/internal/destregistry/registry_test.go index 4fb76cc8a..bf7d49016 100644 --- a/internal/destregistry/registry_test.go +++ b/internal/destregistry/registry_test.go @@ -166,21 +166,27 @@ func TestRegistryConcurrentPublisherManagement(t *testing.T) { // Track where events are published by provider type and config type publishTarget struct { - providerType string - config string + providerType string + config string + credential string + deliveryMetadata string } var publishedEvents = make(map[publishTarget][]models.Event) type mockPublisherWithConfig struct { - providerType string - config string + providerType string + config string + credential string + deliveryMetadata string } func (p *mockPublisherWithConfig) Publish(ctx context.Context, event *models.Event) (*destregistry.Delivery, error) { target := publishTarget{ - providerType: p.providerType, - config: p.config, + providerType: p.providerType, + config: p.config, + credential: p.credential, + deliveryMetadata: p.deliveryMetadata, } publishedEvents[target] = append(publishedEvents[target], *event) return &destregistry.Delivery{ @@ -211,8 +217,10 @@ func newMockProviderWithConfig(providerType string) (*mockProviderWithConfig, er func (p *mockProviderWithConfig) CreatePublisher(ctx context.Context, destination *models.Destination) (destregistry.Publisher, error) { return &mockPublisherWithConfig{ - providerType: p.providerType, - config: destination.Config["id"], + providerType: p.providerType, + config: destination.Config["id"], + credential: destination.Credentials["secret"], + deliveryMetadata: destination.DeliveryMetadata["source"], }, nil } @@ -338,6 +346,115 @@ func TestDestinationChanges(t *testing.T) { assert.JSONEq(t, `{"msg":"second"}`, string(secondEvents[0].Data)) } }) + + t.Run("credentials change", func(t *testing.T) { + publishedEvents = make(map[publishTarget][]models.Event) + registry := destregistry.NewRegistry(&destregistry.Config{}, testutil.CreateTestLogger(t)) + provider, err := newMockProviderWithConfig("mock1") + require.NoError(t, err) + registry.RegisterProvider("mock1", provider) + + dest := &models.Destination{ + ID: "test-dest", + Type: "mock1", + Config: map[string]string{ + "id": "config1", + }, + Credentials: map[string]string{ + "secret": "secret1", + }, + } + + e1 := testutil.EventFactory.Any(testutil.EventFactory.WithDataMap(map[string]interface{}{"msg": "first"})) + _, err = registry.PublishEvent(context.Background(), dest, &e1) + require.NoError(t, err) + + // Same ID, type and config; only credentials rotated + destUpdated := &models.Destination{ + ID: "test-dest", + Type: "mock1", + Config: map[string]string{ + "id": "config1", + }, + Credentials: map[string]string{ + "secret": "secret2", + "previous_secret": "secret1", + }, + } + + e2 := testutil.EventFactory.Any(testutil.EventFactory.WithDataMap(map[string]interface{}{"msg": "second"})) + _, err = registry.PublishEvent(context.Background(), destUpdated, &e2) + require.NoError(t, err) + + firstTarget := publishTarget{providerType: "mock1", config: "config1", credential: "secret1"} + secondTarget := publishTarget{providerType: "mock1", config: "config1", credential: "secret2"} + + firstEvents := publishedEvents[firstTarget] + assert.Len(t, firstEvents, 1, "only first event should be published with secret1") + if len(firstEvents) > 0 { + assert.JSONEq(t, `{"msg":"first"}`, string(firstEvents[0].Data)) + } + + secondEvents := publishedEvents[secondTarget] + assert.Len(t, secondEvents, 1, "second event should be published with secret2") + if len(secondEvents) > 0 { + assert.JSONEq(t, `{"msg":"second"}`, string(secondEvents[0].Data)) + } + }) + + t.Run("delivery metadata change", func(t *testing.T) { + publishedEvents = make(map[publishTarget][]models.Event) + registry := destregistry.NewRegistry(&destregistry.Config{}, testutil.CreateTestLogger(t)) + provider, err := newMockProviderWithConfig("mock1") + require.NoError(t, err) + registry.RegisterProvider("mock1", provider) + + dest := &models.Destination{ + ID: "test-dest", + Type: "mock1", + Config: map[string]string{ + "id": "config1", + }, + DeliveryMetadata: map[string]string{ + "source": "source1", + }, + } + + e1 := testutil.EventFactory.Any(testutil.EventFactory.WithDataMap(map[string]interface{}{"msg": "first"})) + _, err = registry.PublishEvent(context.Background(), dest, &e1) + require.NoError(t, err) + + // Same ID, type, config and credentials; only delivery metadata changed + destUpdated := &models.Destination{ + ID: "test-dest", + Type: "mock1", + Config: map[string]string{ + "id": "config1", + }, + DeliveryMetadata: map[string]string{ + "source": "source2", + }, + } + + e2 := testutil.EventFactory.Any(testutil.EventFactory.WithDataMap(map[string]interface{}{"msg": "second"})) + _, err = registry.PublishEvent(context.Background(), destUpdated, &e2) + require.NoError(t, err) + + firstTarget := publishTarget{providerType: "mock1", config: "config1", deliveryMetadata: "source1"} + secondTarget := publishTarget{providerType: "mock1", config: "config1", deliveryMetadata: "source2"} + + firstEvents := publishedEvents[firstTarget] + assert.Len(t, firstEvents, 1, "only first event should use source1") + if len(firstEvents) > 0 { + assert.JSONEq(t, `{"msg":"first"}`, string(firstEvents[0].Data)) + } + + secondEvents := publishedEvents[secondTarget] + assert.Len(t, secondEvents, 1, "second event should use source2") + if len(secondEvents) > 0 { + assert.JSONEq(t, `{"msg":"second"}`, string(secondEvents[0].Data)) + } + }) } func TestPublisherExpiration(t *testing.T) { From d0342dbf8643eebe24919ec4331ea51778d5aa2a Mon Sep 17 00:00:00 2001 From: Alex Luong Date: Mon, 28 Sep 2026 22:48:11 +0700 Subject: [PATCH 2/2] fix(apirouter): keep destination credentials out of attempts and request logs (#1095) * fix(apirouter): omit credentials from include=destination on attempts Attempts embed the destination without credentials. Credentials are only returned by the destination endpoints. The spec gets an AttemptDestination schema, since the per-type Destination schemas require credentials. Refs #1094 Co-Authored-By: Claude Opus 5.5 (1M context) * fix(apirouter): redact all credential values from logged request bodies Redaction relied on provider metadata, which skipped credentials not marked sensitive (webhook secrets) and bodies without a type (PATCH). Every credential value is now redacted. Refs #1094 Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) --- docs/apis/openapi.yaml | 83 +++++++++++++++++-- internal/apirouter/log_handlers.go | 9 +- internal/apirouter/log_handlers_test.go | 7 +- .../logger_middleware_integration_test.go | 18 +++- internal/apirouter/sanitizer.go | 13 +-- internal/apirouter/sanitizer_test.go | 17 ++++ 6 files changed, 130 insertions(+), 17 deletions(-) diff --git a/docs/apis/openapi.yaml b/docs/apis/openapi.yaml index b514cc505..06edd5480 100644 --- a/docs/apis/openapi.yaml +++ b/docs/apis/openapi.yaml @@ -2751,6 +2751,75 @@ components: additionalProperties: true example: { "user_id": "userid", "status": "active" } # Attempt schemas for attempts-first API + AttemptDestination: + type: object + description: The destination an attempt was sent to, without credentials. Use the destination endpoints to retrieve credentials. + required: [id, tenant_id, type, topics, config, created_at, updated_at, disabled_at, target] + properties: + id: + type: string + description: Control plane generated ID or user provided ID for the destination. + example: "des_12345" + tenant_id: + type: string + description: The tenant the destination belongs to. + example: "tenant_123" + type: + type: string + description: Type of the destination. + example: "webhook" + topics: + $ref: "#/components/schemas/Topics" + filter: + $ref: "#/components/schemas/Filter" + config: + type: object + additionalProperties: + type: string + description: Type-specific destination configuration. + example: { "url": "https://my-service.com/webhook/handler" } + delivery_metadata: + type: object + additionalProperties: + type: string + nullable: true + description: Static key-value pairs merged into event metadata on every attempt. + example: { "app-id": "my-app", "region": "us-east-1" } + metadata: + type: object + additionalProperties: + type: string + nullable: true + description: Arbitrary contextual information stored with the destination. + example: { "internal-id": "123", "team": "platform" } + disabled_at: + type: string + format: date-time + nullable: true + description: ISO Date when the destination was disabled, or null if enabled. + example: null + created_at: + type: string + format: date-time + description: ISO Date when the destination was created. + example: "2024-01-01T00:00:00Z" + updated_at: + type: string + format: date-time + description: ISO Date when the destination was last updated. + example: "2024-01-01T00:00:00Z" + target: + type: string + description: A human-readable representation of the destination target (e.g., URL host). Read-only. + readOnly: true + example: "my-service.com" + target_url: + type: string + format: url + nullable: true + description: A URL link to the destination target. Read-only. + readOnly: true + example: "https://my-service.com/webhook/handler" Attempt: type: object description: An attempt represents a single delivery attempt of an event to a destination. @@ -2812,8 +2881,8 @@ components: description: The associated event object. Only present when include=event or include=event.data. destination: nullable: true - $ref: "#/components/schemas/Destination" - description: The destination object. Only present when include=destination. + $ref: "#/components/schemas/AttemptDestination" + description: The destination object, without credentials. Only present when include=destination. EventSummary: type: object description: Event object without data (returned when include=event). @@ -3357,7 +3426,7 @@ tags: - `include=event`: Include event summary (id, topic, time, eligible_for_retry, metadata) - `include=event.data`: Include full event with payload data - `include=response_data`: Include response body and headers from the attempt - - `include=destination`: Include the full destination object with target information + - `include=destination`: Include the destination object with target information, without credentials - name: Events description: | An event represents a payload published to Outpost. Events are matched against all destinations whose topic subscriptions match the event topic, then delivered. @@ -4034,7 +4103,7 @@ paths: - `event`: Include event summary (id, topic, time, eligible_for_retry, metadata) - `event.data`: Include full event with payload data - `response_data`: Include response body and headers - - `destination`: Include the full destination object + - `destination`: Include the destination object, without credentials - name: order_by in: query required: false @@ -4153,7 +4222,7 @@ paths: - `event`: Include event summary (id, topic, time, eligible_for_retry, metadata) - `event.data`: Include full event with payload data - `response_data`: Include response body and headers - - `destination`: Include the full destination object + - `destination`: Include the destination object, without credentials responses: "200": description: Attempt details. @@ -4726,7 +4795,7 @@ paths: - `event`: Include event summary (id, topic, time, eligible_for_retry, metadata) - `event.data`: Include full event with payload data - `response_data`: Include response body and headers - - `destination`: Include the full destination object + - `destination`: Include the destination object, without credentials - name: order_by in: query required: false @@ -4824,7 +4893,7 @@ paths: - `event`: Include event summary - `event.data`: Include full event with payload data - `response_data`: Include response body and headers - - `destination`: Include the full destination object + - `destination`: Include the destination object, without credentials responses: "200": description: Attempt details. diff --git a/internal/apirouter/log_handlers.go b/internal/apirouter/log_handlers.go index 54053c111..581359f6e 100644 --- a/internal/apirouter/log_handlers.go +++ b/internal/apirouter/log_handlers.go @@ -98,6 +98,13 @@ type APIAttempt struct { Destination interface{} `json:"destination,omitempty"` } +// APIAttemptDestination is the destination object when include=destination. +// Credentials are only returned by the destination endpoints. +type APIAttemptDestination struct { + *destregistry.DestinationDisplay + Credentials *struct{} `json:"credentials,omitempty"` // shadows the embedded credentials +} + // APIEventSummary is the event object when expand=event (without data) type APIEventSummary struct { ID string `json:"id"` @@ -193,7 +200,7 @@ func toAPIAttempt(ar *logstore.AttemptRecord, opts IncludeOptions, destDisplay * } if opts.Destination && destDisplay != nil { - api.Destination = destDisplay + api.Destination = APIAttemptDestination{DestinationDisplay: destDisplay} } return api diff --git a/internal/apirouter/log_handlers_test.go b/internal/apirouter/log_handlers_test.go index 360b228bd..a9ff8aa15 100644 --- a/internal/apirouter/log_handlers_test.go +++ b/internal/apirouter/log_handlers_test.go @@ -1293,7 +1293,7 @@ func TestAPI_Attempts(t *testing.T) { h := newAPITest(t) require.NoError(t, h.tenantStore.UpsertTenant(t.Context(), tf.Any(tf.WithID("t1")))) - dest := df.Any(df.WithID("d1"), df.WithTenantID("t1")) + dest := df.Any(df.WithID("d1"), df.WithTenantID("t1"), df.WithCredentials(map[string]string{"secret": "whsec_test"})) require.NoError(t, h.tenantStore.CreateDestination(t.Context(), dest)) e := ef.AnyPointer(ef.WithID("e1"), ef.WithTenantID("t1"), ef.WithDestinationID("d1")) @@ -1314,13 +1314,15 @@ func TestAPI_Attempts(t *testing.T) { require.True(t, ok, "destination should be an object when include=destination") assert.Equal(t, "d1", destMap["id"]) assert.Equal(t, "t1", destMap["tenant_id"]) + assert.Contains(t, destMap, "target") + assert.NotContains(t, destMap, "credentials") }) t.Run("include destination on list populates destination", func(t *testing.T) { h := newAPITest(t) require.NoError(t, h.tenantStore.UpsertTenant(t.Context(), tf.Any(tf.WithID("t1")))) - dest := df.Any(df.WithID("d1"), df.WithTenantID("t1")) + dest := df.Any(df.WithID("d1"), df.WithTenantID("t1"), df.WithCredentials(map[string]string{"secret": "whsec_test"})) require.NoError(t, h.tenantStore.CreateDestination(t.Context(), dest)) e := ef.AnyPointer(ef.WithID("e1"), ef.WithTenantID("t1"), ef.WithDestinationID("d1")) @@ -1346,6 +1348,7 @@ func TestAPI_Attempts(t *testing.T) { destMap, ok := attempt["destination"].(map[string]any) require.True(t, ok, "destination should be an object when include=destination") assert.Equal(t, "d1", destMap["id"]) + assert.NotContains(t, destMap, "credentials") }) t.Run("without include destination field is omitted", func(t *testing.T) { diff --git a/internal/apirouter/logger_middleware_integration_test.go b/internal/apirouter/logger_middleware_integration_test.go index 60062a942..f1bfaab34 100644 --- a/internal/apirouter/logger_middleware_integration_test.go +++ b/internal/apirouter/logger_middleware_integration_test.go @@ -470,6 +470,20 @@ func TestLoggerMiddleware_DifferentDestinationTypes(t *testing.T) { expectedMask: []string{}, expectedKeep: []string{"https://public-api.example.com/webhook", "POST", "webhook"}, }, + { + name: "credential not marked sensitive in metadata", + requestBody: map[string]interface{}{ + "type": "webhook", + "config": map[string]interface{}{ + "url": "https://example.com/webhook", + }, + "credentials": map[string]interface{}{ + "secret": "whsec_abc123def456", + }, + }, + expectedMask: []string{"whsec_abc123def456"}, + expectedKeep: []string{"webhook", "https://example.com/webhook"}, + }, { name: "unknown destination type", requestBody: map[string]interface{}{ @@ -478,8 +492,8 @@ func TestLoggerMiddleware_DifferentDestinationTypes(t *testing.T) { "some_field": "some_value", }, }, - expectedMask: []string{}, - expectedKeep: []string{"unknown_type", "some_value"}, // Unknown type won't be sanitized + expectedMask: []string{"some_value"}, + expectedKeep: []string{"unknown_type"}, }, } diff --git a/internal/apirouter/sanitizer.go b/internal/apirouter/sanitizer.go index 97ec8d188..9875bf5b5 100644 --- a/internal/apirouter/sanitizer.go +++ b/internal/apirouter/sanitizer.go @@ -75,6 +75,14 @@ func (s *RequestBodySanitizer) sanitizeDestinationRequest(data map[string]interf sanitized[k] = v } + if credentials, ok := sanitized["credentials"].(map[string]interface{}); ok { + redacted := make(map[string]interface{}, len(credentials)) + for k := range credentials { + redacted[k] = SensitiveFieldMask + } + sanitized["credentials"] = redacted + } + // Get the destination type to load metadata destinationType, ok := sanitized["type"].(string) if !ok { @@ -87,11 +95,6 @@ func (s *RequestBodySanitizer) sanitizeDestinationRequest(data map[string]interf return sanitized // If we can't load metadata, return as-is } - // Sanitize credentials field based on metadata - if credentials, ok := sanitized["credentials"].(map[string]interface{}); ok { - sanitized["credentials"] = s.sanitizeFieldsMap(credentials, meta.CredentialFields) - } - // Also check config fields for any marked as sensitive if config, ok := sanitized["config"].(map[string]interface{}); ok { sanitized["config"] = s.sanitizeFieldsMap(config, meta.ConfigFields) diff --git a/internal/apirouter/sanitizer_test.go b/internal/apirouter/sanitizer_test.go index 23f1bb33a..9e90e838f 100644 --- a/internal/apirouter/sanitizer_test.go +++ b/internal/apirouter/sanitizer_test.go @@ -97,6 +97,23 @@ func TestRequestBodySanitizer_SanitizeDestinationRequest(t *testing.T) { } } +func TestRequestBodySanitizer_RedactsAllCredentials(t *testing.T) { + sanitizer := &RequestBodySanitizer{} + + // No type, as in a PATCH body, so metadata can't be loaded. + input := `{"credentials":{"secret":"whsec_abc123","username":"app_user"},"topics":["user.created"]}` + + result, err := sanitizer.SanitizeRequestBody(strings.NewReader(input)) + if err != nil { + t.Fatalf("SanitizeRequestBody() error = %v", err) + } + + expected := `{"credentials":{"secret":"[REDACTED]","username":"[REDACTED]"},"topics":["user.created"]}` + if string(result) != expected { + t.Errorf("SanitizeRequestBody() = %s, want %s", result, expected) + } +} + func TestRequestBodySanitizer_SizeLimit(t *testing.T) { sanitizer := &RequestBodySanitizer{}