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

DataGrid: Custom Formatter breaks on special characters #975

Closed
anhallbe opened this issue Jan 19, 2021 · 7 comments · Fixed by infor-design/enterprise#5343
Closed

DataGrid: Custom Formatter breaks on special characters #975

anhallbe opened this issue Jan 19, 2021 · 7 comments · Fixed by infor-design/enterprise#5343
Assignees
Labels
team: homepages Issues for the homepages team type: bug 🐛 [2] Velocity rating (Fibonacci)

Comments

@anhallbe
Copy link
Contributor

Describe the bug
A custom formatter breaks when it returns certain characters. We're not sure what's actually causing the issue, but it has to with inserting (escaped) HTML like ' onsomething=..... Example:

function formatterFunction(): string {
  return `
           <span title='&#x27; onx=y'>
            a
          </span>
  `;
}

To Reproduce
Steps to reproduce the behavior:

  1. Set up a datagrid with the following gridOptions:
    const formatter: SohoDataGridColumnFormatterFunction = (_row, _cell, _fieldValue, _col, rowData: any) => {
      if (rowData.value === "a") {
        return `
          <span title='&#x27; onx=y'>
            a
          </span>
          `;
      } else {
        return `
            <span title='&#x27; abc=y'>
              b
            </span>
        `;
      }
    }
    const columns: SohoDataGridColumn[] = [
      {
        id: "asd",
        name: "Asd",
        field: "",
        formatter,
      },
    ];
    this.gridOptions = {
      columns,
      dataset: [
        { value: "b" },
        { value: "b" },
        { value: "b" },
        { value: "a" },
        { value: "a" },
        { value: "a" },
      ],
      userObject: this,
      selectable: "single",
    }
  1. Hover over the different rows
  2. Hovering over a row with "b" correctly displays a tooltip ' abc=y
  3. Hovering over a row with "a" incorrectly displays HTML that is not part of the declared "title" attribute (see screenshot below), and rows after that are not correctly displayed at all.

Expected behavior
The tooltip (standard title attribute) should only display exactly what has been declared, regardless of whether it is escaped HTML or not.

Version

  • ids-enterprise-ng: 9.1

Screenshots
If applicable, add screenshots to help explain your problem.

datagrid-format.mp4

Platform

  • OS Version: Windows 10
  • Browser Name: Chrome

Additional context
Homepages issue: https://jira.infor.com/browse/LIME-6636

@tmcconechy tmcconechy added [2] Velocity rating (Fibonacci) team: homepages Issues for the homepages team type: bug 🐛 labels Jan 19, 2021
@tmcconechy
Copy link
Member

tmcconechy commented Jan 19, 2021

We had been trying to strip inline JS functions for security reasons. For example onclick ect... Is this really necessary to include onxxx functions, as i would be worried about removing that stripping?

Could you do it with JS instead?
What im unclear about is why you want to inject it from that issue?

Thanks

@anhallbe
Copy link
Contributor Author

Well the strings that are causing problems are user-defined. So it's not so much that we want them there, it's just that them being there are causing rendering problems.

So basically if User1 inserts a weird string that will be included in a data grid, then User2 will not be able to properly view the grid.

@tmcconechy
Copy link
Member

tmcconechy commented Jan 19, 2021

So it would be ok to make this:

           <span title='&#x27; onx=y'>
            a
          </span>

Into this?

           <span title='&#x27'>
            a
          </span>

I actually thought it was title='&#x27' onx=y ? So maybe this makes sense but not sure if we can ignore it without redoing all the xss stripping code.

Otherwise i guess we could make a less secure option to not strip these for a formatter/column? Otherwise Might be tricky?

@anhallbe
Copy link
Contributor Author

anhallbe commented Jan 19, 2021

I think the issue is that template that should be part of the datagrid is instead inserted in the title attribute in this case.

So the fact that xss stuff is stripped is not really an issue as far as I know. The issue is that certain characters/words seem to break the xss stripping. I haven't looked at the code that removes the js functions, but it looks like maybe the escaped ' character (&#x27;) is causing some quotation mismatch, but only when the string also contains e.g onclick?

@tmcconechy
Copy link
Member

OK i can look at that at the function and see, if thats the case it alleviates some concerns i have. Are you really using a custom formatter to do this?

If so a workaround might be to use the xss clean functions in the formatter to append the title to the html in a separate action to code around it, rather than a string with it all.

@anhallbe
Copy link
Contributor Author

Yeah a custom formatter may not be ideal. But I'm not aware other ways of achieving a UI like this without it?

image

This is the formatter, for more context:

	private doTheFormatting(item): string {
		const title = HtmlUtil.escapeStringForHtml(item.tooltip);
		const text = HtmlUtil.escapeStringForHtml(item.cellText);
		const template =
			`
			<div class="lm-truncate-text">
				<svg class="icon datagrid-alert-icon ${item.iconClass}"
					focusable="false" aria-hidden="true" role="presentation">
					<use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-${item.icon}"/>
				</svg>
				<p style="display:inline; margin-left: 5px;"
					title='${title}'>
					${text}
				</p>
			</div>
			`;
		return template;
	}

...where escapeStringForHtml escapes characters like <, >, ' and ".

@nganotice
Copy link

This has been QA tested and passed on v4.53.0-dev.

https://main-enterprise.demo.design.infor.com/components/datagrid/test-custom-formatter-special-char.html
Custom Formatter

Moving this ticket to Done.

Thanks!

@nganotice nganotice moved this from Ready for QA (beta) to Done in Enterprise 4.53.x (June 2021) Sprint Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team: homepages Issues for the homepages team type: bug 🐛 [2] Velocity rating (Fibonacci)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants