feat: add basic html sanitizer w/regex to avoid xss scripting attack

[ghiscoding]

• Use a basic regex to prevent xss scripting attack with the following regex. The basic is to sanitize any html string being sent to innerHTML for example $el.innerHTML = '<div><script>alert('XSS')</script></div>

(\b)(on\S+)(\s*)=|javascript:([^>]*)[^>]*|(<\s*)(\/*)script([<>]*).*(<\s*)(\/*)script(>*)|(&lt;)(\/*)(script|script defer)(.*)(&gt;|&gt;">)

• Does it prevent every possible XSS attack?
  • No, a better solution is to use external libraries like DomPurify
• Does it help anyway?
  • Yes, it's certainly better than nothing (prior to this PR) ,it should cover over 75% of the cases, it's like wearing a mask or gloves to avoid catching the flu. It's not perfect but it's better than nothing 🥶
• for more info on XSS scripting attacks, you can read this article

TODOs

• test in all 3 SlickGrid libs that I maintain
• test in our Salesforce environment

For example, see below for what it covers and what gets replaced/removed (the top portion shows some common xss attack & the bottom portion shows the cleaner string after regex string replacement)

[ghiscoding]

@RasmusBergHogia you might be interested in this PR, it should help and it's simple

[ghiscoding]

@6pac I just finished testing in my 3rd and last lib, seems all good on my side, feel free to merge. That is also the last PR I wanted to do before a new release. Thanks mate

[syonfox]

ffc682b#commitcomment-61225015

This broke prod for me rolled back to 2.4.42.

this removes the onclick= part of html tags to sanitize stuff. I think it also prevented href='javascript:globalFunction()'
If I understand correctly this is to prevent js injection when unsanitized data is used in the formater ie

//in formatter 
return "<span onclick="myGoodCode()">"+ evil + "<span>";

evil could theoretical be a script tag ... we need to sanitize the variables we use before injecting them into html.
I dont think its the place of slick grid to also sanitize the html that the developer writes. perhaps it should only sanitize data before giving it to a formatter? and possibly offer both sanitized and plain options.

if this borked it for you and you are looking for a fix npm install slickgrid@2.4.42

[syonfox]

Is possibly go around this by use addEventListener on the element instead of use onclick/href="javascript..." on html-tag.

but then I have to look up the dom element when slick grid renders it and add the even and if slick girid destroys it for memory optimization the listeners would get wiped out. I though that was a relitivly clean solution... if anyone knows a nicer way let me know maybe.

This is what I am doing if you are curious it let me programmatically add action buttons to a row or any HTML thing. theoretical and get a callback i ran into a few issues trying to use addEventListener I think but dont remember details. this whole thing is in a dynamic jsPanel4 which sometimes messes with listeners too since the whole dom gets creates and destroyed.

    /**
     * We have to do it this way so the formatters have access to 'this' since js class objects are just normal functions :/
     */
    initFormatters() {

        this.actionFormatter = (row, cell, value, columnDef, dataContext) => {

            if (typeof this._actions == "undefined") {
                this._actions = [];
            }

            let html = ``;

            this._actions.forEach(item => {
                if (typeof item.formatter === 'function') {
                    html += item.formatter(item, dataContext, this, row, cell, value, columnDef)
                } else {
                    html += item.string.replace('$id', dataContext.id);
                }
            });

            // html += `<button onclick="_globalGrids[${this._id}].deleteByFileId('${dataContext.id}')" class="slick-icon-btn btn btn-danger btn-sm  float-right" title="Delete This Version">
            //                 <i class="fa fa-trash" aria-hidden="true"></i>
            //              </button>`
            //
            // html += `<button onclick="Files.downloadFile('${dataContext.id}')" class="slick-icon-btn btn btn-primary btn-sm  float-right" title="Download This Version">
            //                         <i class="fa fa-download" aria-hidden="true"></i>
            //                      </button>`
            // html += `<button onclick="_globalGrids[${this._id}].viewFile('${dataContext.id}', '${dataContext.file_name}')" class="slick-icon-btn btn btn-success btn-sm float-right" title="View This Version">
            //                         <i class="fa fa-eye" aria-hidden="true"></i>
            //                      </button>`

            return html;
        };
    }


    /**
     * add an action WARNING USE addActionButton or make sure string calls the fn you need
     * @param item - a full item or the css selector for the item
     * @param string - the html string for the item $id in this string will be replaced with the row.id in the formatter
     * @param fn - the fn to be executed on click
     */
    addAction(item, string, fn, formatter) {
        // this.formatter = formatter;

        if (typeof item == "object") {
            string = item.string || string;
            fn = item.fn || fn;
            formatter = item.formatter || formatter;
            item = item.selector;
        }

        if (typeof string == 'undefined') {
            throw Error("item.string not provided in object and string is undefined");
        }
        if (typeof fn == 'undefined') {
            throw Error("item.fn not provided in object and fn is undefined");
        }
        if (typeof item == 'undefined') {
            console.warn("No Item");
        }
        item = {
            selector: item,
            string: string,
            fn: fn,
            formatter: formatter
        }

        if (!this._actions) {//init
            this._actions = [];
        }

        this._actions.push(item);

        //this is so that the first few rows that have already rendered get updated with the new action
        if (this.grid) {// only if the grid is initialized
            this.grid.invalidateAllRows();
            this.grid.render();
        }

    }

    updateActionString(itemSelector, string) {
        let a = this._actions.find(a => a.selector == itemSelector)
        if (!a) {
            console.error("No Action found with selector: ", itemSelector);
            return;
        }

        a.string = string;

        if (this.grid) {// only if the grid is initialized
            this.grid.invalidateAllRows();
            this.grid.render();
        }
    }

    /**
     * This is convoluted but it works there is probably a better way
     * @param index
     * @param row_id
     * @private
     */
    _executeAction(index, row_id) {
        this._actions[index].fn(row_id, this);
    }

    /**
     * Add an action to the grid this will show up in the actions column
     * this is a utility extension of addAction
     * @param name - the name for a selector not really used
     * @param title - the tooltip
     * @param html - innerHtml for button
     * @param fn - fn(row_id, this) called onclick
     * @param options - optional options
     * @param {string} [options.colorClass='btn-info'] - a class for styling the button ususaly color
     * @param {function} [options.formatter] - a function to be called from the slick formatter see initFormatters()

     */
    addActionButton(name, title, html, fn, options) {

        console.debug("Action: add Action Button: ", name)
        options = options || {}
        options.colorClass = options.colorClass || 'btn-info'


        // options = bs.mergeDeep(defaults, options);

        if (!this._actions) this._actions = [];

        // `<span name="${name}" title="${title}" class="btn btn-sm btn-outline-dark">${html}</span>`,
        let actionIndex = this._actions.length;
        let item = {
            selector: `button[name=${name}]`,
            string: `<button name="${name}" onclick="_globalGrids[${this._id}]._executeAction(${actionIndex},'$id')" class="slick-icon-btn btn btn-sm float-right ${options.colorClass}" title="${title}">
                            ${html}
                         </button>`,
            fn: fn
        }
        if (typeof options.formatter == 'function') {
            item.formatter = options.formatter;
            console.warn("Action: FORMATTER: ", item.formatter)
        } else {
            console.warn("Action: NO FORMATTER: ", options.formatter)
        }

        this.addAction(item);
    }

[syonfox]

When we build elements in our formatters we make them by document.createElement(...) and then add event listner etc to element. At the end of formatter we just return element.outerHtml. Maybe it doesn't work for your project, but much cleaner way to do it and works with latest version.

Does that work? how is the event listener getting atached to the dom element slick grid constructs out of the outer html string.

//adding this into the addActionButton instead of just a string doesn't work 
        let btn = document.createElement('button');
        btn.name = name;
        btn.setAttribute('class', `slick-icon-btn btn btn-sm float-right ${options.colorClass}` );
        btn.setAttribute('title', title);
        btn.innerHTML = html;
        btn.addEventListener("click", e=>{
            console.log("WOW THIS WORKS:", e);
            this._executeAction(actionIndex,'$id')

        })
//... item.string = btn.outerHTML;

this doesn't work could it be that the js engine is caching the dome node when its constructed very closely in the formatted? do you have an example this seems like it shouldn't work.

... It seems the intended way is to add data attributes to your elements and then check if they were clicked on in the grid.onClick event... this doesn't let you have en event on input change. A solution might be to add event listeners when a cell gets clicked on? ... jquery events might magically work too.

[mcallegario]

I also went through the same problem you are having, I understand the importance of applying XSS, but I have many deployments using "onclick" in the content of the cells.
@ghiscoding , helped me a lot with the code suggestion:

He also made some adjustments to "disable" this option:
feat: add grid option to provide optional sanitizer function #657

For more information I suggest you see this topic:
ghiscoding/slickgrid-universal#548

[ghiscoding]

As I wrote on another post, I had another user saying the same thing, but the thing is that you're never supposed to put JS code in a formatter because that is an open door to XSS scripting attack, the correct and more secure way of dealing with events is the use of the SlickGrid exported events, for example the onClick SlickGrid event (you also get all the args for free doing it this way: column, dataContext, ...). When I created the PR to add the sanitizer, my thought was that everyone knew about this and were already using SlickGrid events and never adding JS code in Formatters (which again is risky), so I seem to have assumed wrong and I did not have any bad intention to break anyone's code but you should consider rewriting your code eventually.

On another note, I did another PR #657 to optionally override the sanitizer to avoid having too much breaking, which is merged, however we didn't release a new SlickGrid version yet.

The risk with JS code in Formatters is that a user with bad intention could take advantage of scripting attack by saving cell data that include bad JS code directly in the cell data that could end up being used because you have JS code in your formatter. Note that I never tested that technique but my main reason of creating the sanitizer PR was to prevent such attack (which I have never tested). in my mind it's better to play safe than say "sorry you got hacked..."

[ghiscoding]

Is possibly go around this by use addEventListener on the element instead of use onclick/href="javascript..." on html-tag.

but then I have to look up the dom element when slick grid renders it and add the even and if slick girid destroys it for memory optimization the listeners would get wiped out. I though that was a relitivly clean solution... if anyone knows a nicer way let me know maybe.

This is what I am doing if you are curious it let me programmatically add action buttons to a row or any HTML thing. theoretical and get a callback i ran into a few issues trying to use addEventListener I think but dont remember details. this whole thing is in a dynamic jsPanel4 which sometimes messes with listeners too since the whole dom gets creates and destroyed.

Since I see "Action" throughout your post, so perhaps you can use the Cell Menu plugin that I created which was created for that exact reason of having a way to create a nice popup menu with few different actions/commands.

[syonfox]

            this._actions.forEach(item => {
                if (typeof item.formatter === 'function') {
                    html += item.formatter(item, dataContext, this, row, cell, value, columnDef)
                } else {
                    html += item.string.replace('$id', dataContext.id);
                }
            });

never is a strong word in this case the only xss would be a malicious id. from the database so it should be safeish.
I agree its risky and I should sanitize(dataContext.id) or something. it is a built in feature that works pretty good when its needed. if im wrong and there's an attack vector im not considering lmk.

I wonder if there is a way for slick grid to expose this functionality.

the command api is almost there

SlickGrid/plugins/slick.contextmenu.js

Line 58 in 416992e

* commandItems: Array of Command item objects (command/title pair)

that is probably good enough thanks for that example I wish I saw it sooner :)

