jenkinsci / groovy-sandbox

(Deprecated) Compile-time transformer to run Groovy code in a restrictive sandbox
MIT License
123 stars 60 forks source link

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

Open jglick opened 10 years ago

jglick commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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.