Skip to content

Commit

Permalink
ks-404 validate ids used in transmission schedule
Browse files Browse the repository at this point in the history
  • Loading branch information
ettec committed Aug 19, 2024
1 parent 3399dd6 commit fc8df4a
Show file tree
Hide file tree
Showing 13 changed files with 103 additions and 65 deletions.
5 changes: 5 additions & 0 deletions .changeset/slimy-cars-sparkle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

#internal ks-404 validate ids before using as seed of transmission schedule
2 changes: 1 addition & 1 deletion core/capabilities/ccip/launcher/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 0 additions & 1 deletion core/capabilities/remote/dispatcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
9 changes: 7 additions & 2 deletions core/capabilities/remote/target/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion core/capabilities/remote/target/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions core/capabilities/remote/trigger_publisher.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand Down
24 changes: 0 additions & 24 deletions core/capabilities/remote/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ import (

const (
maxLoggedStringLen = 256
validWorkflowIDLen = 64
maxIDLen = 128
)

func ValidateMessage(msg p2ptypes.Message, expectedReceiver p2ptypes.PeerID) (*remotetypes.MessageBody, error) {
Expand Down Expand Up @@ -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
}
12 changes: 0 additions & 12 deletions core/capabilities/remote/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
}
16 changes: 8 additions & 8 deletions core/capabilities/transmission/local_target_capability_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
},
}

Expand Down
12 changes: 8 additions & 4 deletions core/capabilities/transmission/transmission.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
22 changes: 12 additions & 10 deletions core/capabilities/transmission/transmission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
},
}
Expand All @@ -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,
},
}
Expand Down
38 changes: 38 additions & 0 deletions core/capabilities/validation/validation.go
Original file line number Diff line number Diff line change
@@ -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
}
19 changes: 19 additions & 0 deletions core/capabilities/validation/validation_test.go
Original file line number Diff line number Diff line change
@@ -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"))
}

0 comments on commit fc8df4a

Please sign in to comment.