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

Return error with invalid length #180

Merged
merged 5 commits into from
Jul 13, 2022
Merged

Return error with invalid length #180

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f4ae3ee
removed packed check, update composite field
mfdeveloper508 Jun 27, 2022
03f402f
added comment
mfdeveloper508 Jul 5, 2022
03cf22d
remove unnecessary code. it just works
alovak Jul 11, 2022
4735ea4
add tests for zero value and fixed length
alovak Jul 11, 2022
c23e866
for var len field unpack only fields we have data for
alovak Jul 12, 2022
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
4 changes: 0 additions & 4 deletions field/binary.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ func (f *Binary) Pack() ([]byte, error) {
return nil, fmt.Errorf("failed to encode content: %w", err)
}

if len(packed) == 0 {
return []byte{}, nil
}

packedLength, err := f.spec.Pref.EncodeLength(f.spec.Length, len(packed))
if err != nil {
return nil, fmt.Errorf("failed to encode length: %w", err)
Expand Down
10 changes: 0 additions & 10 deletions field/binary_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,6 @@ func TestBinaryField(t *testing.T) {
require.Equal(t, in, packed)
})

t.Run("Pack returns empty struct when given zero-length data", func(t *testing.T) {
bin := NewBinaryValue([]byte{})
bin.SetSpec(spec)

packed, err := bin.Pack()

require.NoError(t, err)
require.Equal(t, []byte{}, packed)
})

t.Run("String returns binary data encoded in HEX", func(t *testing.T) {
bin := NewBinary(spec)
bin.Value = in
Expand Down
27 changes: 8 additions & 19 deletions field/composite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,10 +238,6 @@ func (f *Composite) Pack() ([]byte, error) {
return nil, err
}

if len(packed) == 0 {
return []byte{}, nil
}

packedLength, err := f.spec.Pref.EncodeLength(f.spec.Length, len(packed))
if err != nil {
return nil, fmt.Errorf("failed to encode length: %w", err)
Expand All @@ -259,15 +255,10 @@ func (f *Composite) Unpack(data []byte) (int, error) {
return 0, fmt.Errorf("failed to decode length: %w", err)
}

hasPrefix := false
if offset != 0 {
hasPrefix = true
}

// data is stripped of the prefix before it is provided to unpack().
// Therefore, it is unaware of when to stop parsing unless we bound the
// length of the slice by the data length.
read, err := f.unpack(data[offset:offset+dataLen], hasPrefix)
read, err := f.unpack(data[offset : offset+dataLen])
if err != nil {
return 0, err
}
Expand All @@ -283,7 +274,7 @@ func (f *Composite) Unpack(data []byte) (int, error) {
// pack all subfields in full. However, unlike Unpack(), it requires the
// aggregate length of the subfields not to be encoded in the prefix.
func (f *Composite) SetBytes(data []byte) error {
_, err := f.unpack(data, false)
_, err := f.unpack(data)
return err
}

Expand Down Expand Up @@ -347,7 +338,6 @@ func (f *Composite) pack() ([]byte, error) {
field, ok := f.subfields[tag]
if !ok {
return nil, fmt.Errorf("no subfield for tag %s", tag)
// continue
}

if _, set := f.setSubfields[tag]; !set {
Expand All @@ -371,18 +361,20 @@ func (f *Composite) pack() ([]byte, error) {
return nil, fmt.Errorf("failed to pack subfield %v: %w", tag, err)
}
packed = append(packed, packedBytes...)

}

return packed, nil
}

func (f *Composite) unpack(data []byte, hasPrefix bool) (int, error) {
func (f *Composite) unpack(data []byte) (int, error) {
if f.spec.Tag.Enc != nil {
return f.unpackSubfieldsByTag(data)
}
return f.unpackSubfields(data, hasPrefix)
return f.unpackSubfields(data)
}

func (f *Composite) unpackSubfields(data []byte, hasPrefix bool) (int, error) {
func (f *Composite) unpackSubfields(data []byte) (int, error) {
offset := 0
for _, tag := range f.orderedSpecFieldTags {
field, ok := f.subfields[tag]
Expand All @@ -398,11 +390,8 @@ func (f *Composite) unpackSubfields(data []byte, hasPrefix bool) (int, error) {
f.setSubfields[tag] = struct{}{}

offset += read

if hasPrefix && offset >= len(data) {
break
}
}

return offset, nil
}

Expand Down
188 changes: 46 additions & 142 deletions field/composite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ func TestCompositePacking(t *testing.T) {
require.EqualError(t, err, "failed to pack subfield 1: failed to encode length: field length: 4 should be fixed: 2")
})

t.Run("Pack returns error on failure to encode length", func(t *testing.T) {
t.Run("Pack returns error when encoded data length is larger than specified fixed max length", func(t *testing.T) {
invalidSpec := &Spec{
// Base field length < summation of lengths of subfields
// This will throw an error when encoding the field's length.
Expand Down Expand Up @@ -330,7 +330,7 @@ func TestCompositePacking(t *testing.T) {
}

composite := NewComposite(invalidSpec)
err := composite.SetData(data)
err := composite.Marshal(data)
require.NoError(t, err)

_, err = composite.Pack()
Expand Down Expand Up @@ -450,11 +450,8 @@ func TestCompositePacking(t *testing.T) {
}),
},
}
data := &CompositeTestData{}

composite := NewComposite(invalidSpec)
err := composite.SetData(data)
require.NoError(t, err)

// Length of input too long, causing failure to decode length.
read, err := composite.Unpack([]byte("ABCD123"))
Expand Down Expand Up @@ -496,7 +493,7 @@ func TestCompositePacking(t *testing.T) {
}

func TestCompositePackingWithTags(t *testing.T) {
t.Run("Pack returns error on failure to encode length", func(t *testing.T) {
t.Run("Pack returns error when encoded data length is larger than specified fixed max length", func(t *testing.T) {
// Base field length < summation of (lengths of subfields + IDs).
// This will throw an error when encoding the field's length.
invalidSpec := &Spec{
Expand Down Expand Up @@ -542,6 +539,49 @@ func TestCompositePackingWithTags(t *testing.T) {
require.EqualError(t, err, "failed to encode length: field length: 12 should be fixed: 6")
})

t.Run("Pack returns error when encoded data length is larger than specified variable max length", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there was a test for fixed length, I added one for variable length.

invalidSpec := &Spec{
Length: 8,
Pref: prefix.ASCII.LL,
Tag: &TagSpec{
Length: 2,
Enc: encoding.ASCII,
Pad: padding.Left('0'),
Sort: sort.StringsByInt,
},
Subfields: map[string]Field{
"1": NewString(&Spec{
Length: 2,
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"2": NewString(&Spec{
Length: 2,
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"3": NewNumeric(&Spec{
Length: 2,
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
},
}
data := &CompositeTestData{
F1: NewStringValue("AB"),
F2: NewStringValue("CD"),
F3: NewNumericValue(12),
}

composite := NewComposite(invalidSpec)
err := composite.Marshal(data)
require.NoError(t, err)

b, err := composite.Pack()
require.Nil(t, b)
require.EqualError(t, err, "failed to encode length: field length: 12 is larger than maximum: 8")
})

t.Run("Pack correctly serializes fully populated data to bytes", func(t *testing.T) {
data := &CompositeTestData{
F1: NewStringValue("AB"),
Expand Down Expand Up @@ -595,142 +635,6 @@ func TestCompositePackingWithTags(t *testing.T) {
require.Equal(t, "120102AB0202CD", string(packed))
})

t.Run("Pack correctly ignores excess subfields in excess of the length described by the prefix", func(t *testing.T) {
type ExcessSubfieldsTestData struct {
F1 *String
F2 *Numeric
F3 *String
F4 *String
F5 *String
F6 *String
F7 *String
F8 *String
F9 *Numeric
F10 *String
F11 *String
F12 *String
F13 *String
F14 *String
}

excessSubfieldsSpec := &Spec{
Length: 26,
Description: "POS Data",
Pref: prefix.ASCII.LLL,
Tag: &TagSpec{
Sort: sort.StringsByInt,
},
Subfields: map[string]Field{
"1": NewString(&Spec{
Length: 1,
Description: "POS Terminal Attendance",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"2": NewNumeric(&Spec{
Length: 1,
Description: "Reserved for Future Use",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"3": NewString(&Spec{
Length: 1,
Description: "POS Terminal Location",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"4": NewString(&Spec{
Length: 1,
Description: "POS Cardholder Presence",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"5": NewString(&Spec{
Length: 1,
Description: "POS Card Presence",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"6": NewString(&Spec{
Length: 1,
Description: "POS Card Capture Capabilities",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"7": NewString(&Spec{
Length: 1,
Description: "POS Transaction Status",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"8": NewString(&Spec{
Length: 1,
Description: "POS Transaction Security",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"9": NewNumeric(&Spec{
Length: 1,
Description: "Reserved for Future Use",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
Pad: padding.Left('0'),
}),
"10": NewString(&Spec{
Length: 1,
Description: "Cardholder-Activated Terminal Level",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"11": NewString(&Spec{
Length: 1,
Description: "POS Card Data Terminal Input Capability Indicator",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"12": NewString(&Spec{
Length: 2,
Description: "POS Authorization Life Cycle",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"13": NewString(&Spec{
Length: 3,
Description: "POS Country Code",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
"14": NewString(&Spec{
Length: 10,
Description: "POS Postal Code",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
}),
},
}

data := &ExcessSubfieldsTestData{}

composite := NewComposite(excessSubfieldsSpec)
err := composite.SetData(data)
require.NoError(t, err)

// Subfield 12, 13 & 14 fall outside of the bounds of the
// 11 byte limit imposed by the prefix. [011 | 10000100012]
// Therefore, it won't be included in the packed bytes.

packed := []byte("01110000100012")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is proper packed data for the LLL field:

  • length - 011
  • then the fields from 1 to 11 are unpacked
  • fields 12, 13, 14 are not set


read, err := composite.Unpack(packed)

require.NoError(t, err)
require.Equal(t, 14, read)

packedBytes, err := composite.Pack()
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we just pack it back. Unset fields will not be included in the packed value. It's just expected behavior.

So, test is removed as we don't test here anything. Or I'm missing something.

require.NoError(t, err)
require.Equal(t, packedBytes, packed)
})

t.Run("Unpack returns an error on failure of subfield to unpack bytes", func(t *testing.T) {
data := &CompositeTestData{}

Expand Down
2 changes: 1 addition & 1 deletion field/field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type Field interface {
// a pointer it returns error.
Unmarshal(v interface{}) error

// Marshal sets field Value into provided v. If v is nil or not
// Marshal sets field Value from provided v. If v is nil or not
// a pointer it returns error.
Marshal(v interface{}) error

Expand Down
4 changes: 0 additions & 4 deletions field/string.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,6 @@ func (f *String) Pack() ([]byte, error) {
return nil, fmt.Errorf("failed to encode content: %w", err)
}

if len(packed) == 0 {
return []byte{}, nil
}

packedLength, err := f.spec.Pref.EncodeLength(f.spec.Length, len(packed))
if err != nil {
return nil, fmt.Errorf("failed to encode length: %w", err)
Expand Down
15 changes: 0 additions & 15 deletions field/string_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,6 @@ func TestStringFieldUnmarshal(t *testing.T) {
require.Equal(t, "hello", val.Value)
}

func TestStringFieldZeroLength(t *testing.T) {
str := NewStringValue("")
str.SetSpec(&Spec{
Length: 10,
Description: "Field",
Enc: encoding.ASCII,
Pref: prefix.ASCII.Fixed,
})

packed, err := str.Pack()
require.NoError(t, err)
require.Equal(t, []byte{}, packed)
require.Equal(t, "", string(packed))
}

func TestStringJSONUnmarshal(t *testing.T) {
input := []byte(`"4000"`)

Expand Down
4 changes: 0 additions & 4 deletions field/track1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,6 @@ func (f *Track1) Pack() ([]byte, error) {
return nil, fmt.Errorf("failed to encode content: %w", err)
}

if len(packed) == 0 {
return []byte{}, nil
}

packedLength, err := f.spec.Pref.EncodeLength(f.spec.Length, len(packed))
if err != nil {
return nil, fmt.Errorf("failed to encode length: %w", err)
Expand Down
Loading