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

Allow calling Closure elements of Maps as methods #199

Merged
merged 2 commits into from
Apr 26, 2018

Conversation

rudolfwalter
Copy link
Contributor

The sandbox rejects valid Groovy code when trying to call a Closure element of a Map as if it were one of the Map's methods. See JENKINS-50843.

@abayer abayer requested review from abayer, jglick and svanoort April 17, 2018 14:08
Copy link
Member

@abayer abayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, but want to get someone else's eyes on it before merging. Thanks!

Copy link
Member

@svanoort svanoort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything obviously wrong with this, but wonder a bit if there's additional cases we might want to cover such as Closures with arguments and first-class functions.

@rudolfwalter
Copy link
Contributor Author

@svanoort By Closures with arguments you mean in this same scenario, as Map elements? If so, this covers them as well. Should I add this to the unit test?

Also, I don't think I know what you mean by first-class functions.

@svanoort
Copy link
Member

@rudolfwalter I think it would be helpful to have Closures taking parameters in the test, yeah.

Ignore the comment about functions.

@rudolfwalter
Copy link
Contributor Author

@svanoort done :)

@@ -114,6 +116,14 @@
throw StaticWhitelist.rejectStaticMethod(foundDgmMethod);
}

// allow calling Closure elements of Maps as methods
if (receiver instanceof Map) {
Object element = onMethodCall(invoker, receiver, "get", method);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not technically correct to delegate to onMethodCall here: if the whitelist did not pass this particular Map.get(Object) call but the receiver was allowed to work in some other way like the invokeMethod below, then we would be breaking something that worked before. OTOH Map.get(Object) at the interface level is whitelisted, so using the standard whitelist there is no way this would not pass.

Just another example of the problem noted in jenkinsci/groovy-sandbox#7 (comment).

@rudolfwalter
Copy link
Contributor Author

@abayer Kind reminder that this needs to be merged.

@svanoort
Copy link
Member

@rudolfwalter Don't worry, I've got this.

@svanoort svanoort merged commit 23fdbbb into jenkinsci:master Apr 26, 2018
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

Successfully merging this pull request may close these issues.

5 participants