fix #418 Use of inline styles doesn't play nice with Content Security Policy #1976

shaurya-sisodia · 2022-06-13T10:59:09Z

Description

updating styles directly on HTMLELement style description rather than updating dynamic stylesheets
removing logic related to dynamic stylesheets as they aren't required and violate CSP strict style-src rule
removed UTs related to Dynamic stylesheets

Checklist

Created tests which fail without the change (if possible)
All tests passing (yarn test)
Extended the README / documentation, if necessary

…an updating dynamic stylesheets * removing logic related to dynamic stylesheets as they aren't required and violate CSP sctrict style-src rule * removed UTs related to Dynamic stylesheets

adumesny · 2022-06-14T15:09:48Z

src/gridstack.ts

@@ -776,7 +775,7 @@ export class GridStack {
    } else {
      this.el.parentNode.removeChild(this.el);
    }
-    this._removeStylesheet();
+    this._max = undefined;


no need, already undefined.

adumesny

thank you for looking into this. Have a few comments and I will need to pull it locally later to test as well.

adumesny · 2022-06-14T15:11:20Z

src/gridstack.ts

-  /** @internal */
-  protected _styles: GridCSSStyleSheet;
+  /** @internal max height of the gridstack*/
+  public _max: number;


does it need to be public ? if so '_' isn't the right prefix, and rename to curRow or something (need to see how it's used)

adumesny · 2022-06-14T15:13:33Z

src/gridstack.ts

@@ -240,6 +235,8 @@ export class GridStack {
  protected _extraDragRow = 0;
  /** @internal true if nested grid should get column count from our width */
  protected _autoColumn?: boolean;
+  /** @internal calculates height for a given row */
+  private _getHeight: (row: number) => string;


make this a proper TS method, not var you assign to function later.

adumesny · 2022-06-14T15:13:43Z

src/gridstack.ts

@@ -338,6 +335,8 @@ export class GridStack {

    this._setStaticClass();

+    this._getHeight = (rows: number) => (rows * (this.opts.cellHeight as number)) + this.opts.cellHeightUnit;


adumesny · 2022-06-14T15:17:50Z

src/gridstack.ts

-      this._styles._id = id;
-      this._styles._max = 0;
+    if (!this._max) {
+      this._max = 0;


doesn't prevent if (!this._max) {...} clause, so likely incorrect placement.

adumesny · 2022-06-14T15:22:35Z

src/gridstack.ts

    }

+    // apply layout styles on all widgets
+    const widgets: HTMLElement[] = Utils.getElements(prefix);
+    for (const widget of widgets) {


Utils.getElements(prefix).forEach(w => Utils.updatePositionStyleOnWidget(w, this._getHeight));

adumesny · 2022-06-14T15:24:29Z

src/utils.ts

-
-// list of fields we will skip during cloneDeep (nested objects, other internal)
-const skipFields = ['_isNested', 'el', 'grid', 'subGrid', 'engine'];
+ import { GridStackElement, GridStackNode, GridStackOptions, numberOrString, GridStackPosition, GridStackWidget } from './types';


indent all left so we can see actual changes...

* fixed indent in utils.ts * incorporated review comments

shaurya-sisodia · 2022-06-15T05:13:49Z

Hi @adumesny

Thank you so much for such quick feedback.

I have incorporated the review comments. I have a couple of questions. I have fixed the wrong indent that I committed.
Please have a look.

Regards,
Shaurya

adumesny · 2022-06-16T08:12:25Z

src/gridstack.ts

@@ -240,6 +235,8 @@ export class GridStack {
  protected _extraDragRow = 0;
  /** @internal true if nested grid should get column count from our width */
  protected _autoColumn?: boolean;
+  /** @internal calculates height for a given row */
+  private readonly _getHeight = (rows: number) => (rows * (this.opts.cellHeight as number)) + this.opts.cellHeightUnit;


/** @internal returns the CSS height for a given row */
private _getCSSHeight(rows: number): string {
return (rows * (this.opts.cellHeight as number)) + this.opts.cellHeightUnit;
}

and move it to the function area.

adumesny · 2022-06-16T08:15:26Z

src/gridstack.ts

-      this._styles._id = id;
-      this._styles._max = 0;
+    if (!this._maxRowIndex) {
+      this._maxRowIndex = 0;


again this doesn't seem right... set to actual value instead ?

adumesny · 2022-06-16T08:16:12Z

src/gridstack.ts

-    if (!this._styles) {
-      let id = 'gridstack-style-' + (Math.random() * 100000).toFixed();
-      // insert style to parent (instead of 'head' by default) to support WebComponent
-      let styleLocation = this.opts.styleInHead ? undefined : this.el.parentNode as HTMLElement;


styleInHead needs to be removed everywhere (doc and code) as it no longer applies

adumesny · 2022-06-16T08:17:44Z

src/gridstack.ts


      // these are done once only
-      Utils.addCSSRule(this._styles, prefix, `min-height: ${cellHeight}${cellHeightUnit}`);
+      Utils.updateStyleOnElements(prefix, {"min-height": cellHeight+cellHeightUnit});


we use ' (single) instead of " in TS code.

adumesny · 2022-06-16T08:20:13Z

src/gridstack.ts

      // content margins
      let top: string = this.opts.marginTop + this.opts.marginUnit;
      let bottom: string = this.opts.marginBottom + this.opts.marginUnit;
      let right: string = this.opts.marginRight + this.opts.marginUnit;
      let left: string = this.opts.marginLeft + this.opts.marginUnit;
      let content = `${prefix} > .grid-stack-item-content`;
      let placeholder = `.${this.opts._styleSheetClass} > .grid-stack-placeholder > .placeholder-content`;
-      Utils.addCSSRule(this._styles, content, `top: ${top}; right: ${right}; bottom: ${bottom}; left: ${left};`);
-      Utils.addCSSRule(this._styles, placeholder, `top: ${top}; right: ${right}; bottom: ${bottom}; left: ${left};`);
+      Utils.updateStyleOnElements(content, {top, right, bottom, left});


every single new element will also need those attributes, so how do you re-use this code since it isn't a css style anymore ?

adumesny · 2022-06-16T08:36:41Z

src/utils.ts

@@ -44,7 +44,7 @@ export function obsoleteAttr(el: HTMLElement, oldName: string, newName: string,
  if (oldAttr !== null) {
    el.setAttribute(newName, oldAttr);
    console.warn('gridstack.js: attribute `' + oldName + '`=' + oldAttr + ' is deprecated on this object in ' + rev + ' and has been replaced with `' +
-      newName + '`. It will be **completely** removed in v1.0');
+       newName + '`. It will be **completely** removed in v1.0');


adumesny · 2022-06-16T13:35:37Z

src/gridstack.ts

@@ -1305,6 +1280,7 @@ export class GridStack {
    if (n.y !== undefined && n.y !== null) { el.setAttribute('gs-y', String(n.y)); }
    if (n.w) { el.setAttribute('gs-w', String(n.w)); }
    if (n.h) { el.setAttribute('gs-h', String(n.h)); }
+    Utils.updatePositionStyleOnWidget(el, this._getHeight);


I would pass n as that is the source of truth, not the gs-*** attributes

adumesny · 2022-06-16T13:37:50Z

src/utils.ts

+    const minH= Utils.toNumber(el.getAttribute('gs-min-h'));
+    const maxH = Utils.toNumber(el.getAttribute('gs-max-h'));
+    const y = Utils.toNumber(el.getAttribute('gs-y'));
+    el.style.height = getHeight(h);


I would pass GridStackPosition instead.
and you are setting the height(s) and top only - what about left and widths ? seem one sided...

also I would not set properties (like min/max) that are not set

adumesny · 2022-06-16T13:40:18Z

src/utils.ts

-      parent.insertBefore(style, parent.firstChild);
+  /** update CSS style on elements with given selector */
+  static updateStyleOnElements(selector: string, styles: {[props: string]: string |  string[] }): void {
+    const elements = Utils.getElements(selector);


Utils.getElements(selector).forEach(el => Utils.addElementStyle(el, styles));

adumesny · 2022-06-16T13:48:47Z

src/utils.ts

-    let el = document.querySelector('STYLE[gs-style-id=' + id + ']');
-    if (el && el.parentNode) el.remove();
+  /** update CSS style on element */
+  static addElementStyle(el: HTMLElement, styles: {[props: string]: string |  string[] }): void {


if (!(styles instanceof Object)) return; // not sure this even needed if (!styles) is enough
for (const key in styles) {
if (!styles.hasOwnProperty(key)) return;
const style = styles[key];
if (Array.isArray(style)) { // why do we need this ?
style.forEach(val => el.style[key] = val);
} else {
el.style[key] = style;
}
}

adumesny

also your code will update all the elements styles when one of them changes, which will be very inefficient (prev code only did incremental stylesheet update as maxRow was reached).

without deleting gridstack.css (or at least any widget location/size info) and gridstack-extra.css you can't tell if you have handled all cases (I doubt it). Are you ONLY removing JS code that create inline styles, but still allowing static styles sheets (with dynamic class setting). I don't know enough about CSP to tell how far we would need to go...

this will need very different architecture.

adumesny · 2022-06-16T14:45:22Z

I tried demo/two.html and your branch is pretty broken as you resize items around...

shaurya-sisodia · 2022-06-17T10:43:19Z

Hi @adumesny

CSP strict style rule prohibits the following:

Inserting rule using CSSStyleSheet.insertRule() or CSSStyleSheet.addRule()
Appending the an element with inline style in the dom: Eg- El.appendChild(<div style="width:100px"></div>);
Appending dynamically generated CSSStyleSheets into the DOM.

That's why I had to change the implementation such that we set styles on each element.
In my architecture, we have to update Styles on each element because we have to take into account that the position on multiple elements might have changed too besides the CSS style calculation itself.

We can use the old architecture and still make it CSP compliant for all browsers except SAFARI.
As written here in the following issue:
I have come across replaceSync accepts inline CSS which is not CSP compliant · Issue #98 · WICG/construct-stylesheets (github.com)

We can use CSSStyleSheet.replaceSync() instead of insertRule() to update styles on dynamic style sheet. We can also instead of appending this dynamic stylesheet can use
document.adoptedStyleSheets = document.adoptedStyleSheets.concat(sheet);

We can use old architecture with adoptedStyleSheets and replaceSync is all the browsers and new architecture in case of SAFARI. Or ask applications to use precalculated stylesheets in case of safari similar to IE 8 Support.

adumesny · 2022-06-17T11:13:25Z

so we have a mix of css rules + classes for thing that don't change (eg column based layout) but attributes for height stuff...nthat would satisfy CSP but looks odd. Also we do NOT need to update all elements as we know when a node is dirty and needs to be updated (we already track that) so the call to generate styles need to taken out as it was done only of we added rows (or cellHeight/margin changed). Your logic isn't correct.

also keep in m id you changed break some of the demos so there are issues there - just playing with two.html shows issues dragging/resizing around. https://gridstackjs.com/demo/two.html is fine while your version using demo/two.html is not

shaurya-sisodia · 2022-06-17T11:24:03Z

Hi @adumesny

I am aware of the demos breaking and have fixed the changes locally and will commit shortly.

Also so we can use the architecture but update styles on elements only when they are dirty?

shaurya-sisodia · 2022-06-19T19:05:45Z

Hi @adumesny

I have fixed the code and incorporated the review you sent. The demo is also working perfectly. We are updating styles on dirty nodes only.

Can you please review the architecture? If you feel it's risky I will look to accommodate the risky part.

The proposed architecture is:

To update any dynamically calculated styles directly on elements. These are height, min-height, max-height, and top attributes on widgets. And position attributes top, left, right, and bottom on widget content and resizable icon.
We update these on styling on all dirty nodes in _updateStyles method. And when widget is added or dropped into the gridstack.
We are still using the gs-** attributes as we still need all static stylesheets like before.

I have fixed the demo it was breaking before because I wasn't updating the position styling on widget content and resizable icon when they are dropped into the grid. But now it all should be fixed.

Please let me know your concerns. Thanks for putting your time into this.

Regards

…sn't-play-nice-with-Content-Security-Policy

…nc to insert css rules * safari doesn't support contructable stylesheets and replaceSync yet so we are updating styles directly on elements to make gridstack csp compliant

…sn't-play-nice-with-Content-Security-Policy

adumesny · 2023-05-01T02:59:14Z

this is likely not needed (and quite a risky change) given nonce support we now have.
#2229

I would like to close it - we can always get back to it from 'won't fix' issue listed.

* updating styles directly on HTMLELement style description rather th…

170ebba

…an updating dynamic stylesheets * removing logic related to dynamic stylesheets as they aren't required and violate CSP sctrict style-src rule * removed UTs related to Dynamic stylesheets

adumesny reviewed Jun 14, 2022

View reviewed changes

shaurya-sisodia added 2 commits June 15, 2022 10:32

review fix

168b27a

* fixed indent in utils.ts * incorporated review comments

indent fix

0d01844

adumesny reviewed Jun 16, 2022

View reviewed changes

incorporating review fix and fix bugs breaking the demo

8545cc1

shaurya-sisodia and others added 4 commits June 20, 2022 08:55

Merge branch 'master' into fix-gridstack#418-Use-of-inline-styles-doe…

b89bee7

…sn't-play-nice-with-Content-Security-Policy

Some additional clean up

031b3ba

* using adoptedStyleSheets to create dynamic stylesheet and replaceSy…

a2e3318

…nc to insert css rules * safari doesn't support contructable stylesheets and replaceSync yet so we are updating styles directly on elements to make gridstack csp compliant

Merge branch 'master' into fix-gridstack#418-Use-of-inline-styles-doe…

e3a032e

…sn't-play-nice-with-Content-Security-Policy

adumesny closed this May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #418 Use of inline styles doesn't play nice with Content Security Policy #1976

fix #418 Use of inline styles doesn't play nice with Content Security Policy #1976

shaurya-sisodia commented Jun 13, 2022 •

edited

Loading

adumesny Jun 14, 2022

adumesny left a comment

adumesny Jun 14, 2022

adumesny Jun 14, 2022

adumesny Jun 14, 2022

adumesny Jun 14, 2022

adumesny Jun 14, 2022

adumesny Jun 14, 2022

shaurya-sisodia commented Jun 15, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny Jun 16, 2022

adumesny left a comment •

edited

Loading

adumesny commented Jun 16, 2022 •

edited

Loading

shaurya-sisodia commented Jun 17, 2022

adumesny commented Jun 17, 2022

shaurya-sisodia commented Jun 17, 2022

shaurya-sisodia commented Jun 19, 2022 •

edited

Loading

adumesny commented May 1, 2023

		@@ -338,6 +335,8 @@ export class GridStack {

		this._setStaticClass();

		this._getHeight = (rows: number) => (rows * (this.opts.cellHeight as number)) + this.opts.cellHeightUnit;

fix #418 Use of inline styles doesn't play nice with Content Security Policy #1976

fix #418 Use of inline styles doesn't play nice with Content Security Policy #1976

Conversation

shaurya-sisodia commented Jun 13, 2022 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

adumesny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaurya-sisodia commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adumesny left a comment • edited Loading

Choose a reason for hiding this comment

adumesny commented Jun 16, 2022 • edited Loading

shaurya-sisodia commented Jun 17, 2022

adumesny commented Jun 17, 2022

shaurya-sisodia commented Jun 17, 2022

shaurya-sisodia commented Jun 19, 2022 • edited Loading

adumesny commented May 1, 2023

shaurya-sisodia commented Jun 13, 2022 •

edited

Loading

adumesny left a comment •

edited

Loading

adumesny commented Jun 16, 2022 •

edited

Loading

shaurya-sisodia commented Jun 19, 2022 •

edited

Loading