[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/groovy/pull/532


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117609738
  
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object 
object, String name, boolean useS
 } catch (IllegalArgumentException e) {
 // can't access the field directly but there may be a 
getter
 mp = null;
+} catch (GroovyRuntimeException e) {
--- End diff --

Done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117188820
  
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object 
object, String name, boolean useS
 } catch (IllegalArgumentException e) {
 // can't access the field directly but there may be a 
getter
 mp = null;
+} catch (GroovyRuntimeException e) {
--- End diff --

How do you see it?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117188688
  
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object 
object, String name, boolean useS
 } catch (IllegalArgumentException e) {
 // can't access the field directly but there may be a 
getter
 mp = null;
+} catch (GroovyRuntimeException e) {
--- End diff --

On the other side if it does not work because AccessControlExceptions are 
not handled by the calling code properly `public class 
CacheAccessControlException extends GroovyRuntimeException` could also be taken 
as a hack.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117188226
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +92,12 @@ public CachedClass getDeclaringClass() {
 
 public final Object invoke(Object object, Object[] arguments) {
 try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new InvokerInvocationException(new 
GroovyRuntimeException("Illegal access to method" + cachedMethod.getName()));
+}
+try {
 return cachedMethod.invoke(object, arguments);
--- End diff --

I think that InvokerInvocationException is specific to call of invoke() and 
it should be not thrown  by AccessPermissionChecker directly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117188013
  
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object 
object, String name, boolean useS
 } catch (IllegalArgumentException e) {
 // can't access the field directly but there may be a 
getter
 mp = null;
+} catch (GroovyRuntimeException e) {
--- End diff --

I think AccessPermissionChecker should only throw exceptions extending from 
AccessControlException. because it specifically checks access permissions. 

So I suggest to introduce `public class CacheAccessControlException extends 
AccessControlException`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[blackdrag]

Github user blackdrag commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117177738
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +92,12 @@ public CachedClass getDeclaringClass() {
 
 public final Object invoke(Object object, Object[] arguments) {
 try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new InvokerInvocationException(new 
GroovyRuntimeException("Illegal access to method" + cachedMethod.getName()));
+}
+try {
 return cachedMethod.invoke(object, arguments);
--- End diff --

can we move thos try-catch into checkAccessPermission ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[blackdrag]

Github user blackdrag commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117177601
  
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object 
object, String name, boolean useS
 } catch (IllegalArgumentException e) {
 // can't access the field directly but there may be a 
getter
 mp = null;
+} catch (GroovyRuntimeException e) {
--- End diff --

At this point mp should be a CachedField. Since 
AccessPermissionChecker.checkAccessPermission(field); may now throw a 
GroovyRuntimeException, we have here the requirement to catch it. Problem is, 
that mp might not be a CachedField. If we will call a user method here instead, 
this would lead to a wrong flow. Thus I think the GroovyRuntimeException usage 
is a bad idea. Better make a new Exception, that extends GroovyRuntimeException 
and throw that in AccessPermissionChecker, which we then can catch


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r117078128
  
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object 
object, String name, boolean useS
 } catch (IllegalArgumentException e) {
 // can't access the field directly but there may be a 
getter
 mp = null;
+} catch (GroovyRuntimeException e) {
+// can't access the field directly but there may be a 
getter
+mp = null;
--- End diff --

I don't understand why this was added.  Was there a test that failed 
because of the other changes that required this.  I would have assumed an 
`AccessControlException` from `CachedField.getProperty` would be treated 
similar to the `IllegalAccessException` in the same method.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116377499
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -124,6 +131,12 @@ public String getSignature() {
 }
 
 public final Method setAccessible() {
+try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to method" 
+ cachedMethod.getName());
--- End diff --

As above


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116377497
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +91,12 @@ public CachedClass getDeclaringClass() {
 
 public final Object invoke(Object object, Object[] arguments) {
 try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new InvokerInvocationException(new 
IllegalArgumentException("Illegal access to method" + cachedMethod.getName()));
--- End diff --

As above


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116377501
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -324,6 +337,12 @@ else if (o2 instanceof CachedMethod)
 }
 
 public Method getCachedMethod() {
+try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to method" 
+ cachedMethod.getName());
--- End diff --

As above


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116377495
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -65,6 +72,12 @@ public Object getProperty(final Object object) {
  * @throws RuntimeException if the property could not be set
  */
 public void setProperty(final Object object, Object newValue) {
+try {
+AccessPermissionChecker.checkAccessPermission(field);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to field" + 
" " + field.getName());
--- End diff --

As above


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116377482
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -51,6 +52,12 @@ public int getModifiers() {
  */
 public Object getProperty(final Object object) {
 try {
+AccessPermissionChecker.checkAccessPermission(field);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to field" + 
" " + field.getName());
--- End diff --

To use GroovyRuntimeException we need to patch also 
MetaClassImpl.getProperty . It was not possible with byte buddy, but as a 
groovy patch it is possible. I shall do it in a separate commit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

Github user dpolivaev commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116377418
  
--- Diff: 
src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.reflection;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.ReflectPermission;
+
+import groovy.lang.GroovyObject;
+
+class AccessPermissionChecker {
+
+   private static final ReflectPermission REFLECT_PERMISSION = new 
ReflectPermission("suppressAccessChecks");
+
+   static private void checkAccessPermission(Class declaringClass, 
final int modifiers, boolean isAccessible) {
+   final SecurityManager securityManager = 
System.getSecurityManager();
--- End diff --

Signature changed. The check is performed only if (securityManager != null 
&& isAccessible)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365638
  
--- Diff: 
src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.reflection;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.ReflectPermission;
+
+import groovy.lang.GroovyObject;
+
+class AccessPermissionChecker {
--- End diff --

Might be good to add a private constructor to signal that no instances of 
this method are desired, since it only contains static methods.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365514
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -324,6 +337,12 @@ else if (o2 instanceof CachedMethod)
 }
 
 public Method getCachedMethod() {
+try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to method" 
+ cachedMethod.getName());
--- End diff --

see comment on `setAccessible`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365223
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -65,6 +72,12 @@ public Object getProperty(final Object object) {
  * @throws RuntimeException if the property could not be set
  */
 public void setProperty(final Object object, Object newValue) {
+try {
+AccessPermissionChecker.checkAccessPermission(field);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to field" + 
" " + field.getName());
--- End diff --

see above comment about `GroovyRuntimeException`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365151
  
--- Diff: 
src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.reflection;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.ReflectPermission;
+
+import groovy.lang.GroovyObject;
+
+class AccessPermissionChecker {
+
+   private static final ReflectPermission REFLECT_PERMISSION = new 
ReflectPermission("suppressAccessChecks");
+
+   static private void checkAccessPermission(Class declaringClass, 
final int modifiers, boolean isAccessible) {
+   final SecurityManager securityManager = 
System.getSecurityManager();
+   if (isAccessible && securityManager != null) {
+   if ((modifiers & Modifier.PRIVATE) != 0
+   || ((modifiers & 
(Modifier.PUBLIC | Modifier.PROTECTED)) == 0
+&& 
packageCanNotBeAddedAnotherClass(declaringClass))
+   && 
!GroovyObject.class.isAssignableFrom(declaringClass)) {
+
securityManager.checkPermission(REFLECT_PERMISSION);
+}
+else if ((modifiers & (Modifier.PROTECTED)) != 0
+   && 
declaringClass.equals(ClassLoader.class)){
+   
securityManager.checkCreateClassLoader();
+   }
+   }
+   }
+
+   private static boolean packageCanNotBeAddedAnotherClass(Class 
declaringClass) {
+   return declaringClass.getName().startsWith("java.");
+   }
+
+   static public void checkAccessPermission(Method method) {
+   checkAccessPermission(method.getDeclaringClass(), 
method.getModifiers(), method.isAccessible()
+   );
+   }
+
+   public static void checkAccessPermission(Field field) {
--- End diff --

`public` isn't needed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365503
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -124,6 +131,12 @@ public String getSignature() {
 }
 
 public final Method setAccessible() {
+try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to method" 
+ cachedMethod.getName());
--- End diff --

No arguments to this method so `IllegalArgumentException` doesn't seem 
right.  Think it might be good to either just remove the `try/catch` or wrap in 
a `GroovyRuntimeException` (to capture cachedMethod.getName).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365354
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +91,12 @@ public CachedClass getDeclaringClass() {
 
 public final Object invoke(Object object, Object[] arguments) {
 try {
+AccessPermissionChecker.checkAccessPermission(cachedMethod);
+}
+catch (AccessControlException ex) {
+throw new InvokerInvocationException(new 
IllegalArgumentException("Illegal access to method" + cachedMethod.getName()));
--- End diff --

Throwing an `IllegalArgumentException` could be confusing since it's not 
the `arguments` passed to the invoked method causing the problem.  Maybe

`throw new InvokerInvocationException(ex)`

or 

```
throw new InvokerInvocationException(
new AccessControlException("Illegal access to method" + 
cachedMethod.getName(), ex)
)
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365210
  
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -51,6 +52,12 @@ public int getModifiers() {
  */
 public Object getProperty(final Object object) {
 try {
+AccessPermissionChecker.checkAccessPermission(field);
+}
+catch (AccessControlException ex) {
+throw new IllegalArgumentException("Illegal access to field" + 
" " + field.getName());
--- End diff --

Any reason for using `IllegalArgumentException`?  I think the following may 
be more appropriate:

```java
throw new GroovyRuntimeException("Illegal access to field " + 
field.getName() + ".", ex);
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365112
  
--- Diff: 
src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.reflection;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.ReflectPermission;
+
+import groovy.lang.GroovyObject;
+
+class AccessPermissionChecker {
+
+   private static final ReflectPermission REFLECT_PERMISSION = new 
ReflectPermission("suppressAccessChecks");
+
+   static private void checkAccessPermission(Class declaringClass, 
final int modifiers, boolean isAccessible) {
+   final SecurityManager securityManager = 
System.getSecurityManager();
--- End diff --

Might be good to exit early if `securityManager == null`, then wont have to 
check again below.

Also, be good to change order to be `private static void ...`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[jwagenleitner]

Github user jwagenleitner commented on a diff in the pull request:

https://github.com/apache/groovy/pull/532#discussion_r116365147
  
--- Diff: 
src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.reflection;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.ReflectPermission;
+
+import groovy.lang.GroovyObject;
+
+class AccessPermissionChecker {
+
+   private static final ReflectPermission REFLECT_PERMISSION = new 
ReflectPermission("suppressAccessChecks");
+
+   static private void checkAccessPermission(Class declaringClass, 
final int modifiers, boolean isAccessible) {
+   final SecurityManager securityManager = 
System.getSecurityManager();
+   if (isAccessible && securityManager != null) {
+   if ((modifiers & Modifier.PRIVATE) != 0
+   || ((modifiers & 
(Modifier.PUBLIC | Modifier.PROTECTED)) == 0
+&& 
packageCanNotBeAddedAnotherClass(declaringClass))
+   && 
!GroovyObject.class.isAssignableFrom(declaringClass)) {
+
securityManager.checkPermission(REFLECT_PERMISSION);
+}
+else if ((modifiers & (Modifier.PROTECTED)) != 0
+   && 
declaringClass.equals(ClassLoader.class)){
+   
securityManager.checkCreateClassLoader();
+   }
+   }
+   }
+
+   private static boolean packageCanNotBeAddedAnotherClass(Class 
declaringClass) {
+   return declaringClass.getName().startsWith("java.");
+   }
+
+   static public void checkAccessPermission(Method method) {
--- End diff --

`public` isn't needed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...

[dpolivaev]

GitHub user dpolivaev opened a pull request:

https://github.com/apache/groovy/pull/532

Prevent CachedField and CachedMethod from leaking access permissions …

…to scripts

https://issues.apache.org/jira/browse/GROOVY-8163

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dpolivaev/groovy master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/groovy/pull/532.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #532


commit 20741fe4f61940a2e5ab56c67d0710a17ac5583f
Author: Dimitry Polivaev 
Date:   2017-05-01T20:58:12Z

Prevent CachedField and CachedMethod from leaking access permissions to 
scripts

https://issues.apache.org/jira/browse/GROOVY-8163




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---