From 955c2504cfc0f854732b57c44c2f6cc9f1427d53 Mon Sep 17 00:00:00 2001 From: Alex Luong Date: Fri, 12 Jun 2026 15:03:56 +0700 Subject: [PATCH 1/2] test(apirouter): rotation without invalid_at must re-apply 24h default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the rotation test: rotate with an explicit previous_secret_invalid_at, then rotate again without one. The second rotation must default to now+24h, but the stored window is carried forward instead — the Update handler merge-patches stored credentials into the request before Preprocess, so rotateSecret mistakes the stored timestamp for a caller-provided one. Once that timestamp is in the past, every plain rotation produces an already-expired previous secret. Failing test; fix follows. Co-Authored-By: Claude Fable 5 --- .../apirouter/destination_credentials_test.go | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/internal/apirouter/destination_credentials_test.go b/internal/apirouter/destination_credentials_test.go index 6aab079d4..c1a4ef516 100644 --- a/internal/apirouter/destination_credentials_test.go +++ b/internal/apirouter/destination_credentials_test.go @@ -90,6 +90,42 @@ func TestDestinationCredentials_SecretRotationViaAPI(t *testing.T) { "previous_secret should be the old secret") assert.NotEmpty(t, rotated.Credentials["previous_secret_invalid_at"], "previous_secret_invalid_at should be set") + + // Rotate again with an explicit invalidation window + customInvalidAt := time.Now().Add(1 * time.Hour).Format(time.RFC3339) + secondRotateReq := h.jsonReq(http.MethodPatch, "/api/v1/tenants/t1/destinations/d1", map[string]any{ + "credentials": map[string]any{ + "rotate_secret": true, + "previous_secret_invalid_at": customInvalidAt, + }, + }) + secondRotateResp := h.do(h.withAPIKey(secondRotateReq)) + require.Equal(t, http.StatusOK, secondRotateResp.Code) + + var secondRotated destregistry.DestinationDisplay + require.NoError(t, json.Unmarshal(secondRotateResp.Body.Bytes(), &secondRotated)) + assert.Equal(t, customInvalidAt, secondRotated.Credentials["previous_secret_invalid_at"], + "explicit previous_secret_invalid_at should be respected") + + // Rotate once more without previous_secret_invalid_at: the 24h default + // must be re-applied, not the previously stored window carried forward + thirdRotateReq := h.jsonReq(http.MethodPatch, "/api/v1/tenants/t1/destinations/d1", map[string]any{ + "credentials": map[string]any{ + "rotate_secret": true, + }, + }) + thirdRotateResp := h.do(h.withAPIKey(thirdRotateReq)) + require.Equal(t, http.StatusOK, thirdRotateResp.Code) + + var thirdRotated destregistry.DestinationDisplay + require.NoError(t, json.Unmarshal(thirdRotateResp.Body.Bytes(), &thirdRotated)) + assert.Equal(t, secondRotated.Credentials["secret"], thirdRotated.Credentials["previous_secret"], + "previous_secret should be the secret from the previous rotation") + + invalidAt, err := time.Parse(time.RFC3339, thirdRotated.Credentials["previous_secret_invalid_at"]) + require.NoError(t, err) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), invalidAt, time.Minute, + "omitting previous_secret_invalid_at should default to now+24h") } func TestDestinationCredentials_TenantCannotSetCustomSecret(t *testing.T) { From bc735f3535b86d72c38a9206bfb934f8dd6ecfbb Mon Sep 17 00:00:00 2001 From: Alex Luong Date: Fri, 12 Jun 2026 15:09:18 +0700 Subject: [PATCH 2/2] fix(destwebhook): re-apply 24h rotation default by reading the request, not merged state The Update handler merge-patches stored credentials into the destination before Preprocess, so on any rotation after the first rotateSecret saw the stored previous_secret_invalid_at as caller-provided and skipped the documented now+24h default. Once the stored timestamp was in the past, every subsequent plain rotation produced an already-expired previous secret, which ObfuscateDestination then stripped from API responses. Give Preprocess the caller's view of the provider-owned fields via PreprocessDestinationOpts.Request (config and credentials exactly as sent, before merging), and have rotateSecret read the custom invalidation window from there instead of inferring intent from the merged map. Applies to both webhook providers (legacy and standard), which duplicate the rotation logic. Co-Authored-By: Claude Fable 5 --- internal/apirouter/destination_handlers.go | 32 ++++++++---- .../providers/destwebhook/destwebhook.go | 12 +++-- .../destwebhook/destwebhook_validate_test.go | 52 ++++++++++++++++++- .../destwebhookstandard.go | 12 +++-- .../destwebhookstandard_validate_test.go | 52 ++++++++++++++++++- internal/destregistry/registry.go | 13 +++++ 6 files changed, 148 insertions(+), 25 deletions(-) diff --git a/internal/apirouter/destination_handlers.go b/internal/apirouter/destination_handlers.go index 3339a7500..38e7ff635 100644 --- a/internal/apirouter/destination_handlers.go +++ b/internal/apirouter/destination_handlers.go @@ -121,6 +121,10 @@ func (h *DestinationHandlers) Create(c *gin.Context) { } if err := h.registry.PreprocessDestination(&destination, nil, &destregistry.PreprocessDestinationOpts{ Role: mustRoleFromContext(c), + Request: destregistry.PreprocessRequest{ + Config: destination.Config, + Credentials: destination.Credentials, + }, }); err != nil { AbortWithValidationError(c, err) return @@ -193,7 +197,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // Config (merge-patch) - configResult, configChanged, err := applyMergePatchStringMap(originalDestination.Config, input.Config) + configResult, configRequest, configChanged, err := applyMergePatchStringMap(originalDestination.Config, input.Config) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid config: %w", err)) return @@ -204,7 +208,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // Credentials (merge-patch) - credsResult, credsChanged, err := applyMergePatchStringMap(originalDestination.Credentials, input.Credentials) + credsResult, credsRequest, credsChanged, err := applyMergePatchStringMap(originalDestination.Credentials, input.Credentials) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid credentials: %w", err)) return @@ -229,7 +233,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // DeliveryMetadata (merge-patch) - dmResult, dmChanged, err := applyMergePatchStringMap(originalDestination.DeliveryMetadata, input.DeliveryMetadata) + dmResult, _, dmChanged, err := applyMergePatchStringMap(originalDestination.DeliveryMetadata, input.DeliveryMetadata) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid delivery_metadata: %w", err)) return @@ -239,7 +243,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // Metadata (merge-patch) - metaResult, metaChanged, err := applyMergePatchStringMap(originalDestination.Metadata, input.Metadata) + metaResult, _, metaChanged, err := applyMergePatchStringMap(originalDestination.Metadata, input.Metadata) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid metadata: %w", err)) return @@ -279,6 +283,10 @@ func (h *DestinationHandlers) Update(c *gin.Context) { // Always preprocess before updating if err := h.registry.PreprocessDestination(&updatedDestination, originalDestination, &destregistry.PreprocessDestinationOpts{ Role: mustRoleFromContext(c), + Request: destregistry.PreprocessRequest{ + Config: configRequest, + Credentials: credsRequest, + }, }); err != nil { AbortWithValidationError(c, err) return @@ -507,26 +515,28 @@ func isJSONNull(raw json.RawMessage) bool { } // applyMergePatchStringMap applies RFC 7396 merge-patch semantics for a map[string]string field. -// Returns (result, changed, error): +// Returns (result, request, changed, error), where request is the patch as +// sent by the caller (string-coerced, nulls dropped): // - raw is nil (field omitted): returns original unchanged // - raw is "null": returns nil (clear field) // - raw is "{}": returns original unchanged (empty merge = no-op) // - raw is an object: merge-patch into original -func applyMergePatchStringMap(original map[string]string, raw json.RawMessage) (map[string]string, bool, error) { +func applyMergePatchStringMap(original map[string]string, raw json.RawMessage) (map[string]string, map[string]string, bool, error) { if raw == nil { - return original, false, nil + return original, nil, false, nil } if isJSONNull(raw) { - return nil, true, nil + return nil, nil, true, nil } var patch map[string]any if err := json.Unmarshal(raw, &patch); err != nil { - return nil, false, err + return nil, nil, false, err } if len(patch) == 0 { - return original, false, nil + return original, nil, false, nil } - return maputil.MergePatchStringMap(original, patch), true, nil + request := maputil.MergePatchStringMap(nil, patch) + return maputil.MergePatchStringMap(original, patch), request, true, nil } // tenantSnapshot captures the tenant's derived state before a destination mutation. diff --git a/internal/destregistry/providers/destwebhook/destwebhook.go b/internal/destregistry/providers/destwebhook/destwebhook.go index 21ebef459..0162a2e23 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook.go +++ b/internal/destregistry/providers/destwebhook/destwebhook.go @@ -443,7 +443,7 @@ func (d *WebhookDestination) resolveConfig(ctx context.Context, destination *mod } // rotateSecret handles secret rotation and returns clean credentials -func (d *WebhookDestination) rotateSecret(newDest, origDest *models.Destination) (map[string]string, error) { +func (d *WebhookDestination) rotateSecret(origDest *models.Destination, opts *destregistry.PreprocessDestinationOpts) (map[string]string, error) { if origDest == nil { return nil, destregistry.NewErrDestinationValidation([]destregistry.ValidationErrorDetail{ { @@ -474,9 +474,11 @@ func (d *WebhookDestination) rotateSecret(newDest, origDest *models.Destination) } creds["secret"] = secret - // Keep custom invalidation time if provided, otherwise set default - if newDest.Credentials["previous_secret_invalid_at"] != "" { - creds["previous_secret_invalid_at"] = newDest.Credentials["previous_secret_invalid_at"] + // Keep custom invalidation time if provided, otherwise set default. + // The merged credentials can't tell us whether the caller sent the + // field — the raw request credentials can. + if invalidAt := opts.Request.Credentials["previous_secret_invalid_at"]; invalidAt != "" { + creds["previous_secret_invalid_at"] = invalidAt } else { creds["previous_secret_invalid_at"] = time.Now().Add(24 * time.Hour).Format(time.RFC3339) } @@ -607,7 +609,7 @@ func (d *WebhookDestination) Preprocess(newDestination *models.Destination, orig var cleanCredentials map[string]string var err error if isTruthy(newDestination.Credentials["rotate_secret"]) { - cleanCredentials, err = d.rotateSecret(newDestination, originalDestination) + cleanCredentials, err = d.rotateSecret(originalDestination, opts) } else { cleanCredentials, err = d.updateSecret(newDestination, originalDestination, opts) // For new destinations, ensure credentials are initialized if needed diff --git a/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go b/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go index d85dbe27c..860715d35 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go +++ b/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go @@ -606,17 +606,65 @@ func TestWebhookDestination_Preprocess(t *testing.T) { }), ) - // Merge both config and credentials to simulate handler behavior + // Merge both config and credentials to simulate handler behavior, + // passing the raw request credentials via opts as the handler does + requestCredentials := newDestination.Credentials newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) - err := webhookDestination.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{}) + err := webhookDestination.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) require.NoError(t, err) // Verify that the custom invalidation time was preserved assert.Equal(t, customInvalidAt, newDestination.Credentials["previous_secret_invalid_at"]) }) + t.Run("should apply 24h default on rotation when request omits invalid_at", func(t *testing.T) { + t.Parallel() + storedInvalidAt := time.Now().Add(1 * time.Hour).Format(time.RFC3339) + originalDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "secret": "current-secret", + "previous_secret": "older-secret", + "previous_secret_invalid_at": storedInvalidAt, + }), + ) + + newDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "rotate_secret": "true", + }), + ) + + // Merge both config and credentials to simulate handler behavior: + // the stored previous_secret_invalid_at is merged into newDestination + // even though the caller's request didn't contain it. + requestCredentials := newDestination.Credentials + newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) + newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) + + err := webhookDestination.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) + require.NoError(t, err) + + // The merged-in stored value must not be carried forward; the + // default window applies. + invalidAt, err := time.Parse(time.RFC3339, newDestination.Credentials["previous_secret_invalid_at"]) + require.NoError(t, err) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), invalidAt, 5*time.Second) + }) + t.Run("should set default previous_secret_invalid_at when previous_secret is provided", func(t *testing.T) { t.Parallel() originalDestination := testutil.DestinationFactory.Any( diff --git a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go index 62acf0fb5..fc4f098b1 100644 --- a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go +++ b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go @@ -344,7 +344,7 @@ func (d *StandardWebhookDestination) resolveConfig(ctx context.Context, destinat } // rotateSecret handles secret rotation and returns clean credentials -func (d *StandardWebhookDestination) rotateSecret(newDest, origDest *models.Destination) (map[string]string, error) { +func (d *StandardWebhookDestination) rotateSecret(origDest *models.Destination, opts *destregistry.PreprocessDestinationOpts) (map[string]string, error) { if origDest == nil { return nil, destregistry.NewErrDestinationValidation([]destregistry.ValidationErrorDetail{ { @@ -375,9 +375,11 @@ func (d *StandardWebhookDestination) rotateSecret(newDest, origDest *models.Dest } creds["secret"] = secret - // Keep custom invalidation time if provided, otherwise set default - if newDest.Credentials["previous_secret_invalid_at"] != "" { - creds["previous_secret_invalid_at"] = newDest.Credentials["previous_secret_invalid_at"] + // Keep custom invalidation time if provided, otherwise set default. + // The merged credentials can't tell us whether the caller sent the + // field — the raw request credentials can. + if invalidAt := opts.Request.Credentials["previous_secret_invalid_at"]; invalidAt != "" { + creds["previous_secret_invalid_at"] = invalidAt } else { creds["previous_secret_invalid_at"] = time.Now().Add(24 * time.Hour).Format(time.RFC3339) } @@ -508,7 +510,7 @@ func (d *StandardWebhookDestination) Preprocess(newDestination *models.Destinati var cleanCredentials map[string]string var err error if isTruthy(newDestination.Credentials["rotate_secret"]) { - cleanCredentials, err = d.rotateSecret(newDestination, originalDestination) + cleanCredentials, err = d.rotateSecret(originalDestination, opts) } else { cleanCredentials, err = d.updateSecret(newDestination, originalDestination, opts) // For new destinations, ensure credentials are initialized if needed diff --git a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go index 0d1c23d8e..115a47191 100644 --- a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go +++ b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go @@ -553,17 +553,65 @@ func TestStandardWebhookDestination_Preprocess(t *testing.T) { }), ) - // Merge both config and credentials to simulate handler behavior + // Merge both config and credentials to simulate handler behavior, + // passing the raw request credentials via opts as the handler does + requestCredentials := newDestination.Credentials newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) - err := provider.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{}) + err := provider.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) require.NoError(t, err) // Verify that the custom invalidation time was preserved assert.Equal(t, customInvalidAt, newDestination.Credentials["previous_secret_invalid_at"]) }) + t.Run("should apply 24h default on rotation when request omits invalid_at", func(t *testing.T) { + t.Parallel() + storedInvalidAt := time.Now().Add(1 * time.Hour).Format(time.RFC3339) + originalDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "secret": "whsec_CurrentSecretBase64EncodedString", + "previous_secret": "whsec_OlderSecretBase64EncodedStringgg", + "previous_secret_invalid_at": storedInvalidAt, + }), + ) + + newDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "rotate_secret": "true", + }), + ) + + // Merge both config and credentials to simulate handler behavior: + // the stored previous_secret_invalid_at is merged into newDestination + // even though the caller's request didn't contain it. + requestCredentials := newDestination.Credentials + newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) + newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) + + err := provider.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) + require.NoError(t, err) + + // The merged-in stored value must not be carried forward; the + // default window applies. + invalidAt, err := time.Parse(time.RFC3339, newDestination.Credentials["previous_secret_invalid_at"]) + require.NoError(t, err) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), invalidAt, 5*time.Second) + }) + t.Run("should set default previous_secret_invalid_at when previous_secret is provided", func(t *testing.T) { t.Parallel() originalDestination := testutil.DestinationFactory.Any( diff --git a/internal/destregistry/registry.go b/internal/destregistry/registry.go index 7bf6ba840..385c6d70b 100644 --- a/internal/destregistry/registry.go +++ b/internal/destregistry/registry.go @@ -19,6 +19,19 @@ import ( // PreprocessDestinationOpts contains options for preprocessing a destination type PreprocessDestinationOpts struct { Role string + // Request holds the destination fields exactly as the caller sent them in + // the API request. On updates, newDestination carries the result of + // merge-patching the request into the stored values, so it cannot answer + // "did the caller provide this field" — the request can. + Request PreprocessRequest +} + +// PreprocessRequest is the caller's view of the provider-owned destination +// fields, before any merging with stored state. Maps are nil when the request +// did not contain the corresponding field. +type PreprocessRequest struct { + Config map[string]string + Credentials map[string]string } // Registry manages providers, their metadata, and publishers