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-48954] - Whitelist model objects #18

Conversation

Projects
None yet
4 participants
@oleg-nenashev
Copy link
Member

commented Jan 16, 2018

It is a lame attempt to add some tooling around library whitelisting without whitelisting the entire library. I do not like that, and it likely should not be merged in such way. But it gives some tooling for investigation

  • JENKINS-48954 requires only org.kohsuke.github.GHPullRequestCommitDetail$Authorship which is safe to whitelist
  • The most of the model objects include GitHub class which holds connections, etc. This class does not seem to be fine to be serialized to XStream

Actions:

  • - Add tooling for checking class serializability
  • - Create a wide whitelist - replacement for library-side whitelist
  • - Filter entries we do not like

@reviewbybees @stephenc @jglick

@reviewbybees

This comment has been minimized.

Copy link

commented Jan 16, 2018

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.

@oleg-nenashev oleg-nenashev requested review from jglick and stephenc Jan 16, 2018

@jglick
Copy link
Member

left a comment

Close this and just patch the JAR manifest.

@@ -1 +1 @@
buildPlugin()
buildPlugin(jenkinsVersions: [null, '2.102'])

This comment has been minimized.

Copy link
@jglick

jglick Jan 16, 2018

Member

Good, we should start doing this in lots of plugins…

import java.util.Set;

/**
* Verifies that the whitelisted classes can be actually saved on disk.

This comment has been minimized.

Copy link
@jglick

jglick Jan 16, 2018

Member

I do not understand the purpose of this test. Of course if you add a whitelist entry for something, it can be saved. What does this prove?

@@ -0,0 +1,179 @@
# Generated by GitHubAPIClassListerTest
org.kohsuke.github.AbuseLimitHandler

This comment has been minimized.

Copy link
@jglick

jglick Jan 16, 2018

Member

Delete this and just add Jenkins-ClassFilter-Whitelisted to the library.

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

GitHub class which holds connections, etc. This class does not seem to be fine to be serialized to XStream

And presumably it was not anyway.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

OK, patching the lib. Was no sense to do it earlier since we need KK to spin the release IIRC

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

patching the lib

From what I can see, we do not really need to do that either—we just need to fix the silly code in ghprb which tries to serialize these types to begin with.

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

You are hacking shared library that will automatically affect everybody even if devs want serialize something from this library.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

@jglick I think it makes sense to move the ClassFilter file tests to JTH injected tests, WDYT?

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

I think it makes sense to move the ClassFilter file tests to JTH injected tests

No, I do not think so. Anything defined in the plugin itself is already passed. Things defined in other libraries we do not normally wish to pass.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

No need in that so far

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.