Merge pull request #170 from kzantow-anchore/fix/encode-decode-issues

fix: JSON encoding and decoding not properly handling SPDXRef- prefixes

2 files changed, 383 insertions, 29 deletions

diff --git a/spdx/common/identifier.go b/spdx/common/identifier.go
index d656867..806a815 100644
--- a/spdx/common/identifier.go
+++ b/spdx/common/identifier.go
@@ -8,12 +8,58 @@ import (
 	"strings"
 )
 
+const (
+	spdxRefPrefix     = "SPDXRef-"
+	documentRefPrefix = "DocumentRef-"
+)
+
 // ElementID represents the identifier string portion of an SPDX element
 // identifier. DocElementID should be used for any attributes which can
 // contain identifiers defined in a different SPDX document.
 // ElementIDs should NOT contain the mandatory 'SPDXRef-' portion.
 type ElementID string
 
+// MarshalJSON returns an SPDXRef- prefixed JSON string
+func (d ElementID) MarshalJSON() ([]byte, error) {
+	return json.Marshal(prefixElementId(d))
+}
+
+// UnmarshalJSON validates SPDXRef- prefixes and removes them when processing ElementIDs
+func (d *ElementID) UnmarshalJSON(data []byte) error {
+	// SPDX identifier will simply be a string
+	idStr := string(data)
+	idStr = strings.Trim(idStr, "\"")
+
+	e, err := trimElementIdPrefix(idStr)
+	if err != nil {
+		return err
+	}
+	*d = e
+	return nil
+}
+
+// prefixElementId adds the SPDXRef- prefix to an element ID if it does not have one
+func prefixElementId(id ElementID) string {
+	val := string(id)
+	if !strings.HasPrefix(val, spdxRefPrefix) {
+		return spdxRefPrefix + val
+	}
+	return val
+}
+
+// trimElementIdPrefix removes the SPDXRef- prefix from an element ID string or returns an error if it
+// does not start with SPDXRef-
+func trimElementIdPrefix(id string) (ElementID, error) {
+	// handle SPDXRef-
+	idFields := strings.SplitN(id, spdxRefPrefix, 2)
+	if len(idFields) != 2 {
+		return "", fmt.Errorf("failed to parse SPDX identifier '%s'", id)
+	}
+
+	e := ElementID(idFields[1])
+	return e, nil
+}
+
 // DocElementID represents an SPDX element identifier that could be defined
 // in a different SPDX document, and therefore could have a "DocumentRef-"
 // portion, such as Relationships and Annotations.
@@ -34,9 +80,24 @@ type DocElementID struct {
 	SpecialID     string
 }
 
+// MarshalJSON converts the receiver into a slice of bytes representing a DocElementID in string form.
+// This function is also used when marshalling to YAML
+func (d DocElementID) MarshalJSON() ([]byte, error) {
+	if d.DocumentRefID != "" && d.ElementRefID != "" {
+		idStr := prefixElementId(d.ElementRefID)
+		return json.Marshal(fmt.Sprintf("%s%s:%s", documentRefPrefix, d.DocumentRefID, idStr))
+	} else if d.ElementRefID != "" {
+		return json.Marshal(prefixElementId(d.ElementRefID))
+	} else if d.SpecialID != "" {
+		return json.Marshal(d.SpecialID)
+	}
+
+	return []byte{}, fmt.Errorf("failed to marshal empty DocElementID")
+}
+
 // UnmarshalJSON takes a SPDX Identifier string parses it into a DocElementID struct.
 // This function is also used when unmarshalling YAML
-func (d *DocElementID) UnmarshalJSON(data []byte) error {
+func (d *DocElementID) UnmarshalJSON(data []byte) (err error) {
 	// SPDX identifier will simply be a string
 	idStr := string(data)
 	idStr = strings.Trim(idStr, "\"")
@@ -49,9 +110,9 @@ func (d *DocElementID) UnmarshalJSON(data []byte) error {
 
 	var idFields []string
 	// handle DocumentRef- if present
-	if strings.HasPrefix(idStr, "DocumentRef-") {
+	if strings.HasPrefix(idStr, documentRefPrefix) {
 		// strip out the "DocumentRef-" so we can get the value
-		idFields = strings.SplitN(idStr, "DocumentRef-", 2)
+		idFields = strings.SplitN(idStr, documentRefPrefix, 2)
 		idStr = idFields[1]
 
 		// an SPDXRef can appear after a DocumentRef, separated by a colon
@@ -65,29 +126,8 @@ func (d *DocElementID) UnmarshalJSON(data []byte) error {
 		}
 	}
 
-	// handle SPDXRef-
-	idFields = strings.SplitN(idStr, "SPDXRef-", 2)
-	if len(idFields) != 2 {
-		return fmt.Errorf("failed to parse SPDX Identifier '%s'", idStr)
-	}
-
-	d.ElementRefID = ElementID(idFields[1])
-
-	return nil
-}
-
-// MarshalJSON converts the receiver into a slice of bytes representing a DocElementID in string form.
-// This function is also used when marshalling to YAML
-func (d DocElementID) MarshalJSON() ([]byte, error) {
-	if d.DocumentRefID != "" && d.ElementRefID != "" {
-		return json.Marshal(fmt.Sprintf("DocumentRef-%s:SPDXRef-%s", d.DocumentRefID, d.ElementRefID))
-	} else if d.ElementRefID != "" {
-		return json.Marshal(fmt.Sprintf("SPDXRef-%s", d.ElementRefID))
-	} else if d.SpecialID != "" {
-		return json.Marshal(d.SpecialID)
-	}
-
-	return []byte{}, fmt.Errorf("failed to marshal empty DocElementID")
+	d.ElementRefID, err = trimElementIdPrefix(idStr)
+	return err
 }
 
 // TODO: add equivalents for LicenseRef- identifiers
@@ -114,7 +154,7 @@ func MakeDocElementSpecial(specialID string) DocElementID {
 // RenderElementID takes an ElementID and returns the string equivalent,
 // with the SPDXRef- prefix reinserted.
 func RenderElementID(eID ElementID) string {
-	return "SPDXRef-" + string(eID)
+	return spdxRefPrefix + string(eID)
 }
 
 // RenderDocElementID takes a DocElementID and returns the string equivalent,
@@ -127,7 +167,7 @@ func RenderDocElementID(deID DocElementID) string {
 	}
 	prefix := ""
 	if deID.DocumentRefID != "" {
-		prefix = "DocumentRef-" + deID.DocumentRefID + ":"
+		prefix = documentRefPrefix + deID.DocumentRefID + ":"
 	}
-	return prefix + "SPDXRef-" + string(deID.ElementRefID)
+	return prefix + spdxRefPrefix + string(deID.ElementRefID)
 }
diff --git a/spdx/common/identifier_test.go b/spdx/common/identifier_test.go
new file mode 100644
index 0000000..8df2ea3
--- /dev/null
+++ b/spdx/common/identifier_test.go
@@ -0,0 +1,314 @@
+package common
+
+import (
+	"encoding/json"
+	"fmt"
+	"reflect"
+	"strings"
+	"testing"
+)
+
+func Test_DocElementIDEncoding(t *testing.T) {
+	tests := []struct {
+		name     string
+		value    DocElementID
+		expected string
+		err      bool
+	}{
+		{
+			name: "ElementRefID",
+			value: DocElementID{
+				ElementRefID: "some-id",
+			},
+			expected: "SPDXRef-some-id",
+		},
+		{
+			name: "DocumentRefID:ElementRefID",
+			value: DocElementID{
+				DocumentRefID: "a-doc",
+				ElementRefID:  "some-id",
+			},
+			expected: "DocumentRef-a-doc:SPDXRef-some-id",
+		},
+		{
+			name: "DocumentRefID no ElementRefID",
+			value: DocElementID{
+				DocumentRefID: "a-doc",
+			},
+			err: true,
+		},
+		{
+			name: "SpecialID",
+			value: DocElementID{
+				SpecialID: "special-id",
+			},
+			expected: "special-id",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result, err := json.Marshal(test.value)
+			switch {
+			case !test.err && err != nil:
+				t.Fatalf("unexpected error: %v", err)
+			case test.err && err == nil:
+				t.Fatalf("expected error but got none")
+			case test.err:
+				return
+			}
+			s := string(result)
+			if !strings.HasPrefix(s, `"`) || !strings.HasSuffix(s, `"`) {
+				t.Fatalf("string was not returned: %s", s)
+			}
+			s = strings.Trim(s, `"`)
+			if test.expected != s {
+				t.Fatalf("%s != %s", test.expected, s)
+			}
+		})
+	}
+}
+
+func Test_DocElementIDDecoding(t *testing.T) {
+	tests := []struct {
+		name     string
+		value    string
+		expected DocElementID
+		err      bool
+	}{
+		{
+			name:  "ElementRefID",
+			value: "SPDXRef-some-id",
+			expected: DocElementID{
+				ElementRefID: "some-id",
+			},
+		},
+		{
+			name:  "DocumentRefID:ElementRefID",
+			value: "DocumentRef-a-doc:SPDXRef-some-id",
+			expected: DocElementID{
+				DocumentRefID: "a-doc",
+				ElementRefID:  "some-id",
+			},
+		},
+		{
+			name:  "DocumentRefID no ElementRefID",
+			value: "DocumentRef-a-doc",
+			expected: DocElementID{
+				DocumentRefID: "a-doc",
+			},
+		},
+		{
+			name:  "DocumentRefID invalid ElementRefID",
+			value: "DocumentRef-a-doc:invalid",
+			err:   true,
+		},
+		{
+			name:  "invalid format",
+			value: "some-id-without-spdxref",
+			err:   true,
+		},
+		{
+			name:  "SpecialID NONE",
+			value: "NONE",
+			expected: DocElementID{
+				SpecialID: "NONE",
+			},
+		},
+		{
+			name:  "SpecialID NOASSERTION",
+			value: "NOASSERTION",
+			expected: DocElementID{
+				SpecialID: "NOASSERTION",
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			out := DocElementID{}
+			s := fmt.Sprintf(`"%s"`, test.value)
+			err := json.Unmarshal([]byte(s), &out)
+			switch {
+			case !test.err && err != nil:
+				t.Fatalf("unexpected error: %v", err)
+			case test.err && err == nil:
+				t.Fatalf("expected error but got none")
+			case test.err:
+				return
+			}
+			if !reflect.DeepEqual(test.expected, out) {
+				t.Fatalf("unexpected value: %v != %v", test.expected, out)
+			}
+		})
+	}
+}
+
+func Test_ElementIDEncoding(t *testing.T) {
+	tests := []struct {
+		name     string
+		value    ElementID
+		expected string
+		err      bool
+	}{
+		{
+			name:     "appends spdxref",
+			value:    ElementID("some-id"),
+			expected: "SPDXRef-some-id",
+		},
+		{
+			name:     "appends spdxref",
+			value:    ElementID("SPDXRef-some-id"),
+			expected: "SPDXRef-some-id",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result, err := json.Marshal(test.value)
+			switch {
+			case !test.err && err != nil:
+				t.Fatalf("unexpected error: %v", err)
+			case test.err && err == nil:
+				t.Fatalf("expected error but got none")
+			case test.err:
+				return
+			}
+			s := string(result)
+			if !strings.HasPrefix(s, `"`) || !strings.HasSuffix(s, `"`) {
+				t.Fatalf("string was not returned: %s", s)
+			}
+			s = strings.Trim(s, `"`)
+			if test.expected != s {
+				t.Fatalf("%s != %s", test.expected, s)
+			}
+		})
+	}
+}
+
+func Test_ElementIDDecoding(t *testing.T) {
+	tests := []struct {
+		name     string
+		value    string
+		expected ElementID
+		err      bool
+	}{
+		{
+			name:     "valid id",
+			value:    "SPDXRef-some-id",
+			expected: ElementID("some-id"),
+		},
+		{
+			name:  "invalid format",
+			value: "some-id-without-spdxref",
+			err:   true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var out ElementID
+			s := fmt.Sprintf(`"%s"`, test.value)
+			err := json.Unmarshal([]byte(s), &out)
+			switch {
+			case !test.err && err != nil:
+				t.Fatalf("unexpected error: %v", err)
+			case test.err && err == nil:
+				t.Fatalf("expected error but got none")
+			case test.err:
+				return
+			}
+			if !reflect.DeepEqual(test.expected, out) {
+				t.Fatalf("unexpected value: %v != %v", test.expected, out)
+			}
+		})
+	}
+}
+
+func Test_ElementIDStructEncoding(t *testing.T) {
+	type typ struct {
+		Id ElementID `json:"id"`
+	}
+	tests := []struct {
+		name     string
+		value    typ
+		expected string
+		err      bool
+	}{
+		{
+			name: "appends spdxref",
+			value: typ{
+				Id: ElementID("some-id"),
+			},
+			expected: `{"id":"SPDXRef-some-id"}`,
+		},
+		{
+			name: "appends spdxref",
+			value: typ{
+				Id: ElementID("SPDXRef-some-id"),
+			},
+			expected: `{"id":"SPDXRef-some-id"}`,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result, err := json.Marshal(test.value)
+			switch {
+			case !test.err && err != nil:
+				t.Fatalf("unexpected error: %v", err)
+			case test.err && err == nil:
+				t.Fatalf("expected error but got none")
+			case test.err:
+				return
+			}
+			s := string(result)
+			if test.expected != s {
+				t.Fatalf("%s != %s", test.expected, s)
+			}
+		})
+	}
+}
+
+func Test_ElementIDStructDecoding(t *testing.T) {
+	type typ struct {
+		Id ElementID `json:"id"`
+	}
+	tests := []struct {
+		name     string
+		value    string
+		expected typ
+		err      bool
+	}{
+		{
+			name: "valid id",
+			expected: typ{
+				Id: ElementID("some-id"),
+			},
+			value: `{"id":"SPDXRef-some-id"}`,
+		},
+		{
+			name:  "invalid format",
+			value: `{"id":"some-id"}`,
+			err:   true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			out := typ{}
+			err := json.Unmarshal([]byte(test.value), &out)
+			switch {
+			case !test.err && err != nil:
+				t.Fatalf("unexpected error: %v", err)
+			case test.err && err == nil:
+				t.Fatalf("expected error but got none")
+			case test.err:
+				return
+			}
+			if !reflect.DeepEqual(test.expected, out) {
+				t.Fatalf("unexpected value: %v != %v", test.expected, out)
+			}
+		})
+	}
+}