Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

toJson is serializing private fields by default for Jackson. Possible regression in 2.8.x #10523

Closed
onilton opened this issue Nov 6, 2020 · 11 comments
Milestone

Comments

@onilton
Copy link
Contributor

onilton commented Nov 6, 2020

Play Version

2.8.x

API

Java, but may affect Scala if someone is using Jackson for Json.

Expected Behavior

Private fields should not be serialized by default when calling toJson (previous behavior in 2.7.x)

Actual Behavior

Private fiels are being serialized by default when calling toJson

Reproducible Test Case

Just serialize a class using toJson that has a private fiel.

Investigation/More info

In my investigation, I found out that the main issue is related with ObjectMapper's visibility for fields. The previous default implementation used by Play 2.7 that came from Jackson was using PUBLIC_ONLY for field detection.

From https://github.com/FasterXML/jackson-databind/blob/jackson-databind-2.10.5/src/main/java/com/fasterxml/jackson/databind/introspect/VisibilityChecker.java#L169

       protected final static Std DEFAULT = new Std(
                Visibility.PUBLIC_ONLY, // getter
                Visibility.PUBLIC_ONLY, // is-getter
                Visibility.ANY, // setter
                Visibility.ANY, // creator -- legacy, to support single-arg ctors
                Visibility.PUBLIC_ONLY // field
                );

So

public class MyClass { private String myField }
// myField  won't be in the json

Now that Play 2.8 is using Jackson Serialization Support [1], akka-serialization-jackson seems to be overriding visibility for fields to ALL (even private/protected ones)

From https://github.com/akka/akka/blob/v2.6.10/akka-serialization-jackson/src/main/scala/akka/serialization/jackson/JacksonObjectMapperProvider.scala#L248

 mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY)

So

public class MyClass { private String myField }
// myField  will be in the json

You can test that this is the problem by annotating your ebean with @JsonAutoDetect(fieldVisibility = Visibility.PUBLIC_ONLY), the problem will be fixed.

The main issue is that there is no mention to this fact in Play's documentation and also it doesn't seem to exist any easy way to fix/override that setting.

I am wondering if I should open a feature/bug request in akka (for customization of visibility), but since it seems like a regression from a playframework's user perspective.

I think the place we could override this setting in Play is this one:

val mapper = JacksonObjectMapperProvider.get(actorSystem).getOrCreate(BINDING_NAME, Option.empty)

Workaround

You will probably have to create your own module and override the ObjectMapper. By doing this, you will also lose the advantages of having Akka Jackson Serialization (and configs).

@PromanSEW
Copy link
Contributor

Note: Private Ebean-specific fields after enhancing are also serialized to Json, but they should not be there

@franzgranlund
Copy link

This is a quite serious bug/undocumented change, since it might expose internal or sensitive information that is hard to detect for the developers when migrating.

@ignasi35
Copy link
Member

ignasi35 commented Nov 6, 2020

@onilton can you confirm if this issue happens in both Play 2.8.2 and 2.8.4?

@onilton
Copy link
Contributor Author

onilton commented Nov 6, 2020

@ignasi35 I can't build an example right now, but looking at the source code, it seems nothing was changed:

  val akkaVersion: String = sys.props.getOrElse("akka.version", "2.6.8")
  val akkaHttpVersion     = "10.1.12"

https://github.com/playframework/playframework/blob/2.8.4/project/Dependencies.scala#L10

And in akka 2.6.8:

mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY)

https://github.com/akka/akka/blob/v2.6.8/akka-serialization-jackson/src/main/scala/akka/serialization/jackson/JacksonObjectMapperProvider.scala#L248

@ignasi35
Copy link
Member

ignasi35 commented Nov 6, 2020

@onilton
Other changes in 2.8.3 and 2.8.4 may have introduced the regression.
Also, this may be a different default settings between the jackson versions used in 2.7.x and 2.8.x.

I am no asking for a full reproducer right now (though that would be awesome), but if we could narrow down in what version this bug was introduced we may be able to expedite a workaround and a full-time solution.

@onilton
Copy link
Contributor Author

onilton commented Nov 6, 2020

@ignasi35 I think the regression started in 2.8.0 series, I noticed at my first migration to 2.8.x

@ignasi35
Copy link
Member

ignasi35 commented Nov 6, 2020

@onilton you have a couple of options:

  1. disable the ObjectMapperModule ( which is enabled by default), then create your own copy of the play code which tunes the ObjectMapper how you prefer

  2. Go straight and modify the ObjectMapper Akka provides. ObjectMapper is a mutable structure so you can simply do Json.mapper().setVisibility(...) and it'll modify the instance Akka manages.

Note, Akka may handle multiple ObjectMapper instances and keeps a Map[String, ObjectMapper]. Both options above will only modify the instance under the "play" key on that map.

Then, you may even load all instances in Akka's JacksonObjectMapperProvider.get(actorSystem) .getOrCreate(ObjectMapperProvider.BINDING_NAME, Option.empty) and tune them at will.

@onilton
Copy link
Contributor Author

onilton commented Nov 6, 2020

@ignasi35 that's what I did, the 2. ,as I hinted at the issue description:

You will probably have to create your own module and override the ObjectMapper. By doing this, you will also lose the advantages of having Akka Jackson Serialization (and configs).

The thing that didn't strike me at first is @franzgranlund comment:

This is a quite serious bug/undocumented change, since it might expose internal or sensitive information that is hard to detect for the developers when migrating.

And now I realized that is exactly what happened to me, toJson was serializaning a hashedPassword field where in Play 2.7.x this was not true, by default.

@ignasi35
Copy link
Member

ignasi35 commented Nov 6, 2020

@onilton I've raised akka/akka#29797 to standardize how visibility is tuned in Akka-managed ObjectMapper instances.

@onilton
Copy link
Contributor Author

onilton commented Nov 6, 2020

@ignasi35 great! Added a comment there.

@ignasi35
Copy link
Member

ignasi35 commented Nov 9, 2020

Fixed in #10526

@ignasi35 ignasi35 added this to the 2.8.5 milestone Nov 9, 2020
@ignasi35 ignasi35 closed this as completed Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants