Skip to main content

Terrible Practices: Injecting JavaScript in a HTTP Response using eval()

At work, for the last couple weeks we have been refactoring a system of our software. We offer a standalone desktop version (where most data processing happens) and a 'Web Portal' as an add-on to allow a company's customers to preview the data that the company has. I was in charge of implementing the new process on the Web Portal. One of these features is a 'Waste Report' which collates a group of records with varying Weights and Percentages. However this is not relevant.

 I'm writing this post to reflect on a terrible hack I implemented in the server side code to output the Pie Charts to represent the data. As I was rushing to get this done, instead of creating multiple endpoints to gather the data needed and then generating the waste chart on the client side , I hacked it all into one response on the server.

I did this because the Web Portal is built as a monolith using primitive PHP for frontend and backend, and since I want the Web Portal to function as a web app and not a website, I have to do some trickery with calls / responses using HTMX and JQuery.

 

Here's what I did:

1. Aggregate the relevant data. This step is super generic, do a couple SQL Queries with a combination of dictionaries to collate data together.

2. Loop over the results and echo HTML Tables back in the response and replace an element on client side using JQuery.

3. This is where the juice is. Since the first iteration of this system had no issue being dumped into a HTML Response, this second version needed charts... I used chart.js for this and as much as I appreciate its in-depth customisability, it can be quite a challenge to wrangle. In a mad dash to get this done I decided the easiest way to collate this data within PHP itself is just to send the JavaScript code (to generate the pre-filled chart) as part of the HTTP Response. 

I was surprised how easy this was since all I had to do on the client side is ensure that the right dependencies (chart.js) were included and I could do a simple JS eval() on the incoming script.

 I slapped the generated code into a 'div' block which was hidden and sent it back to the client, it looked something like this:


<div class='noprint' hidden id='chartscript'>

                    const ctx = document.getElementById('waste-chart');
                    console.log('generating pie chart');
                    console.log('ctx='+ctx);

                    Chart.overrides['pie'].plugins.legend.position = 'right';
                    Chart.overrides['pie'].plugins.legend.align = 'middle';
                    Chart.defaults.maintainAspectRatio = false;
                    Chart.defaults.aspectRatio = 1;
                    Chart.register(ChartDataLabels);

                    new Chart(ctx,{
                        type:'pie',
                        data:{
                            labels: <?= $this->get_codes_for_chart($used_codes) ?>,
                            datasets:[{
                                label: 'Tonnes',
                                data: <?= $this->get_data_for_chart($used_codes) ?>,
                                borderWidth: 2,
            ...

                    

</div>

And on the client side:

 

        $.ajax({
            type: "post",
            url: "base/generate_wastereport.php",
            data: {sites : siteArray, date_from : dateFrom, date_to : dateTo, show_detail : wants_detail},
            success: function (response) {
                console.log('Successfully received response from Waste Report Generator');
                document.getElementById('waste-result').innerHTML = response;

                try {eval(document.getElementById('chartscript').innerHTML);}
                catch (err){
                    console.log('Failed to parse incoming script.');
                    console.log(err);
                }

                setTimeout(enableExportButtons,2000);
            }
        });


To make sure the data and label output in the PHP response was formatted in correct JSON I hacked in a 'fake' array format using strings:

 

   private function get_codes_for_chart($used_ewc)
    {
        $finalString = '';
        $len_minusone = count($used_ewc) - 1;
        $i = 0;

        $finalString = '[';

        foreach ($used_ewc as $key=>$value)
        {
            

            if ($i != $len_minusone) 
                $finalString .= '"'.$key . ' (' . $this->get_desc($key) . ')'." [{$value} tonnes]".'"'.',';

            else //final element
                $finalString .= '"'.$key . ' (' . $this->get_desc($key) . ')'." [{$value} tonnes]".'"';

            $i++;
        }

        $finalString .= ']';

        return $finalString;
    }

This worked lovely without having to write this on the client side as the data was already encoded correctly. But with hindsight, why did I ever do this?

How I would re-write this

 If I were to do this again I would definitely just send the chart data in JSON from a different end point. Then, use use a JS function to construct the chart on the client side after the server responded with the JSON.

Why is this terrible? 

- As answered by Andrew Hedges in 2008 on StackOverflow:


Passing user input to eval() is a security risk, but also each invocation of eval() creates a new instance of the JavaScript interpreter. This can be a resource hog.

- A lot of StackOverflow comments described it as being incredibly hard to debug, however since this was just a Chartjs function with some JSON I did not encounter this issue.

- Security was mentioned a lot during my research however a couple people noted that eval is only dangerous in the case of user input being involved, however since I was only evaluating code sent back from the server this shouldn't be a problem.

 - Reading this Medium Article by Hui covers the issues quite well. It goes into detail how code in eval() statements cannot be optimised due to no caching, lack of pre-compilation and potential overriding of existing variables and functions. 

So yeah, there is actually no reason for using eval() in my case. However it was a function I've never used before so it was fun embracing the power of procedural languages :) 

 

Labels: