Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-45776

Script Security translates some groovy functions incorrectly/not as expected

    XMLWordPrintable

Details

    • Pipeline - July/August

    Description

      I have a custom class which I am using inside of some groovy functions with a list of custom methods. When I use that custom class inside of a script needed to be approved by script security it is being transformed differently than expected.

       

      When I try to do a for loop through an array and do `i++` it is being translated as i.next() instead of adding 1 and then when I am calling the creator method `def $ITEM` it is being created as a hashmap. Neither of those methods exists inside of my custom class so it is throwing errors like `No signature of method X.next()` and `unclassified new X java.util.LinkedHashMap`

       

      I would either like to get more visibility about hot the translation is happening or potentially have other default method calls.

       

      Let me know if I can provide more context here.

      Attachments

        Issue Links

          Activity

            abayer Andrew Bayer added a comment - - edited

            I've got a minimal reproduction with script-security 1.29:

            def l = ['a/b/c', 'd/e', 'f']
            
            def str = ''
            l.each { s ->
                def tokens = s.split('/')
                for (int i = 0; i < tokens.size(); i++) {
                    str += tokens[i]
                }
            }
            
            return str
            

            Passed through the sandbox, you get org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object (Script1 next). This only happens when you've got the for (...) loop inside the .each loop, so there's something going wrong there specifically.

            abayer Andrew Bayer added a comment - - edited I've got a minimal reproduction with script-security 1.29: def l = [ 'a/b/c' , 'd/e' , 'f' ] def str = '' l.each { s -> def tokens = s.split( '/' ) for ( int i = 0; i < tokens.size(); i++) { str += tokens[i] } } return str Passed through the sandbox, you get org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object (Script1 next) . This only happens when you've got the for (...) loop inside the .each loop, so there's something going wrong there specifically.
            abayer Andrew Bayer added a comment -

            Even more minimal!

            def cnt = 0
            [0, 1, 2, 3, 4].each {
              cnt++
            }
            return cnt
            

            That gets org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object (Script1 next) as well. So the problem seems to be the ++ inside the each.

            abayer Andrew Bayer added a comment - Even more minimal! def cnt = 0 [0, 1, 2, 3, 4].each { cnt++ } return cnt That gets org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object (Script1 next) as well. So the problem seems to be the ++ inside the each .
            jglick Jesse Glick added a comment -

            Somehow the next method is being applied to the Script rather than the Number as it should. No idea why. Some sort of confusion about Closure handling. Is this reproducible in general script-security (not Pipeline)?

            jglick Jesse Glick added a comment - Somehow the next method is being applied to the Script rather than the Number as it should. No idea why. Some sort of confusion about Closure handling. Is this reproducible in general script-security (not Pipeline)?
            abayer Andrew Bayer added a comment -

            jglick Yup, this is script-security, not Pipeline - I haven't verified but am pretty sure it can't be reproduced in Pipeline at all.

            I thought for a bit that it was a local vs not local variable issue, but it's not that. Still digging.

            abayer Andrew Bayer added a comment - jglick Yup, this is script-security , not Pipeline - I haven't verified but am pretty sure it can't be reproduced in Pipeline at all. I thought for a bit that it was a local vs not local variable issue, but it's not that. Still digging.
            abayer Andrew Bayer added a comment -

            The difference between calling cnt++ outside a each loop and inside a each loop is that the first argument to Checker.checkedCall changes from this for outside:

            org.codehaus.groovy.ast.expr.VariableExpression@5c30a9b0[variable: cnt]
            

            to this for inside:

            - method 'ConstantExpression[getOwner]':
              - method 'ConstantExpression[asWritable]':
                - org.codehaus.groovy.ast.expr.VariableExpression@15c43bd9[variable: this]
                  - args:
                - args:
            

            (excuse the formatting - I've got some tools I'm polishing for debugging AST transformations that I'm using here)
            Now I'm figuring out why. =)

            abayer Andrew Bayer added a comment - The difference between calling cnt++ outside a each loop and inside a each loop is that the first argument to Checker.checkedCall changes from this for outside: org.codehaus.groovy.ast.expr.VariableExpression@5c30a9b0[variable: cnt] to this for inside: - method 'ConstantExpression[getOwner]' : - method 'ConstantExpression[asWritable]' : - org.codehaus.groovy.ast.expr.VariableExpression@15c43bd9[variable: this ] - args: - args: (excuse the formatting - I've got some tools I'm polishing for debugging AST transformations that I'm using here) Now I'm figuring out why. =)
            abayer Andrew Bayer added a comment -

            Ok, looks like the problem is that our checks to determine whether to use the getOwner etc closure call are based on whether we're in a closure and the expression in question returns true for isImplicitThis() - but that almost always returns true, and isn't fine-grained enough a heuristic. I'm working on a better solution.

            abayer Andrew Bayer added a comment - Ok, looks like the problem is that our checks to determine whether to use the getOwner etc closure call are based on whether we're in a closure and the expression in question returns true for isImplicitThis() - but that almost always returns true, and isn't fine-grained enough a heuristic. I'm working on a better solution.
            abayer Andrew Bayer added a comment - Preliminary PR up at https://github.com/jenkinsci/groovy-sandbox/pull/36

            People

              abayer Andrew Bayer
              ataylor Alex Taylor
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: