Require registration of types before they can be used in a session #1592
Conversation
| * @see SessionModule | ||
| * @since 1.9 | ||
| */ | ||
| @Inherited |
There was a problem hiding this comment.
This is a bad idea. I would not suggest making this an @Inherited annotation.
There was a problem hiding this comment.
the annotation is gone entirely.
It turns out that inspecting class annotations causes the class to be initialized. Theoretically, a class could do something malicious in its static initializer.
|
|
||
| import ratpack.session.SessionTypeFilter; | ||
|
|
||
| public class AllowAllSessionTypeFilter implements SessionTypeFilter { |
There was a problem hiding this comment.
This should have a name like "Unsafe" or "Insecure" in the name and also be marked Deprecated.
|
|
||
| static { | ||
| ALLOWED_PACKAGES.add(String.class.getPackage()); // java.lang | ||
| ALLOWED_PACKAGES.add(List.class.getPackage()); // java.util |
There was a problem hiding this comment.
Will this mean that all subtypes are also supported? If so, this is not a good idea.
There was a problem hiding this comment.
I ended up removing this.
| } | ||
| } | ||
|
|
||
| private static final class TypeFilteringObjectInputStream extends ObjectInputStream { |
There was a problem hiding this comment.
Looks like this is a less feature-rich version of the ValidatingObjectInputStream from Apache Commons.
I just want to make sure that you're not reinventing the wheel here.
There was a problem hiding this comment.
We don't need any of those features, or the extra dependency.
| public class ClassWithStaticInit { | ||
|
|
||
| static { | ||
| System.out.println("HERE!!!!!!!!!!!!!!!!!!!"); |
There was a problem hiding this comment.
This seems like a debugging class that shouldn't have been committed?
There was a problem hiding this comment.
What gave it away?
Will remove.
| expect: | ||
| test(String, "foo") | ||
| test(Integer, 1) | ||
| test(List, [[1, 2] as int[]]) |
There was a problem hiding this comment.
This is serializing a List of integer arrays, correct? Is JdkSessionTypeFilterPlugin thus allowing serialization of any class as long as you embed it in a List or Map?
There was a problem hiding this comment.
No, you also need to register the component type.
If Integer wasn't registered this would fail.
| public class AllowedAnnotationSessionTypeFilterPlugin implements SessionTypeFilterPlugin { | ||
|
|
||
| @Override | ||
| public boolean allow(String className) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm confused what this is doing. It seems like it just forbids all classes? Some documentation explaining what's going on here would be helpful.
| @@ -19,12 +19,16 @@ | |||
| import ratpack.api.Nullable; | |||
| import ratpack.session.SessionKey; | |||
|
|
|||
| import java.util.Objects; | |||
|
|
|||
| public class DefaultSessionKey<T> implements SessionKey<T> { | |||
There was a problem hiding this comment.
I don't have enough context here, but doesn't unintentionally having a DefaultSessionKey(null, null) potentially introduce a security risk in itself?
I don't know if it would/wouldn't. I'm asking to try to understand the security implications of such a configuration.
There was a problem hiding this comment.
That state isn't possible. I've moved the check to make that clearer.
I can't see how it would be a risk. There'd be a runtime error before trying to do anything with it.
This change is