Have you considered supporting CSP header?

JesperJakobsenCIM commented 1 year ago

As far as I can see, the only real issue is with "style-src unsafe-inline" in the Content Security Policy (CSP). However, you can work around this by using:

element.style.display = "block"
element.style.height = "242px"

These properties allow you to bypass CSP and still use inline CSS. Please don't ask me why this works, but it should enable you to use SlickGrid with a CSP header.

You can also simplify the application of CSS using a function like this:

function applyCss(element, css) {
    let keys = Object.keys(css);
    for (let i = 0; i < keys.length; i++) {
        let key = keys[i];
        let value = css[key];

        element.style[key] = value;
    }
}

This function makes applying CSS styles easier.

ghiscoding commented 1 year ago

You are barely providing details about the problem and it looks like you're using an older version of the grid but I'm not sure since you just provided code that I don't think is part of SlickGrid... Anyway, the best would be for you to contribute because I don't think we will look into this ourselves.

JesperJakobsenCIM commented 1 year ago

This is mainly for someone else who wants to attempt to fix it or as a reminder to myself of what the issue is when I come back to try fixing it.

For information, after I attempted to reproduce it using one of your examples

The issue lies the the way you append the css to the html

<!DOCTYPE HTML>
<html>
<head>
  <link rel="shortcut icon" type="image/ico" href="favicon.ico" />
  <link rel="stylesheet" href="../dist/styles/css/example-demo.css" type="text/css"/>
  <link rel="stylesheet" href="../dist/styles/css/slick-alpine-theme.css" type="text/css"/>
  <meta http-equiv="Content-Security-Policy" content="default-src 'self' https://cdn.jsdelivr.net">
</head>
<body>
<table width="100%">
  <tr>
    <td valign="top" width="50%">
      <div id="myGrid"></div>
    </td>
    <td valign="top">
      <h2>
        <a href="/examples/index.html" id="link">&#x2302;</a>
        Demonstrates:
      </h2>
      <ul>
        <li>column span</li>
      </ul>
        <h2>View Source:</h2>
        <ul>
            <li><A href="https://github.com/6pac/SlickGrid/blob/master/examples/example-colspan.html" target="_sourcewindow"> View the source for this example on Github</a></li>
        </ul>
    </td>
  </tr>
</table>

<script src="https://cdn.jsdelivr.net/npm/sortablejs/Sortable.min.js"></script>
<script src="sortable-cdn-fallback.js"></script>

<script src="../dist/browser/slick.core.js"></script>
<script src="../dist/browser/slick.interactions.js"></script>
<script src="../dist/browser/slick.grid.js"></script>
<script src="../dist/browser/plugins/slick.cellrangedecorator.js"></script>
<script src="../dist/browser/plugins/slick.cellrangeselector.js"></script>
<script src="../dist/browser/plugins/slick.cellselectionmodel.js"></script>

<script src="./example-csp-header.js"></script>
</body>
</html>

example-csp-header.js

var grid;
var columns = [
    { id: "title", name: "Title", field: "title" },
    { id: "duration", name: "Duration", field: "duration" },
    { id: "%", name: "% Complete", field: "percentComplete", selectable: false, width: 100 },
    { id: "start", name: "Start", field: "start" },
    { id: "finish", name: "Finish", field: "finish" },
    { id: "effort-driven", name: "Effort Driven", field: "effortDriven", width: 100 }
];

var options = {
    enableCellNavigation: true,
    enableColumnReorder: false
};

document.addEventListener("DOMContentLoaded", function () {
    let gridElement = document.getElementById("myGrid");
    gridElement.style.width = "600px";
    gridElement.style.height = "500px";

    let linkElement = document.getElementById("link");
    //text-decoration: none; font-size: 22px
    linkElement.style.textDecoration = "none";
    linkElement.style.fontSize = "22px";

    var data = [];
    for (var i = 0; i < 500; i++) {
        data[i] = {
            title: "Task " + i,
            duration: "5 days",
            percentComplete: Math.round(Math.random() * 100),
            start: "01/01/2009",
            finish: "01/05/2009",
            effortDriven: (i % 5 == 0)
        };
    }

    data.getItemMetadata = function (row) {
        if (row % 2 === 1) {
            return {
                "columns": {
                    "duration": {
                        "colspan": 3
                    }
                }
            };
        } else {
            return {
                "columns": {
                    0: {
                        "colspan": "*"
                    }
                }
            };
        }
    };

    grid = new Slick.Grid("#myGrid", data, columns, options);

    grid.setSelectionModel(new Slick.CellSelectionModel());
});

