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?

[itsmoonrack]

Hi there, any chances to see that landing one day ?

[cowtowncoder]

@sylvainlecoy In some form, yes. Either by letting users configure default, or, perhaps, by allowing both bindings. But I do not have timeline for this. 2.11.0 is being finalized and I doubt I have time to tackle this for that.

[itsmoonrack]

@cowtowncoder thanks for the quick reply. I am currently using @JsonCreator to fix this issue but I don’t like the idea having to put an annotation for these classes as the rest works flawlessly without the need of annotation. Did I missed a configuration property which could help me avoid having to put such annotations or using MixIns ? Thanks for your time.

[cowtowncoder]

@sylvainlecoy No, unfortunately there is no such option.

However, you can make it work in the meantime for your case by overriding this method:

public JsonCreator.Mode findCreatorAnnotation(MapperConfig<?> config, Annotated a) { }

in JacksonAnnotationIntrospector (or implement in custom introspector).
This is used by databind to find @JsonCreator annotation, but you can make it think it saw appropriate mode for specific cases, to prevent heuristics from being applied.
Or even use for cases where no annotation exists (but make sure to filter out unwanted cases to avoid conflicts).

[itsmoonrack]

That worked perfectly for my use case thanks

[lpandzic]

@cowtowncoder I just noticed the most wanted tag. Any reason why this issue is not on that list?

[cowtowncoder]

@lpandzic Only because I just added the label and have added whenever encountering one. This definitely warrants one-- thank you for update.

I know I have promised to tackle this "real soon now" for a while. But with caveat of this sorry history, I do plan to have another look with 2.12, having some new ideas.

[cowtowncoder]

Was about to add a simple setting ("defaultCreatorMode" of type JsonCreator.Mode) for smaller problem of ambiguous annotated 1-argument creator (just @JsonCreator, no mode), as that would be easy enough to add.
But once again re-reading the issue, bigger ask is introspection of un-annotated constructors.

It seems pointless to add a new configuration option that does not solve the main problem, but I think there is one possible case if would cover: legacy code that does not use mode property of @JsonCreator.
If anyone thinks such smaller patch (which does not affect possible work for 2.12 regarding introspection improvement) is worth addition, please let me know.

[cowtowncoder]

On 2.12 work wrt introspection: this would likely mean addition of an Enum that defines aspects of introspection for un-annotated constructors; configuration using "config overrides" style which allows for at least defaults, but also with some work per-type different definition.

The main question is that of options to use.

But some constraints first:

1. I think this should only apply to cases where no explicit creator annotation is found (should be obvious but mention it still)
2. @JsonAutoDetect/visibility should be used for finding (or not) of candidates.
3. Only properties-based constructors would be detected, not delegation-based.
4. Security considerations are up to user, now that polymorphic deserialization has been changed to require validation (except for deprecated legacy methods)

With that, here are some possibilities:

1. Limited discovery (default, current behavior): only introspected if explicitly named arguments (and/or @JacksonInject)
2. Discovery allowed if there is one and only one constructor: including possible 0-args constructor
3. Discovery allowed if there is one constructor OR 0-args constructor and one other constructor (in which case 0-arg constructor would be ignored)

but I assume other cases might make sense -- I did not consider possibility of what to do if parameter names are not found (quietly ignore otherwise valid candidate, or throw exception?), for example.

@lpandzic What do you think? Thank you for all your work, feedback; I know it has been a frustrating journey.

[lpandzic]

I think it's a good approach but I wouldn't dwell on it. For 0 arg constructor case or more constructors present - use the default behavior we had until now. I don't see any value in solving or rethinking that part really, especially after introduction of records.

So for me, jackson should handle javabean and record style of data holders out of the box and for everything else you have options that you can take but with a cost - it's not out of the box but certain effort is required like annotations or something else.

No problem, I'm glad I can help make java ecosystem a little bit more robust and user friendly :)

[cowtowncoder]

Finally finding time to work on this, possibly the last "must-have"/most-wanted feature for 2.12.
Current thinking:

• Add new handler type (ConstructorDetector), with 4 pre-configured instances to use (plus builder/mutant-factory to create different configurations)
• A few aspects to configure:
  • (boolean) Require annotation for non-default Constructor? (default 0-arg won't require for historical reasons)
  • (boolean) Allow auto-detect of JDK types' (java., javax.) constructors? (for security reasons)
  • (handler, ConstructorSelector?) In case of multiple alternatives, gets called to select one (and only one) to use (or throw exception)
  • (enum, SingleArgConstructor?) In case of @JsonCreator without "mode" or @JsonProperty for name, what to do: HEURISTIC (current, try to guess), PROPERTIES (mode=PROPERTIES, must find name), DELEGATING (mode=DELEGATING), FAIL (throw an exception for ambiguous case)

With this, pre-configured instances would be:

• DEFAULT:
  • Require annotations: false (that is, can auto-detect if argument names available etc)
  • Allow JDK type auto-detect: false (for security reasons; should not be necessary unless testing proves otherwise)
  • ConstructorResolver: throw exception if multiple applicable choices (cannot decide)
  • SingleArgMode: HEURISTIC (current behavior)
• PROPERTIES_BASED: same as DEFAULT except SingleArgConstructor = PROPERTIES
• DELEGATING: same as DEFAULT except SingleArgConstructor = DELEGATING
• EXPLICIT_ONLY: same as DEFAULT except SingleArgConstructor = FAIL

The real power would probably come from ConstructorResolver as that could resolve potentially complex case of multiple constructor -- for example by selecting the one that takes most arguments, or using some application-/model-specific heuristics.

[cowtowncoder]

Work here progressing, and now the original case passes when mapper is constructed like so:

ObjectMapper mapper = JsonMapper.builder()
        .constructorDetector(ConstructorDetector.USE_PROPERTIES_BASED)
        // add modules, configure etc
        .build();

I realized that implementing ConstructorSelector for selecting across multiple alternatives would get quite complicated so will for now leave only 3 configurable pieces:

1. SingleArgConstructor, the most relevant here (choice of SingleArgConstructor.PROPERTIES)
2. requireCtorAnnotation: Whether to always require annotation (@JsonCreator) or not (for those who want more not less explicit) (default: false)
3. allowJDKTypeCtors: Even if implicit discovery allowed in general, is it allowed for JDK types (java.*, javax.)? (default: false)

Of these I implemented the bigger piece (1) and need to add (2) and (3).

[lpandzic]

To anyone that can't upgrade jackson you might want to try this module - https://github.com/infobip/infobip-jackson-extension#SingleArgumentPropertyCreatorAnnotationlessSupport.
It might even work for some cases where ConstructorDetector.USE_PROPERTIES_BASED doesn't work as it works differently:

• it doesn't change default creator logic which might be wanted in some cases
• it handles only cases where there's only one constructor and that has 1 parameter that matches 1 field for name and type
• for all other cases it backs off and lets jackson do the default behavior.

Here's a test that probably speaks better for itself what is supported - https://github.com/infobip/infobip-jackson-extension/blob/master/infobip-jackson-extension-module/src/test/java/com/infobip/jackson/SingleArgumentPropertiesCreatorModeAnnotationIntrospectorTest.java.