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

Script Security translates some groovy functions incorrectly/not as expected

    • Pipeline - July/August

      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.

          [JENKINS-45776] Script Security translates some groovy functions incorrectly/not as expected

          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.

          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.

          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.

          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 .

          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)?

          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)?

          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.

          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.

          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. =)

          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. =)

          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.

          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.

          Andrew Bayer added a comment -

          Andrew Bayer added a comment - Preliminary PR up at https://github.com/jenkinsci/groovy-sandbox/pull/36

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

              Created:
              Updated:
              Resolved: