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

[JENKINS-37129] unclassified method vs MethodMissingException #134

Merged
merged 1 commit into from Jul 19, 2017

Conversation

Projects
None yet
4 participants
@kohsuke
Copy link
Member

kohsuke commented Jul 14, 2017

JENKINS-37129

As an user, I don't always precisely remember what methods an object has, an I often end up making mistakes. From workflow-cps perspective, this results in trying to call a method that does not exist.

This must result in MethodMissingException, just like normal Groovy interpreter. The code currently makes this a RejectedAccessException, making the user believe that the method exists but the access it not allowed.

[JENKINS-37129] unclassified method vs MethodMissingException
As an user, I don't always precisely remember what methods an object
has, an I often end up making mistakes. From workflow-cps perspective,
this results in trying to call a method that does not exist.

This must result in MethodMissingException, just like normal Groovy
interpretor. The code currently makes this a RejectedAccessException,
making the user believe that the method exists but the access it not
allowed.
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 14, 2017

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jul 14, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@abayer

abayer approved these changes Jul 14, 2017

Copy link
Member

abayer left a comment

Generally approve, but I feel like there are cases currently where unclassified method shows up that aren't really MethodMissingExceptions...however, I can't come up with one of them, so... =)

// the case where the unsafe receiver type causes the security check to fail
try {
assertEvaluate(new GenericWhitelist(), "should fail", "[].noSuchMethod()");
} catch (MissingMethodException e) {

This comment has been minimized.

Copy link
@jglick

jglick Jul 17, 2017

Member

Will need to be reworked slightly after #132.

try {
assertEvaluate(new GenericWhitelist(), "should fail", "[].class.classLoader");
} catch (RejectedAccessException e) {
assertEquals("method java.lang.Class getClassLoader", e.getSignature());

This comment has been minimized.

Copy link
@jglick

jglick Jul 17, 2017

Member

This is already covered by assertRejected.

assertEquals("method java.lang.Class getClassLoader", e.getSignature());
}

// the case where the safe receiver type causes the security check to pass

This comment has been minimized.

Copy link
@jglick

jglick Jul 17, 2017

Member

What is the difference between a “safe” and “unsafe” receiver type? I am not following the distinction between the first and third cases here.

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 19, 2017

Author Member

Prior to the fix, I noticed that the error message was different between those two cases. 1.noSuchMethod() already result in MissingMethodException, and I assumed it was because the receiver type Integer is already whitelisted so the sandbox invoker gets a green light to attempt to invoke a method.

[].noSuchMethod() failed differently, presumably because whitelist rule rejects this.

This comment has been minimized.

Copy link
@jglick

jglick Jul 20, 2017

Member

Misanalyzed; corrected in #135.

@@ -110,7 +111,8 @@
return super.onMethodCall(invoker, receiver, method, args);
}

throw new RejectedAccessException("unclassified method " + EnumeratingWhitelist.getName(receiver.getClass()) + " " + method + printArgumentTypes(args));
// no such method exists
throw new MissingMethodException(method, receiver.getClass(), args);

This comment has been minimized.

Copy link
@jglick

jglick Jul 17, 2017

Member

What about MissingFieldException and MissingPropertyException? (I do not think MissingClassException is relevant here.)

@kohsuke kohsuke merged commit 1ff48ff into master Jul 19, 2017

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details

@kohsuke kohsuke deleted the MissingMethodException branch Jul 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.