From 4bad36ea39d4e65feddb5ed895d66e07c08f89c8 Mon Sep 17 00:00:00 2001
From: Martin Holst Swende <martin@swende.se>
Date: Fri, 21 Apr 2023 10:17:24 +0200
Subject: [PATCH] stores: test for duplicate keys, reseve keyword (yaml only
 now)

Signed-off-by: Martin Holst Swende <martin@swende.se>
---
 stores/json/store.go      |  4 +++-
 stores/json/store_test.go | 23 +++++++++++++++++++++++
 stores/yaml/store.go      | 13 +++++++++++++
 stores/yaml/store_test.go | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/stores/json/store.go b/stores/json/store.go
index 81b8bfeadd..90cabd7268 100644
--- a/stores/json/store.go
+++ b/stores/json/store.go
@@ -221,7 +221,9 @@ func (store Store) jsonFromTreeBranch(branch sops.TreeBranch) ([]byte, error) {
 
 func (store Store) treeBranchFromJSON(in []byte) (sops.TreeBranch, error) {
 	dec := json.NewDecoder(bytes.NewReader(in))
-	dec.Token()
+	if _, err := dec.Token(); err != nil {
+		return nil, err
+	}
 	return store.treeBranchFromJSONDecoder(dec)
 }
 
diff --git a/stores/json/store_test.go b/stores/json/store_test.go
index be5e30be3e..b251067379 100644
--- a/stores/json/store_test.go
+++ b/stores/json/store_test.go
@@ -394,3 +394,26 @@ func TestEmitValueString(t *testing.T) {
 	assert.Nil(t, err)
 	assert.Equal(t, []byte("\"hello\""), bytes)
 }
+
+func TestConflictingAttributes(t *testing.T) {
+	// See https://stackoverflow.com/a/23195243
+	// Duplicate keys in json is technically valid, but discouraged.
+	// Implementations may handle them differently. ECMA-262 says
+	//
+	// > In the case where there are duplicate name Strings within an object,
+	// > lexically preceding values for the same key shall be overwritten.
+
+	data := `
+{
+  "hello": "Sops config file", 
+  "hello": "Doubles are ok", 
+  "hello": ["repeatedly"],
+  "hello": 3.14
+}
+`
+	s := new(Store)
+	_, err := s.LoadPlainFile([]byte(data))
+	if err != nil {
+		t.Errorf("did not expect error, got %v", err)
+	}
+}
diff --git a/stores/yaml/store.go b/stores/yaml/store.go
index 1481dff8f6..e916ca5ca4 100644
--- a/stores/yaml/store.go
+++ b/stores/yaml/store.go
@@ -294,6 +294,13 @@ func (store *Store) LoadEncryptedFile(in []byte) (sops.Tree, error) {
 // sops.Tree runtime object
 func (store *Store) LoadPlainFile(in []byte) (sops.TreeBranches, error) {
 	var branches sops.TreeBranches
+	{
+		// This is needed to make the yaml-decoder check for uniqueness of keys
+		// Can probably be removed when https://github.com/go-yaml/yaml/issues/814 is merged.
+		if err := yaml.NewDecoder(bytes.NewReader(in)).Decode(make(map[string]interface{})); err != nil {
+			return nil, err
+		}
+	}
 	d := yaml.NewDecoder(bytes.NewReader(in))
 	for {
 		var data yaml.Node
@@ -309,6 +316,12 @@ func (store *Store) LoadPlainFile(in []byte) (sops.TreeBranches, error) {
 		if err != nil {
 			return nil, fmt.Errorf("Error unmarshaling input YAML: %s", err)
 		}
+		// Prevent use of reserved keywords
+		for _, item := range branch {
+			if item.Key == "sops" {
+				return nil, fmt.Errorf("YAML doc used reserved word '%v'", item.Key)
+			}
+		}
 		branches = append(branches, branch)
 	}
 	return branches, nil
diff --git a/stores/yaml/store_test.go b/stores/yaml/store_test.go
index f37e3deb44..1f2be1e29e 100644
--- a/stores/yaml/store_test.go
+++ b/stores/yaml/store_test.go
@@ -281,3 +281,42 @@ func TestComment7(t *testing.T) {
 	assert.Equal(t, string(COMMENT_7_OUT), string(bytes))
 	assert.Equal(t, COMMENT_7_OUT, bytes)
 }
+func TestDuplicateAttributes(t *testing.T) {
+	// Duplicate keys are _not_ valid yaml.
+	//
+	// See https://yaml.org/spec/1.2.2/
+	// > The content of a mapping node is an unordered set of key/value node pairs,
+	// > with the restriction that each of the keys is unique.
+	//
+	data := `
+hello: Sops config file
+hello: Duplicates are not ok
+rootunique:
+  key2: "value"
+  key2: "foo"
+`
+	s := new(Store)
+	_, err := s.LoadPlainFile([]byte(data))
+	if err == nil {
+		t.Errorf("Expected error")
+	}
+	if have, want := err.Error(), `yaml: unmarshal errors:
+  line 3: mapping key "hello" already defined at line 2`; have != want {
+		t.Errorf("have '%v', want '%v'", have, want)
+	}
+}
+
+func TestReservedAttributes(t *testing.T) {
+	data := `
+hello: Sops config file
+sops: The attribute 'sops' must be rejected, otherwise the file cannot be opened later on 
+`
+	s := new(Store)
+	_, err := s.LoadPlainFile([]byte(data))
+	if err == nil {
+		t.Errorf("Expected error")
+	}
+	if have, want := err.Error(), `YAML doc used reserved word 'sops'`; have != want {
+		t.Errorf("have '%v', want '%v'", have, want)
+	}
+}