Objects with toJSON defined will use JSON.stringify instead of canonicalize causing the output to change when canonicalizing POJOs
frosty5689 opened this issue · 5 comments
Hi,
I ran into an issue with the order of properties when serializing Objection.js model instances using canonicalize. All Objection.js model instances have a toJSON
method which exports the model as a simple JSON object after doing some mapping.
The problem occurs when JSON.stringify
is used to serialize the Objection.js records. The order of properties becomes dependent on the implementation of JSON.stringify
and not canonicalize
.
This is not a problem when you're always working with Objection models, but this is not always the case. Especially when the JSON string is used to sign and verify the data has not been tampered using JWS.
When the signed data is sent back to the client, it becomes a POJO, so when canoicalize
serializes it, it orders the properties as expected. This results in the properties being in a different order than when it was generated for signing.
I think the code needs to return serialize(object.toJSON()
of object.toJSON()
is defined.
Example of something that will cause problems after being converted to a POJO.
{
a: 123,
b: 456,
toJSON: function() {
return {
b: this.b,
a: this.a
}
}
}
Expected:
{"a":123,"":456}
Actual:
{"b":345,"a":123}
If I misunderstood the specification, then apologies. I just think the way it is now, creates uncertainties when a non-simple object is serialized by JSON.stringify
If it makes sense, I can create a PR to correct this behavior if that is preferred.
Please do create a PR and I´ll have a look
@cyberphone what are your thoughts on this?
I guess I never considered that toJSON
could be applied to composite objects as well...
A PR would nice and I believe the shown "cure" is correct.
We might even file an errata for the RFC.
@frosty5689 I created a fix using the code from your PR, but created my own to also include a test.
@frosty5689 I created a fix using the code from your PR, but created my own to also include a test.
All good! Sorry, it took me so long to get a PR in and didn't get around to adding tests :/