search

jenkinsci / groovy-sandbox

(Deprecated) Compile-time transformer to run Groovy code in a restrictive sandbox
MIT License
122 stars 60 forks source link

[FIXED JENKINS-46088] Stop double-transforming casts in declarations #37

Closed abayer closed 7 years ago

abayer commented 7 years ago

JENKINS-46088

We were, well, double-transforming here. Transforming the DeclarationExpression after adding the transformed CastExpression to it resulted in a checkedStaticCall to Checker.checkedCast! That...was not desirable. I believe this caused some other issues where you could get mysterious Collections.toArray, Checker.checkedStaticCall, and Checker.checkedCall related RejectedAccessExceptions, based on inspecting the resulting AST from the double-transform approach.

So let's just transform the whole DeclarationExpression once. Gets us the goal we want without the double transformation.

cc @reviewbybees

abayer commented 7 years ago

Downstream PR up at https://github.com/jenkinsci/script-security-plugin/pull/139 - note that there is no groovy-cps or workflow-cps downstream PR, because this code path is literally never touched by CPS code, since CPS code doesn't use SandboxTransformer from here.

ghost commented 7 years ago

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

daniel-beck commented 7 years ago

@abayer Are we sure this retains the security fix?

abayer commented 7 years ago

Yes. checkedCast is still getting called. Once I'm a bit more awake, I can paste copies of what the transformed AST looked like before and after.

abayer commented 7 years ago

@jglick I am confident that this fixes the problem in question without ramifications, but I do agree that a more thorough change may be best.

jglick commented 7 years ago

If the test coverage were strengthened to ensure that your change does not drop sandbox transformation where it is needed, I could approve this.

abayer commented 7 years ago

Merged - could you cut a release, @jglick? Thanks.