palantir / plottable

:bar_chart: A library of modular chart components built on D3
http://plottablejs.org/
MIT License
2.97k stars 222 forks source link

Guard against unsafe inputs / sanitize inputs #862

Open bluong opened 10 years ago

bluong commented 10 years ago

For classes like Labels, they assume that a string is inputted when it could easily not be. We should investigate our code base and make sure that arbitrary inputs can generate expected behavior.

bluong commented 10 years ago

So far there is one argument presented for the above simple case where we could stringify any input to guarantee a string is passed in. However, there is another argument presented that we should fail fast and let them know about the error and error out. I also present a compromise that we can stringify and warn the user in the console. All of these ideas have advantages and disadvantages...

bluong commented 10 years ago

Adding the hard label, since unless typescript actually puts an inherent guard around our api calls (I can imagine it doesn't), we might potentially have to add some layer of security to every public endpoint (ensure input is a string, ensure input is a number). Even if typescript places a guard on types, we might have to clean up the input anyways.

bluong commented 10 years ago

Suggestion is to call a method at the very start of a public-facing method that essentially verifies the argument and either converts it or have the method return a boolean if the argument is ok to go through. For example,

function(foo: number, bar: string) verifyType(foo, number) verifyType(bar, string)

In the above case, if we decide to make a conversion, then the usage might change to foo = ensureType(foo, number)...

Or if we instead decide to have a flag, then the usage could change to if (!verifyType(foo, number)) { console.log("Wrong type"); return; }

Thoughts?

bluong commented 10 years ago

This is a more generalized version of #627 Closing that one for this generalized version

bluong commented 10 years ago

As we are getting more and more sanitization bugs, I'm moving the priority up.

bluong commented 10 years ago

Associated discussion thread #1096

jtlan commented 10 years ago

Related: inputs to project() sometimes also need to be scrubbed, which is kind of odd: #592.

crmorford commented 9 years ago

@jtlan @bluong is it time to close this?

bluong commented 9 years ago

Not yet. Now that we have made a decision, we have made a big step into closing this issue. Now we just need to do a global sweep and ensure we throw errors under the wrong type expected.