Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-34753] Provide safe parameters to ParametersAction #285

Merged
merged 11 commits into from May 30, 2016

Conversation

Projects
None yet
7 participants
@rsandell
Copy link
Member

commented May 18, 2016

This fix assumes that jenkinsci/jenkins#2353 gets merged and released.

@reviewbybees

rsandell added some commits May 17, 2016

First fix all tests that should still succeed on 2.x
Mostly due to more mocking and some legacy use of Hudson
[JENKINS-34753] Provide safe parameters to ParametersAction
And fixed some checkstyle errors in the tests

/**
* Constructor.
* @param config the global config.
* @param hudson the Hudson instance.
*/
public ParameterExpander(IGerritHudsonTriggerConfig config, Hudson hudson) {
public ParameterExpander(IGerritHudsonTriggerConfig config, Jenkins hudson) {

This comment has been minimized.

Copy link
@scoheb

scoheb May 18, 2016

Contributor

one more? Jenkins hudson -> Jenkins jenkins

This comment has been minimized.

Copy link
@rsandell

rsandell May 18, 2016

Author Member

They are everywhere :)

@@ -1553,9 +1554,10 @@ public String toString() {
@Test
public void shouldReturnEmptySlaveListWhenGerritServerNotFound() {
// setup
PowerMockito.mockStatic(PluginImpl.class);
mockConfig();
/*PowerMockito.mockStatic(PluginImpl.class);

This comment has been minimized.

Copy link
@scoheb

scoheb May 18, 2016

Contributor

Remove commented code?

This comment has been minimized.

Copy link
@rsandell

rsandell May 18, 2016

Author Member

Yes, I was testing different approaches and missed cleaning up, thanks.

@reviewbybees

This comment has been minimized.

Copy link

commented May 18, 2016

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.

Collection.class);
return constructor.newInstance(parameters, GerritTriggerParameters.getNamesSet());
} catch (NoSuchMethodException e) {
logger.debug("Running on an old core before safe parameters, we are safe.", e);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2016

Member

Not safe on 2.3, 2.4 and 2.5

rsandell added some commits May 18, 2016

@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 18, 2016

@oleg-nenashev I'm hoping the version checks are correct?
I'm assuming there won't be a backport to 1.651.x

rsandell added some commits May 18, 2016

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 22, 2016

@rsandell
I'm assuming there won't be a backport to 1.651.x
This is a tough question, because SECURITY-170 nuked this LTS baseline as well.

@olivergondza WDYT?

version.isNewerThan(new VersionNumber("2.2"))
|| (
version.isNewerThan(new VersionNumber("1.651.1"))
&& version.isOlderThan(new VersionNumber("1.651.300"))

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 22, 2016

Member

Yay. You've reserved enough versions :)

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 22, 2016

@rsandell
I see two issues:

  • Pushing of LTS users to 2.x non-LTS
  • There are people/companies, which maintain their own "old" baselines on the top of LTS. E.g. CloudBees. If they backport the security fix, this version check will behave incorrectly

Hence 🐛 . My suggestion would be to determine affected versions by checking existence of ParametersAction.KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME or safeParameters field via reflection. @daniel-beck WDYT?

@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 23, 2016

@@ -251,6 +257,42 @@ protected Job asJob() {
protected ParametersAction createParameters(GerritTriggeredEvent event, Job project) {

This comment has been minimized.

Copy link
@varmenise

varmenise May 25, 2016

NIT: you may want to update the javadoc of the method mentioning that now the parametersAction add the safe parameters to the default ones since it already has a description that says Creates a ParameterAction and fills it with the project's default parameters + the Standard Gerrit parameters.

}

/**
* If the system property ..keepSafeParameters is set and set to true.

This comment has been minimized.

Copy link
@varmenise

varmenise May 25, 2016

s/keepSafeParameters/keepUndefinedParameters ?

+ " to self-specify safe parameters.");
txt.append(" You should consider upgrading to a new Jenkins core version.\n");
if (inspection.isKeepUndefinedParameters()) {
txt.append(".keepSafeParameters is set so the trigger should behave normally.");

This comment has been minimized.

Copy link
@varmenise

varmenise May 25, 2016

keepSafeParameters or keepUndefined?

@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 26, 2016

@varmenise thanks for the review! I believe I've addressed your comments

@varmenise

This comment has been minimized.

Copy link

commented May 26, 2016

LGTM 🐝

@rsandell rsandell merged commit 8b46525 into master May 30, 2016

1 check passed

Jenkins This pull request looks good
Details

@rsandell rsandell deleted the JENKINS-34753 branch May 30, 2016

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 30, 2016

@rsandell sorry for the delayed response. LGTM as well

@brianpreuss

This comment has been minimized.

Copy link

commented Jun 6, 2016

On Jenkins 1.651.2 this gives me:

Jun 06, 2016 11:17:13 AM SEVERE com.sonymobile.tools.gerrit.gerritevents.GerritHandler notifyListener

Exception thrown during event handling.
java.lang.reflect.InvocationTargetException
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at com.sonymobile.tools.gerrit.gerritevents.GerritHandler.notifyListener(GerritHandler.java:316)
    at com.sonymobile.tools.gerrit.gerritevents.GerritHandler.notifyListeners(GerritHandler.java:296)
    at com.sonyericsson.hudson.plugins.gerrit.trigger.JenkinsAwareGerritHandler.notifyListeners(JenkinsAwareGerritHandler.java:77)
    at com.sonymobile.tools.gerrit.gerritevents.workers.AbstractGerritEventWork.perform(AbstractGerritEventWork.java:46)
    at com.sonymobile.tools.gerrit.gerritevents.workers.GerritEventWork.perform(GerritEventWork.java:48)
    at com.sonymobile.tools.gerrit.gerritevents.workers.EventThread.run(EventThread.java:66)
    at com.sonyericsson.hudson.plugins.gerrit.trigger.SystemEventThread.run(SystemEventThread.java:66)
Caused by: java.lang.NoClassDefFoundError: org/apache/commons/lang3/StringUtils
    at com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener$ParametersActionInspection.<init>(EventListener.java:435)
    at com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener.getParametersInspection(EventListener.java:391)
    at com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener.createParameters(EventListener.java:269)
    at com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener.schedule(EventListener.java:188)
    at com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener.schedule(EventListener.java:164)
    at com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener.gerritEvent(EventListener.java:126)
    ... 11 more
Caused by: java.lang.ClassNotFoundException: org.apache.commons.lang3.StringUtils
    at jenkins.util.AntClassLoader.findClassInComponents(AntClassLoader.java:1376)
    at jenkins.util.AntClassLoader.findClass(AntClassLoader.java:1326)
    at jenkins.util.AntClassLoader.loadClass(AntClassLoader.java:1079)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:358)
    ... 17 more
@rsandell

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2016

Should be fixed in the last release

* @see #getParametersInspection()
* @see #createParameters(GerritTriggeredEvent, Job)
*/
private static class ParametersActionInspection {

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Nov 20, 2016

Member

would even make sense to make as separate plugin because all other trigger plugins needs to do the same with same quality level of checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.