Change DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES default to false

[odrotbohm]

DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES currently defaults to true. So assuming you're using Jackson to deserialize HTTP request bodies this will mean, deserialization fails if the clients sends a single property unknown to the server. This blatantly violates Postel's law.

So if all the world is reconfiguring that default immediately, is it maybe worth considering to change it? I know that's a tough thing to do (and probably - if ever - will only make it into a new major release), but I thought I'd raise the question as in general, not failing is definitely the better default.

[jhiemer]

+1

[daschl]

+1

[basti1302]

+1

[kimble]

+1

[gregturn]

+1

[leifhanack]

+1

[bclozel]

+1
Are there uses cases that rely on that feature? If not, changing this default is not that risky - it won't introduce new exceptions in existing apps, but rather make them more robust.

[pgelinas]

I disagree. I leave that feature to the default because I prefer my code to document exactly what properties I can ignore (with JsonIgnoreProperties). This document both the code and the json representation I'm working with and clearly state my expectations about it. Every defaults in the Jackson feature set ensure a strict conformance to the JSON specification and to the classes being mapped.

A "law" shouldn't be followed blindly; one of the best example of this "law" being wrongly applied is HTML: it's less of a problem now, but earlier some browsers were more tolerant then others, so you'd get compatibility problems because some HTML pages could only be read by specific browser.

Also, if anyone wish to continue this discussion, we should do so on the user mailing list/google group.

[odrotbohm]

Interesting, so you think it's right to by default fail, if you get a JSON data structure that meets all requirements of the type it's supposed to be projected on (which you can of course happily use to document what you expect) just because a single additional element is present which you actually don't care about at all?

Why is it important to document which properties you don't care about? If you think through that, wouldn't you have to mention every possible property that could appear? You document what you expect, not what you not expect.

Let's say you order a steak at a restaurant. As long as the waiter brings steak, you're fine. Do you return the order if there's lettuce on the plate and say: "No, I wanted steak!". If you don't like the extras, ignore them. Also, you don't order: "A steak, but without a burger, chips, lettuce, coffee, tissues, a new iPhone and a pair of socks".

I don't follow a law blindly, I think there's value in it, cause you can build resilient systems with it. :)

[pgelinas]

I've continued the discussion on the google group to reach a larger audience (not everyone checks github).

[dacofr]

+1

[cowtowncoder]

@olivergierke I don't follow your example: if my order came with side of some unknown stuff I didn't order, yes, I'd probably send it back; but I most definitely would want to know about it. Throwing an exception is perfectly valid thing to do, if unknown stuff comes in. In fact, blindly accepting any kind of thing is often (but not always) pretty bad strategy -- my experience has been that it will hide issues and delay finding of things you should have found earlier.
Your logic and preferences may well differ, and I am not trying to convince you to change what you want; however, your preferences are not universal.

Regardless I see no point in changing defaults for 2.x. If 3.x is ever released, yes, by all means we can change this (with brief discussion as appropriate). So I'll change the title, and there won't be much action for this item until then.

FWIW, I used to curse at JAXB that had (so I thought) idiotic and stupid default settings of NOT complaining about unknown elements -- I wasted plenty of time before realizing that a typo made things not match. And I could not understand how anyone anywhere could possibly choose such "Obviously Wrong" defaults. So I've come full circle in some ways... and I nowadays agree with "Open Content" style modelling; specifying things you want, and improving forward-compatibility by allowing inclusion of possibly other content.

[odrotbohm]

One more comment on the follow some arbitrary law: Postel's law has been encoded into RFC1122.

[mikecollard]

+1

[sdeleuze]

+1

[Tristan971]

+1

[vrammohan]

I agree with the debate made by cowtowncoder. That's absolutely correct. One should restrict accessing APIs in which the request contains UNKNOWN PROPERTIES.

[samnegri]

+1

[sdouglass]

+1

[oleksii-ziabkin]

+1

[JooHyukKim]

So what's the verdict on this one? disabled by default?

[cowtowncoder]

I would still bring this up on jackson-dev, but I think that over time there has been significant support for "disabled by default" option. Possibly the other side is quiet because default up until now has been "enabled by default" (that is, some devs might be disappointed with the change), but we can't really base things on unsaid preferences.

