Hi, I added the Jenkins security scan to my plugin on GitHub and resolved all the issues.Or, so I thought.
One of the main classes of reported issues (aside from adding @POST to pretty much all Stapler doXXX methods) was the "no access check done". While most of the methods I have (mostly doCheck and DoFill...Items) are very simple (no file/net/... access), I figured it would not hurt to add a basic check either. For the bulk of the methods, typically involved in configuring a step, I did the following: - add an @AncestorInPath Item item parameter to the method - add if (item != null) item.checkPermission(Permission.CONFIGURE); near the top of the method. This seemed to make sense - you'd need to have access to the Configure menu item to see the interface that calls those methods anyway For those bits related to the global tool setup, I simply use Jenkins.get ().checkPermission(Jenkins.MANAGE); instead. Again, this seems to make sense, given the tool setup lives among the Manage Jenkins options. However, when I set up users and activate the matrix strategy, then a user who has the Job/Configure permission will get an Unauthorized error under one of the drop-downs, saying they do not have the N/A/GenericConfigure permission. Does this mean I should be testing for FreeStyleProject.CONFIGURE instead (these Permission things seem rather hard to discover, I must say)? Or will that prevent using the same UI/methods in other contexts (e.g. what permissions make the pipeline syntax generator available? that uses the same interface as the freestyle project configuration)? Am I right in using checkPermission, or should I be using checkAnyPermission to check for multiples (say Permission.CONFIGURE, FreeStyleProject,CONFIGURE, Jenkins.MANAGE)? Or should I be using has(Any)Permission() and simply fail silently (i.e. return OK validation, or filling no items)? If the suggestion is "just don't do any checking if there's no real need" then I would really appreciate a way to be able to declare that in code so that the security scan will not raise an issue for it. E.g. a @Unsecured annotation or an empty Jenkins.noPermissionsNeeded() method that will satisfy the "Stapler methods must check access rights" requirement. Come to that, if a method doesn't really need @POST, is there any harm (e.g. a perf hit) in adding it? If so, maybe a @GET/@NoPOST annotation to say "this does not need @POST, stop bugging me" would be nice too. Security scans become less useful if they mostly produce false positives. Thanks in advance for any guidance. -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAKMi--AGnAGGMA6OrwV3zR6U-%3DtHRU03Y4piZjvctgP2Lf%2BXqg%40mail.gmail.com.
