d3 / d3-dsv

A parser and formatter for delimiter-separated values, such as CSV and TSV.
https://d3js.org/d3-dsv
ISC License
437 stars 76 forks source link

Add a CSP warning for csvParse, tsvParse, parse #67

Closed tdelmas closed 4 years ago

tdelmas commented 4 years ago

Fix #66

Because when coming from the D3 API documentation (https://github.com/d3/d3/blob/master/API.md#delimiter-separated-values-d3-dsv) the "Content Security Policy" warning could be missed.

New visual:

image

curran commented 4 years ago

I'm curious, would a refactor that avoids usage of the Function constructor be a desirable change?

Apparently dynamic code generation was chosen for speed, but are there alternatives?

tdelmas commented 4 years ago

@curran When I asked about it I got a negative answer at that time: https://github.com/d3/d3-fetch/issues/35#issuecomment-586739138 but maybe now they are more open to the idea? But anyway, the CSP warning of this PR is important as long as this code use Function.

archmoj commented 3 years ago

I'm curious, would a refactor that avoids usage of the Function constructor be a desirable change?

Apparently dynamic code generation was chosen for speed, but are there alternatives?

@curran @tdelmas See https://github.com/plotly/plotly.js/pull/5359/files#r547514970

Also cc: https://github.com/d3/d3-dsv/issues/65

curran commented 3 years ago

Very cool. Capturing the code from there:

    return dsv.parseRows(text, function(row, i) {
      if (o) return o(row, i - 1);
      // Old implementation:
      // var a = new Function("d", "return {" + row.map(function(name, i) {
      //   return JSON.stringify(name) + ": d[" + i + "]";
      // }).join(",") + "}");

      // New implementation:
      var a = function(d) {
        var obj = {};
        for(var k = 0; k < row.length; k++) {
          obj[row[k]] = d[k];
        }
        return obj;
      }

      o = f ? function(row, i) { return f(a(row), i); } : a;
    });
  };

Looks reasonable to me if it does the job.

IMO it would be better to avoid use of the Function constructor if possible, as it is a potential JavaScript injection vector.

I wonder, has anyone tried to hack this? I wonder if it is actually possible to craft some input that performs a JavaScript injection attack. If we had an example of that in hand, it would be a great argument in favor of making this change.

curran commented 3 years ago

Also curious how performance between these two implementations compares. Some benchmarks would be interesting to see.