Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

validate ids prior to using as seed for transmission schedule #14116

Merged
merged 1 commit into from
Aug 20, 2024
Merged

validate ids prior to using as seed for transmission schedule #14116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/slimy-cars-sparkle.md
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind keeping it in utils.go? Or alternatively moving all the validators to validation.go and their tests to validation_test.go? We are now in a messy state where some methods are here and some there... My preference is to keep stuff in utils.go as it minimizes the total amount of changes needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah crap, yeah I should have at least moved the corresponding tests, my bad. The utils stuff is used outside of the remote package as well now so should sit outside that, I'll tidy it up

Copy link
Collaborator Author

@ettec ettec Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the validation logic used in remote and other packages in capabilities into validation package along with the corresponding tests. All other logic that is used only in remote package has been kept in the remote/utils.go file.

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
View file
Open in desktop
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"))
}
Loading