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-63171] Add more generic whitelist entries #304

Merged
merged 4 commits into from Jul 23, 2020

Conversation

jvz
Copy link
Member

@jvz jvz commented Jul 23, 2020

@@ -642,6 +642,7 @@ staticMethod org.codehaus.groovy.runtime.DateGroovyMethods upto java.util.Calend
staticMethod org.codehaus.groovy.runtime.DateGroovyMethods upto java.util.Date java.util.Date groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods abs java.lang.Number
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods addAll java.util.Collection java.lang.Object[]
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods and java.lang.Boolean java.lang.Boolean
Copy link
Member

Choose a reason for hiding this comment

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

@@ -965,7 +966,9 @@ staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods take java.util.Lis
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods takeRight java.lang.Iterable int
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods takeRight java.util.List int
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods times java.lang.Number groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods toBoolean java.lang.Boolean
Copy link
Member

@dwnusbaum dwnusbaum Jul 23, 2020

Choose a reason for hiding this comment

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

Looks like this method is @Deprecated and delegates to StringGroovyMethods.toBoolean(String), so I would also add that method at the same time. The implementation is safe, although a bit weird (special cases for "y" and "1"): https://github.com/apache/groovy/blob/731d4daa78b9cd32bea724d2d651aa1164eef6cc/src/main/org/codehaus/groovy/runtime/StringGroovyMethods.java#L3240-L3248

staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods toBoolean java.lang.String
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods toDouble java.lang.String
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, this method is @Deprecated and delegates to StringGroovyMethods.toDouble(CharSequence), so I would add that method at the same time. The implementation looks safe: https://github.com/apache/groovy/blob/731d4daa78b9cd32bea724d2d651aa1164eef6cc/src/main/org/codehaus/groovy/runtime/StringGroovyMethods.java#L3270-L3272

Comment on lines +1122 to +1123
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods toInteger java.lang.CharSequence
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods toInteger java.lang.String
Copy link
Member

Choose a reason for hiding this comment

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

Ok one last thing, but DefaultGroovyMethods.toInteger(String) (which is deprecated) is (I think) the method that SandboxInterceptor will actually think you are calling if you write something like "123".toInteger(), so you probably need to add that one too. Regular Groovy does not consider deprecated methods when selecting extension methods, so I asked you to add the non-deprecated StringGroovyMethods equivalents in case we ever fix that bug 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I still have that in the file, don't I?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I missed that it was already present before your changes, so yeah it should be fine as-is.

@dwnusbaum dwnusbaum merged commit e9ec442 into jenkinsci:master Jul 23, 2020
@jvz jvz deleted the JENKINS-63171 branch July 23, 2020 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants