Fix #844 alternative 2: do not allow access to unexported struct fields (#846)

* add regression test

* fix #844: disallow direct access to unexported struct fields

---------

Co-authored-by: Anton Medvedev <[email protected]>

checker/checker_test.go

@@ -760,8 +760,8 @@ func TestCheck_TaggedFieldName(t *testing.T) {
 
 	config := conf.CreateNew()
 	expr.Env(struct {
-		x struct {
-			y bool `expr:"bar"`
+		X struct {
+			Y bool `expr:"bar"`
 		} `expr:"foo"`
 	}{})(config)
 	expr.AsBool()(config)

checker/nature/utils.go

@@ -52,12 +52,13 @@ func (s *structData) structField(c *Cache, parentEmbed *structData, name string)
 	// Lookup own fields first.
 	for ; s.ownIdx < s.numField; s.ownIdx++ {
 		field := s.rType.Field(s.ownIdx)
-		// BUG: we should skip if !field.IsExported() here
-
 		if field.Anonymous && s.anonIdx < 0 {
 			// start iterating anon fields on the first instead of zero
 			s.anonIdx = s.ownIdx
 		}
+		if !field.IsExported() {
+			continue
+		}
 		fName, ok := fieldName(field.Name, field.Tag)
 		if !ok || fName == "" {
 			// name can still be empty for a type created at runtime with

test/issues/844/issue_test.go

@@ -0,0 +1,194 @@
+package main
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/expr-lang/expr"
+	"github.com/expr-lang/expr/checker"
+	"github.com/expr-lang/expr/conf"
+	"github.com/expr-lang/expr/internal/testify/require"
+	"github.com/expr-lang/expr/parser"
+)
+
+func TestIssue844(t *testing.T) {
+	testCases := []struct {
+		name       string
+		env        any
+		expression string
+		shouldFail bool
+	}{
+		{
+			name:       "exported env, exported field",
+			env:        ExportedEnv{},
+			expression: `ExportedEmbedded`,
+			shouldFail: false,
+		},
+		{
+			name:       "exported env, unexported field",
+			env:        ExportedEnv{},
+			expression: `unexportedEmbedded`,
+			shouldFail: true,
+		},
+		{
+			name:       "exported env, exported field inherited from exported field",
+			env:        ExportedEnv{},
+			expression: `Str`,
+			shouldFail: false,
+		},
+		{
+			name:       "exported env, unexported field inherited from exported field",
+			env:        ExportedEnv{},
+			expression: `str`,
+			shouldFail: true,
+		},
+		{
+			name:       "exported env, exported field inherited from exported field",
+			env:        ExportedEnv{},
+			expression: `Integer`,
+			shouldFail: false,
+		},
+		{
+			name:       "exported env, unexported field inherited from exported field",
+			env:        ExportedEnv{},
+			expression: `integer`,
+			shouldFail: true,
+		},
+		{
+			name:       "exported env, exported field directly accessed from exported field",
+			env:        ExportedEnv{},
+			expression: `ExportedEmbedded.Str`,
+			shouldFail: false,
+		},
+		{
+			name:       "exported env, unexported field directly accessed from exported field",
+			env:        ExportedEnv{},
+			expression: `ExportedEmbedded.str`,
+			shouldFail: true,
+		},
+		{
+			name:       "exported env, exported field directly accessed from exported field",
+			env:        ExportedEnv{},
+			expression: `unexportedEmbedded.Integer`,
+			shouldFail: true,
+		},
+		{
+			name:       "exported env, unexported field directly accessed from exported field",
+			env:        ExportedEnv{},
+			expression: `unexportedEmbedded.integer`,
+			shouldFail: true,
+		},
+		{
+			name:       "unexported env, exported field",
+			env:        unexportedEnv{},
+			expression: `ExportedEmbedded`,
+			shouldFail: false,
+		},
+		{
+			name:       "unexported env, unexported field",
+			env:        unexportedEnv{},
+			expression: `unexportedEmbedded`,
+			shouldFail: true,
+		},
+		{
+			name:       "unexported env, exported field inherited from exported field",
+			env:        unexportedEnv{},
+			expression: `Str`,
+			shouldFail: false,
+		},
+		{
+			name:       "unexported env, unexported field inherited from exported field",
+			env:        unexportedEnv{},
+			expression: `str`,
+			shouldFail: true,
+		},
+		{
+			name:       "unexported env, exported field inherited from exported field",
+			env:        unexportedEnv{},
+			expression: `Integer`,
+			shouldFail: false,
+		},
+		{
+			name:       "unexported env, unexported field inherited from exported field",
+			env:        unexportedEnv{},
+			expression: `integer`,
+			shouldFail: true,
+		},
+		{
+			name:       "unexported env, exported field directly accessed from exported field",
+			env:        unexportedEnv{},
+			expression: `ExportedEmbedded.Str`,
+			shouldFail: false,
+		},
+		{
+			name:       "unexported env, unexported field directly accessed from exported field",
+			env:        unexportedEnv{},
+			expression: `ExportedEmbedded.str`,
+			shouldFail: true,
+		},
+		{
+			name:       "unexported env, exported field directly accessed from exported field",
+			env:        unexportedEnv{},
+			expression: `unexportedEmbedded.Integer`,
+			shouldFail: true,
+		},
+		{
+			name:       "unexported env, unexported field directly accessed from exported field",
+			env:        unexportedEnv{},
+			expression: `unexportedEmbedded.integer`,
+			shouldFail: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			config := conf.New(tc.env)
+			tree, err := parser.ParseWithConfig(tc.expression, config)
+			require.NoError(t, err)
+
+			_, err = new(checker.Checker).PatchAndCheck(tree, config)
+			if tc.shouldFail {
+				require.Error(t, err)
+				errStr := err.Error()
+				if !strings.Contains(errStr, "unknown name") &&
+					!strings.Contains(errStr, " has no field ") {
+					t.Fatalf("expected a different error, got: %v", err)
+				}
+			} else {
+				require.NoError(t, err)
+			}
+
+			// We add this because the issue was actually not catching something
+			// that sometimes failed with the error:
+			//	reflect.Value.Interface: cannot return value obtained from unexported field or method
+			// This way, we test that everything we allow passing is also
+			// allowed later
+			_, err = expr.Eval(tc.expression, tc.env)
+			if tc.shouldFail {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+type ExportedEnv struct {
+	ExportedEmbedded
+	unexportedEmbedded
+}
+
+type unexportedEnv struct {
+	ExportedEmbedded
+	unexportedEmbedded
+}
+
+type ExportedEmbedded struct {
+	Str string
+	str string
+}
+
+type unexportedEmbedded struct {
+	Integer int
+	integer int
+}