From 6fd78c2f05f2b1835a880a1a76a49d9bf81d7282 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Wed, 8 May 2024 16:56:28 +0200
Subject: [PATCH] clienterrors: detect and error on deeply nested errors

Florian pointed out that `clienterror.Error.Details` could contain
arbitrarily deeply nested error values. Add code to detect this.

Also deal with the (common?) case that `.Details` contains a list
of errors.
---
 internal/worker/clienterrors/errors.go      | 42 ++++++++++++++++++++-
 internal/worker/clienterrors/errors_test.go | 39 ++++++++++++++++---
 2 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/internal/worker/clienterrors/errors.go b/internal/worker/clienterrors/errors.go
index 8a25877499..65f45038fb 100644
--- a/internal/worker/clienterrors/errors.go
+++ b/internal/worker/clienterrors/errors.go
@@ -3,6 +3,7 @@ package clienterrors
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 )
 
 const (
@@ -59,13 +60,52 @@ func (e *Error) String() string {
 	return fmt.Sprintf("Code: %d, Reason: %s, Details: %v", e.ID, e.Reason, e.Details)
 }
 
+func ensureNoErrs(v reflect.Value) error {
+	switch v.Kind() {
+	case reflect.Interface:
+		if errIf, ok := v.Interface().(error); ok {
+			if errIf != nil {
+				return fmt.Errorf("%v", errIf.Error())
+			}
+		}
+	case reflect.Ptr:
+		if err := ensureNoErrs(v.Elem()); err != nil {
+			return err
+		}
+	case reflect.Struct:
+		for i := 0; i < v.NumField(); i++ {
+			if err := ensureNoErrs(v.Field(i)); err != nil {
+				return err
+			}
+		}
+	case reflect.Slice, reflect.Array:
+		for i := 0; i < v.Len(); i++ {
+			if err := ensureNoErrs(v.Index(i)); err != nil {
+				return err
+			}
+		}
+	case reflect.Map:
+		for _, key := range v.MapKeys() {
+			if err := ensureNoErrs(v.MapIndex(key)); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 func (e *Error) MarshalJSON() ([]byte, error) {
 	var details interface{}
 	switch v := e.Details.(type) {
 	case error:
 		details = v.Error()
+	case []error:
+		details = fmt.Sprintf("%v", v)
 	default:
-		details = v
+		if err := ensureNoErrs(reflect.ValueOf(v)); err != nil {
+			return nil, fmt.Errorf("found nested error in %+v: %v", e.Details, err)
+		}
+		details = e.Details
 	}
 
 	return json.Marshal(&struct {
diff --git a/internal/worker/clienterrors/errors_test.go b/internal/worker/clienterrors/errors_test.go
index 2e7eb9401a..e39855f4d7 100644
--- a/internal/worker/clienterrors/errors_test.go
+++ b/internal/worker/clienterrors/errors_test.go
@@ -24,15 +24,42 @@ func TestErrorInterface(t *testing.T) {
 		{fmt.Errorf("some error"), "some error"},
 		{&customErr{}, "customErr"},
 	} {
-		wce := clienterrors.WorkerClientError(2, "details", tc.err)
-		assert.Equal(t, fmt.Sprintf("Code: 2, Reason: details, Details: %s", tc.expectedStr), wce.String())
+		wce := clienterrors.WorkerClientError(2, "reason", tc.err)
+		assert.Equal(t, fmt.Sprintf("Code: 2, Reason: reason, Details: %s", tc.expectedStr), wce.String())
 	}
 }
 
 func TestErrorJSONMarshal(t *testing.T) {
-	err := fmt.Errorf("some-error")
+	for _, tc := range []struct {
+		err         interface{}
+		expectedStr string
+	}{
+		{fmt.Errorf("some-error"), "some-error"},
+		{[]error{fmt.Errorf("err1"), fmt.Errorf("err2")}, "[err1 err2]"},
+		{"random detail", "random detail"},
+	} {
+		json, err := json.Marshal(clienterrors.WorkerClientError(2, "reason", tc.err))
+		assert.NoError(t, err)
+		assert.Equal(t, fmt.Sprintf(`{"id":2,"reason":"reason","details":"%s"}`, tc.expectedStr), string(json))
+	}
+}
 
-	json, err := json.Marshal(clienterrors.WorkerClientError(2, "details", err))
-	assert.NoError(t, err)
-	assert.Equal(t, `{"id":2,"reason":"details","details":"some-error"}`, string(json))
+func TestErrorJSONMarshalDetectsNestedErrs(t *testing.T) {
+	details := struct {
+		Unrelated string
+		NestedErr error
+		Nested    struct {
+			DeepErr error
+		}
+	}{
+		Unrelated: "unrelated",
+		NestedErr: fmt.Errorf("some-nested-error"),
+		Nested: struct {
+			DeepErr error
+		}{
+			DeepErr: fmt.Errorf("deep-err"),
+		},
+	}
+	_, err := json.Marshal(clienterrors.WorkerClientError(2, "reason", details))
+	assert.Equal(t, `json: error calling MarshalJSON for type *clienterrors.Error: found nested error in {Unrelated:unrelated NestedErr:some-nested-error Nested:{DeepErr:deep-err}}: some-nested-error`, err.Error())
 }