Skip to content

Conversation

hikari-no-yume
Copy link
Contributor

This request fixes the JSON parser's handling of top-level true/false/null. By that I mean a lone true/false/null value, not one inside an array of object. Hence, json_decode("True"); (currently valid) is affected but json_decode("[True]"); (currently invalid) is not.

At present, there is a bug where non-lowercase forms (e.g. True or tRUE) of these top-level values are accepted by the parser without erroring. This behaviour is not compliant with the JSON.org standard, nor the IETF RFC, nor the ECMA standard. It is also inconsistent with other JSON implementations. Were I to try JSON.parse('True'); in JavaScript or json.loads('true') in Python, I would be met with an error. The function is even inconsistent with itself in this respect, as [True] will fail but True will not. I suspect that this behaviour was not intended and simply came about because of someone failing to read the specification and assuming case-insensitivity.

Hence I propose this patch. It replaces strcasecmp with strcmp where the former was used incorrectly. It is a minor backwards-compatibility break, so I am targeting this at 5.6. However I believe that it is very unlikely that any code relied on this bug, since all JSON serialisers output lowercase literals, and any hand-written JSON is likely not a lone true, false or null value.

In the event that code relied on this bug, they could simply do something like this:

function json_decode_bad($json) {
    $json = trim($json, ' \t\n\r');
    $values = ['true' => true, 'false' => false, 'null' => null];
    foreach ($values as $key => $value) {
        if (strcasecmp($key, $json) === 0) {
            return $value;
        }
    }
    return json_decode($json);
}

So, I ask that this is merged, as a tiny backwards-compatibility break but bringing PHP into harmony with other JSON implementations and the specification.

EDIT: I should note that this is a literal 3-line change, replacing strcasecmp for strcmp on three different lines.

EDIT 2: dsp asked me where in the JSON standard it says that it must be lowercase. It's in RFC 4627, page 2:

The literal names MUST be lowercase. No other literal names are allowed.

@Tyrael
Copy link
Contributor

Tyrael commented Nov 18, 2013

I've merged the PR to 5.6 and master.
Thank you for your contribution!

@Tyrael Tyrael closed this Nov 18, 2013
@hikari-no-yume
Copy link
Contributor Author

Thank you very much for merging it!

@craigjbass
Copy link

This becomes a vector for detecting whether PHP >= 5.6 is running on a server if the custom application code exposes an API that accepts JSON input.

I think this fact should be in the PHP 5.6 migration documentation, as it may be of concern to some sys admins.

Forgive me in advance, I was not sure where the best place is to raise this concern.

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

Successfully merging this pull request may close these issues.

3 participants