Implement a simple, robust and predictable KV parser

.travis.yml

@@ -8,7 +8,7 @@ before_install:
   - if [ $TRAVIS_OS_NAME = linux ]; then sudo apt-get install git gnupg2; else brew install git gnupg || true; fi
 
 go:
-  - 1.9
+  - "1.10"
 
 script:
   - make all

store/secret/kv.go

@@ -0,0 +1,80 @@
+package secret
+
+import (
+	"bufio"
+	"sort"
+	"strings"
+)
+
+func (s *Secret) decodeKV() error {
+	mayBeYAML := false
+	scanner := bufio.NewScanner(strings.NewReader(s.body))
+	data := make(map[string]interface{}, strings.Count(s.body, "\n"))
+	for scanner.Scan() {
+		line := scanner.Text()
+		if strings.HasPrefix(line, "---") {
+			mayBeYAML = true
+		}
+		parts := strings.SplitN(line, ":", 2)
+		if len(parts) < 1 {
+			continue
+		}
+		if len(parts) == 1 && strings.HasPrefix(parts[0], "  ") {
+			mayBeYAML = true
+		}
+		for i, part := range parts {
+			parts[i] = strings.TrimSpace(part)
+		}
+		// preserve key only entries
+		if len(parts) < 2 {
+			data[parts[0]] = ""
+			continue
+		}
+		if strings.HasPrefix(parts[1], "|") {
+			mayBeYAML = true
+		}
+		data[parts[0]] = parts[1]
+	}
+	if mayBeYAML {
+		docSep, err := s.decodeYAML()
+		if docSep && err == nil && s.data != nil {
+			return nil
+		}
+	}
+	s.data = data
+	return nil
+}
+
+func (s *Secret) encodeKV() error {
+	if s.data == nil {
+		return nil
+	}
+	keys := make([]string, 0, len(s.data))
+	for key := range s.data {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	var buf strings.Builder
+	mayBeYAML := false
+	for _, key := range keys {
+		sv, ok := s.data[key].(string)
+		if !ok {
+			mayBeYAML = true
+			continue
+		}
+		buf.WriteString(key)
+		buf.WriteString(": ")
+		buf.WriteString(sv)
+		buf.WriteString("\n")
+		if strings.Contains(sv, "\n") {
+			mayBeYAML = true
+		}
+	}
+	if mayBeYAML {
+		if err := s.encodeYAML(); err == nil {
+			return nil
+		}
+	}
+	s.body = buf.String()
+	return nil
+}

store/secret/kv_test.go

@@ -0,0 +1,43 @@
+package secret
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestYAMLFromHereDoc(t *testing.T) {
+	t.Logf("Parse K/V w/ HereDoc as YAML, not K/V")
+	mlValue := `somepw
+---
+foo:  |
+  bar
+  baz
+key: value
+`
+	s, err := Parse([]byte(mlValue))
+	assert.NoError(t, err)
+	assert.NotNil(t, s)
+	v, err := s.Value("foo")
+	assert.NoError(t, err)
+	assert.Equal(t, "bar\nbaz\n", v)
+}
+
+func TestKVContentFromInvalidYAML(t *testing.T) {
+	t.Logf("Retrieve content from invalid YAML (#375)")
+	mlValue := `somepasswd
+---
+Test / test.com
+username: [email protected]
+password: somepasswd
+url: http://www.test.com/`
+	s, err := Parse([]byte(mlValue))
+	assert.NoError(t, err)
+	assert.NotNil(t, s)
+	v, err := s.Value("Test / test.com")
+	assert.NoError(t, err)
+	assert.Equal(t, "", v)
+
+	// read back key
+	assert.Equal(t, mlValue, s.String())
+}

store/secret/secret.go

@@ -2,13 +2,8 @@ package secret
 
 import (
 	"bytes"
-	"fmt"
 	"strings"
 	"sync"
-
-	"github.com/justwatchcom/gopass/store"
-
-	yaml "gopkg.in/yaml.v2"
 )
 
 // Secret is a decoded secret
@@ -38,41 +33,12 @@ func Parse(buf []byte) (*Secret, error) {
 	if len(lines) > 1 {
 		s.body = string(bytes.TrimSpace(lines[1]))
 	}
-	if _, err := s.decodeYAML(); err != nil {
+	if err := s.decode(); err != nil {
 		return s, err
 	}
 	return s, nil
 }
 
-// decodeYAML attempts to decode an optional YAML part of a secret
-func (s *Secret) decodeYAML() (bool, error) {
-	if !strings.HasPrefix(s.body, "---\n") && s.password != "---" {
-		return false, nil
-	}
-	d := make(map[string]interface{})
-	err := yaml.Unmarshal([]byte(s.body), &d)
-	if err != nil {
-		return true, err
-	}
-	s.data = d
-	return true, nil
-}
-
-func (s *Secret) encodeYAML() (err error) {
-	defer func() {
-		if r := recover(); r != nil {
-			err = fmt.Errorf("panic: %s", r)
-		}
-	}()
-	// update body
-	yb, err := yaml.Marshal(s.data)
-	if err != nil {
-		return err
-	}
-	s.body = "---\n" + string(yb)
-	return err
-}
-
 // Bytes encodes an secret
 func (s *Secret) Bytes() ([]byte, error) {
 	buf := &bytes.Buffer{}
@@ -86,7 +52,7 @@ func (s *Secret) Bytes() ([]byte, error) {
 
 // String encodes and returns a string representation of a secret
 func (s *Secret) String() string {
-	buf := &bytes.Buffer{}
+	var buf strings.Builder
 	_, _ = buf.WriteString(s.password)
 	_, _ = buf.WriteString("\n")
 	_, _ = buf.WriteString(s.body)
@@ -135,60 +101,10 @@ func (s *Secret) SetBody(b string) error {
 	s.body = b
 	s.data = nil
 
-	_, err := s.decodeYAML()
+	err := s.decode()
 	return err
 }
 
-// Value returns the value of the given key if the body contained valid
-// YAML
-func (s *Secret) Value(key string) (string, error) {
-	s.Lock()
-	defer s.Unlock()
-
-	if s.data == nil {
-		if !strings.HasPrefix(s.body, "---\n") {
-			return "", store.ErrYAMLNoMark
-		}
-		if _, err := s.decodeYAML(); err != nil {
-			return "", err
-		}
-	}
-	if v, found := s.data[key]; found {
-		if sv, ok := v.(string); ok {
-			return sv, nil
-		}
-		return "", store.ErrYAMLValueUnsupported
-	}
-	return "", store.ErrYAMLNoKey
-}
-
-// SetValue sets a key to a given value. Will fail if an non-empty body exists
-func (s *Secret) SetValue(key, value string) error {
-	s.Lock()
-	defer s.Unlock()
-
-	if s.body == "" && s.data == nil {
-		s.data = make(map[string]interface{}, 1)
-	}
-	if s.data == nil {
-		return store.ErrYAMLNoMark
-	}
-	s.data[key] = value
-	return s.encodeYAML()
-}
-
-// DeleteKey key will delete a single key from an decoded map
-func (s *Secret) DeleteKey(key string) error {
-	s.Lock()
-	defer s.Unlock()
-
-	if s.data == nil {
-		return store.ErrYAMLNoMark
-	}
-	delete(s.data, key)
-	return s.encodeYAML()
-}
-
 // Equal returns true if two secrets are equal
 func (s *Secret) Equal(other *Secret) bool {
 	if s == nil && other == nil {
@@ -224,3 +140,11 @@ func (s *Secret) Equal(other *Secret) bool {
 
 	return true
 }
+
+func (s *Secret) encode() error {
+	return s.encodeKV()
+}
+
+func (s *Secret) decode() error {
+	return s.decodeKV()
+}

store/secret/secret_test.go

@@ -34,18 +34,18 @@ func TestNew(t *testing.T) {
 	// delete non-existing key
 	assert.NoError(t, sec.DeleteKey("some-key"))
 
-	// set invalid YAML
-	assert.Error(t, sec.SetBody("---\nkey-only\n"))
+	// set invalid YAML, should parse as K/V
+	assert.NoError(t, sec.SetBody("---\nkey-only\n"))
 	assert.Equal(t, "---\nkey-only\n", sec.Body())
 
 	// non-YAML body
 	assert.NoError(t, sec.SetBody("key-only\n"))
 
 	// try to set value on non-YAML body
-	assert.EqualError(t, sec.SetValue("key", "value"), store.ErrYAMLNoMark.Error())
+	assert.NoError(t, sec.SetValue("key", "value"))
 
 	// delete non-existing key
-	assert.EqualError(t, sec.DeleteKey("some-key"), store.ErrYAMLNoMark.Error())
+	assert.NoError(t, sec.DeleteKey("some-key"))
 }
 
 func TestEqual(t *testing.T) {
@@ -173,7 +173,10 @@ key2: value2`),
 				"key1": "value1",
 				"key2": "value2",
 			},
-			Fail: true,
+			Body: `---
+	key1: value1
+key2: value2`,
+			Fail: false,
 		},
 		{
 			Desc: "missing YAML marker",
@@ -187,14 +190,14 @@ key2: value2`,
 	} {
 		sec, err := Parse(tc.In)
 		if tc.Fail {
-			assert.Error(t, err)
+			assert.Error(t, err, tc.Desc)
 			continue
 		} else if err != nil {
 			assert.NoError(t, err)
 			continue
 		}
-		assert.Equal(t, tc.Password, sec.Password())
-		assert.Equal(t, tc.Body, sec.Body())
+		assert.Equal(t, tc.Password, sec.Password(), tc.Desc)
+		assert.Equal(t, tc.Body, sec.Body(), tc.Desc)
 		for k, v := range tc.Data {
 			rv, err := sec.Value(k)
 			assert.NoError(t, err)