ks-404 validate ids used in transmission schedule (#14116)

.changeset/slimy-cars-sparkle.md

@@ -0,0 +1,5 @@
+---
+"chainlink": patch
+---
+
+#internal ks-404 validate ids before using as seed of transmission schedule

core/capabilities/ccip/launcher/test_helpers.go

@@ -24,7 +24,7 @@ var (
 	p2pID1            = getP2PID(1)
 	p2pID2            = getP2PID(2)
 	defaultCapCfgs    = map[string]registrysyncer.CapabilityConfiguration{
-		defaultCapability.ID: registrysyncer.CapabilityConfiguration{},
+		defaultCapability.ID: {},
 	}
 	defaultRegistryDon = registrysyncer.DON{
 		DON:                      getDON(1, []ragep2ptypes.PeerID{p2pID1}, 0),

core/capabilities/remote/dispatcher.go

@@ -13,7 +13,6 @@ import (
 
 	"github.com/smartcontractkit/chainlink-common/pkg/services"
 	"github.com/smartcontractkit/chainlink-common/pkg/types/core"
-
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types"
 	remotetypes "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types"
 	"github.com/smartcontractkit/chainlink/v2/core/logger"

core/capabilities/remote/target/client.go

@@ -12,6 +12,7 @@ import (
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote"
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/target/request"
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types"
+	"github.com/smartcontractkit/chainlink/v2/core/capabilities/validation"
 	"github.com/smartcontractkit/chainlink/v2/core/logger"
 )
 
@@ -172,8 +173,12 @@ func (c *client) Receive(ctx context.Context, msg *types.MessageBody) {
 }
 
 func GetMessageIDForRequest(req commoncap.CapabilityRequest) (string, error) {
-	if !remote.IsValidWorkflowOrExecutionID(req.Metadata.WorkflowID) || !remote.IsValidWorkflowOrExecutionID(req.Metadata.WorkflowExecutionID) {
-		return "", errors.New("workflow ID and workflow execution ID in request metadata are invalid")
+	if err := validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowID); err != nil {
+		return "", fmt.Errorf("workflow ID is invalid: %w", err)
+	}
+
+	if err := validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowExecutionID); err != nil {
+		return "", fmt.Errorf("workflow execution ID is invalid: %w", err)
 	}
 
 	return req.Metadata.WorkflowID + req.Metadata.WorkflowExecutionID, nil

core/capabilities/remote/target/server.go

@@ -14,6 +14,7 @@ import (
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote"
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/target/request"
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types"
+	"github.com/smartcontractkit/chainlink/v2/core/capabilities/validation"
 	p2ptypes "github.com/smartcontractkit/chainlink/v2/core/services/p2p/types"
 
 	"github.com/smartcontractkit/chainlink/v2/core/logger"
@@ -210,7 +211,7 @@ func (r *server) getMessageHash(msg *types.MessageBody) ([32]byte, error) {
 
 func GetMessageID(msg *types.MessageBody) (string, error) {
 	idStr := string(msg.MessageId)
-	if !remote.IsValidID(idStr) {
+	if !validation.IsValidID(idStr) {
 		return "", fmt.Errorf("invalid message id")
 	}
 	return idStr, nil

core/capabilities/remote/trigger_publisher.go

@@ -10,6 +10,7 @@ import (
 	"github.com/smartcontractkit/chainlink-common/pkg/capabilities/pb"
 	"github.com/smartcontractkit/chainlink-common/pkg/services"
 	"github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types"
+	"github.com/smartcontractkit/chainlink/v2/core/capabilities/validation"
 	"github.com/smartcontractkit/chainlink/v2/core/logger"
 	p2ptypes "github.com/smartcontractkit/chainlink/v2/core/services/p2p/types"
 )
@@ -102,8 +103,8 @@ func (p *triggerPublisher) Receive(_ context.Context, msg *types.MessageBody) {
 			p.lggr.Errorw("sender not a member of its workflow DON", "capabilityId", p.capInfo.ID, "callerDonId", msg.CallerDonId, "sender", sender)
 			return
 		}
-		if !IsValidWorkflowOrExecutionID(req.Metadata.WorkflowID) {
-			p.lggr.Errorw("received trigger request with invalid workflow ID", "capabilityId", p.capInfo.ID, "workflowId", SanitizeLogString(req.Metadata.WorkflowID))
+		if err = validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowID); err != nil {
+			p.lggr.Errorw("received trigger request with invalid workflow ID", "capabilityId", p.capInfo.ID, "workflowId", SanitizeLogString(req.Metadata.WorkflowID), "err", err)
 			return
 		}
 		p.lggr.Debugw("received trigger registration", "capabilityId", p.capInfo.ID, "workflowId", req.Metadata.WorkflowID, "sender", sender)

core/capabilities/remote/utils.go

@@ -19,8 +19,6 @@ import (
 
 const (
 	maxLoggedStringLen = 256
-	validWorkflowIDLen = 64
-	maxIDLen           = 128
 )
 
 func ValidateMessage(msg p2ptypes.Message, expectedReceiver p2ptypes.PeerID) (*remotetypes.MessageBody, error) {
@@ -115,25 +113,3 @@ func SanitizeLogString(s string) string {
 	}
 	return s + tooLongSuffix
 }
-
-// Workflow IDs and Execution IDs are 32-byte hex-encoded strings
-func IsValidWorkflowOrExecutionID(id string) bool {
-	if len(id) != validWorkflowIDLen {
-		return false
-	}
-	_, err := hex.DecodeString(id)
-	return err == nil
-}
-
-// Trigger event IDs and message IDs can only contain printable characters and must be non-empty
-func IsValidID(id string) bool {
-	if len(id) == 0 || len(id) > maxIDLen {
-		return false
-	}
-	for i := 0; i < len(id); i++ {
-		if !unicode.IsPrint(rune(id[i])) {
-			return false
-		}
-	}
-	return true
-}

core/capabilities/remote/utils_test.go

@@ -129,15 +129,3 @@ func TestSanitizeLogString(t *testing.T) {
 	}
 	require.Equal(t, longString[:256]+" [TRUNCATED]", remote.SanitizeLogString(longString))
 }
-
-func TestIsValidWorkflowID(t *testing.T) {
-	require.False(t, remote.IsValidWorkflowOrExecutionID("too_short"))
-	require.False(t, remote.IsValidWorkflowOrExecutionID("nothex--95ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0"))
-	require.True(t, remote.IsValidWorkflowOrExecutionID("15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0"))
-}
-
-func TestIsValidTriggerEventID(t *testing.T) {
-	require.False(t, remote.IsValidID(""))
-	require.False(t, remote.IsValidID("\n\n"))
-	require.True(t, remote.IsValidID("id_id_2"))
-}

core/capabilities/transmission/local_target_capability_test.go

@@ -54,8 +54,8 @@ func TestScheduledExecutionStrategy_LocalDON(t *testing.T) {
 			name:     "position 0; oneAtATime",
 			position: 0,
 			schedule: "oneAtATime",
-			low:      300 * time.Millisecond,
-			high:     400 * time.Millisecond,
+			low:      200 * time.Millisecond,
+			high:     300 * time.Millisecond,
 		},
 		{
 			name:     "position 1; oneAtATime",
@@ -68,15 +68,15 @@ func TestScheduledExecutionStrategy_LocalDON(t *testing.T) {
 			name:     "position 2; oneAtATime",
 			position: 2,
 			schedule: "oneAtATime",
-			low:      0 * time.Millisecond,
-			high:     100 * time.Millisecond,
+			low:      300 * time.Millisecond,
+			high:     400 * time.Millisecond,
 		},
 		{
 			name:     "position 3; oneAtATime",
 			position: 3,
 			schedule: "oneAtATime",
-			low:      100 * time.Millisecond,
-			high:     300 * time.Millisecond,
+			low:      0 * time.Millisecond,
+			high:     100 * time.Millisecond,
 		},
 		{
 			name:     "position 0; allAtOnce",
@@ -121,8 +121,8 @@ func TestScheduledExecutionStrategy_LocalDON(t *testing.T) {
 			req := capabilities.CapabilityRequest{
 				Config: m,
 				Metadata: capabilities.RequestMetadata{
-					WorkflowID:          "mock-workflow-id",
-					WorkflowExecutionID: "mock-execution-id-1",
+					WorkflowID:          "15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0",
+					WorkflowExecutionID: "32c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce1",
 				},
 			}
 

core/capabilities/transmission/transmission.go

@@ -4,10 +4,10 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/pkg/errors"
-
 	"github.com/smartcontractkit/libocr/permutation"
 
+	"github.com/smartcontractkit/chainlink/v2/core/capabilities/validation"
+
 	"github.com/smartcontractkit/chainlink-common/pkg/capabilities"
 	"github.com/smartcontractkit/chainlink-common/pkg/values"
 	"github.com/smartcontractkit/chainlink/v2/core/services/p2p/types"
@@ -56,8 +56,12 @@ func GetPeerIDToTransmissionDelay(donPeerIDs []types.PeerID, req capabilities.Ca
 		return nil, fmt.Errorf("failed to extract transmission config from request: %w", err)
 	}
 
-	if req.Metadata.WorkflowID == "" || req.Metadata.WorkflowExecutionID == "" {
-		return nil, errors.New("workflow ID and workflow execution ID must be set in request metadata")
+	if err = validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowID); err != nil {
+		return nil, fmt.Errorf("workflow ID is invalid: %w", err)
+	}
+
+	if err = validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowExecutionID); err != nil {
+		return nil, fmt.Errorf("workflow execution ID is invalid: %w", err)
 	}
 
 	transmissionID := req.Metadata.WorkflowID + req.Metadata.WorkflowExecutionID

core/capabilities/transmission/transmission_test.go

@@ -36,38 +36,40 @@ func Test_GetPeerIDToTransmissionDelay(t *testing.T) {
 			"one",
 			"oneAtATime",
 			"100ms",
-			"mock-execution-id",
+			"15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0",
 			map[string]time.Duration{
 				"one":   300 * time.Millisecond,
-				"two":   100 * time.Millisecond,
-				"three": 0 * time.Millisecond,
+				"two":   0 * time.Millisecond,
+				"three": 100 * time.Millisecond,
 				"four":  200 * time.Millisecond,
 			},
 		},
+
 		{
 			"TestAllAtOnce",
 			"one",
 			"allAtOnce",
 			"100ms",
-			"mock-execution-id",
+			"15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0",
 			map[string]time.Duration{
 				"one":   0 * time.Millisecond,
 				"two":   0 * time.Millisecond,
 				"three": 0 * time.Millisecond,
 				"four":  0 * time.Millisecond,
 			},
 		},
+
 		{
 			"TestOneAtATimeWithDifferentExecutionID",
 			"one",
 			"oneAtATime",
 			"100ms",
-			"mock-execution-id2",
+			"16c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce1",
 			map[string]time.Duration{
-				"one":   0 * time.Millisecond,
-				"two":   200 * time.Millisecond,
-				"three": 100 * time.Millisecond,
-				"four":  300 * time.Millisecond,
+				"one":   300 * time.Millisecond,
+				"two":   100 * time.Millisecond,
+				"three": 200 * time.Millisecond,
+				"four":  0 * time.Millisecond,
 			},
 		},
 	}
@@ -83,7 +85,7 @@ func Test_GetPeerIDToTransmissionDelay(t *testing.T) {
 			capabilityRequest := capabilities.CapabilityRequest{
 				Config: transmissionCfg,
 				Metadata: capabilities.RequestMetadata{
-					WorkflowID:          "mock-workflow-id",
+					WorkflowID:          "17c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0",
 					WorkflowExecutionID: tc.workflowExecutionID,
 				},
 			}

core/capabilities/validation/validation.go

@@ -0,0 +1,38 @@
+package validation
+
+import (
+	"encoding/hex"
+	"errors"
+	"unicode"
+)
+
+const (
+	validWorkflowIDLen = 64
+	maxIDLen           = 128
+)
+
+// Workflow IDs and Execution IDs are 32-byte hex-encoded strings
+func ValidateWorkflowOrExecutionID(id string) error {
+	if len(id) != validWorkflowIDLen {
+		return errors.New("must be 32 bytes long")
+	}
+	_, err := hex.DecodeString(id)
+	if err != nil {
+		return errors.New("must be a hex-encoded string")
+	}
+
+	return nil
+}
+
+// Trigger event IDs and message IDs can only contain printable characters and must be non-empty
+func IsValidID(id string) bool {
+	if len(id) == 0 || len(id) > maxIDLen {
+		return false
+	}
+	for i := 0; i < len(id); i++ {
+		if !unicode.IsPrint(rune(id[i])) {
+			return false
+		}
+	}
+	return true
+}

core/capabilities/validation/validation_test.go

@@ -0,0 +1,19 @@
+package validation
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestIsValidWorkflowID(t *testing.T) {
+	require.NotNil(t, ValidateWorkflowOrExecutionID("too_short"))
+	require.NotNil(t, ValidateWorkflowOrExecutionID("nothex--95ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0"))
+	require.NoError(t, ValidateWorkflowOrExecutionID("15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0"))
+}
+
+func TestIsValidTriggerEventID(t *testing.T) {
+	require.False(t, IsValidID(""))
+	require.False(t, IsValidID("\n\n"))
+	require.True(t, IsValidID("id_id_2"))
+}