Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add CSP safe option for DataView filtering and adjusting inline css for CSP #908

Merged
merged 12 commits into from
Nov 13, 2023
Merged

fix: add CSP safe option for DataView filtering and adjusting inline css for CSP #908

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
990d17d
V1 of change, big example, old method 5-8 ms vs new is 8-11 ms
JesperJakobsenCIM Nov 3, 2023
b9cd46c
after some minor optimizations on the 2 functions
JesperJakobsenCIM Nov 3, 2023
b70eee8
added 'useCSPSafeFilter' to dataview options
JesperJakobsenCIM Nov 6, 2023
9d37139
Changed the last inline style to get applied properly
JesperJakobsenCIM Nov 6, 2023
ad54e65
Adjusted test cases in for CSP safe style tag
JesperJakobsenCIM Nov 6, 2023
d7f2b4b
Changed to check if undefined instead
JesperJakobsenCIM Nov 7, 2023
0fd72a4
added semicolons
JesperJakobsenCIM Nov 7, 2023
113b613
Added option for html rendering instead of dynamic string rendering
JesperJakobsenCIM Nov 8, 2023
7cae5a2
Changed the check to if the node is not set
JesperJakobsenCIM Nov 9, 2023
966c804
fixed indentation
JesperJakobsenCIM Nov 9, 2023
99f0443
removed old method
JesperJakobsenCIM Nov 13, 2023
22ad66a
renamed stringArray to divArray
JesperJakobsenCIM Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 107 additions & 52 deletions src/slick.grid.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3395,9 +3395,9 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
}

if (this.options.mixinDefaults) {
Utils.applyDefaults(m, this._columnDefaults);
if (!m.autoSize) { m.autoSize = {}; }
Utils.applyDefaults(m.autoSize, this._columnAutosizeDefaults);
Utils.applyDefaults(m, this._columnDefaults);
if (!m.autoSize) { m.autoSize = {}; }
Utils.applyDefaults(m.autoSize, this._columnAutosizeDefaults);
} else {
m = this.columns[i] = Utils.extend({}, this._columnDefaults, m);
m.autoSize = Utils.extend({}, this._columnAutosizeDefaults, m.autoSize);
Expand Down Expand Up @@ -3507,7 +3507,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
this.makeActiveCellNormal();
}

