Fix for unicode conversion in json UPDATE to html engine. fixing #1106 #1673
Conversation
| try { | ||
| if (browser_ != nullptr && browser_->GetMainFrame() != nullptr) { | ||
| // Safe UTF-8 conversion with error handling | ||
| std::string utf8_javascript; |
There was a problem hiding this comment.
I'm confused as to what this is solving too.
In what scenarios could u8(javascript) fail? We are using that in so many places, that it feels like it would be a bigger problem if there are strings that cause it to throw an exception
| CASPAR_LOG(info) << L"Received message from " << client->address() << ": " << message << L"\\r\\n"; | ||
| // Sanitize message for logging to prevent conversion errors | ||
| std::wstring sanitized_log_message = message; | ||
| caspar::log::replace_nonprintable(sanitized_log_message, L'?'); |
There was a problem hiding this comment.
This seems redundant, we are already using replace_nonprintable at the point where the log gets written anywhere. Or at least that is the intention, so is there a case where this isnt happening?
There was a problem hiding this comment.
there were problems when you did a console.log() with unicode chars from a template. ccg would abort/crash. Console messages are now cleaned up in OnConsoleMessage in the html_producer.cpp.
| // Trim the data | ||
| std::wstring trimmed_data; | ||
| try { | ||
| trimmed_data = boost::algorithm::trim_copy_if(data, boost::is_any_of(" \"")); |
There was a problem hiding this comment.
can this actually throw in a meaningful way? Looking at the signature, I suspect the only throws is if it fails to allocate the output string. Which is not something worth handling here.
I am happy to be wrong though
| // Now escape for JavaScript | ||
| std::wstring escaped_data; | ||
| try { | ||
| escaped_data = escape_javascript_string(json_safe_data); |
There was a problem hiding this comment.
Again, I dont see where/how this could throw an error, so this handling adds a lot of I assume unnecessary noise
|
|
||
| // Robust error handling to prevent app crashes | ||
| std::wstring javascript_call; | ||
| try { |
There was a problem hiding this comment.
Another try catch that seems unnecessary
| } | ||
|
|
||
| // Pre-validate and clean the JSON to prevent crashes | ||
| // Remove any control characters that might cause JSON parsing issues |
There was a problem hiding this comment.
I am wondering about this, whether it is a good idea for us to be mangling the json or if we should continue assuming that the client gave us valid json.
I am not sure, so am open to discussion on this.
Also, have you tested this with xml? that is likely less common to use these days, but in the past was the recommended/only supported format for this data
|
Thanks for your review. It was a bit messed up. I created a test scenario for a lot of cases. They all pass now. The backup scenario's for the conversion were there for extra safety but not used in real world scenario's or test-cases. so they are removed. As well as the intensive try catches. Test script and template to invoke and update various utf-8/unicode strings in json, xml and invoke commands. |
2 participants
This is an attempt to resolve bug #1106, whereby Unicode characters in JSON via an AMCP CG UPDATE command rendered the AMCP channel unusable.
This solution has been extensively tested with many different Unicode payloads, and the AMCP connection now continues to function.
A try/catch has also been added so that the template continues to work even if the JSON is incorrect and no error message is logged.