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 sanitizer for descriptions. #2785

Merged
merged 32 commits into from
Aug 3, 2021
Merged

Conversation

zerline
Copy link
Contributor

@zerline zerline commented Feb 16, 2020

An HTML sanitizer for descriptions. On the Python side: that is, we sanitize as early as possible. We may have to think about jslinks, though. Should address issue #2636

@zerline zerline marked this pull request as ready for review February 17, 2020 09:25
@jasongrout jasongrout added this to the 8.0 milestone Mar 30, 2020
@SylvainCorlay
Copy link
Member

Should we sanitize this on the HTML side using a known package? JupyterLab does that.

Note: someone could JSLink this attribute which would not go through the validation.

Jason idea: make the sanitizer part of the manager, so that the environment can provide it. In the base manager, this could be a pass-through, and in the case of JupyterLab, we could use the lab sanitizer.

@jasongrout jasongrout self-requested a review May 11, 2020 15:46
@zerline
Copy link
Contributor Author

zerline commented May 29, 2020

A new proposal for an inline sanitizer:
(in jupyterlab/packages/apputils/src/sanitizer.js)

class InlineSanitizer implements ISanitizer {

  /**
   * Sanitize an HTML string.
   *
   * @param dirty - The dirty text.
   *
   * @param options - The optional sanitization options.
   *
   * @returns The sanitized string.
   */
  sanitize(dirty: string, options?: ISanitizer.IOptions): string {
    return sanitize(dirty, { ...this._options, ...(options || {}) });
  }

  private _options: sanitize.IOptions = {
    // Note: <script> content is already stripped by sanitize-html
    // HTML tags that are allowed to be used in our labels or descriptions.
    allowedTags: ['a', 'abbr', 'b', 'blockquote', 'code', 'em', 'i', 'img', 'li', 'ol', 'strong', 'style', 'svg', 'ul'],
    // Attributes that HTML tags are allowed to have in our labels or descriptions
    allowedAttributes: {
      '*': ['alt', 'aria-*', 'class', 'id', 'lang', 'name', 'style', 'title', 'translate'],
      a: ['href', 'hreflang', 'tabindex'],
      img: ['src','usemap'],
      li: ['value'],
      ol: ['reversed', 'start'],
      style: ['media'],
      svg: ['*']
    },
    // Inline CSS styles that HTML tags may have (and their allowed values)
    // allowedStyles: {
      // To simplify the data, all styles are allowed on all tags that allow the style attribute
     // '*': {}};
    transformTags: {
      // Set the "rel" attribute for <a> tags to "nofollow".
      a: sanitize.simpleTransform('a', { rel: 'nofollow' }),
    }
  };
}

@zerline
Copy link
Contributor Author

zerline commented May 29, 2020

Of course, the actual _options is matter of discussion!

@jasongrout
Copy link
Member

I think for now, let's keep the inline sanitizer in the ipywidgets codebase until there is a wider need for it in jlab.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Jun 8, 2020

TODO:

  • check interaction with mathjax?
  • sanitize the content of $...$
    Check where this happens in jupyterlab's notebook.

@vidartf
Copy link
Member

vidartf commented Jun 8, 2020

$ y < a, x > a$

@SylvainCorlay
Copy link
Member

Hey @blois, you may be interested in this addresses Issue #2636, which you opened.

Would you have a bit of time to look at this before we merge?

We have added this PR to the 8.0 milestone.

@davidbrochart
Copy link
Member

@zerline I wanted to try out your PR but I get the following error, any idea?

$ bash dev-install.sh 
Checking yarn... 1.22.10
Checking pip... pip 21.0.1 from /home/david/mambaforge/envs/ipywidgets-dev/lib/python3.9/site-packages/pip (python 3.9)
Checking jupyter lab... 3.0.7
yarn install v1.22.10
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
info [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
info [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[4/5] Linking dependencies...
warning "@typescript-eslint/eslint-plugin > [email protected]" has unmet peer dependency "typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta".
warning "workspace-aggregator-efb47fc1-ff0a-4819-bee3-3af235de72f9 > @jupyter-widgets/controls > [email protected]" has unmet peer dependency "caniuse-lite@^1.0.30000697".
warning "workspace-aggregator-efb47fc1-ff0a-4819-bee3-3af235de72f9 > @jupyter-widgets/jupyterlab-manager > @jupyterlab/application > @jupyterlab/[email protected]" has unmet peer dependency "react@~16.9.0".
[5/5] Building fresh packages...
success Saved lockfile.
Done in 7.68s.
yarn run v1.22.10
$ lerna run build --ignore "@jupyter-widgets/example-*"
lerna notice cli v3.20.2
lerna info versioning independent
lerna notice filter excluding "@jupyter-widgets/example-*"
lerna info filter [ '!@jupyter-widgets/example-*' ]
lerna info Executing command in 7 packages: "yarn run build"
lerna ERR! yarn run build exited 1 in '@jupyter-widgets/base'
lerna ERR! yarn run build stdout:
$ npm run build:src

> @jupyter-widgets/[email protected] build:src
> tsc --build

../../node_modules/dom-serializer/lib/index.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/dom-serializer/lib/index.d.ts(1,27): error TS1005: ';' expected.
../../node_modules/domutils/lib/helpers.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/domutils/lib/helpers.d.ts(1,27): error TS1005: ';' expected.
../../node_modules/domutils/lib/legacy.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/domutils/lib/legacy.d.ts(1,36): error TS1005: ';' expected.
../../node_modules/domutils/lib/manipulation.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/domutils/lib/manipulation.d.ts(1,36): error TS1005: ';' expected.
../../node_modules/domutils/lib/querying.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/domutils/lib/querying.d.ts(1,36): error TS1005: ';' expected.
../../node_modules/domutils/lib/stringify.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/domutils/lib/stringify.d.ts(1,27): error TS1005: ';' expected.
../../node_modules/domutils/lib/tagtypes.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/domutils/lib/tagtypes.d.ts(1,64): error TS1005: ';' expected.
../../node_modules/domutils/lib/traversal.d.ts(1,13): error TS1005: '=' expected.
../../node_modules/domutils/lib/traversal.d.ts(1,54): error TS1005: ';' expected.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

lerna ERR! yarn run build stderr:
npm ERR! code 1
npm ERR! path /home/david/github/davidbrochart/ipywidgets/packages/base
npm ERR! command failed
npm ERR! command sh -c tsc --build

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/david/.npm/_logs/2021-02-16T21_11_34_450Z-debug.log
error Command failed with exit code 1.

lerna ERR! yarn run build exited 1 in '@jupyter-widgets/base'
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@davidbrochart
Copy link
Member

@zerline I was wondering how the logic in latex.ts compares to https://github.com/vberlier/extract-math, could we rely on the later instead of maintaining our own parsing, or do we have other requirements?

Copy link
Contributor

@blois blois left a comment

Choose a reason for hiding this comment

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

Really excited to see progress on this!

packages/base-manager/package.json Show resolved Hide resolved
packages/base-manager/src/manager-base.ts Outdated Show resolved Hide resolved
packages/controls/src/widget_bool.ts Outdated Show resolved Hide resolved
@jasongrout
Copy link
Member

Thank you so so much @zerline for being patient with this process.

I've implemented a few of @blois's suggestions (thanks!) and merged in master to pick up recent changes, and I think it is ready for one more person to do a final review.

@jasongrout
Copy link
Member

and I think it is ready for one more person to do a final review.

Err, I see that lots of the changes are related to trivial lint formatting changes. I'll try to get those out of the review so we can concentrate on the substantial changes here.

@jasongrout
Copy link
Member

I pulled in the lint updates from #3233, then ran the lint here. That means once #3233 is merged into master, the diff here will be much better and not contain spurious formatting changes. Alternatively, you can compare this to #3233 to see the real differences this PR introduces.

@jasongrout
Copy link
Member

I also needed to regenerate the spec to add these description_allow_html fields to the new date time pickers.

@jasongrout
Copy link
Member

That means once #3233 is merged into master, the diff here will be much better and not contain spurious formatting changes.

Now that the linter PR has been merged (thanks @ibdafna!), the changes here reflect just the changes relevant to this PR.

@ibdafna
Copy link
Member

ibdafna commented Aug 2, 2021

@zerline @jasongrout I reviewed and this looks good to me. I'll do a more thorough test run before the meeting on Tues and if I can't find anything before then we can merge!

@ibdafna
Copy link
Member

ibdafna commented Aug 3, 2021

Did more testing and it works beautifully - really cool PR! I tried breaking it and I didn't see any issues other than <strong></strong> not highlighting text (I may have used it incorrectly), but it's not a real issue because <b></b> does work as expected. Interop with MathJax is also 👌

@ibdafna ibdafna merged commit 8a2b049 into jupyter-widgets:master Aug 3, 2021
@slongwell
Copy link

One of the above commits states that No HTML is possible inside the tag '<button>'.

Issue #3204, also mentioned above, suggests that Buttons/ToggleButtons may be able to have HTML descriptions in v8.0.

Are HTML descriptions in Buttons/ToggleButtons something that is still being developed, or has that been found to be technically infeasible?

dazza-codes added a commit to dazza-codes/kepler.gl that referenced this pull request Mar 26, 2024
Bump ipywidgets >=8.0 to resolve CVEs:

```
-> Vulnerability found in ipywidgets version 7.8.1
   Vulnerability ID: 50664
   Affected spec: <8.0.0
   ADVISORY: Ipywidgets 8.0.0 sanitizes descriptions by default.jupyter-widgets/ipywidgets#2785
   PVE-2022-50664
   For more information about this vulnerability, visit https://data.safetycli.com/v/50664/97c
   To ignore this vulnerability, use PyUp vulnerability id 50664 in safety’s ignore command-line argument or add the ignore to your safety policy file.


-> Vulnerability found in ipywidgets version 7.8.1
   Vulnerability ID: 50463
   Affected spec: <8.0.0rc2
   ADVISORY: Ipywidgets 8.0.0rc2 makes descriptions plaintext by default for security.jupyter-widgets/ipywidgets#2785
   PVE-2022-50463
   For more information about this vulnerability, visit https://data.safetycli.com/v/50463/97c
   To ignore this vulnerability, use PyUp vulnerability id 50463 in safety’s ignore command-line argument or add the ignore to your safety policy file.
```
dazza-codes added a commit to dazza-codes/kepler.gl that referenced this pull request Mar 26, 2024
Bump ipywidgets >=8.0 to resolve CVEs:

```
-> Vulnerability found in ipywidgets version 7.8.1
   Vulnerability ID: 50664
   Affected spec: <8.0.0
   ADVISORY: Ipywidgets 8.0.0 sanitizes descriptions by default.jupyter-widgets/ipywidgets#2785
   PVE-2022-50664
   For more information about this vulnerability, visit https://data.safetycli.com/v/50664/97c
   To ignore this vulnerability, use PyUp vulnerability id 50664 in safety’s ignore command-line argument or add the ignore to your safety policy file.

-> Vulnerability found in ipywidgets version 7.8.1
   Vulnerability ID: 50463
   Affected spec: <8.0.0rc2
   ADVISORY: Ipywidgets 8.0.0rc2 makes descriptions plaintext by default for security.jupyter-widgets/ipywidgets#2785
   PVE-2022-50463
   For more information about this vulnerability, visit https://data.safetycli.com/v/50463/97c
   To ignore this vulnerability, use PyUp vulnerability id 50463 in safety’s ignore command-line argument or add the ignore to your safety policy file.
```

Signed-off-by: Darren Weber <[email protected]>
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

Successfully merging this pull request may close these issues.

8 participants