Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposing an interface will not prevent calling methods from the underlying type #744

Closed
StarpTech opened this issue Jan 4, 2025 · 7 comments
Assignees
Labels

Comments

@StarpTech
Copy link

StarpTech commented Jan 4, 2025

Hello, as titled. We figured out that the library doesn't hide methods that aren't accessible by the interface definition. We would expect that interface boundaries are enforced.

Simple example:

type Context {
	Header RequestHeaders
}
type RequestHeaders interface {
	Get(key string) string
}

Pseudocode:

code := `header.Set('foo', 'bar')`

program, err := expr.Compile(code, expr.Env(Context{}))
if err != nil {
    panic(err)
}

output, err := expr.Run(program, Context{Header: req.header})
if err != nil {
    panic(err)
}

Workaround

It looks like expr:"-"can be used as a tag to prevent exposing fields. I haven't find it in the docs unfortunately.

@antonmedv
Copy link
Member

Hello! Yes, I think this is a problem. Expr uses reflect to check if method is available on provided instance (not interface). As during runtime real object is present.

We need to improve the type checker, and enforce strict validation during compile time.

Nice idea with expr:"-". Please open separate issue to track it.

@breml
Copy link

breml commented Jan 7, 2025

I hit this issue as well, but slightly different. In my case, I pass an interface to expr during compile time with expr.Env(...) and I observed, that compiling the expression fails, if I just pass a nil instance of the interface and the expression contains one of the methods defined in the interface. My use case for this is, that I want to validate an expression provided by the user, that later will be executed against a concrete instances implementing the interface. But these instances are not yet known when the user provides the expression.

Minimal reproducer:

package main

import "github.com/expr-lang/expr"

type greeter interface {
	Hi() string
}

type say struct {
	name string
}

func (s say) Hi() string {
	return "Hi " + s.name
}

var _ greeter = say{}

func main() {
	var err error
	s := say{name: "foo"}

	_, err = expr.Compile(`Hi() == "Hi foo"`, expr.Env(s))
	if err != nil {
		panic("struct: " + err.Error())
	}

	var g greeter = say{name: "foo"}

	_, err = expr.Compile(`Hi() == "Hi foo"`, expr.Env(g))
	if err != nil {
		panic("non nil interface: " + err.Error())
	}

	var nilGreeter greeter

	_, err = expr.Compile(`Hi() == "Hi foo"`, expr.Env(nilGreeter))
	if err != nil {
		panic("nil interface: " + err.Error())
	}
}

Expected output: no panic

Actual output:

panic: nil interface: unknown name Hi (1:1)
 | Hi() == "Hi foo"
 | ^

goroutine 1 [running]:
main.main()
        /workspaces/migration-manager/internal/batch/test/main.go:39 +0x1f4
exit status 2

Side note: when I created the above reproducer, I found that expr also is not able to handle types derived from base types, which provide methods. My initial definition for say was like this (which also implements the above mentioned interface):

type say string

func (s say) Hi() string {
	return "Hi " + string(s)
}

@antonmedv
Copy link
Member

antonmedv commented Mar 7, 2025

@breml @StarpTech I fixed the problem with interfaces. Now its possible to "hide" parts of methods with interfaces.

package interface_test

import (
	"testing"

	"github.com/expr-lang/expr"
	"github.com/expr-lang/expr/internal/testify/assert"
)

type StoreInterface interface {
	Get(string) int
}

type StoreImpt struct{}

func (f StoreImpt) Get(s string) int {
	return 42
}

func (f StoreImpt) Set(s string, i int) bool {
	return true
}

type Env struct {
	Store StoreInterface `expr:"store"`
}

func TestInterfaceHide(t *testing.T) {
	var env Env
	p, err := expr.Compile(`store.Get("foo")`, expr.Env(env))
	assert.NoError(t, err)

	out, err := expr.Run(p, Env{Store: StoreImpt{}})
	assert.NoError(t, err)
	assert.Equal(t, 42, out)

	_, err = expr.Compile(`store.Set("foo", 100)`, expr.Env(env))
	assert.Error(t, err)
	assert.Contains(t, err.Error(), "type interface_test.StoreInterface has no method Set")
}

@antonmedv
Copy link
Member

@breml about those types, yes. This is another thing which need to be developed.

type say string

But we need to figure out a correct way to do so. For example, Golang requires explicit conversion.

@breml
Copy link

breml commented Mar 10, 2025

@breml @StarpTech I fixed the problem with interfaces. Not is it possible to "hide" part of methods.

@antonmedv Thanks for the fix. Can you elaborate, why it is not possible to "hide" part of the methods?

@breml about those types, yes. This is another thing which need to be developed.

type say string
But we need to figure out a correct way to do so. For example, Golang requires explicit conversion.

I created #766 for the issue with the derived types.

@antonmedv
Copy link
Member

A typo! Now it is possible)

@StarpTech
Copy link
Author

That's great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants