Added json_encode/json_decode to list of insecure functions

[elevinskii]

I've added json_encode and json_decode to list of insecure functions, now it throws warnings if these functions are encountered.

[magento-cicd2]

All committers have signed the CLA.

[fooman]

Why are json_encode and json_decode insecure?

[elevinskii]

@fooman I think we should avoid using json_encode and json_decode directly, and use Magento\Framework\Serialize\SerializerInterface instead. On the other hand, I could choose the wrong sniff, perhaps json functions should be included in Magento2.Functions.DiscouragedFunction.

Thanks for your reply!

[lenaorobei]

@elevinskii thank you for the proposed changes.
I suggest adding this topic to the next architecture discussion magento/architecture#153

It's a kind of change we need to discuss before merging.

[lenaorobei]

As per magento/architecture#153 it was decided to move json_encode, json_decode and serialize to the DiscouragedFunctionSniff.

@elevinskii could you please update your PR addressing this changes.

[Vinai]

I'll continue to use json_encode and json_decode for any internal serialization where possible.

Data is the most valuable thing, and having a guaranteed known serialization format is valuable, because it enables us to create tooling to work with that data not limited to PHP.

The SerializerInterface takes that away as it supposedly keeps the serialization format as an implementation detail.
From my point of view that reduces the value of the data a lot.
If the format is changed in future my tooling will break.

That is the reason to avoid using the Magento serializer and use the built in PHP functions instead.

Just my .02$

[fooman]

In what instance is json_encode instead of "\Magento\Framework\Serialize\SerializerInterface" making it worse? Sure it has some better error handling but as @Vinai mentioned there are good reasons not to use it.

Also wanted to add that json_encode is likely not going to be deprecated as quickly.

And lastly merging this means that code targeting 2.1 is going to trigger this as SerializerInterface is only 2.2+.

[lenaorobei]

@Vinai @fooman thanks for the valuable feedback.

Data perspective is the one I haven't taken into account. It looks like such question required more deep investigation now.

[df2k2]

I'll continue to use json_encode and json_decode for any internal serialization where possible.

Data is the most valuable thing, and having a guaranteed known serialization format is valuable, because it enables us to create tooling to work with that data not limited to PHP.

The SerializerInterface takes that away as it supposedly keeps the serialization format as an implementation detail.
From my point of view that reduces the value of the data a lot.
If the format is changed in future my tooling will break.

That is the reason to avoid using the Magento serializer and use the built in PHP functions instead.

Just my .02$

I agree with you, Vinai, that having it as an implementable interface leaves room for some risk, but if you are adamant about ensuring that your tooling won't break, is there a reason that you couldn't use the \Magento\Framework\Serialize\JsonConverter::convert($data) similar to how a DataObject uses it?

It's pretty straight-forward vendor/magento/framework/Serialize/JsonConverter.php

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
namespace Magento\Framework\Serialize;

/**
 * This class was introduced only for usage in the \Magento\Framework\DataObject::toJson method.
 * It should not be used in other cases and instead \Magento\Framework\Serialize\Serializer\Json::serialize
 * should be used.
 */
class JsonConverter
{
    /**
     * This method should only be used by \Magento\Framework\DataObject::toJson
     * All other cases should use \Magento\Framework\Serialize\Serializer\Json::serialize directly
     *
     * @param string|int|float|bool|array|null $data
     * @return bool|string
     * @throws \InvalidArgumentException
     */
    public static function convert($data)
    {
        $serializer = new \Magento\Framework\Serialize\Serializer\Json();
        return $serializer->serialize($data);
    }
}

[fooman]

@df2k2 I believe @Vinai was referring to tooling that might not be in php (I know he uses Closure a lot...).

Also the code you referenced illustrates one of the points I tried to explain above. The code specifically states it shouldn't be used in the comments (not sure why as it doesn't include any reason). So now we need to expand even more mental capacity to work out what to do when we have json encoded data.

We are currently making something that should be straight forward - harder.

[df2k2]

@df2k2 I believe @Vinai was referring to tooling that might not be in php (I know he uses Closure a lot...).

Also the code you referenced illustrates one of the points I tried to explain above. The code specifically states it shouldn't be used in the comments (not sure why as it doesn't include any reason). So now we need to expand even more mental capacity to work out what to do when we have json encoded data.

We are currently making something that should be straight forward - harder.

I do not know enough about the tooling Vinai refers to and uses, nor do I know anything about clojure, so I can't say anything against his case for using json_encode/json_decode.

Sorry for the confusion about the comments, I meant to highlight the part in there that says:
instead \Magento\Framework\Serialize\Serializer\Json::serialize should be used.
which is pretty much hidden unless you horizontal scroll.

Is that straight-forward?

[Vinai]

I've used bash (with jq), JavaScript, Clojure and mysql native JSON support to extract and use serialized JSON data within the last 12 months, and I'm sure I'll use more in future.

I also agree with @fooman that using the classes feels very clunky, and they also make it harder to support older Magento versions with the same module code. Using json_encode and json_decode is much simpler, quicker, introduces less dependencies and allows for data interop.

[piotrekkaminski]

Serialize should stay as insecure not discouraged. It poses a significant risk of enabling remote code exec in a way that is hard to predict. The only advantage is speed so it should not be used outside very limited cases. Json functions ok to use directly. I think the advantage of class is just if there is a faster format included as a library in future php, it could be used instead.

[IvanChepurnyi]

Putting json_* functions into discouraged might result in worse impact on security, as people would not have and option to ouput their array structures within init scripts in templates. Serilizer, IMO, should be used only for storage related code, where Magento has complete control over.

[navarr]

I 100% recommend fellow employees to use json_encode in templates for JavaScript initialization using a PHP array instead of hand coding/escaping JavaScript directly

It seems overkill to use a ViewModel for that instead

[lenaorobei]

@elevinskii thanks for your input, however after the architecture discussion magento/architecture#174 we decided not to enforce developers to avoid using native json_encode and json_decode.

We are working on contract for JSON magento/architecture#161 and will keep you updated about further recommendations.