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

npm audit warning for aurelia-framework - XSS vulnerability in default HTML sanitizer implementation #992

Closed
josundt opened this issue Mar 1, 2022 · 15 comments

Comments

@josundt
Copy link

josundt commented Mar 1, 2022

I'm submitting a security vulnerability audit report

  • Library Version:
    aurelia-framework 1.3.1

Please tell us about your environment:

  • Operating System:
    Windows 11 (N/A)

  • Node Version:
    16.14.1 (LTS)

  • NPM Version:
    8.3.1 (LTS)

  • Aurelia CLI OR JSPM OR Webpack AND Version
    N/A

  • Browser:
    N/A

  • Language:
    all

Current behavior:
When installing Aurelia 1 (aurelia-framework) using npm, audit warnings are displayed, with reference to this vulnerability description.

The Aurelia products developed by my company are not really affected since we have implemented our own improved Aurelia HTMLSanitizer package (stored on our internal npm repository). We use this as replacement for the default, limited sanitizer implementation included with aurelia-framework (as recommended in your documentation pages).

Our sanitizer package is a pure ESM package that works both in browser AND node (using jsdom) environments, and it is configurable by "allow-listing" html element names/attributes per element type, and also inline CSS style properties...

const options: HtmlSanitizerOptions = {
    htmlAllow: {
        p: [ "style" ]
    },
    cssAllow: [
        "text-align"
    ]
};

If it is of interest, I could ask if it is OK to make the source code for our sanitizer package public, so that you could review/test it.
Then we could discuss making the package public, or if you prefer dissecting the code to make it an integral part of the aurelia-framework source code, we could maybe "donate" the code for this purpose as well.
I would need some confirmations from management first though.

Whatever you prefer, I think something should be done to get rid of the vulnerability audit warnings.
Awaiting reply.

Expected/desired behavior:
Aurelia 1 should mitigate the vulnerability by including a better html sanitization feature to get rid of audit warnings when installed from npm repository.

@bigopon
Copy link
Member

bigopon commented Mar 16, 2022

Thank you for reporting this issue. We will be releasing a new minor version of the templating package, with a throw, instead of the current way of doing it. After that, we will be upgrading the min dependency requirement here.

@josundt
Copy link
Author

josundt commented Mar 29, 2022

@bigopon Any ETA for the templating package security update?

@Sayan751
Copy link
Member

Sayan751 commented Mar 29, 2022

@josundt The release is already out with v1.14.0: https://github.com/aurelia/templating-resources/releases/tag/1.14.0. I think this issue can now be closed.

Edit: Just realized, there might be some further changes that are required.

@josundt
Copy link
Author

josundt commented Mar 29, 2022

@Sayan751 I found that aurelia-templating-resources@1.14.0 actually is the version installed in my node_modules folder.
Still the the npm warning appears.

Even if the latest aurelia-templating-resources release mitigates the problem, the reported vulnerability is for the aurelia-framework package (Cross-site Scripting in aurelia-framework).

Maybe new releases of the upstream dependencies with bumped version constraint for aurelia-templating-resources is required to resolve the npm warning?

aurelia-bootstrapper@bump
├ aurelia-templating-resources@^1.14.0
└ aurelia-framework@^bump
  └ aurelia-templating@^bump
    └ aurelia-templating-resources@^1.14.0

@bigopon
Copy link
Member

bigopon commented Mar 31, 2022

with aurelia-framework@1.4.1 and aurelia-templating@1.11.1 and aurelia-templating-resources@1.14.0, the reported vulnerability should be resolved.

Thanks @mroeling, @josundt for reporting & help resolving the issue & @Sayan751 for the discussion.

Though I'm not sure how we should actually make npm aware of this. @mroeling can you have a check and update the status?

@mroeling
Copy link

with aurelia-framework@1.4.1 and aurelia-templating@1.11.1 and aurelia-templating-resources@1.14.0, the reported vulnerability should be resolved.

Thanks @mroeling, @josundt for reporting & help resolving the issue & @Sayan751 for the discussion.

Though I'm not sure how we should actually make npm aware of this. @mroeling can you have a check and update the status?

Even though I'm happy to see this resolved, @bigopon are you sure you meant to tag me in here? :)

@Sayan751
Copy link
Member

@mroeling I think that's because you are credited in the reported vulnerability: GHSA-m6j2-v3gq-45r5.

@mroeling
Copy link

mroeling commented Mar 31, 2022

@mroeling I think that's because you are credited in the reported vulnerability: GHSA-m6j2-v3gq-45r5.

A, yes, I've contributed by adding I think the last line to the references :) Thanks for pointing that out. But I had nothing to do with the original findings. So no, I don't think I'm the appropriate person in this matter...

Edit: I've sent the link to this thread to the raiser of the issue on the Aurelia forum.

@bigopon
Copy link
Member

bigopon commented Apr 2, 2022

I've submitted a resolution at github/advisory-database#175
Will update here.

@bigopon
Copy link
Member

bigopon commented Apr 6, 2022

It's been merged github/advisory-database#175

This issue is resolved. Thanks everyone.

@milkshakeuk
Copy link

@bigopon what's the official recommended way to fix the new error which is thrown is there a plugin you recommend for html sanitisation? I found this https://www.npmjs.com/package/@appex/aurelia-dompurify/v/0.5.0 but it's not been touched in a year.

@bigopon
Copy link
Member

bigopon commented May 13, 2022

@milkshakeuk you can do the following:
main.js

import { HTMLSanitizer } from 'aurelia-templating-resources'
import createDOMPurify from 'dompurify'

export function configure(aurelia) {
  ...
  aurelia.container.registerSingleton(HTMLSanitizer, class MySanitizer {
    constructor() {
      this.purifier = createDOMPurify(window);
    } 
    sanitize(html) {
      return this.purifier.sanitize(dirty);
    }
  })
}

@jamesg1
Copy link

jamesg1 commented Aug 12, 2022

Hi it is a bit disappointing this security fix has resulted in existing functionality to stop working without much warning. The docs also should provide the solution above to the error.

@jamesg1
Copy link

jamesg1 commented Aug 15, 2022

A typescript version for the above

 import createDOMPurify from 'dompurify';
export class CustomHtmlSanitizer {
  purifier: createDOMPurify.DOMPurifyI;

  constructor() {
    this.purifier = createDOMPurify(window);
  }
  sanitize(html: string | Node) {
    return this.purifier.sanitize(html);
  }
}

aurelia.container.registerSingleton(HTMLSanitizer, CustomHtmlSanitizer);

@bigopon
Copy link
Member

bigopon commented Aug 15, 2022

Hi @jamesg1 , thanks for the suggestion. Glad you got it working for you. I'd also say that if you don't have any HTML from user, maybe no need to use a sanitizer.
For the doc, cc @Vheissu

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

6 participants