diff --git a/.changeset/slimy-cars-sparkle.md b/.changeset/slimy-cars-sparkle.md new file mode 100644 index 00000000000..aa7658ae908 --- /dev/null +++ b/.changeset/slimy-cars-sparkle.md @@ -0,0 +1,5 @@ +--- +"chainlink": patch +--- + +#internal ks-404 validate ids before using as seed of transmission schedule diff --git a/core/capabilities/ccip/launcher/test_helpers.go b/core/capabilities/ccip/launcher/test_helpers.go index a2ebf3fdba9..e1b47fa3521 100644 --- a/core/capabilities/ccip/launcher/test_helpers.go +++ b/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), diff --git a/core/capabilities/remote/target/client.go b/core/capabilities/remote/target/client.go index 4273169d23e..8572efed155 100644 --- a/core/capabilities/remote/target/client.go +++ b/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 diff --git a/core/capabilities/remote/trigger_publisher.go b/core/capabilities/remote/trigger_publisher.go index 23b778f6018..4aac821bc14 100644 --- a/core/capabilities/remote/trigger_publisher.go +++ b/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) diff --git a/core/capabilities/remote/utils.go b/core/capabilities/remote/utils.go index 7e303eefc8f..f9dc0567fea 100644 --- a/core/capabilities/remote/utils.go +++ b/core/capabilities/remote/utils.go @@ -19,7 +19,6 @@ import ( const ( maxLoggedStringLen = 256 - validWorkflowIDLen = 64 maxIDLen = 128 ) @@ -116,15 +115,6 @@ 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 { diff --git a/core/capabilities/remote/utils_test.go b/core/capabilities/remote/utils_test.go index 177ab5a7d14..a6ece02da6c 100644 --- a/core/capabilities/remote/utils_test.go +++ b/core/capabilities/remote/utils_test.go @@ -16,6 +16,7 @@ import ( "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote" remotetypes "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" ) @@ -131,9 +132,9 @@ func TestSanitizeLogString(t *testing.T) { } 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")) + require.NotNil(t, validation.ValidateWorkflowOrExecutionID("too_short")) + require.NotNil(t, validation.ValidateWorkflowOrExecutionID("nothex--95ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0")) + require.NoError(t, validation.ValidateWorkflowOrExecutionID("15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0")) } func TestIsValidTriggerEventID(t *testing.T) { diff --git a/core/capabilities/transmission/local_target_capability_test.go b/core/capabilities/transmission/local_target_capability_test.go index 93bf708ccef..cdca854986b 100644 --- a/core/capabilities/transmission/local_target_capability_test.go +++ b/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", }, } diff --git a/core/capabilities/transmission/transmission.go b/core/capabilities/transmission/transmission.go index b41be5bcaa5..88ce0fa3edd 100644 --- a/core/capabilities/transmission/transmission.go +++ b/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 diff --git a/core/capabilities/transmission/transmission_test.go b/core/capabilities/transmission/transmission_test.go index fba233eadb0..aaa367e78cf 100644 --- a/core/capabilities/transmission/transmission_test.go +++ b/core/capabilities/transmission/transmission_test.go @@ -36,20 +36,21 @@ 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, @@ -57,17 +58,18 @@ func Test_GetPeerIDToTransmissionDelay(t *testing.T) { "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, }, } diff --git a/core/capabilities/validation/validation.go b/core/capabilities/validation/validation.go new file mode 100644 index 00000000000..00f24b8b71a --- /dev/null +++ b/core/capabilities/validation/validation.go @@ -0,0 +1,23 @@ +package validation + +import ( + "encoding/hex" + "errors" +) + +const ( + validWorkflowIDLen = 64 +) + +// 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 +}