ghiscoding commented 1 year ago

Searching a little bit and found this SO Javascript: Can i dynamically create a CSSStyleSheet object and insert it? and followed 1 of the answer below to create stylesheet dynamically then insert the dynamic css rules and it seems to work but I don't know how you test the CSP so you could maybe test yourself and let me know. At least it's no longer using innerHtml so it should be better in any case

replace the entire createCssRules() method with the following code and try it out, I have no clue if this passes the CSP but it's a start I guess. Perhaps more info can be found on this official W3C page Dynamic style - manipulating CSS with JavaScript

  protected createCssRules() {
    this._style = document.createElement('style');
    document.head.appendChild(this._style); // must append before you can access sheet property
    const sheet = this._style.sheet;
    if (sheet) {
      const rowHeight = (this._options.rowHeight! - this.cellHeightDiff);
      sheet.insertRule(`.${this.uid} .slick-group-header-column { left: 1000px; }`);
      sheet.insertRule(`.${this.uid} .slick-header-column { left: 1000px; }`);
      sheet.insertRule(`.${this.uid} .slick-top-panel { height: ${this._options.topPanelHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-preheader-panel { height: ${this._options.preHeaderPanelHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-headerrow-columns { height: ${this._options.headerRowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-footerrow-columns { height: ${this._options.footerRowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-cell { height: ${rowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-row { height: ${this._options.rowHeight}px; }`);

      for (let i = 0; i < this.columns.length; i++) {
        if (!this.columns[i] || this.columns[i].hidden) { continue; }

        sheet.insertRule(`.${this.uid} .l${i} { }`);
        sheet.insertRule(`.${this.uid} .r${i} { }`);
      }
    }
  }

for example here is 1 of the dynamically created css rule

P.S. I have no clue why the original author of SlickGrid decided to create dynamic CSS rules, so we have to try to keep the original logic as much as possible. I think it's because by doing it this way, we don't need any external stylesheet file loaded, this css will be applied every time and will also create a css rule by the guid UID which is not possible via an external stylesheet file.

@JesperJakobsenCIM I created PR #883, if you could please test it out and provide feedback, Thanks

All Cypress E2E tests are passing so it's already promising, I think this code might not work in IE but we don't support it anymore so we should be good anyway

JesperJakobsenCIM commented 1 year ago

Its still running into issues. however this can be fixed by just adding a nonce (should be given by the user and is a random string)

and then adding the nonce to the element

 protected createCssRules() {
    this._style = document.createElement('style');
    this._style.nonce = "random-string";
    document.head.appendChild(this._style); // must append before you can access sheet property
    const sheet = this._style.sheet;
    if (sheet) {
      const rowHeight = (this._options.rowHeight! - this.cellHeightDiff);
      sheet.insertRule(`.${this.uid} .slick-group-header-column { left: 1000px; }`);
      sheet.insertRule(`.${this.uid} .slick-header-column { left: 1000px; }`);
      sheet.insertRule(`.${this.uid} .slick-top-panel { height: ${this._options.topPanelHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-preheader-panel { height: ${this._options.preHeaderPanelHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-headerrow-columns { height: ${this._options.headerRowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-footerrow-columns { height: ${this._options.footerRowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-cell { height: ${rowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-row { height: ${this._options.rowHeight}px; }`);

      for (let i = 0; i < this.columns.length; i++) {
        if (!this.columns[i] || this.columns[i].hidden) { continue; }

        sheet.insertRule(`.${this.uid} .l${i} { }`);
        sheet.insertRule(`.${this.uid} .r${i} { }`);
      }
    }
  }

However this seems to trigger a new error this error comes from however this could by fixed by using the elm.setAttribute() instead, tho you'll run into naming issue from there since ariaOrientation ends in areaorientation instead of aria-orientation as it should be, tho you could create some mapping to translate these

