[SECURITY] Don't accept any new parameter names from pull request comments#85
[SECURITY] Don't accept any new parameter names from pull request comments#85jakub-bochenski merged 1 commit intojenkinsci:masterfrom proski:parameter-security
Conversation
|
Suppressed an issue reported by with FindBugs. The fix is in #86, but I don't want to mix it with a security fix. |
|
Rebased, added instructions to |
proski
changed the title
I tried writing unit tests, and it looks like I'm writing throwaway code. It's hard to create an The first reason is the control flow. The The second reason is the assumption that Would it be possible to merge #86 first? Then this PR would deal with Sure, tests for |
Take the values for the environment variables directly from StashCause. This fixes warnings about parameters for freestyle projects. Also, the environment variables are listed at one place now.
|
I've added the unit tests. It was an interesting exercise. I used |
| package stashpullrequestbuilder.stashpullrequestbuilder; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.*; |
There was a problem hiding this comment.
minor: I would prefer we don't use wildcard imports, most IDEs have the option to expand them
There was a problem hiding this comment.
Done. It's common to use wildcards for matchers in unit tests, but it's not a style requirement, so I'm fine either way.
There was a problem hiding this comment.
I plan to apply google style guide to this someday, it will be less changes if we don't use wildcards
| } | ||
|
|
||
| private List<ParameterValue> captureJobParameters() { | ||
| when(project.getQuietPeriod()).thenReturn(0); |
There was a problem hiding this comment.
this is a bit awkward, doing both expectations and verification in the same method
any chance we could split this without making the code yet more complex?
There was a problem hiding this comment.
Strictly speaking, the only purpose of verification is to capture the parameter, so it's a bit misleading. But I split that method anyway without making the unit tests any longer.
Thanks a lot for figuring it out! |
|
Addressed the PR comments. |
Parameters extracted from Stash comments are only allowed to override the default values of the parameters defined for the job. Accepting arbitrary parameters from Stash comments can be exploited by a user who can post comments but not administer the Jenkins job. Pass only one copy of the parameter to the job. Remove null parameters only after the values from the Stash comments have been applied. Suppress a FindBugs warning about this.job being null. That cannot happen, but FindBugs cannot figure it out.
| additionalParameters); | ||
| } | ||
|
|
||
| private void jobSetup(List<ParameterDefinition> parameterDefinitions) { |
There was a problem hiding this comment.
varargs would make this even easier to use I think
2 participants
Parameters extracted from Stash comments should only be allowed to
override the default values of the parameters defined for the job.
Accepting arbitrary parameters from Stash comments can be exploited by a
user that can post comments but not administer the job in Jenkins.
Pass only one copy of the parameter to the job. Remove null parameters
only after the values from the Stash comments have been applied.