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-34858] - Listed Parameters should reflect what was used when the build ran #2353

Merged
merged 3 commits into from May 19, 2016

Conversation

7 participants
@rsandell
Member

rsandell commented May 16, 2016

… the build ran

And provided a way for plugins to define safe parameters by extending ParametersAction

@jenkinsci/code-reviewers
@reviewbybees
@oleg-nenashev @daniel-beck @amuniz

[JENKINS-34858] - Listed Parameters should reflect what was used when…
… the build ran

And provided a way for plugins to define safe parameters
by extending ParametersAction
@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees May 16, 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.

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.

@@ -74,7 +75,7 @@
public static final String SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME = ParametersAction.class.getName() +
".safeParameters";
private transient List<String> safeParameters;
private Set<String> safeParameters;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 16, 2016

Member

I doubt i may be backported in such case

@oleg-nenashev

oleg-nenashev May 16, 2016

Member

I doubt i may be backported in such case

This comment has been minimized.

@rsandell

rsandell May 16, 2016

Member

It's a private var, what would break backporting?

@rsandell

rsandell May 16, 2016

Member

It's a private var, what would break backporting?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 17, 2016

Member
  1. The fix gets into 2.7
  2. User installs 2.5.1 LTS with the fix (just an example version)
  3. Then he installs 2.0 to get a missing feature
  4. After installation he gets issues from the OldData monitor

It's not a blocker for merge, this fix is valuable in any case

@oleg-nenashev

oleg-nenashev May 17, 2016

Member
  1. The fix gets into 2.7
  2. User installs 2.5.1 LTS with the fix (just an example version)
  3. Then he installs 2.0 to get a missing feature
  4. After installation he gets issues from the OldData monitor

It's not a blocker for merge, this fix is valuable in any case

This comment has been minimized.

@jtnord

jtnord May 17, 2016

Member

Then he installs 2.0 to get a missing feature

but features in 2.0 will by virtue be in 2.5.1..... and this is a downgrade which is never guaranteed to work.

So maybe if you installed 2.5.1 and then upgraded to 2.6...

But in either case I don't think you will get OldDataMonitor issues as the data can be successfully restored without exception as the field is still there - so can be restored.

@jtnord

jtnord May 17, 2016

Member

Then he installs 2.0 to get a missing feature

but features in 2.0 will by virtue be in 2.5.1..... and this is a downgrade which is never guaranteed to work.

So maybe if you installed 2.5.1 and then upgraded to 2.6...

But in either case I don't think you will get OldDataMonitor issues as the data can be successfully restored without exception as the field is still there - so can be restored.

hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted1"));
assertTrue("whitelisted2 parameter is listed in getParameters",
hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted2"));
assertFalse("whitelisted3 parameter is listed in getParameters",

This comment has been minimized.

@amuniz

amuniz May 16, 2016

Contributor

I guess you mean whitelisted2 parameter is *not* listed in getParameters

@amuniz

amuniz May 16, 2016

Contributor

I guess you mean whitelisted2 parameter is *not* listed in getParameters

This comment has been minimized.

@rsandell

rsandell May 16, 2016

Member

Well, the error message is that it is when it shouldn't :)

@rsandell

rsandell May 16, 2016

Member

Well, the error message is that it is when it shouldn't :)

Show outdated Hide outdated core/src/main/java/hudson/model/ParametersAction.java
* @return an additional list of safe parameter names
* @since TODO
*/
protected Collection<String> getAdditionalSafeParameters() {

This comment has been minimized.

@amuniz

amuniz May 16, 2016

Contributor

Is there some real use of this extension point?

@amuniz

amuniz May 16, 2016

Contributor

Is there some real use of this extension point?

This comment has been minimized.

@amuniz

amuniz May 16, 2016

Contributor

AFAIK all plugins broken by SECURITY-170 could inject their parameters as environment variables (using EnvironmentContributor)

@amuniz

amuniz May 16, 2016

Contributor

AFAIK all plugins broken by SECURITY-170 could inject their parameters as environment variables (using EnvironmentContributor)

This comment has been minimized.

@rsandell

rsandell May 16, 2016

Member

If they do that then there will be big warning messages in the jenkins log due to the non safe parameters.

@rsandell

rsandell May 16, 2016

Member

If they do that then there will be big warning messages in the jenkins log due to the non safe parameters.

This comment has been minimized.

@rsandell

rsandell May 17, 2016

Member

It is not an extension point, but when a plugin want's to schedule with parameters it can extend ParametersAction and override this method.

@rsandell

rsandell May 17, 2016

Member

It is not an extension point, but when a plugin want's to schedule with parameters it can extend ParametersAction and override this method.

@oleg-nenashev oleg-nenashev changed the title from [JENKINS-34858] - Listed Parameters should reflect what was used when… to [JENKINS-34858] - Listed Parameters should reflect what was used when the build ran May 17, 2016

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 17, 2016

Member

It is not clear to me from JENKINS-34858 what problem you are trying to solve here.

Member

jglick commented May 17, 2016

It is not clear to me from JENKINS-34858 what problem you are trying to solve here.

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell May 17, 2016

Member

@jglick I believe the added test case explains it, but basically if you restart Jenkins with a new set of safe parameters all past build's "Parameters" page will reflect the new list of safe parameters and not what parameters where actually used.

And it also has a protected method that plugins can override to provide their specific list of safe parameters to the current build.

Member

rsandell commented May 17, 2016

@jglick I believe the added test case explains it, but basically if you restart Jenkins with a new set of safe parameters all past build's "Parameters" page will reflect the new list of safe parameters and not what parameters where actually used.

And it also has a protected method that plugins can override to provide their specific list of safe parameters to the current build.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 18, 2016

Member

My suggestion would be to decouple ParametersAction extensibility to a separate pull request. IMHO this feature is a must-have for reducing the fixing efforts in SECURITY-170 regressions

Member

oleg-nenashev commented May 18, 2016

My suggestion would be to decouple ParametersAction extensibility to a separate pull request. IMHO this feature is a must-have for reducing the fixing efforts in SECURITY-170 regressions

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell May 18, 2016

Member

Well these two fixes sort of go hand in hand and mixes with each other, when one is merged the other one would need to be changed.

Member

rsandell commented May 18, 2016

Well these two fixes sort of go hand in hand and mixes with each other, when one is merged the other one would need to be changed.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 18, 2016

Member

@rsandell OK, that's let's get this PR over the fence

Member

oleg-nenashev commented May 18, 2016

@rsandell OK, that's let's get this PR over the fence

Show outdated Hide outdated core/src/main/java/hudson/model/ParametersAction.java
* @since TODO
*/
protected Collection<String> getAdditionalSafeParameters() {
return Collections.emptyList();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 18, 2016

Member

Maybe an additional constructor and additionalSafeParameters field could simplify the code a bit for new usages. It's NIT

@oleg-nenashev

oleg-nenashev May 18, 2016

Member

Maybe an additional constructor and additionalSafeParameters field could simplify the code a bit for new usages. It's NIT

This comment has been minimized.

@rsandell

rsandell May 18, 2016

Member

Well, in that case you wouldn't need to extend the original action which is a plus. But would that make it too simple and potentially open a new hole for undisciplined maintainers?

It would make it slightly harder, for those like me, who wants to stay on an older core dependency but have the safe parameters activated when running on a newer core since you'd need to use some reflection.

But I like the idea better than mine, just some concerns.

@rsandell

rsandell May 18, 2016

Member

Well, in that case you wouldn't need to extend the original action which is a plus. But would that make it too simple and potentially open a new hole for undisciplined maintainers?

It would make it slightly harder, for those like me, who wants to stay on an older core dependency but have the safe parameters activated when running on a newer core since you'd need to use some reflection.

But I like the idea better than mine, just some concerns.

Show outdated Hide outdated core/src/main/java/hudson/model/ParametersAction.java
@@ -231,9 +238,23 @@ public ParametersAction createUpdated(Collection<? extends ParameterValue> overr
@Nonnull
public ParametersAction merge(@CheckForNull ParametersAction overrides) {
if (overrides == null) {
return new ParametersAction(parameters);
ParametersAction parametersAction = new ParametersAction(parameters);
loadSafeParameters();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 18, 2016

Member

🐜 Why not just getSafeParameters() with all logic inside?

@oleg-nenashev

oleg-nenashev May 18, 2016

Member

🐜 Why not just getSafeParameters() with all logic inside?

This comment has been minimized.

@rsandell

rsandell May 18, 2016

Member

Possible, If I do with the constructor approach then there would be no need through.

@rsandell

rsandell May 18, 2016

Member

Possible, If I do with the constructor approach then there would be no need through.

Show outdated Hide outdated core/src/main/java/hudson/model/ParametersAction.java
* and {@link #getAdditionalSafeParameters()} into {@link #safeParameters}.
* @since TODO
*/
private void loadSafeParameters() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 18, 2016

Member

🐛 for potential concurrency issues if two threads get into the safeParameters == null at the same time. Should be synchronized

@oleg-nenashev

oleg-nenashev May 18, 2016

Member

🐛 for potential concurrency issues if two threads get into the safeParameters == null at the same time. Should be synchronized

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 18, 2016

Member

🐛 for two things:

  • Concurrency issue in loadSafeParameters()
  • If the System Property gets changed, ParametersAction does not reload safeParameters list => No way to fix the parameters on the flight without Jenkins restart
Member

oleg-nenashev commented May 18, 2016

🐛 for two things:

  • Concurrency issue in loadSafeParameters()
  • If the System Property gets changed, ParametersAction does not reload safeParameters list => No way to fix the parameters on the flight without Jenkins restart
@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell May 18, 2016

Member

@oleg-nenashev I don't this it should be able to reload that list since it is populated once during the build and should reflect what parameters where used during that build even if the system property gets updated.

Member

rsandell commented May 18, 2016

@oleg-nenashev I don't this it should be able to reload that list since it is populated once during the build and should reflect what parameters where used during that build even if the system property gets updated.

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell May 18, 2016

Member

@oleg-nenashev I think I've addressed your comments

Member

rsandell commented May 18, 2016

@oleg-nenashev I think I've addressed your comments

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 18, 2016

Member

@rsandell Yes and now. I was not proposing to remove the getter method, because it's required for fixing the issues in plugins without bumping the core dependency.

Member

oleg-nenashev commented May 18, 2016

@rsandell Yes and now. I was not proposing to remove the getter method, because it's required for fixing the issues in plugins without bumping the core dependency.

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell May 18, 2016

Member

Well it is relatively easy to do a reflection call of the constructor, that way you don't have to implement your own ParametersAction and the xml api stays intact as well.

Member

rsandell commented May 18, 2016

Well it is relatively easy to do a reflection call of the constructor, that way you don't have to implement your own ParametersAction and the xml api stays intact as well.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 18, 2016

Member

Sounds reasonable. 🐝 if it gets documented on the SECURITY advisory page.

Member

oleg-nenashev commented May 18, 2016

Sounds reasonable. 🐝 if it gets documented on the SECURITY advisory page.

@rsandell

This comment has been minimized.

Show comment
Hide comment
Member

rsandell commented May 19, 2016

@rsandell rsandell merged commit 74d0412 into jenkinsci:master May 19, 2016

1 check passed

Jenkins This pull request looks good
Details

@rsandell rsandell deleted the rsandell:safe-parameters branch May 24, 2016

olivergondza added a commit that referenced this pull request May 25, 2016

Merge pull request #2353 from rsandell/safe-parameters
[JENKINS-34858] - Listed Parameters should reflect what was used when the build ran
(cherry picked from commit 74d0412)
@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Nov 9, 2016

Member

So whatin the end? 1.x.3 got security-170 but didn't get API for safeParameters plugins?

Member

KostyaSha commented Nov 9, 2016

So whatin the end? 1.x.3 got security-170 but didn't get API for safeParameters plugins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment