Repository navigation
Conversation
| * {@code @JSONPropertyIgnore} (which only suppresses the bean to JSONObject | ||
| * direction). | ||
| */ | ||
| public class CustomClassK { |
There was a problem hiding this comment.
Please rename this class to something that reflects its purpose (e.g. FromJsonKeyBindingExample)
There was a problem hiding this comment.
Renamed to FromJsonKeyBindingExample in 88b6be9 — the class now says what it pins down.
| */ | ||
| public class CustomClassK { | ||
| public String displayName; | ||
| private boolean admin; |
There was a problem hiding this comment.
Please rename sessionKey, tenantId, admin, and superAdmin to names that reflect their purpose in the test (e.g. privateString, transientString, etc)
There was a problem hiding this comment.
Renamed all four: privateString, transientString, privateBoolean, ignoredGetterBoolean, with the getters and the JSON keys in the test following along.
| JSONObject object = new JSONObject() | ||
| .put("displayName", "bob") | ||
| .put("admin", true) | ||
| .put("tenantId", "victim-tenant") |
There was a problem hiding this comment.
Please remove all references to victim and attacker. This reads more like an exploit than an existing behavior test.
There was a problem hiding this comment.
Agreed, that framing was a bad fit. The values are now "private-value" / "transient-value", so the test reads as a binding example instead of an exploit.
The fromJson(Class) Javadoc claimed that only classes "explicitly supported and registered within the JSONObject context" can be deserialized, and declared an IllegalArgumentException for classes outside that set. No such registry exists: fromJson instantiates any class with a no-arg constructor and writes every non-static declared field whose name matches a JSON key, widening access with setAccessible(true). Private and transient fields are populated, and JSONPropertyIgnore does not apply because it only suppresses the bean to JSONObject direction. The binding rules are already released, so changing them here would break existing users. Instead document what actually happens, point out that binding is driven by whatever keys the JSON contains, and replace the non-existent IllegalArgumentException with the JSONException the method really throws. Adds JSONObjectTest#jsonObjectParseFromJson_10 to pin the current binding behavior so the documentation cannot drift from the implementation again. The test bean is named FromJsonKeyBindingExample and its fields spell out how they are declared (privateString, privateBoolean, transientString, ignoredGetterBoolean), so the test reads as a binding example rather than as a scenario. Fixes stleary#1076
c5d0c5a to
88b6be9
Compare
|
|
Addressed all three review comments in
The inline comments point at the old path, so the file shows up as deleted plus added — it is the same bean, renamed. |
|
@stleary Done — all three addressed and pushed.
The class now spells out how each field is declared in its own name, so the test documents the behavior instead of narrating a scenario. Head updated to 88b6be9. |
Description
Fixes #1076.
The
fromJson(Class)Javadoc documented a protection that was never implemented:There is no registry anywhere in the source. The method calls
setAccessible(true)on every non-static declared field and writes it when its name matches a key in the JSON, soprivate,transientand@JSONPropertyIgnore-exposed fields are all populated from whatever keys the input contains. The@throws IllegalArgumentExceptionline was wrong too: nothing in the method throws it, failures surface asJSONException.Since the binding rules themselves are already released, this PR leaves them untouched, per #1076 (comment), and only makes the Javadoc match reality:
@throws IllegalArgumentExceptionis replaced with theJSONExceptionthe method throws when the class has no accessible no-arg constructor or a field cannot be setfromJson(String, Class)now links to those rules instead of silently inheriting themTests
JSONObjectTest#jsonObjectParseFromJson_10binds a bean with a privateadmin, a privatetenantId, a transientsessionKeyand asuperAdminhidden behind an@JSONPropertyIgnoregetter. It asserts all four are populated, which is the behavior the Javadoc now describes. This is a characterization test: it passes before and after the change, and exists so the documentation cannot drift from the implementation again without a failing test.mvn javadoc:javadocsucceeds, so the new{@link}references and HTML are valid.No behavior change.