Allow handling of single-arg constructor as property based by default

[lpandzic]

class Foo {
    private final Bar bar;

    Foo(Bar bar) {
        this.bar = bar;
    }
}

With the parameter-names-module enabled I'd expect this class to be correctly deserialized from {"bar":{...}}.
More than once have users hit this issue and asked why isn't it handled this way (#1135, #8, #21).
It might also be a good idea to provide an option to change this behavior (HANDLE_SINGLE_ARG_CONSTRUCTOR_AS_PROPERTY_CREATOR).

[cowtowncoder]

I still have big misgivings on this helping some users and hurting others; mostly for cases where constructor with String argument would lend itself naturally to delegation.

But! With 3.0 planning, I think this is reasonable thing to do. Support for JDK datatypes that rely on delegation will need to be handled separately, but I also don't know how many such types are left.

[lpandzic]

Since there are users that rely on this feature a feature flag should also be provided.

[itsmoonrack]

+1

I think it should be the default behavior in 3.x.

[cowtowncoder]

Yet no one has explained how to protect against very likely case of accidental discovery of constructor that are not meant to be used as creators. In fact this would likely open a new attack vector in case of polymorphic deserialization, as constructors of JDK/3rd-party libraries that were not previously considered will now be discovered.
Further, ambiguity between constructors is unsolved issue.

With Jackson 3, new discovery mechanism needs to be defined (not just or mainly for this issue), and we can and should tackle this. But I have concerns about how far we can get with zero annotations.

[itsmoonrack]

@cowtowncoder do you have a valid use case for a constructor not meant to be used as creator ?

[lpandzic]

@cowtowncoder, what does it mean accidental discovery of constructor? I don't see a case where that would happen because we always know the type we want jackson to deserialize and it's either our own classes or JDK ones. JDK ones aren't compiled with '-parameters'.

[cowtowncoder]

@sylvainlecoy About any class that has not been primarily created for use as json POJO, such as JDK types. I am not worried about ones designed for use, but (especially for security reasons) ones that are not -- with polymorphic typing it is possible in some cases to exploit methods/constructors/fields in clever ways.

@lpandzic Ok, good info wrt JDK parameter names (or lack thereof). You may have mentioned it earlier but I keep forgetting.

Other than that:

1. With Jackson 3.0 we can and should change property discovery significantly. Conversely I want to keep 2.9 stable and limit any changes here.
2. We need to decide rules for Creator discovery carefully, and make it easy to both require explicitness (some users do that for getters & setters too) and to minimize need for explicitness.
   • Rules should not only consider visibility, but also rules for selecting Creator or Creators (it is up to debate whether more than one should be allowed -- while convenient to allow more, it complicates handling significantly)
3. We can re-evaluate whether single-arg Creators should be class of their own or not
   • May also re-evaluate whether delegating creator should have precedence similar to property-based or not -- if one has precedence (configurable or not), handling can be simplified and there's less need for heuristics.

I will need to create issues at:

https://github.com/FasterXML/jackson3-dev

to kick start design discussions on mailing list: this will need to occur after 2.9.1 patch, and probably will not be the first thing to tackle. But should be among earlier ones as it was number 3 on postponed features at https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.9

[cowtowncoder]

@lpandzic forgot one other thing: even if JDK types do not have parameter names (which is good here), 3rd party libraries probably would. A good document of problems is this:

https://github.com/mbechler/marshalsec/blob/master/marshalsec.pdf

which outlines existing security problems: expansion here could allow more constructors as potential attack vectors (esp. if it'd be for any number of parameters, not just one).

Note that this specifically relates to polymorphic deserialization, in which attacker specifies class name of class to use -- in which case all they have to do is to fine suitable attack vector.
While not trivial, it is quite doable with enough material. And with modern Java services, dependencies, there's a lot of material to sift through.

It is difficult to really gauge potential for abuse here since I do not think many Java deserializers allow detection of such constructors (and since parameter name inclusion is still relatively new feature for bytecode). But absence of known cases at this point does not prove, in my opinion, that there are none. Rather that we simply do not know, but that there is good reason to suspect there are constructors that would be problematic, within widely used 3rd party libraries.

This is still different, of course, from my earlier objection of mismatched constructors.
I think that is still somewhat valid concern too. But possibly something that we can design around, with good combination of visibility requirements, precedence.

[lpandzic]

@cowtowncoder I understand your point, but is this a blocker for this feature? Considering default constructors and setters vs constructor I'm not sure why it would make much of a difference for an attacker? Don't you have to specify for polymorphic classes all instances of hierarchy on parent class and don't you always have to specify parent target in your code for deserialization? If target is ever concluded from serialized format (json) then that is the cause of security risks as you can program through serialized format and that is not good security wise.

If I want to deserialize library class and that library is compromised, there is no rescue.

[lpandzic]

@cowtowncoder any update on this?