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

ISPN-7781 Add java deserial white list for client #5116

Merged
merged 1 commit into from Jun 8, 2017

Conversation

galderz
Copy link
Member

@galderz galderz commented May 3, 2017

https://issues.jboss.org/browse/ISPN-7781

  • Added new option to Hot Rod client that enables a list of regular
    expressions to be plugged that define classes that can be deserialized
    using standard Java serialization.

@danberindei
Copy link
Member

@galderz What's the use case?

@@ -192,7 +192,7 @@ public void start() {
asyncExecutorService = executorFactory.getExecutor(configuration.asyncExecutorFactory().properties());
}

listenerNotifier = ClientListenerNotifier.create(codec, marshaller, transportFactory);
listenerNotifier = ClientListenerNotifier.create(codec, marshaller, transportFactory, configuration.serialWhitelist());
Copy link
Member

Choose a reason for hiding this comment

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

-1 for the abbreviation

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you propose then?

Copy link
Member

Choose a reason for hiding this comment

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

How about serializationWhiteList?

@@ -282,6 +285,12 @@ public ConfigurationBuilder maxRetries(int maxRetries) {
}

@Override
public ConfigurationBuilder javaSerialWhiteLists(String... regExs) {
Copy link
Member

Choose a reason for hiding this comment

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

No need for plural.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, why? It's an array of String that's passed in.

Copy link
Member

Choose a reason for hiding this comment

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

array ~= list, so the plurality is already in the word "list".

Copy link
Member

Choose a reason for hiding this comment

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

Also this method should be named something like add... since we aren't overriding existing white list with subsequent calls.

Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

Seems good overall to me.

@danberindei My guess is the use case is not really to allow specific classes but rather deny all of them unless they are allowed.

@@ -282,6 +285,12 @@ public ConfigurationBuilder maxRetries(int maxRetries) {
}

@Override
public ConfigurationBuilder javaSerialWhiteLists(String... regExs) {
Copy link
Member

Choose a reason for hiding this comment

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

Also this method should be named something like add... since we aren't overriding existing white list with subsequent calls.

@@ -405,6 +421,9 @@ public ConfigurationBuilder read(Configuration template) {
this.valueSizeEstimate = template.valueSizeEstimate();
this.maxRetries = template.maxRetries();
this.nearCache.read(template.nearCache());
for (String className : template.serialWhitelist())
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this doesn't use addAll as well?

@galderz
Copy link
Member Author

galderz commented May 30, 2017

Addressed concerns and rebased

* Added new option to Hot Rod client that enables a list of regular
  expressions to be plugged that define classes that can be deserialized
  using standard Java serialization.
@wburns
Copy link
Member

wburns commented Jun 8, 2017

Tests look good to me.

@wburns wburns merged commit f2989a9 into infinispan:master Jun 8, 2017
@wburns
Copy link
Member

wburns commented Jun 8, 2017

Integrated into master, thanks @galderz !

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