So I'd ask on jackson-dev -- or maybe even jackson-user, come to think of it? -- wait a bit, and then, proceeding unless feedback contradicted the plan.

[JooHyukKim]

but we can't really base things on unsaid preferences.

Agreed.

So I'd ask on jackson-dev -- or maybe even jackson-user, come to think of it? -- wait a bit, and then, proceeding unless feedback contradicted the plan.

I guess it wouldn't hurt asking the same question to both group 👍🏼. I will write to the google groups later today, after work.

[cowtowncoder]

Based on limited discussion, support for the change was expressed; no one against the change.
Hence, this will be implemented via #4625.

[pjfanning]

I will belatedly add my -1. It is a very easy exploit to send a giant JSON file with a valid looking message that has an extra dummy element that contains a massive amount of data. We have StreamReadConstraints that may catch some issues.

If we are going down this route of making the default Jackson setup less secure, can we consider adding a simple way to enforce the most secure setup?

Maybe something like:

JsonMapper.builder()
  .securityMode(SecurityMode.Strong)
  .build();

We could add more SecurityModes over time - so better than having a Boolean setting.
SecurityMode.Strong would have DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES=false
and possibly set tighter security levels on StreamReadConstraints than the default.

[cowtowncoder]

@pjfanning This was discussed on jackson-dev mailing list, but you did not bring up any objections. Why?

And everyone who did voice their opinion then -- and pretty any time, ever -- has been +1 for this change. Personally I don't really favor this, but I don't see it as a security question. I am actually not sure how would it result in DoS -- skipping values has very little cost; size of content increases transfer sizes and all, but limited processing.
This seems like theoretical more than practical concern.

And finally, I think most other processing/data-binding packages default to "lenient" mode (JAXB definitely does). Are they missing something wrt security?

[pjfanning]

@pjfanning This was discussed on jackson-dev mailing list, but you did not bring up any objections. Why?

And everyone who did voice their opinion then -- and pretty any time, ever -- has been +1 for this change. Personally I don't really favor this, but I don't see it as a security question. I am actually not sure how would it result in DoS -- skipping values has very little cost; size of content increases transfer sizes and all, but limited processing. This seems like theoretical more than practical concern.

And finally, I think most other processing/data-binding packages default to "lenient" mode (JAXB definitely does). Are they missing something wrt security?

I don't follow Google Groups. I have so many other forums to track. For me, GitHub Discussions or Issues are a better to discuss things like this.

[pjfanning]

In terms of security, the JSON Tree nodes could be made to use up a lot of memory. From my understanding, jackson-databind looks at the parsed JSON tree nodes as opposed to having a parser that is aware of what to skip.

[cowtowncoder]

@pjfanning This was discussed on jackson-dev mailing list, but you did not bring up any objections. Why?
And everyone who did voice their opinion then -- and pretty any time, ever -- has been +1 for this change. Personally I don't really favor this, but I don't see it as a security question. I am actually not sure how would it result in DoS -- skipping values has very little cost; size of content increases transfer sizes and all, but limited processing. This seems like theoretical more than practical concern.
And finally, I think most other processing/data-binding packages default to "lenient" mode (JAXB definitely does). Are they missing something wrt security?

I don't follow Google Groups. I have so many other forums to track. For me, GitHub Discussions or Issues are a better to discuss things like this.

Ok. Maybe better way to do Discussions then, pointing to them from Google groups.
Will keep this in mind for the next thing -- we did try to reach out, given it's a non-trivial discussion.
(this issue did exist of course but it's an old one).

[cowtowncoder]

In terms of security, the JSON Tree nodes could be made to use up a lot of memory. From my understanding, jackson-databind looks at the parsed JSON tree nodes as opposed to having a parser that is aware of what to skip.

Ahhh.... I think I can see where you coming from then.

No; JsonDeserializers operate directly on JsonParser and indeed call parser.skipChildren() on unknown values (if not failing). They do not work on JsonNodes, to allow incremental parsing.
But if they did, it'd be moot point as well since then the whole content would be completely read before determining whether properties are recognized or not.

JsonNode deserialization is different but it also has no notion of "unknown".