but somthing like this might be nice:

//collumdef or even grid wide.
events: [
{selector: 'name=[delete-btn]', event: 'mouseenter', fn: ()=>{alert(1)}}
]
//then internally 
//this could happen after scrolling stopped only as optimization or with onclick for most stuff
oncellcreation => columnDef.events.forEach(e=>{
  cell.container.querySelector(e.selector).addEventListener(e.event, e.fn)
})

then in my code actions are events and just need to be updated in the columDef when someone adds an action.

this could possibly be done by in the sanitize function replacing slick-event with onmouseover="addSlickEventsIfNotAdded()"
... overriding the onClick is probably all most people need though.

thanks for all the info lmk what you thing might spend some time on rewriting it later.

[ghiscoding]

never is a strong word

yeah maybe that word is too strong, a better word may be "it's preferable to use SlickGrid events"

but somthing like this might be nice:

I'm not sure what you mean by that code, where would that go and what would it do?

I'm not sure if it's related or not (I think it is) but what I've done in my lib is to wrap the grid onClick event see wrapper code here into a cell click event that I can then use directly into my column definitions like so:

this.columnDefinitions = [{ 
  id: 'firstName', /*....*/ 
  onCellClick: (event, args) => {
     const row, cell, dataView, columnDef, dataContext = args;
  }
}];

