Encouraging safe practices around content/innerHTML

[jlai]

Subject of the issue

Hi, I recently started using the library and it's really nice.

However, I noticed most of the examples use a content string to put HTML in the grid nodes. This is OK for static content, but from a security perspective it opens up some potential for accidental XSS.

Modern frameworks are pretty good at preventing inexperienced developers from accidentally allowing XSS bugs by not allowing or by strongly discouraging use of innerHTML, for example React's dangerouslySetInnerHTML, but this security measure is bypassed by using content with this library.

Many of these issues can be avoided by passing el when creating the component, although there's still a few loopholes and it doesn't solve the problem of inexperienced developers using content inappropriately.

Scenario 1: String interpolation

A novice developer might try to do this:

grid.addWidget(`<b>${name}</b>`);

which would very easily open up an XSS attack if name came from somewhere untrusted. Example: https://jsfiddle.net/7094edhx/

Scenario 2: Load/save

The docs encourage using load()/save() to serialize a grid into an object, which can be serialized into a JSON string. The default save option serializes the HTML into content.

This is fine if the user is storing the JSON in localStorage, but opens up a lot of avenues for stored XSS attacks when stored elsewhere. For example, if someone built a dashboard editor that saved a grid to a server to share with other users without sanitizing it on save or load, this could allow users to inject Javascript that would be displayed to other users. Even the serialization demo shows an example of saving onclick handlers intact (although only locally).

A developer might wisely decide that saving user-created HTML is not a good idea, and do something like this:

async saveGrid() {
  const serialized = grid.save(false); // don't save HTML
  await upload(JSON.stringify(serialized));
}

async loadGrid() {
  const layout= JSON.parse(await getLayoutFromServer());
  grid.load(JSON.parse(serialized));

  document.querySelectorAll('.grid-stack-item').forEach((el) => {
    const id = el.getAttribute("gs-id");
    // do stuff to populate el based on id
  });
}

but get caught by the fact that load could inject HTML if they don't sanitize or validate the layout object. A malicious user could manually (e.g. by making direct calls to the server using curl instead of using the web page) upload a layout that includes content.

This can also affect widgets created with addWidget(el, opts) if update(el, opts) is called on it later with an untrusted opts object, although this is less likely than the load() scenario.

Solutions

Document the issue

The simplest change would be to add a section to the documentation educating users on potential XSS issues and how to avoid them. This puts the onus on developers to read it and stick to best practices, but doesn't protect against accidents.

Security works best with defense-in-depth, which is why I think we should do more than just educate the user.

Sanitize content

Most of the examples use a simple text string for the content, or simple markup that doesn't have any interactivity. Potentially, content could be sanitized by default to allow for these simple use cases.

The downside is that you'd need a library like sanitize-html or DOMPurify to do it correctly, which would add an external dependency (this library currently has none).

This feels like it's beyond the scope of a grid layout library, so I don't actually recommend this as an option.

A simpler option would be to only allow text content, rendered using el.textContent = w.textContent, which would be mostly useful for examples/demos. This would a breaking change. A transitional step could be use textContent and htmlContent (or unsafeHtmlContent to make it clear that it's potentially unsafe).

Disable HTML content by default

Only allow htmlContent and passing HTML strings to addWidget if GridStack.init is passed a dangerouslyAllowHtmlContent option to enable it. This would help with migration and protect code that mostly uses addWidget(el) from accidental content during update or load.

Data property / rendering function

To provide users an alternative to content, maybe allow setting a rendering function, and add a data object to the widget properties. It might work something like this:

interface MyGridData {
  type: 'chart' | 'note';
  chartConfig: ChartConfig;
  note: string;
}

const grid = GridStack<MyGridData>.init();
grid.setRenderFunc((nodeEl, node) => {
  const data = node.data;

  switch (data.type) {
    case "chart":
      const chartEl = document.createElement('div');
      setupChart(el, data.chartConfig);
      nodeEl.appendchild(el);
      break;
    case "note":
     parentEl.textContent = data.noteText;
     break;
  }
});

grid.load([
  {
    w: 3,
    h: 3,
    data: {type: 'chart', chartConfig: {series: 'agriculture'}}
  }
]);

grid.addWidget({
  w: 3,
  h: 3,
  data: {type: 'note', noteText: "Hello world"}}
});

This would basically be a more user-friendly version of the addRemoveCB and would work with addWidget instead of just load.

addRemoveCB is confusing because it lets you manage el creation but only notifies you of removal and does el.remove() itself. Instead, it might be better as three functions that the developer can pass to customize: createDOMFunc, renderFunc, removeDOMFunc.

Create would be used if the user needs to customize inserting the DOM node. Render if the user just wants to add children to the item node and use the default create func. Remove for people who need to customize how it's removed from the DOM (separate from the 'remove' event).

Data would also be useful for serializing additional state for each node instead of having to store it separately and look up by id.

If this sounds like a good approach, I can break it out into a separate Github issue.

Remove HTML strings from the framework

Ultimately, I think the goal should be to remove .innerHTML from the library entirely. If the user needs to populate content this way, they can manually set el.innerHTML, ideally after running it through a sanitizer or their framework's safe render call.

There's a couple cases where .innerHTML is used to create a div with classes which could be replaced with document.createElement followed by setting the classList. Although it's highly unlikely that gridOptions.itemClass could be passed an untrusted string, it's better to just eliminate the risk, which also makes the library more palatable to security teams where innerHTML might show up as a red flag on a scan.

Your environment

• 10.2.1