public static createDomElement<T extends keyof HTMLElementTagNameMap, K extends keyof HTMLElementTagNameMap[T]>(
    tagName: T,
    elementOptions?: null | { [P in K]: InferDOMType<HTMLElementTagNameMap[T][P]> },
    appendToParent?: Element
  ): HTMLElementTagNameMap[T] {
    const elm = document.createElement<T>(tagName);

    if (elementOptions) {
      Object.keys(elementOptions).forEach((elmOptionKey) => {
        const elmValue = elementOptions[elmOptionKey as keyof typeof elementOptions];
        if (typeof elmValue === 'object') {
          Object.assign(elm[elmOptionKey as K] as object, elmValue);
        } else {
          const optionValue = (elementOptions as any)[elmOptionKey as keyof typeof elementOptions];
          console.log(elmOptionKey, optionValue);
          elm.setAttribute(elmOptionKey, optionValue); // [elmOptionKey as K] = (elementOptions as any)[elmOptionKey as keyof typeof elementOptions];
        }
      });
    }
    if (appendToParent?.appendChild) {
      appendToParent.appendChild(elm);
    }
    return elm;
  }

ghiscoding commented 1 year ago

setAttribute is not a good solution because it's not going to work with everything (for example innerHTML is not an attribute). The createDomElement was put it in place to simplify the code into a 1 liner

For example this simple 1 line code from slick.grid

Utils.createDomElement('div', { className: 'slick-cell', id: '', style: { visibility: 'hidden' }, textContent: '-' }, r);

is actually 6 lines when applied

const cellElm = document.createElement('div');
cellElm.className = 'slick-cell';
cellElm.id = '';
cellElm.style.visibility = 'hidden';
cellElm.textContent = '-';
r.appendChild(cellElm);

I guess, but not entirely sure, the 6 lines code would pass CSP? But it would increase the code by a lot of lines. I searched for createDomElement and I see 88 results, some of them only have 1 or 2 assignments, like className, which isn't too bad but some have multiple lines like the one above and again that would increase the code size and number of lines.

I merged the other PR that I referenced earlier, it's only a partial fix and I would suggest you start contributing since you know how to test these things and I don't know what else to change.

EDIT

After trying some more, I created another PR #884, it seems that CSP also doesn't like innerHTML but a lot of SlickGrid is based on its usage, so without this I think it would be impossible for SlickGrid to fully support CSP. It should be allowed in our case if we know that we provided a sanitizer (like DOMPurify) and sanitize the html string before passing it to innerHTML which we do in the case below but still it's blocked by CSP.

adding nonce = 'random-string' doesn't seem to help for that one either

JesperJakobsenCIM commented 1 year ago

For DomPurify to work https://web.dev/articles/trusted-types

you are properly not parsing in

import DOMPurify from 'dompurify';
el.innerHTML = DOMPurify.sanitize(html, {RETURN_TRUSTED_TYPE: true});

missing this {RETURN_TRUSTED_TYPE: true} might be the reason

edit: you can read about trustedHtml here https://developer.mozilla.org/en-US/docs/Web/API/TrustedHTML

ghiscoding commented 1 year ago

even if I add DOMPurify to the CSP Example (in this commit 1e13388), it still throws some errors on the lines with innerHTML

<script src="https://cdn.jsdelivr.net/npm/dompurify@3.0.6/dist/purify.min.js"></script>

then add the grid option sanitizer

var options = {
  enableCellNavigation: true,
  enableColumnReorder: false,
  sanitizer: (dirtyHtml) => DOMPurify.sanitize(dirtyHtml, { RETURN_TRUSTED_TYPE: true })
};

I also tried to create a default policy as suggest here: https://web.dev/articles/trusted-types#use_a_default_policy but that didn't work either, even if it got slightly further

EDIT 1

If I change the CSP to include only the trusted types then it works, but it would be better to make it work with default-src 'self' as well

<meta http-equiv="Content-Security-Policy" content="require-trusted-types-for 'script'; trusted-types dompurify">

EDIT 2

So I also found that CSP doesn't like my createDomElement() method when using innerHTML assignment, but it only fails with innerHTML property, so what we can do is still use createDomElement() for everything except the innerHTML and then CSP is happy. Basically doing this change makes CSP happy

- Utils.createDomElement('span', { className: 'slick-column-name', innerHTML: this.sanitizeHtmlString(m.name as string) }, header);

// split into 2 lines, to have explicit innerHTML assignment
+ const colNameElm = Utils.createDomElement('span', { className: 'slick-column-name' }, header);
+ colNameElm.innerHTML = this.sanitizeHtmlString(m.name as string);

but again, I only got CSP working with trusted-types but without default-src 'self' as described in EDIT 1. So I'm getting closer and I feel that it's probably just a config that I'm missing to get this working

So I decided to go ahead and merge the 2nd PR #884 with the new CSP Example which uses trusted types. I think that is the best I can do with this and I will release a new version later including these fixes. If you know how to make it work with default-src then please contribute a PR. SlickGrid still has to support innerHTML, because not supporting it would be a major breaking change (for example custom Formatter typically provide html string that is then passed to SlickGrid innerHTML)

ghiscoding commented 1 year ago

The 2 PRs #883 and #884 that I previously merged are now available in today's release v5.4.0. This should help with CSP header and I also added the CSP Header Example to the Examples Wiki

JesperJakobsenCIM commented 1 year ago

I'll investigate it further to see if I can make the example work.

The nonce value should also come from the options object so its the developer who controls it.

As a side note, when I attempted to implement it on our website initially, I noticed that it triggered an 'unsafe-eval' operation in 'slick.dataview.ts.' The 'new Function' is considered equivalent to 'eval(),' which is unsafe. I'm unsure if there's an alternative option for this particular case.

protected compileAccumulatorLoop(aggregator: Aggregator) {
    if (aggregator.accumulate) {
      const accumulatorInfo = this.getFunctionInfo(aggregator.accumulate);
      const fn: any = new Function( //here
        "_items",
        "for (var " + accumulatorInfo.params[0] + ", _i=0, _il=_items.length; _i<_il; _i++) {" +
        accumulatorInfo.params[0] + " = _items[_i]; " +
        accumulatorInfo.body +
        "}"
      );
      const fnName = "compiledAccumulatorLoop";
      fn.displayName = fnName;
      fn.name = this.setFunctionName(fn, fnName);
      return fn;
    } else {
      return function noAccumulator() { };
    }
  }

JesperJakobsenCIM commented 1 year ago

The reason it doesn't work appears to be that it contains style tags in the pre-generated HTML, which conflicts with the 'unsafe-inline' directive in the 'style-src' section of the header. You can see this in the HTML example.

slick.grid.ts

protected renderRows(range: { top: number; bottom: number; leftPx: number; rightPx: number; }) {
    const stringArrayL: string[] = [];
    const stringArrayR: string[] = [];
    const rows: number[] = [];
    let needToReselectCell = false;
    const dataLength = this.getDataLength();

    for (let i = range.top as number, ii = range.bottom as number; i <= ii; i++) {
      if (this.rowsCache[i] || (this.hasFrozenRows && this._options.frozenBottom && i === this.getDataLength())) {
        continue;
      }
      this.renderedRows++;
      rows.push(i);

      // Create an entry right away so that appendRowHtml() can
      // start populating it.
      this.rowsCache[i] = {
        rowNode: null,

        // ColSpans of rendered cells (by column idx).
        // Can also be used for checking whether a cell has been rendered.
        cellColSpans: [],

        // Cell nodes (by column idx).  Lazy-populated by ensureCellNodesInRowsCache().
        cellNodesByColumnIdx: [],

        // Column indices of cell nodes that have been rendered, but not yet indexed in
        // cellNodesByColumnIdx.  These are in the same order as cell nodes added at the
        // end of the row.
        cellRenderQueue: []
      };

      this.appendRowHtml(stringArrayL, stringArrayR, i, range, dataLength);
      if (this.activeCellNode && this.activeRow === i) {
        needToReselectCell = true;
      }
      this.counter_rows_rendered++;
    }

    if (!rows.length) { return; }

    const x = document.createElement('div');
    const xRight = document.createElement('div');
//issue appears here, not from this.sanitizeHtmlString(stringArrayL.join('')); but from the assignment, since the HTML contains style tags 
    x.innerHTML = this.sanitizeHtmlString(stringArrayL.join(''));
    xRight.innerHTML = this.sanitizeHtmlString(stringArrayR.join(''));

    for (let i = 0, ii = rows.length; i < ii; i++) {
      if ((this.hasFrozenRows) && (rows[i] >= this.actualFrozenRow)) {
        if (this.hasFrozenColumns()) {
          if (this.rowsCache?.hasOwnProperty(rows[i]) && x.firstChild && xRight.firstChild) {
            this.rowsCache[rows[i]].rowNode = [x.firstChild as HTMLElement, xRight.firstChild as HTMLElement];
            this._canvasBottomL.appendChild(x.firstChild as ChildNode);
            this._canvasBottomR.appendChild(xRight.firstChild as ChildNode);
          }
        } else {
          if (this.rowsCache?.hasOwnProperty(rows[i]) && x.firstChild) {
            this.rowsCache[rows[i]].rowNode = [x.firstChild as HTMLElement];
            this._canvasBottomL.appendChild(x.firstChild as ChildNode);
          }
        }
      } else if (this.hasFrozenColumns()) {
        if (this.rowsCache?.hasOwnProperty(rows[i]) && x.firstChild && xRight.firstChild) {
          this.rowsCache[rows[i]].rowNode = [x.firstChild as HTMLElement, xRight.firstChild as HTMLElement];
          this._canvasTopL.appendChild(x.firstChild as ChildNode);
          this._canvasTopR.appendChild(xRight.firstChild as ChildNode);
        }
      } else {
        if (this.rowsCache?.hasOwnProperty(rows[i]) && x.firstChild) {
          this.rowsCache[rows[i]].rowNode = [x.firstChild as HTMLElement];
          this._canvasTopL.appendChild(x.firstChild as ChildNode);
        }
      }
    }

    if (needToReselectCell) {
      this.activeCellNode = this.getCellNode(this.activeRow, this.activeCell);
    }
  }

Example of the generated HTML

<div class="ui-widget-content slick-row even" style="top:0px">
    <div class="slick-cell l0 r5">Task 0</div>
</div>
<div class="ui-widget-content slick-row odd" style="top:25px">
    <div class="slick-cell l0 r0">Task 1</div>
    <div class="slick-cell l1 r3">5 days</div>
    <div class="slick-cell l4 r4">01/05/2009</div>
    <div class="slick-cell l5 r5">false</div>
</div>
<div class="ui-widget-content slick-row even" style="top:50px">
    <div class="slick-cell l0 r5">Task 2</div>
</div>

One possible solution could be to dynamically add the top style as a class, for example, 'slick-top-{x}' where 'x' represents the height (e.g., 'slick-top-900'). Alternatively, you could modify the styling after appending the HTML to the DOM using 'setStyleSize.'

Here's an example of how to make it work with the first option. slick.grid.ts

protected appendRowHtml(stringArrayL: string[], stringArrayR: string[], row: number, range: CellViewportRange, dataLength: number) {
    const d = this.getDataItem(row);
    const dataLoading = row < dataLength && !d;
    let rowCss = 'slick-row' +
      (this.hasFrozenRows && row <= this._options.frozenRow! ? ' frozen' : '') +
      (dataLoading ? ' loading' : '') +
      (row === this.activeRow && this._options.showCellSelection ? ' active' : '') +
      (row % 2 === 1 ? ' odd' : ' even');

    if (!d) {
      rowCss += ' ' + this._options.addNewRowCssClass;
    }

    const metadata = (this.data as CustomDataView<TData>)?.getItemMetadata?.(row);

    if (metadata?.cssClasses) {
      rowCss += ' ' + metadata.cssClasses;
    }

    const frozenRowOffset = this.getFrozenRowOffset(row);
//changes here
    const rowHtml = `<div class="ui-widget-content slick-top-${(this.getRowTop(row) - frozenRowOffset)} ${rowCss}">`;

    stringArrayL.push(rowHtml);

    if (this.hasFrozenColumns()) {
      stringArrayR.push(rowHtml);
    }

    let colspan: number | string;
    let m: C;
    for (let i = 0, ii = this.columns.length; i < ii; i++) {
      m = this.columns[i];
      if (!m || m.hidden) { continue; }

      colspan = 1;
      if (metadata?.columns) {
        const columnData = metadata.columns[m.id] || metadata.columns[i];
        colspan = columnData?.colspan || 1;
        if (colspan === '*') {
          colspan = ii - i;
        }
      }

      // Do not render cells outside of the viewport.
      if (this.columnPosRight[Math.min(ii - 1, i + (colspan as number) - 1)] > range.leftPx) {
        if (!m.alwaysRenderColumn && this.columnPosLeft[i] > range.rightPx) {
          // All columns to the right are outside the range.
          break;
        }

        if (this.hasFrozenColumns() && (i > this._options.frozenColumn!)) {
          this.appendCellHtml(stringArrayR, row, i, (colspan as number), d);
        } else {
          this.appendCellHtml(stringArrayL, row, i, (colspan as number), d);
        }
      } else if (m.alwaysRenderColumn || (this.hasFrozenColumns() && i <= this._options.frozenColumn!)) {
        this.appendCellHtml(stringArrayL, row, i, (colspan as number), d);
      }

      if ((colspan as number) > 1) {
        i += ((colspan as number) - 1);
      }
    }

    stringArrayL.push('</div>');

    if (this.hasFrozenColumns()) {
      stringArrayR.push('</div>');
    }
  }

And
protected createCssRules() {
    this._style = document.createElement('style');
    this._style.nonce = 'random-string';
    (this._options.shadowRoot || document.head).appendChild(this._style);
    const sheet = this._style.sheet;
    if (sheet) {
      const rowHeight = (this._options.rowHeight! - this.cellHeightDiff);
      sheet.insertRule(`.${this.uid} .slick-group-header-column { left: 1000px; }`);
      sheet.insertRule(`.${this.uid} .slick-header-column { left: 1000px; }`);
      sheet.insertRule(`.${this.uid} .slick-top-panel { height: ${this._options.topPanelHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-preheader-panel { height: ${this._options.preHeaderPanelHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-headerrow-columns { height: ${this._options.headerRowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-footerrow-columns { height: ${this._options.footerRowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-cell { height: ${rowHeight}px; }`);
      sheet.insertRule(`.${this.uid} .slick-row { height: ${this._options.rowHeight}px; }`);

      //this could give issues on update, if so this code should moved to a render function instead
      const Cdata = this.getData();
      for (let I = 0; I < this._options.rowHeight! * (Cdata as any[]).length; I += this._options.rowHeight!) {
        // const element = array[I];
        sheet.insertRule(`.${this.uid} .slick-top-${I} { top: ${I}px; }`);
      }

      for (let i = 0; i < this.columns.length; i++) {
        if (!this.columns[i] || this.columns[i].hidden) { continue; }

        sheet.insertRule(`.${this.uid} .l${i} { }`);
        sheet.insertRule(`.${this.uid} .r${i} { }`);
      }
    }
  }

my meta tag

  <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' https://cdn.jsdelivr.net 'nonce-browser-sync'; style-src 'self' 'nonce-random-string'; require-trusted-types-for 'script'; trusted-types dompurify">

I've moved the values out of default-src and into their proper places

Another approach could involve checking if the rule doesn't exist and, if it doesn't, adding it. Regarding the concern of missing rules when adding more items later, what are your thoughts on this?

ghiscoding commented 1 year ago

I noticed that it triggered an 'unsafe-eval' operation in 'slick.dataview.ts.' The 'new Function' is considered equivalent to 'eval(),' which is unsafe. I'm unsure if there's an alternative option for this particular case.

I already know about this one, but I don't know how to replace this. The original author used for this caching and speed performance I think and we somehow replace this with something we might decrease performance by a lot.

The reason it doesn't work appears to be that it contains style tags in the pre-generated HTML, which conflicts with the 'unsafe-inline' directive in the 'style-src' section of the header.

The innerHTML is the real problem, I tried to change the one that you identified to pure JS but I couldn't get it be be fully working. I started another PR #894 that has some more changes in that direction and you also forgot that custom Formatters also use innerHTML and that is what that PR fixed so far by using a new approach (you should take a look at the PR, there's more info in it). For the innerHTML, I tried to create a trusted policy with DOMPurify, like this article but that doesn't seem to work, unless I change the code to change html tags to html string with string.replace(/\</g, '<'), but then as you can image it breaks the grid bit time.

So anyway, I've worked quite a bit on this but I'm still waiting for you to contribute if you know how to fix some of them. For example the nonce-random-string for the dynamic style creation that you told me about, I think it should be refactored with something better, maybe something that the user can change as you mentioned (perhaps a grid option). I'm new to CSP, you should really contribute to help with this.

ghiscoding commented 1 year ago

Also saw on Ag-Grid that the minimal rules they use is including unsafe-inline, see their docs: https://www.ag-grid.com/javascript-data-grid/security/#summary

In summary, the minimal rule to load the grid is: <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src data:">

JesperJakobsenCIM commented 1 year ago

I'll see if I can find some time to contribute.

The issue of 'unsafe-inline' is widely discussed, and you can read more about it here: Can You Get Pwned with CSS?, authored by the creator of Security Headers.

Currently, the removal of 'unsafe-inline' from 'style-src' is a gradual process that many people are working towards. While it hasn't been widely exploited recently, it still poses a security risk. It's important to consider how the cybersecurity landscape is evolving. You can learn more about CSS injections in this OWASP wiki. OWASP also got a cheatsheet about CSP here

In the case of a grid-type package, the risk can be significant because it's challenging to ensure that every developer who uses it properly sanitizes data before adding it to the grid. Providing developers with the necessary tools is essential.

Ultimately, 'style-src' with 'unsafe-inline' may not be the most significant risk, but it's crucial to stay vigilant, given the evolving nature of cybersecurity threats.

ghiscoding commented 1 year ago

Sure but the thing that you might have missed is that SlickGrid is an Open Source project, we get no money from working on this and most of it come from our spare time which is why contributions is more than welcome. I understand the security risk but still this project is all but free in terms of usage and in terms of development, hence why contributions are even more important

JesperJakobsenCIM commented 1 year ago

Yeah, I know you don't have to worry. This is also a reason why it's an open-source project, so people can contribute to the project. You're doing great work, and I'm sorry if I sounded aggressive. For now, just take it easy; I'll look into the issues around the new Function. We have to remember this will take time to implement anyway.

6pac commented 1 year ago

Might be worth putting a performance benchmark against the compiled aggregates for comparison to any reworked method

ghiscoding commented 1 year ago

Might be worth putting a performance benchmark against the compiled aggregates for comparison to any reworked method

@6pac well the thing is that I don't even know how or what to replace these new Function with. If you know what to replace them with, please go ahead. I'm not expecting to work on this for a long while

JesperJakobsenCIM commented 1 year ago

How do i commit changes to #894? should be hitting the correct branch

fairly new in this apartment

ghiscoding commented 1 year ago

you need to Create a Fork like it suggested and you can't push to a PR since you're not the owner of this repo. The best we could do maybe is to maybe create a next branch and merge my PR there and then you could create PR to merge on the next branch

JesperJakobsenCIM commented 1 year ago

Okay, ill try that, the nonce commit is here

I'm not sure if any of you have experience with DataView, but it seems that changing FilterFn to a Function type doesn't break anything, and the functionality still works as expected. It may only require minor adjustments in other places. I'll need to experiment with it a bit more and also figure out how to conduct some performance benchmarks afterward.

protected compileFilter2(_items: TData[], _args: any): TData[] {
    //const filterInfo = this.getFunctionInfo(this.filter as Function);

    // const filterPath1 = () => { continue _coreloop; };
    // const filterPath2 = () => { _retval[_idx++] = $item$; continue _coreloop; };

    const _retval: TData[] = [];
    let $item$; 

    _coreloop: 
    for (let _i = 0, _il = _items.length; _i < _il; _i++) { 
      $item$ = _items[_i]; 
      if(this.filter){
          const exists = this.filter($item$, _args);
          if(exists){
            _retval.push($item$);
          }
      }
    } 

    return _retval;
  }

ghiscoding commented 1 year ago

Your code change looks pretty good, could you create a new PR?

To create a PR, you should have a screen similar to what is shown below, I made contributions to Vitest in the past and so I got the Vitest fork, from there I have the screen below and I can create a PR from there (my create PR is disabled because I have no change at the moment but yours should be enabled)

I'm not sure if any of you have experience with DataView, but it seems that changing FilterFn to a Function type doesn't break anything, and the functionality still works as expected. It may only require minor adjustments in other places. I'll need to experiment with it a bit more and also figure out how to conduct some performance benchmarks afterward.

I only use the DataView myself, it looks like you remove the dynamic functions entirely but again I'm pretty sure the SlickGrid author created these dynamic function so that they can be cache by the browser and reused for performance reason. However, these were created when IE6 was the big boy and was also super slow, we might not need them anymore since browser are much optimized but we would have to compare with a grid that has a large dataset like this Example Optimized DataView with 500,000 rows and also test with multiple filters (that demo ony has 1 filter), then maybe add some console.time() (see MDN) to show perf differences

JesperJakobsenCIM commented 1 year ago

Thanks, pull request send, yea definitely. At the moment, I'm mainly working on a working concept. It will require a bit more time to finalize the adjustments. I haven't touched the compileFilterWithCaching function yet, but it should receive the same rewrite.

