Better parity with macOS XML plist parsing (#37)

* added test for parity with macOS's plutil -lint

* return error when extraneous tags/chardata are present or start/end tags
are mismatched/missing (parity with plutil)

* mark test cases as failure by filename

decode_test.go

@@ -8,8 +8,10 @@ import (
 	"log"
 	"net/http"
 	"net/http/httptest"
+	"os/exec"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 )
@@ -513,3 +515,43 @@ func TestDecodeTagSkip(t *testing.T) {
 		t.Error("field decoded when it was tagged as -")
 	}
 }
+
+// TestXMLPlutilParity tests parity with plutil -lint on macOS
+func TestXMLPlutilParity(t *testing.T) {
+	type data struct {
+		Key string `plist:"key"`
+	}
+	tests, err := ioutil.ReadDir("testdata/xml/")
+	if err != nil {
+		t.Fatalf("could not open testdata/xml: %v", err)
+	}
+
+	plutil, _ := exec.LookPath("plutil")
+
+	for _, test := range tests {
+		testPath := filepath.Join("testdata/xml/", test.Name())
+		buf, err := ioutil.ReadFile(testPath)
+		if err != nil {
+			t.Errorf("could not read test %s: %v", test.Name(), err)
+			continue
+		}
+		v := new(data)
+		err = Unmarshal(buf, v)
+
+		shouldFail := strings.HasSuffix(test.Name(), ".failure.plist")
+		if plutil != "" {
+			plutilFail := exec.Command(plutil, "-lint", testPath).Run() != nil
+			if shouldFail != plutilFail {
+				t.Errorf("expected plutil test failure: %v for %s, but got test failure: %v", shouldFail, test.Name(), plutilFail)
+			}
+		}
+
+		if shouldFail && err == nil {
+			t.Errorf("expected error for test %s but got: nil", test.Name())
+		} else if !shouldFail && err != nil {
+			t.Errorf("expected no error for test %s but got: %v", test.Name(), err)
+		} else if !shouldFail && v.Key != "val" {
+			t.Errorf("expected key=val for test %s but got: key=%s", test.Name(), v.Key)
+		}
+	}
+}

testdata/xml/empty-doctype.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE>
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/empty-plist.failure.plist

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+</plist>

testdata/xml/empty-xml.plist

@@ -0,0 +1,8 @@
+<?xml?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/invalid-before-plist.failure.plist

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+invalid
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/invalid-data.failure.plist

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+invalid
+</dict>
+</plist>

testdata/xml/invalid-end.plist

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>
+invalid

testdata/xml/invalid-middle.failure.plist

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+invalid
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/invalid-start.failure.plist

@@ -0,0 +1,8 @@
+invalid<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/malformed-xml.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/no-both.plist

@@ -0,0 +1,6 @@
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/no-dict-end.failure.plist

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</plist>

testdata/xml/no-doctype.plist

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/no-plist-end.failure.plist

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>

testdata/xml/no-plist-version.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist>
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/no-xml-tag.plist

@@ -0,0 +1,7 @@
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/swapped.plist

@@ -0,0 +1,8 @@
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<?xml version="1.0" encoding="UTF-8"?>
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/unescaped-plist.failure.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0"
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/unescaped-xml.failure.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

testdata/xml/valid.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>key</key>
+	<string>val</string>
+</dict>
+</plist>

xml_parser.go

@@ -1,6 +1,7 @@
 package plist
 
 import (
+	"bytes"
 	"encoding/base64"
 	"encoding/xml"
 	"errors"
@@ -22,19 +23,28 @@ func newXMLParser(r io.Reader) *xmlParser {
 }
 
 func (p *xmlParser) parseDocument(start *xml.StartElement) (*plistValue, error) {
-	if start == nil {
-		for {
-			tok, err := p.Token()
-			if err != nil {
-				return nil, err
-			}
-			if t, ok := tok.(xml.StartElement); ok {
-				start = &t
-				break
+	if start != nil {
+		return p.parseXMLElement(start)
+	}
+
+	for {
+		tok, err := p.Token()
+		if err != nil {
+			return nil, err
+		}
+		switch el := tok.(type) {
+		case xml.StartElement:
+			return p.parseXMLElement(&el)
+		case xml.ProcInst, xml.Directive:
+			continue
+		case xml.CharData:
+			if len(bytes.TrimSpace(el)) != 0 {
+				return nil, errors.New("plist: unexpected non-empty xml.CharData")
 			}
+		default:
+			return nil, fmt.Errorf("unexpected element: %T", el)
 		}
 	}
-	return p.parseXMLElement(start)
 }
 
 func (p *xmlParser) parseXMLElement(element *xml.StartElement) (*plistValue, error) {
@@ -63,19 +73,32 @@ func (p *xmlParser) parseXMLElement(element *xml.StartElement) (*plistValue, err
 }
 
 func (p *xmlParser) parsePlist(element *xml.StartElement) (*plistValue, error) {
+	var val *plistValue
 	for {
 		token, err := p.Token()
 		if err != nil {
 			return nil, err
 		}
-		if el, ok := token.(xml.EndElement); ok && el.Name.Local == "plist" {
-			break
-		}
-		if el, ok := token.(xml.StartElement); ok {
-			return p.parseXMLElement(&el)
+		switch el := token.(type) {
+		case xml.EndElement:
+			if val == nil {
+				return nil, errors.New("plist: empty plist tag")
+			}
+			return val, nil
+		case xml.StartElement:
+			v, err := p.parseXMLElement(&el)
+			if err != nil {
+				return v, err
+			}
+			val = v
+		case xml.CharData:
+			if len(bytes.TrimSpace(el)) != 0 {
+				return nil, errors.New("plist: unexpected non-empty xml.CharData")
+			}
+		default:
+			return nil, fmt.Errorf("unexpected element: %T", el)
 		}
 	}
-	return nil, errors.New("plist: Invalid plist")
 }
 
 func (p *xmlParser) parseDict(element *xml.StartElement) (*plistValue, error) {
@@ -86,10 +109,10 @@ func (p *xmlParser) parseDict(element *xml.StartElement) (*plistValue, error) {
 		if err != nil {
 			return nil, err
 		}
-		if el, ok := token.(xml.EndElement); ok && el.Name.Local == "dict" {
-			break
-		}
-		if el, ok := token.(xml.StartElement); ok {
+		switch el := token.(type) {
+		case xml.EndElement:
+			return &plistValue{Dictionary, &dictionary{m: subvalues}}, nil
+		case xml.StartElement:
 			if el.Name.Local == "key" {
 				var k string
 				if err := p.DecodeElement(&k, &el); err != nil {
@@ -106,9 +129,14 @@ func (p *xmlParser) parseDict(element *xml.StartElement) (*plistValue, error) {
 				return nil, err
 			}
 			key = nil
+		case xml.CharData:
+			if len(bytes.TrimSpace(el)) != 0 {
+				return nil, errors.New("plist: unexpected non-empty xml.CharData")
+			}
+		default:
+			return nil, fmt.Errorf("unexpected element: %T", el)
 		}
 	}
-	return &plistValue{Dictionary, &dictionary{m: subvalues}}, nil
 }
 
 func (p *xmlParser) parseString(element *xml.StartElement) (*plistValue, error) {
@@ -134,18 +162,23 @@ func (p *xmlParser) parseArray(element *xml.StartElement) (*plistValue, error) {
 		if err != nil {
 			return nil, err
 		}
-		if el, ok := token.(xml.EndElement); ok && el.Name.Local == "array" {
-			break
-		}
-		if el, ok := token.(xml.StartElement); ok {
+		switch el := token.(type) {
+		case xml.EndElement:
+			return &plistValue{Array, subvalues}, nil
+		case xml.StartElement:
 			subv, err := p.parseXMLElement(&el)
 			if err != nil {
 				return nil, err
 			}
 			subvalues = append(subvalues, subv)
+		case xml.CharData:
+			if len(bytes.TrimSpace(el)) != 0 {
+				return nil, errors.New("plist: unexpected non-empty xml.CharData")
+			}
+		default:
+			return nil, fmt.Errorf("unexpected element: %T", el)
 		}
 	}
-	return &plistValue{Array, subvalues}, nil
 }
 
 func (p *xmlParser) parseReal(element *xml.StartElement) (*plistValue, error) {