decode HTML entities in REST/JSON service

[JoshMcCullough]

Description

I worked on this for many hours, trying to figure out why my API calls were never working. After digging through and adding logging, I found that the rest_data parameter was not being properly HTML-entity-decoded.

Motivation and Context

8.7.0 REST API simply does not work. See this thread on the forum: https://community.suitecrm.com/t/v4-1-api-call-not-working-params-not-received/97318

How To Test This

This will fail using the current 8.7.0 API:

curl -X POST http://localhost:3001/service/v4_1/rest.php -H 'Content-Type: application/x-www-form-urlencoded' -d 'input_type=JSON' -d 'response_type=JSON' -d 'method=login' -d 'rest_data={"user_auth":{"user_name":"admin","password":"dd995564b1fa70241364c5926aa27997"},"application":"test"}'

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change) ***

*** I don't think this is a breaking change. As far as I can tell, there is no way to use the API as-is. But I ... must be wrong, right?

Final checklist

• My code follows the code style of this project found here.
• My change requires a change to the documentation.
• I have read the How to Contribute guidelines.

[shubham-pawar]

What about the below code?

$json_data = !empty($_REQUEST['rest_data']) ? html_entity_decode($GLOBALS['RAW_REQUEST']['rest_data']) : '';

[JoshMcCullough]

What about the below code?

That would make sense, but it was unclear to me why $_REQUEST['rest_data'] is used on the left and $GLOBALS['RAW_REQUEST']['rest_data'] is used on the right. So I wrapped the whole line. I suspect this is also a bug, and the solution is actually:

$json_data = !empty($_REQUEST['rest_data']) ? html_entity_decode($_REQUEST['rest_data']) : ''; 

[JoshMcCullough]

I don't "do" PHP, so this is a bit lost on me, but apparently:

# $GLOBALS['RAW_REQUEST']['rest_data']
{"user_auth":{"user_name":"admin","password":"dd995564b1fa70241364c5926aa27997"},"application":"test"}

# $_REQUEST['rest_data']
{&user_auth&:&user_name&:&admin&,&password&:&dd995564b1fa70241364c5926aa27997&},&application&:&test&}

Why would the quotes in $_REQUEST['rest_data'] be replaced with & in the incoming request body?

---

So the solution, then, seems to be:

$raw_json_data = $GLOBALS['RAW_REQUEST']['rest_data'];
$json_data = !empty($raw_json_data)? html_entity_decode($raw_json_data): '';

[pgorod]

Isn't the solution to fix the place where the data is being broken, instead of un-breaking it later?

[JoshMcCullough]

Isn't the solution to fix the place where the data is being broken, instead of un-breaking it later?

Please have a look at the thread I posted. I could not, in any way, get the API to work without this fix. I have to assume that I am somehow wrong, and I'd love some additional insight if so.

[pgorod]

I think you did your job well, you traced the problem and fixed it. If there is an earlier, bigger problem of excessive data clean ups, it's not your fault, and probably it's not something you can easily track down. I believe there is such a problem, and that it has been plaguing SugarCRM and then SuiteCRM for many years. We break things in one place and then patch them back up in numerous other places, accumulating technical debt because if we ever fix the original place, we will break all these other places. Sigh...

That's why I go around polluting these issues and PR's with my comments, but at this point I don't expect solutions, I am just a prophet preaching to the wind.

https://news.ycombinator.com/item?id=14181662

The TL;DR version is: we should filter on output, not on input; filtering should be specific to what is being done with the data (encoding for browser, for database, for JS or for HTML, etc). Since we can't trust our filtering on output, we filter on input. Then when things pop up wrongly filtered, we put them back as they were. Not a good practice.

[JoshMcCullough]

So what is the solution here? Are you suggesting that this PR will fix it for one case but break it for another?

[pgorod]

Yes, that is what I fear is happening in many cases like this one. I will leave the decisions to the project maintainers.