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-50939] - Whitelist java.util.EnumSet #3403

Merged
merged 1 commit into from Apr 24, 2018

Conversation

@marcosbento
Copy link

marcosbento commented Apr 23, 2018

See JENKINS-50939, or see details here.

As suggested in https://jenkins.io/blog/2018/01/13/jep-200/, this merge request adds java.util.EnumMap to the whitelist (as it is "defined in the Java Platform"). Because no new feature is implemented, no autotests are provided.

Proposed changelog entries

  • Whitelist java.util.EnumMap to prevent deserialization exception

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
Marcos Bento
@oleg-nenashev oleg-nenashev changed the title Whitelist java.util.EnumMap Whitelist java.util.EnumSet Apr 23, 2018
Copy link
Member

oleg-nenashev left a comment

@marcosbento Actually you whitelist EnumSet, not EnumMap. Generally it looks good to me, but 2 comments:

  • Could you please describe your use-case for it? Is it an open-source plugin? If yes, please reference the issue or commit
  • Do you want it to be backported to Jenkins LTS? I'd guess so. If yes, we need a Jenkins JIRA ticket describing the issue and referencing this PR

Thanks in advance,

@oleg-nenashev oleg-nenashev self-assigned this Apr 23, 2018
@marcosbento
Copy link
Author

marcosbento commented Apr 23, 2018

@oleg-nenashev yes, the goal is to whitelist java.util.EnumSet. Sorry for the typo.

This has impact on https://wiki.jenkins.io/display/JENKINS/PRQA+Plugin as reported directly reported by a customer (which didn't report a Jira issue). I've just reported the issue as https://issues.jenkins-ci.org/browse/JENKINS-50939.

Yes, it would be important to backport this to the Jenkins LTS.

@oleg-nenashev oleg-nenashev changed the title Whitelist java.util.EnumSet [JENKINS-50939] - Whitelist java.util.EnumSet Apr 23, 2018
@oleg-nenashev oleg-nenashev requested a review from jglick Apr 23, 2018
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Apr 23, 2018

I am fine with that, let's see what @jglick says

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Apr 23, 2018

@marcosbento In any case, you can also add the type to the plugin's whitelist. In such case the class will be whitelisted for your users even before backporting

@jglick
jglick approved these changes Apr 23, 2018
Copy link
Member

jglick left a comment

Fine if it works (and it is not SerializationProxy you need to whitelist).

You should really consider changing your plugin to serialize a simpler type, though, like Set<String>. The current deserialization logic notes that

// instead of cast to E, we should perhaps use elementType.cast()
// to avoid injection of forged stream, but it will slow the implementation

Not a security issue that I can see—you would just get a ClassCastException later—but from the perspective of plugin code being defensive about what it reads from disk, you are better off being explicit about the storage format rather than relying on magic.

@oleg-nenashev oleg-nenashev merged commit e511ff8 into jenkinsci:master Apr 24, 2018
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
olivergondza added a commit that referenced this pull request Apr 24, 2018
[JENKINS-50939] - Whitelist java.util.EnumSet

(cherry picked from commit e511ff8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.