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.

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

HTML injection vulnerability #913

Closed
theodorejb opened this issue May 19, 2016 · 10 comments
Closed

HTML injection vulnerability #913

theodorejb opened this issue May 19, 2016 · 10 comments

Comments

@theodorejb
Copy link

theodorejb commented May 19, 2016

I was very surprised to discover that ag-Grid does nothing to escape field values by default.

With a column definition like: {headerName: "Name", field: "name"} a user can enter a name such as <span onclick="alert('hacked!')">John Smith</span> and effectively initiate a cross-site scripting attack.

The only time field values should not be escaped is if there is a a custom cellRenderer for the column.

Related: #9 (comment)

@AmitMY
Copy link
Contributor

AmitMY commented Jun 7, 2016

+1

@theodorejb
Copy link
Author

Per Mozilla's documentation, node.textContent should be used instead of Element.innerHTML when inserting plain text.

@ceolter
Copy link
Contributor

ceolter commented Jun 8, 2016

ok so i've changed from:

this.eParentOfValue.innerHTML = valueToRender.toString();

to

this.eParentOfValue.textContent = valueToRender.toString();

is that all i need to do??

i'm afraid it will have an impact against some older browsers - and advice on that?

@ceolter ceolter closed this as completed Jun 8, 2016
@theodorejb
Copy link
Author

@ceolter Yes, that should be all you have to do.

i'm afraid it will have an impact against some older browsers

Only if you're trying to support IE8, which I don't think is the case based on #40.

Thanks!

@AmitMY
Copy link
Contributor

AmitMY commented Jun 8, 2016

I am not sure of the context of the code, so it may already be done.
If possible, only do this for 'field' columns. Columns that use cellRenderer may return buttons, or icons. (Example is fontawesome icons)

Don't know what about valueGetter...

@ceolter
Copy link
Contributor

ceolter commented Jun 8, 2016

it's only done when no cellRenderer is used.

@theodorejb
Copy link
Author

@AmitMY valueGetter values should be plain text (and thus inserted via textContent) since they are used for searching, filtering, and sorting.

@ceolter
Copy link
Contributor

ceolter commented Jun 9, 2016

valueGetter isn't used for rendering - the value is then either passed to cellRenderer or rendered by grid (which, as above, uses textContext now)

@kylecannon
Copy link

This is now back as a regression from the new rendering engine. @ceolter

@SebDez
Copy link

SebDez commented Dec 21, 2017

I fixed this in my project by setting a default cellRenderer for my grid :

defaultColDef: {
          cellRenderer: function (params) {
            var eDiv = document.createElement('div');
            eDiv.textContent = params.value;
            return eDiv;
          }
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants