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

Expand property accesses to getter/setter methods when applicable #7

Open
jglick opened this issue Nov 14, 2013 · 4 comments
Open

Expand property accesses to getter/setter methods when applicable #7

jglick opened this issue Nov 14, 2013 · 4 comments

Comments

@jglick
Copy link
Member

jglick commented Nov 14, 2013

It is annoying complication for a GroovyInterceptor implementation that a script calling, say, a.hostAddress where a is an InetAddress will be informed of onGetProperty with a property of hostAddress, when this is really just Groovy syntactic sugar. Would be more comfortable to receive onMethodCall of getHostAddress, which is what is really happening.

@kohsuke
Copy link
Member

kohsuke commented Nov 14, 2013

Indeed. This comes from the fact that we rely on AST transformation to insert interceptors. Hence we generally do not know how the property access syntax like this is routed internally in Groovy.

I think the possible next step from here is to let Groovy perform call site selection, then inspect the obtained CallSite object to understand what Groovy is trying to do. That would allow us to tell apart x.y (field access) x.getY() (getter), ((Map)x).get("y") and any number of other exotic property access resolution one can find in MetaClassImpl.getEffectiveGetMetaProperty().

The downside is that none of the CallSite impls expose key properties that we care, so we'll have to pry in and grab these values via reflection, which creates a rather undesirable internal dependencies. But then, I suppose this library already depends way too much on the internals that more dependencies might not matter all that much.

@kohsuke
Copy link
Member

kohsuke commented Nov 14, 2013

I started thinking what the same approach would mean for method calls. On the regular method call side, most regular calls result in MetaMethodSite, which invokes MetaMethod.invoke().

That would help us correctly identify that x=new Object[3]; x.putAt(1,"foo") is actually an array put, handle mix-in methods properly, and so on.

@kohsuke
Copy link
Member

kohsuke commented Nov 14, 2013

on the method call side, the method to create a call site is strangely defined in CallSiteArray.createCallSite, which is a private method and there's no call path to use this without also invokcing the obtained call site right away, so there's another need for reflection access...

@jglick
Copy link
Member Author

jglick commented Apr 1, 2014

As https://github.com/jenkinsci/script-security-plugin/blob/b9d45a39f022f10ed0bc44d5c1aba2775fa74609/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java indicates, there are a whole lot of cases where the interceptor has no good way of knowing what AccessibleObject it is actually being asked about: bean property syntax, GString parameters, closures cast to SAMs, boxing/unboxing, numeric conversions, pseudomethods for operations on primitives and String concatenation, GroovyObject dynamic properties, array .length, ad nauseam. In practice it is really hard to write an interceptor which is able to classify everything thrown at it.

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

No branches or pull requests

2 participants