JesperJakobsenCIM commented 1 year ago

You can check out this

it seems to be slightly slower however this is where i just changed the code to use the Function concept instead of new Function

old version 6.256932861328125 ms new version 9.81888134765625 ms numbers gotten from 1000 runs of the function with 500.000 rows Both on the non cached version slick.dataview.ts change protected useNewFilter = false; to true if you want to see the new version

Edit: abit faster after some minor optimization's here got new version down to 7.911023681640625 ms avg

Edit: Note I'm also doing these changes on the main branch so we shouldn't get the same issues we had with the nonce PR

ghiscoding commented 1 year ago

so it's 44% difference, so it is notable even if it's in ms. Perhaps we should keep both option in the code and a provide a flag to let the user chose for strict CSP or loose & faster mode. I again think that the new function does some kind of caching which is how it gets better perf. Your new approach is still not that bad though but yeah a flag might be the best approach

JesperJakobsenCIM commented 1 year ago

Super, I'll implement it with a flag (most of the setup is there already) and do some minor cleanup.

Question tho, how do you enable it to run the cached version of the calls 'compileFilterWithCachingNew' and 'compileFilterWithCaching' since i have no idea how to make it use them instead.

Would also still need to do some testing with the security header active since I don't know if how the CSP header reacts if the new Function is still present, hopefully it only blocks if you call the line with new Function.

About the concept with new Function caching i have no idea how it works and makes me wonder how new Function actually works especially with the setup, since i would have assumed that calling the function would be faster, since new Function adds some overhead.

ghiscoding commented 1 year ago

I don't know either how that works, I'm just assuming that it's caching the function because I don't see other reasons for why it creates dynamic function. By taking a look at the commits made by the original author, in this PR and this other PR with comments like below

DataView grouping perf Optimized DataView grouping by dynamically compiling the aggregator accumulation loop and inlining the accumulator function. This removes the function call overhead and considerably speeds up aggregate calculation for large numbers of items. One caveat is that aggregators now have to be self-contained and not reference any variables outside of the "this" scope.

This also makes me think that you should also test perf when using Grouping and Filters. Personally I never use the DataView inlineFilters option because in my libs (Slickgrid-Universal, Angular-Slickgrid, ...) I have my own set of Filters (text, multiple-select, date picker, ...) and inline filters doesn't work with that I think (I don't remember why exactly but anyway I never used that option). However the Grouping also has a dynamic function for the aggregation part. From the comment shown above, it seems that precompiling is what makes this speedier which I believe is kinda interconnected with cache

JesperJakobsenCIM commented 1 year ago

Ohh asked copilot why there was a difference between the 2 functions this was its answer

The compileFilter function is potentially faster than compileFilterNew because it dynamically generates a new function with the filter logic directly embedded in the loop. This can reduce the overhead of function calls.

In compileFilterNew, for each item in _items, there's a function call to this.filterNew. This function call has a cost, especially if _items is large.

In compileFilter, the filter logic is directly embedded into the loop, eliminating the need for a function call for each item. This is achieved by using new Function to create a new function with the filter logic directly in the code. This can potentially make the function faster, especially for large _items.

However, the actual performance can depend on many factors, including the JavaScript engine's ability to optimize function calls, the complexity of the filter logic, and the size of _items. It's always a good idea to measure performance in your specific use case.

This makes kinda sense tho it also tells me that if the dataset is even larger 1.000.000+ the compileFilterNew function would be even slower than compileFilter

JesperJakobsenCIM commented 1 year ago

What do you guys think about these changes #908 last two commits thought i could have split it up in 2 pull requests but github just added my new changes to already existing pull request...

tho this pull request contains useCSPSafeFilter as an option and the last inline css style fix (should maybe be an opt in too or just check if nonce is set) performance wise doesnt seem to be a huge impact did 500.000 rows and it took 0.05 extra ms

ghiscoding commented 1 year ago

I think we can now close this issue since multiple PRs were pushed in regards to CSP

884
891
894
902
904
905
908

I went ahead and merged my other PR and released a new version 5.5.0 which is enough I think to become CSP compliant. Thanks a lot for helping in making this happen 🚀

I also updated the readme homepage, please let me know if there's anything wrong or missing. Thanks

6pac / SlickGrid

Have you considered supporting CSP header? #878

884

891

894

902

904

905

908