Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Issue 765 - Sanitize Id #766

Merged
merged 10 commits into from
May 5, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
4 changes: 2 additions & 2 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
pip install -r dev-requirements.txt --quiet
git clone --depth 1 [email protected]:plotly/dash.git dash-main
pip install -e ./dash-main[dev,testing] --quiet
cd dash-main/dash-renderer && npm run build && pip install -e . && cd ./../..
cd dash-main/dash-renderer && npm ci && npm run build && pip install -e . && cd ./../..

- run:
name: Build
Expand Down Expand Up @@ -215,7 +215,7 @@ jobs:
. venv/bin/activate
git clone --depth 1 [email protected]:plotly/dash.git dash-main
pip install -e ./dash-main[dev,testing] --quiet
cd dash-main/dash-renderer && npm run build && pip install -e . && cd ../..
cd dash-main/dash-renderer && npm ci && npm run build && pip install -e . && cd ../..

- run:
name: Install test requirements
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- [#724](https://github.com/plotly/dash-table/pull/724) Fix `active_cell` docstring: clarify optional nature of the `row_id` nested prop
- [#732](https://github.com/plotly/dash-table/pull/732) Fix a bug where opening a dropdown scrolled the table down its last row
- [#731](https://github.com/plotly/dash-table/pull/731) Fix a bug where `data=None` and `columns=None` caused the table to throw an error
- [#766](https://github.com/plotly/dash-table/pull/766) Sanitize table `id` for stylesheet injection (fixes usage with Pattern-Matching callbacks)

## Changed
- [#758](https://github.com/plotly/dash-table/pull/758) Improve error message for invalid filter queries
Expand Down
8 changes: 7 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
"@types/react-select": "^3.0.10",
"babel-loader": "^8.0.6",
"css-loader": "^3.4.2",
"css.escape": "^1.5.1",
"cypress": "^3.8.1",
"d3-format": "^1.4.3",
"es-check": "^5.1.0",
Expand Down
2 changes: 1 addition & 1 deletion src/dash-table/components/ControlledTable/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const MAX_WIDTH_ITERATIONS = 30;

export default class ControlledTable extends PureComponent<ControlledTableProps> {
private readonly menuRef = React.createRef<HTMLDivElement>();
private readonly stylesheet: Stylesheet = new Stylesheet(`#${this.props.id}`);
private readonly stylesheet: Stylesheet = new Stylesheet(`#${CSS.escape(this.props.id)}`);
private readonly tableFn = derivedTable(() => this.props);
private readonly tableFragments = derivedTableFragments();
private readonly tableStyle = derivedTableStyle();
Expand Down
2 changes: 2 additions & 0 deletions src/dash-table/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import 'css.escape'; // polyfill

import Environment from 'core/environment';
import Logger from 'core/Logger';

Expand Down
38 changes: 38 additions & 0 deletions tests/visual/percy-storybook/Css.percy.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import React from 'react';
import { storiesOf } from '@storybook/react';
import DataTable from 'dash-table/dash/DataTable';

const props = {
setProps: () => { },
data: [
{ a: 1, b: 2, c: 3 },
{ a: 2, b: 4, c: 6 },
{ a: 3, b: 6, c: 9 }
],
columns: [
{ id: 'a', name: 'A' },
{ id: 'b', name: 'B' },
{ id: 'c', name: 'C' }
],
css: [
{ selector: 'td', rule: 'background-color: red !important;' }
]
};

storiesOf('DashTable/CSS override', module)
.add('leading _ without letter', () => (<DataTable
{...props}
id={'_123'}
/>))
.add('leading number', () => (<DataTable
{...props}
id={'123'}
/>))
.add('escaped characters', () => (<DataTable
{...props}
id={'`~!@#$%^&*()=+ \\|/.,:;\'"`?[]<>{}'}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the fix, the table does not render and throws.

Copy link
Collaborator

Choose a reason for hiding this comment

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

leading to the developer screaming "~!@#$%^&*()=+ \|/.,:;'"`?[]<>{}"

/>))
.add('stringified object as id', () => (<DataTable
{...props}
id={{ id: 3, group: 'A' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW you're never going to actually get an object sent to the component by the renderer as an id - the renderer stringifies the id first, and does so as sorted JSON, so you'll get '{"group":"A","id":3}'

Does DataTable separately stringify ids? Because if I just call CSS.escape on an object it just gives "\[object\ Object\]"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does DataTable separately stringify ids?

Oups.
41981cb

/>));