Grid.js formatter uses innerHTML by default, facilitating injection vulnerabilities

[mbomb007]

Grid.js lines 236 - 237:
td.innerHTML = typeof formatter === 'string' && formatterScope ? formatterScope[formatter](value, object) : this.formatter(value, object);

The code above makes it easy for programmers to create applications with script injection vulnerabilities. If the formatter function returns a dynamic string/property that contains malicious HTML/JavaScript content, it will be inserted into the DOM without a second thought.

It would be a better idea to make the attribute dynamic but default it to textContent. That way the user can set some property to change it to use innerHTML if dynamic HTML is desired. Something like this:

td[this.allowHTML ? 'innerHTML' : 'textContent'] = typeof formatter === 'string' && formatterScope ? formatterScope[formatter](value, object) : this.formatter(value, object);

[schallm]

We created an extension (SafeRenderCell) that would give the result you were looking for with a slightly different implementation. If a formatter returns an object with an html property, innerHTML will be used. Otherwise it will be added as text.

An example below shows escaping a value if we want to use html, otherwise just returning a value will be added as text. The escape function is a slight wrapper on dojo's string.escape to support other data types.

I think the intended usage for column.formatter is simple formatting (i.e. uppercase, lowercase, ...) but since dgrid uses innerHTML, it could easily be used for a Cross-Site-Scripting attack. Using column.renderCell may be a better choice if you plan to supply a DOM structure. In that case the developer is in charge of escaping user input.

Usage

    formatter: function (value, object) {
        if (value && object.entryType === "important") {
            return { html: "<span class='important'>" + format.escape(value) + "</span>" };
        } else {
            return value;
        }
    }

format.js (partial)

    escape: function(value) {
        if (value == null /* or undefined */) {
            return "";
        }
        if (typeof value !== "string") {
            value = String(value);
        }
        return string.escape(value);
    }

SafeRenderCell

define("atg/dgrid/extensions/SafeRenderCell", [
	"dojo/_base/declare"
], function (declare, domConstruct) {
	return declare(null, {
		_defaultRenderCell: function (object, value, td) {
			// summary:
			//		Default renderCell implementation.
			//		NOTE: Called in context of column definition object.
			// object: Object
			//		The data item for the row currently being rendered
			// value: Mixed
			//		The value of the field applicable to the current cell
			// td: DOMNode
			//		The cell element representing the current item/field
			// options: Object?
			//		Any additional options passed through from renderRow

			if (this.formatter) {
				// Support formatter, with or without formatterScope
				var formatter = this.formatter,
					formatterScope = this.grid.formatterScope;
				var formattedValue = typeof formatter === "string" && formatterScope ?
					formatterScope[formatter](value, object) : this.formatter(value, object);
				if (formattedValue != null && formattedValue.hasOwnProperty("html")) {
					td.innerHTML = formattedValue.html;
				} else {
					td.appendChild(document.createTextNode(formattedValue));
				}
			}
			else if (value != null) {
				td.appendChild(document.createTextNode(value));
			}
		}
	});
});

[dylans]

I'd generally be ok with either suggestion, though I'd want a specific name for the property for the initial suggestion, e.g. formatterAllowHTML rather than allowHTML.

[schallm]

mbomb007 and I actually work at the same company and decided to move forward with "object with html property" approach since the decision to use html is part of the return statement and can't really be used accidentally by a developer.

Is this something that you would like a pull request to "fix" dgrid? It would be a breaking change if people are currently returning valid html from their formatter functions. However, the change would be "safe by default". Or would you like a pull request for the extension?

[dylans]

While it would be a breaking change, improving default security is a place where a breaking change makes sense to me. So I would suggest a PR to "fix" dgrid. Also, I'm not sure how many people actually follow this code path anyway, so it seems ok to make this breaking change to me.

[schallm]

Added pull request #1422

[msssk]

Fixed in #1422