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

stricter decode("true") - should fail as nonref #41

Closed
rurban opened this issue Nov 15, 2015 · 3 comments
Closed

stricter decode("true") - should fail as nonref #41

rurban opened this issue Nov 15, 2015 · 3 comments
Assignees

Comments

@rurban
Copy link
Owner

rurban commented Nov 15, 2015

"true" (or "false") in decode("true") or decode_json("true") is not a reference, and therefore needs to fail.
decode needs allow_nonref to pass. At least my understanding is that true/false are no JSON objects, they are a primitive. Even "null" is not a valid JSON text/object.

$json->allow_nonref ([$enable])
If $enable is true (or missing), then the encode method can convert a
non-reference into its corresponding string, number or null JSON value,
which is an extension to RFC4627. Likewise, decode will accept those JSON
values instead of croaking.

If $enable is false, then the encode method will croak if it isn't
passed an arrayref or hashref, as JSON texts must either be an object
or array. Likewise, decode will croak if given something that is not a
JSON object or array.

Confirmed by the spec http://www.ietf.org/rfc/rfc4627.txt

2. JSON Grammar

A JSON text is a sequence of tokens. The set of tokens includes six
structural characters, strings, numbers, and three literal names.

A JSON text is a serialized object or array.

 JSON-text = object / array

[...]

2.1. Values

A JSON value MUST be an object, array, number, or string, or one of
the following three literal names:

 false null true

So the literal names are no objects. They are valid JSON values, but not a valid JSON text.

This is wrong in all perl JSON packages, but correct in ruby. php json_decode extends rfc4627 as our allow_nonref flags by allowing null, true and false.

Interestingly the JSON::XS documentation and error message is spec conformant, just the implementation not. => decode_json("false")
JSON text must be an object or array (but found number, string, true, false or null, use allow_nonref to allow this)

Changing the XS internal true/false SV from RV to the direct blessed object of JSON::PP::Boolean fixes this bug, and also enables eq overload.
previously we used a RV to the blessed PVMG JSON::PP::Boolean object.
The problem is with eq overloading, that eq needs the object directly,
it will not call overload magic on the reference < 5.16

I work on this in the branch nonrefbool-gh41 but a whole lot of hell broke loose with this change.
Maybe just disable eq overload and keep the 3.0115 state.

@rurban rurban self-assigned this Nov 15, 2015
@rurban rurban changed the title stricter decode("true") stricter decode("true") - should fail as nonref Nov 15, 2015
@Grinnz
Copy link

Grinnz commented Nov 15, 2015

Note that this is valid in RFC 7159. http://tools.ietf.org/html/rfc7159#section-2 However, this is also what the allow_nonref option was designed for.

@rurban
Copy link
Owner Author

rurban commented Nov 15, 2015

Oh great, it was relaxed. I'm not sure if I should disallow it then, because it might break existing web apps.

rurban pushed a commit that referenced this issue Nov 25, 2015
overload is broken <5.18. use schmorps workaround.
stringify false to "0" to stay false, but true to "true".
change the XS true/false from RV to PVMG. stricter!

Previously we used a RV to the blessed PVMG JSON::PP::Boolean object.
The problem is with eq overloading, that eq needs the object directly,
it will not call overload magic on the reference < 5.16

As sideeffect this made decode("true") stricter.
We need now ->allow_nonref->decode("true").
Also decode_json("true") will now fail as decode_json does not
enable allow_nonref.
This is a regression from JSON::XS and older Cpanel::JSON::XS releases, but
works as documented.
See GH #41
@rurban
Copy link
Owner Author

rurban commented Nov 25, 2015

Implemented the stricter and consistent decode_json option now in master.
The error message was always there, it was just not used due to a JSON::XS bug.

@rurban rurban closed this as completed Nov 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants