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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-63171] Add more generic whitelist entries #304

Merged
merged 4 commits into from
Jul 23, 2020
Merged

[JENKINS-63171] Add more generic whitelist entries #304

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b171b3b
[JENKINS-63171] Add more generic whitelist entries
jvz Jul 23, 2020
d991a88
Add StringGroovyMethods equivalents
jvz Jul 23, 2020
55a2c7f
Fix typos
jvz Jul 23, 2020
0b3a30b
Add one more
jvz Jul 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Definitely safe: https://github.com/apache/groovy/blob/731d4daa78b9cd32bea724d2d651aa1164eef6cc/src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java#L15348.

staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods any java.lang.Iterable groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods any java.lang.Object groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods any java.util.Map groovy.lang.Closure
Expand Down Expand Up @@ -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 •
edited

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

staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods toInteger java.lang.String
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods toList boolean[]
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods toList byte[]
Expand Down Expand Up @@ -1114,6 +1117,11 @@ staticMethod org.codehaus.groovy.runtime.StringGroovyMethods replaceFirst java.l
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods reverse java.lang.String
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods takeWhile groovy.lang.GString groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods takeWhile java.lang.CharSequence groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods toBoolean java.lang.String
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods toDouble java.lang.CharSequence
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods toDouble java.lang.String
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods toInteger java.lang.CharSequence
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods toInteger java.lang.String
Comment on lines +1123 to +1124
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.

staticMethod org.codehaus.groovy.runtime.StringGroovyMethods tr java.lang.CharSequence java.lang.CharSequence java.lang.CharSequence
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods unexpand java.lang.CharSequence
staticMethod org.codehaus.groovy.runtime.StringGroovyMethods unexpand java.lang.CharSequence int
Expand Down