Skip to content

Commit 3d0aec6

Browse files
diegommmantonmedv
diegommm
and
antonmedv
authored
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]>
1 parent 7dbd409 commit 3d0aec6

File tree

3 files changed

+199
-4
lines changed

3 files changed

+199
-4
lines changed

‎checker/checker_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -760,8 +760,8 @@ func TestCheck_TaggedFieldName(t *testing.T) {
760760

761761
config := conf.CreateNew()
762762
expr.Env(struct {
763-
x struct {
764-
y bool `expr:"bar"`
763+
X struct {
764+
Y bool `expr:"bar"`
765765
} `expr:"foo"`
766766
}{})(config)
767767
expr.AsBool()(config)

‎checker/nature/utils.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,13 @@ func (s *structData) structField(c *Cache, parentEmbed *structData, name string)
5252
// Lookup own fields first.
5353
for ; s.ownIdx < s.numField; s.ownIdx++ {
5454
field := s.rType.Field(s.ownIdx)
55-
// BUG: we should skip if !field.IsExported() here
56-
5755
if field.Anonymous && s.anonIdx < 0 {
5856
// start iterating anon fields on the first instead of zero
5957
s.anonIdx = s.ownIdx
6058
}
59+
if !field.IsExported() {
60+
continue
61+
}
6162
fName, ok := fieldName(field.Name, field.Tag)
6263
if !ok || fName == "" {
6364
// name can still be empty for a type created at runtime with

‎test/issues/844/issue_test.go‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
package main
2+
3+
import (
4+
"strings"
5+
"testing"
6+
7+
"github.com/expr-lang/expr"
8+
"github.com/expr-lang/expr/checker"
9+
"github.com/expr-lang/expr/conf"
10+
"github.com/expr-lang/expr/internal/testify/require"
11+
"github.com/expr-lang/expr/parser"
12+
)
13+
14+
func TestIssue844(t *testing.T) {
15+
testCases := []struct {
16+
name string
17+
env any
18+
expression string
19+
shouldFail bool
20+
}{
21+
{
22+
name: "exported env, exported field",
23+
env: ExportedEnv{},
24+
expression: `ExportedEmbedded`,
25+
shouldFail: false,
26+
},
27+
{
28+
name: "exported env, unexported field",
29+
env: ExportedEnv{},
30+
expression: `unexportedEmbedded`,
31+
shouldFail: true,
32+
},
33+
{
34+
name: "exported env, exported field inherited from exported field",
35+
env: ExportedEnv{},
36+
expression: `Str`,
37+
shouldFail: false,
38+
},
39+
{
40+
name: "exported env, unexported field inherited from exported field",
41+
env: ExportedEnv{},
42+
expression: `str`,
43+
shouldFail: true,
44+
},
45+
{
46+
name: "exported env, exported field inherited from exported field",
47+
env: ExportedEnv{},
48+
expression: `Integer`,
49+
shouldFail: false,
50+
},
51+
{
52+
name: "exported env, unexported field inherited from exported field",
53+
env: ExportedEnv{},
54+
expression: `integer`,
55+
shouldFail: true,
56+
},
57+
{
58+
name: "exported env, exported field directly accessed from exported field",
59+
env: ExportedEnv{},
60+
expression: `ExportedEmbedded.Str`,
61+
shouldFail: false,
62+
},
63+
{
64+
name: "exported env, unexported field directly accessed from exported field",
65+
env: ExportedEnv{},
66+
expression: `ExportedEmbedded.str`,
67+
shouldFail: true,
68+
},
69+
{
70+
name: "exported env, exported field directly accessed from exported field",
71+
env: ExportedEnv{},
72+
expression: `unexportedEmbedded.Integer`,
73+
shouldFail: true,
74+
},
75+
{
76+
name: "exported env, unexported field directly accessed from exported field",
77+
env: ExportedEnv{},
78+
expression: `unexportedEmbedded.integer`,
79+
shouldFail: true,
80+
},
81+
{
82+
name: "unexported env, exported field",
83+
env: unexportedEnv{},
84+
expression: `ExportedEmbedded`,
85+
shouldFail: false,
86+
},
87+
{
88+
name: "unexported env, unexported field",
89+
env: unexportedEnv{},
90+
expression: `unexportedEmbedded`,
91+
shouldFail: true,
92+
},
93+
{
94+
name: "unexported env, exported field inherited from exported field",
95+
env: unexportedEnv{},
96+
expression: `Str`,
97+
shouldFail: false,
98+
},
99+
{
100+
name: "unexported env, unexported field inherited from exported field",
101+
env: unexportedEnv{},
102+
expression: `str`,
103+
shouldFail: true,
104+
},
105+
{
106+
name: "unexported env, exported field inherited from exported field",
107+
env: unexportedEnv{},
108+
expression: `Integer`,
109+
shouldFail: false,
110+
},
111+
{
112+
name: "unexported env, unexported field inherited from exported field",
113+
env: unexportedEnv{},
114+
expression: `integer`,
115+
shouldFail: true,
116+
},
117+
{
118+
name: "unexported env, exported field directly accessed from exported field",
119+
env: unexportedEnv{},
120+
expression: `ExportedEmbedded.Str`,
121+
shouldFail: false,
122+
},
123+
{
124+
name: "unexported env, unexported field directly accessed from exported field",
125+
env: unexportedEnv{},
126+
expression: `ExportedEmbedded.str`,
127+
shouldFail: true,
128+
},
129+
{
130+
name: "unexported env, exported field directly accessed from exported field",
131+
env: unexportedEnv{},
132+
expression: `unexportedEmbedded.Integer`,
133+
shouldFail: true,
134+
},
135+
{
136+
name: "unexported env, unexported field directly accessed from exported field",
137+
env: unexportedEnv{},
138+
expression: `unexportedEmbedded.integer`,
139+
shouldFail: true,
140+
},
141+
}
142+
143+
for _, tc := range testCases {
144+
t.Run(tc.name, func(t *testing.T) {
145+
config := conf.New(tc.env)
146+
tree, err := parser.ParseWithConfig(tc.expression, config)
147+
require.NoError(t, err)
148+
149+
_, err = new(checker.Checker).PatchAndCheck(tree, config)
150+
if tc.shouldFail {
151+
require.Error(t, err)
152+
errStr := err.Error()
153+
if !strings.Contains(errStr, "unknown name") &&
154+
!strings.Contains(errStr, " has no field ") {
155+
t.Fatalf("expected a different error, got: %v", err)
156+
}
157+
} else {
158+
require.NoError(t, err)
159+
}
160+
161+
// We add this because the issue was actually not catching something
162+
// that sometimes failed with the error:
163+
// reflect.Value.Interface: cannot return value obtained from unexported field or method
164+
// This way, we test that everything we allow passing is also
165+
// allowed later
166+
_, err = expr.Eval(tc.expression, tc.env)
167+
if tc.shouldFail {
168+
require.Error(t, err)
169+
} else {
170+
require.NoError(t, err)
171+
}
172+
})
173+
}
174+
}
175+
176+
type ExportedEnv struct {
177+
ExportedEmbedded
178+
unexportedEmbedded
179+
}
180+
181+
type unexportedEnv struct {
182+
ExportedEmbedded
183+
unexportedEmbedded
184+
}
185+
186+
type ExportedEmbedded struct {
187+
Str string
188+
str string
189+
}
190+
191+
type unexportedEmbedded struct {
192+
Integer int
193+
integer int
194+
}

0 commit comments

Comments
 (0)