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

Update to commons-beanutils:1.9.4 without disabling the protection #211

Merged
merged 11 commits into from
Mar 31, 2021

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Mar 3, 2021

This pull request attempts to update to commons-beanutils:1.9.4 without disabling the additional protection it offers around the class attribute.

It seems in my (fairly limited so far) testing that the problem is st:include's class attribute. So this pull request renames the attribute to clazz (and keeps the existing setter for compatibility).

This PR additionally proposes a workaround that applies when Jelly files are being parsed and the resulting tags are created: For the st:include tag, when the class attribute would be set, instead set clazz. This attempts to take care of all existing code that sets the class attribute for this tag.

I am unsure how best to go about ensuring this doesn't cause a lot of problems. While this fix looks fairly safe, it may also be incomplete. In the short term, #209 seems like the safer approach.

CC @basil

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Copy link
Member

@rsandell rsandell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've had riskier workarounds in the past.

@daniel-beck daniel-beck marked this pull request as ready for review March 19, 2021 08:44
Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes good sense and is the best proposal about how to move forward and upgrade to the new version. We can't just allow ourselves to remain on the old version and be blocked from continuing forward.

Hopefully this will work. If it doesn't there is a sufficient amount of escape hatches and configuration to allow people with other scenarios to continue while we correct anything additional that might arise.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Very nice change.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right. Kudos to the Jelly team and @jstrachan for such extensibility

@MRamonLeon
Copy link
Contributor

Who's able to merge this PR @daniel-beck, @oleg-nenashev? It has enough approvals for a few days already. The permission updater repo doesn't mention any maintainer: https://github.com/jenkins-infra/repository-permissions-updater/blob/master/permissions/component-stapler.yml

@daniel-beck
Copy link
Member Author

daniel-beck commented Mar 31, 2021

The permission updater repo doesn't mention any maintainer

That's explained in the linked file in a comment.

@jglick Please hold off releasing for another week until we've merged in changes from the upcoming security update (merging is fine though).

@jglick jglick merged commit 2cde543 into jenkinsci:master Mar 31, 2021
@jglick
Copy link
Member

jglick commented Mar 31, 2021

Just let me know when you want a release.

@batmat
Copy link
Member

batmat commented Apr 1, 2021

Why can't we just release and hold-off updating this into Jenkins Core?
We do not understand the rationale here. Thanks!

@daniel-beck
Copy link
Member Author

Because there's a privately staged new release of Stapler. That release you're asking for would never be in a core update anyway (and if it were it'd be a pretty big regression over next week's core release for absolutely no reason).

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

Successfully merging this pull request may close these issues.

9 participants