protected internal_setOptions(suppressRender?: boolean, suppressColumnSet?: boolean, suppressSetOverflow?: boolean) : void {
protected internal_setOptions(suppressRender?: boolean, suppressColumnSet?: boolean, suppressSetOverflow?: boolean): void {
if (this._options.showColumnHeader !== undefined) {
this.setColumnHeaderVisibility(this._options.showColumnHeader);
}
Expand Down Expand Up @@ -3785,7 +3785,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
return item[columnDef.field as keyof TData];
}

protected appendRowHtml(stringArrayL: string[], stringArrayR: string[], row: number, range: CellViewportRange, dataLength: number) {
protected appendRowHtml(stringArrayL: HTMLElement[] | string[], stringArrayR: HTMLElement[] | string[], row: number, range: CellViewportRange, dataLength: number) {
const d = this.getDataItem(row);
const dataLoading = row < dataLength && !d;
let rowCss = 'slick-row' +
Expand All @@ -3806,14 +3806,28 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e

const frozenRowOffset = this.getFrozenRowOffset(row);

const rowHtml = `<div class="ui-widget-content ${rowCss}" data-top="${(this.getRowTop(row) - frozenRowOffset)}px">`; //style="top:${(this.getRowTop(row) - frozenRowOffset)}px"
const rowDiv = document.createElement('div');
let rowDivR: HTMLElement | undefined;
if (!this._options.nonce) {
const rowHtml = `<div class="ui-widget-content ${rowCss}" style="top: ${(this.getRowTop(row) - frozenRowOffset)}px;">`;
(stringArrayL as string[]).push(rowHtml);

stringArrayL.push(rowHtml);

if (this.hasFrozenColumns()) {
stringArrayR.push(rowHtml);
if (this.hasFrozenColumns()) {
(stringArrayR as string[]).push(rowHtml);
}
} else {
rowDiv.className = 'ui-widget-content ' + rowCss;
rowDiv.style.top = `${(this.getRowTop(row) - frozenRowOffset)}px`;
(stringArrayL as HTMLElement[]).push(rowDiv);
if (this.hasFrozenColumns()) {
//it has to be a deep copy otherwise we will have issues with pass by reference in js since
//attempting to add the same element to 2 different arrays will just move 1 item to the other array
rowDivR = rowDiv.cloneNode(true) as HTMLElement;
ghiscoding marked this conversation as resolved.
Show resolved Hide resolved
(stringArrayR as HTMLElement[]).push(rowDivR);
}
}


let colspan: number | string;
let m: C;
for (let i = 0, ii = this.columns.length; i < ii; i++) {
Expand All @@ -3829,6 +3843,15 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
}
}

let tmpRow: HTMLElement | string[] | undefined;
let tmpRowR: HTMLElement | string[] | undefined;
if (this._options.nonce) {
tmpRow = rowDiv;
tmpRowR = rowDivR;
} else {
tmpRow = (stringArrayL as string[]);
tmpRowR = (stringArrayR as string[]);
}
// 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) {
Expand All @@ -3837,27 +3860,29 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
}

if (this.hasFrozenColumns() && (i > this._options.frozenColumn!)) {
this.appendCellHtml(stringArrayR, row, i, (colspan as number), d);
//could add it as a check in the if the !
this.appendCellHtml(tmpRowR!, row, i, (colspan as number), d);
} else {
this.appendCellHtml(stringArrayL, row, i, (colspan as number), d);
this.appendCellHtml(tmpRow, 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);
this.appendCellHtml(tmpRow, row, i, (colspan as number), d);
}

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

stringArrayL.push('</div>');
if(!this._options.nonce){
(stringArrayL as string[]).push('</div>');

if (this.hasFrozenColumns()) {
stringArrayR.push('</div>');
(stringArrayR as string[]).push('</div>');
}
JesperJakobsenCIM marked this conversation as resolved.
Show resolved Hide resolved
}
}

protected appendCellHtml(stringArray: string[], row: number, cell: number, colspan: number, item: TData) {
protected appendCellHtml(stringArray: HTMLElement | string[], row: number, cell: number, colspan: number, item: TData) {
// stringArray: stringBuilder containing the HTML parts
// row, cell: row and column index
// colspan: HTML colspan
Expand Down Expand Up @@ -3899,25 +3924,50 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
if ((formatterResult as FormatterResultObject)?.addClasses) {
addlCssClasses += (addlCssClasses ? ' ' : '') + (formatterResult as FormatterResultObject).addClasses;
}
const toolTip = (formatterResult as FormatterResultObject)?.toolTip ? `title="${(formatterResult as FormatterResultObject).toolTip}"` : '';

let customAttrStr = '';
if (m.hasOwnProperty('cellAttrs') && m.cellAttrs instanceof Object) {
for (const key in m.cellAttrs) {
if (m.cellAttrs.hasOwnProperty(key)) {
customAttrStr += ` ${key}="${m.cellAttrs[key]}" `;
if (this._options.nonce) {
ghiscoding marked this conversation as resolved.
Show resolved Hide resolved
const toolTipText = (formatterResult as FormatterResultObject)?.toolTip ? `${(formatterResult as FormatterResultObject).toolTip}` : '';
const cellDiv = document.createElement('div');
cellDiv.className = cellCss + (addlCssClasses ? ' ' + addlCssClasses : '');
cellDiv.setAttribute('title', toolTipText);
if (m.hasOwnProperty('cellAttrs') && m.cellAttrs instanceof Object) {
for (const key in m.cellAttrs) {
if (m.cellAttrs.hasOwnProperty(key)) {
cellDiv.setAttribute(key, m.cellAttrs[key]);
}
}
}
}

stringArray.push(`<div class="${cellCss + (addlCssClasses ? ' ' + addlCssClasses : '')}" ${toolTip + customAttrStr}>`);
// if there is a corresponding row (if not, this is the Add New row or this data hasn't been loaded yet)
if (item) {
const obj = (Object.prototype.toString.call(formatterResult) !== '[object Object]' ? formatterResult : (formatterResult as FormatterResultObject).text) as string;
cellDiv.innerHTML = this.sanitizeHtmlString(obj);
}

// if there is a corresponding row (if not, this is the Add New row or this data hasn't been loaded yet)
if (item) {
stringArray.push((Object.prototype.toString.call(formatterResult) !== '[object Object]' ? formatterResult : (formatterResult as FormatterResultObject).text) as string);
(stringArray as HTMLElement).appendChild(cellDiv);
} else {
const toolTip = (formatterResult as FormatterResultObject)?.toolTip ? `title="${(formatterResult as FormatterResultObject).toolTip}"` : '';

let customAttrStr = '';
if (m.hasOwnProperty('cellAttrs') && m.cellAttrs instanceof Object) {
for (const key in m.cellAttrs) {
if (m.cellAttrs.hasOwnProperty(key)) {
customAttrStr += ` ${key}="${m.cellAttrs[key]}" `;
}
}
}

(stringArray as string[]).push(`<div class="${cellCss + (addlCssClasses ? ' ' + addlCssClasses : '')}" ${toolTip + customAttrStr}>`);

// if there is a corresponding row (if not, this is the Add New row or this data hasn't been loaded yet)
if (item) {
(stringArray as string[]).push((Object.prototype.toString.call(formatterResult) !== '[object Object]' ? formatterResult : (formatterResult as FormatterResultObject).text) as string);
}

(stringArray as string[]).push('</div>');
}

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


this.rowsCache[row].cellRenderQueue.push(cell);
this.rowsCache[row].cellColSpans[cell] = colspan;
Expand Down Expand Up @@ -4526,7 +4576,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e

protected cleanUpAndRenderCells(range: CellViewportRange) {
let cacheEntry;
const stringArray: string[] = [];
const stringArray: HTMLElement | string[] = this._options.nonce ? document.createElement('div') : [];
const processedRows: number[] = [];
let cellsAdded: number;
let totalCellsAdded = 0;
Expand Down Expand Up @@ -4589,13 +4639,19 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
processedRows.push(row);
}
}
if (this._options.nonce) {
if (!(stringArray as HTMLElement).children.length) {
return;
}
} else {
if (!(stringArray as string[]).length) {
return;
}

if (!stringArray.length) {
return;
}

const x = document.createElement('div');
x.innerHTML = this.sanitizeHtmlString(stringArray.join(''));
x.innerHTML = this.sanitizeHtmlString(this._options.nonce ? (stringArray as HTMLElement).outerHTML : (stringArray as string[]).join(''));

let processedRow: number | null | undefined;
let node: HTMLElement;
Expand All @@ -4605,6 +4661,10 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
while (Utils.isDefined(columnIdx = cacheEntry.cellRenderQueue.pop())) {
node = x.lastChild as HTMLElement;

//no idea why node would be null here but apparently it is..
if (!node && this._options.nonce) {
ghiscoding marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
if (this.hasFrozenColumns() && (columnIdx > this._options.frozenColumn!)) {
cacheEntry.rowNode![1].appendChild(node);
} else {
Expand All @@ -4616,8 +4676,8 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
}

protected renderRows(range: { top: number; bottom: number; leftPx: number; rightPx: number; }) {
const stringArrayL: string[] = [];
const stringArrayR: string[] = [];
const stringArrayL: HTMLElement[] | string[] = [];
const stringArrayR: HTMLElement[] | string[] = [];
const rows: number[] = [];
let needToReselectCell = false;
const dataLength = this.getDataLength();
Expand Down Expand Up @@ -4658,15 +4718,18 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e

const x = document.createElement('div');
const xRight = document.createElement('div');
x.innerHTML = this.sanitizeHtmlString(stringArrayL.join(''));
xRight.innerHTML = this.sanitizeHtmlString(stringArrayR.join(''));
console.time("applying css");
const elements1 = x.querySelectorAll('[data-top]') as NodeListOf<HTMLElement>;
const elements2 = xRight.querySelectorAll('[data-top]') as NodeListOf<HTMLElement>;
this.applyTopStyling(elements1);
this.applyTopStyling(elements2);
console.timeEnd("applying css");

if (this._options.nonce) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as before I don't want to keep 2 implementations to support unless there's a big perf hit which I don't think there is from previous discussions. Have you tried the autoHeight grid yet?

Copy link
Contributor Author

@JesperJakobsenCIM JesperJakobsenCIM Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting tried to add autoHeight: true with 50.000 rows
tho with nonce seems to be faster than the non nonce version, around 1/3 faster sometimes 1/2 faster

ex a search without nonce took 400ms where with nonce it took 200ms

Initial load took 1500 ms without nonce and 1000 ms with nonce (first execution)
second run took 1300 ms without nonce down to 900 with nonce (second execution)

not exactly sure why on initial load it gets called twice tho this might be a concern (not sure current prod does this, havent checked)

Edit:
Hmm interesting the dataView code even on prod requests renderRows twice.

Copy link
Collaborator

@ghiscoding ghiscoding Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I can see that the double doesn't make much difference 1 vs 2ms isn't a big deal I guess, but when you add them up in the autoHeight, 400ms is definitely noticeable. I'm not sure what to do at this point but I feel the worst case should always be considered before making a decision and autoHeight is definitely the worst case usage I assume. If we can find a way to remove that double call of renderRows then perhaps it might be more acceptable to go with the nonce (however that won't help the autoHeight example since it doesn't the DataView in that demo). I'm not sure what to go with at this point... I feel we might have to keep both approach but I'm not too keen about it

Copy link
Contributor Author

@JesperJakobsenCIM JesperJakobsenCIM Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might have misunderstood my text, new code is faster than the old version
first set of times is without nonce so old code and second time is with nonce so new code

Copy link
Collaborator

@ghiscoding ghiscoding Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I got them reversed indeed, if that's the case then yeah let's keep only new code (with nonce) since that will help with CSP compliance. We can also merge my other PR #894 and I think we'll be close enough to be CSP compliant. So I assume you will push the new code change, it's not in the PR yet right?

Hmm interesting the dataView code even on prod requests renderRows twice.

I'm still curious to see if you can find why it does that, but I think that could, and probably should, be a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've commited the cleanup now and everything as far as i know should be in this PR

stringArrayL.forEach(elm => {
x.appendChild(elm as HTMLElement);
});
stringArrayR.forEach(elm => {
xRight.appendChild(elm as HTMLElement);
});
}
else {
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()) {
Expand Down Expand Up @@ -4699,14 +4762,6 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
this.activeCellNode = this.getCellNode(this.activeRow, this.activeCell);
}
}
protected applyTopStyling(elements: NodeListOf<HTMLElement>) {
elements?.forEach((element: HTMLElement) => {
const top = element.dataset.top;
if (top !== undefined) {
element.style.top = top;
}
});
}

protected startPostProcessing() {
if (!this._options.enableAsyncPostRender) {
Expand Down