As for the sanitizer, you'll be able to able to override it so that should help. We just need a new release but I'd like to have another open PR to get through before doing a new release.

[6pac]

@ghiscoding let me know when you're ready for a release

[ghiscoding]

@6pac I'd like my other PR #659 to be merged first before a release, I haven't had a chance to tried what you proposed though

[ghiscoding]

Thanks to @6pac this is now released with version 2.4.44 on NPM, you can now override the sanitizer directly from the grid options

this.gridOptions = {
  sanitizer: function (dirtyHtml) { 
    return dirtyHtml; // return the same input string without sanitizing it

    // or try a different sanitizer that will let `onX` events get through but block any other JavaScript code
    // something like this maybe:
    // return dirtyHtml.replace(/javascript:([^>]*)[^>]*|(<\s*)(\/*)script([<>]*).*(<\s*)(\/*)script(>*)|(&lt;)(\/*)(script|script defer)(.*)(&gt;|&gt;">)/gi, '');
  },
  // ...
}

I still strongly suggest to eventually think about rewriting your code and use the more appropriate way of using SlickGrid events (like onClick) instead of adding JS code into a Formatter for all the reasons I wrote about in this thread.

Happy Coding 👨‍💻

[6pac]

@ghiscoding a heads up that there is also a new example 'example-drag-row-between-grids' that features a new row drag plugin designed to drag rows from one grid to another. I added this one directly to source in my repo. It's 98% the same as the other row drag plugin, just a few tweaks.