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

[BUG] Vulnerability dependency library xmldom #628

Closed
TTmaister opened this issue Aug 6, 2021 · 10 comments
Closed

[BUG] Vulnerability dependency library xmldom #628

TTmaister opened this issue Aug 6, 2021 · 10 comments
Labels

Comments

@TTmaister
Copy link

I noticed that xmldom library <0.7.0 on vulnerability.
GHSA-5fg8-2547-mr8q

@TTmaister TTmaister added the bug label Aug 6, 2021
@forty
Copy link
Contributor

forty commented Aug 6, 2021

See xmldom/xmldom#271

I don't know what others thinks, but my intuition is that since it impacts serialization rather than parsing, it should not be too bad for passport SAML.

I'm not sure if if worth somehow vendoring the dependency in the meantime.

@forty
Copy link
Contributor

forty commented Aug 6, 2021

My bad, actually it could very well impact us, see https://mattermost.com/blog/securing-xml-implementations-across-the-web/

Not sure why but we are re-stringifying things before reparsing https://github.com/node-saml/passport-saml/blob/3.x/src/node-saml/saml.ts#L790

That doesn't seem like a good idea, though there might be good reasons why we would do that.

@TTmaister
Copy link
Author

Sounds like a plan. Personally, I don't know the passport-saml node application so well that I could just go change the version number reference and try it in a private repository.

@shubham-bibliu
Copy link

New version 0.7.0 is available now /@xmldom/xmldom.
xml-crypto already updated

@forty
Copy link
Contributor

forty commented Aug 20, 2021

We will have to wait for 0.7.2 (at least, if I did not miss something again ^^) as there are types issues currently.

Relevant PR is here xmldom/xmldom#288. xmldom maintainer is very reactive and helpful, so it should be merged quickly.

@forty
Copy link
Contributor

forty commented Aug 20, 2021

Note that xml-encryption hasn't updated yet, and it might very well never happen, given the activity on that repo.

PR is here auth0/node-xml-encryption#88

@forty
Copy link
Contributor

forty commented Aug 20, 2021

node-saml/node-saml#17

@christian-hawk
Copy link

Any thoughts @cjbarth ?

@PCOffline
Copy link

Has this been resolved with v3.1.2? If so, let's close this issue

@markstos
Copy link
Contributor

markstos commented Sep 2, 2021

According to the Release notes for 3.1.2, yes.

https://github.com/node-saml/passport-saml/releases/tag/v3.1.2

@markstos markstos closed this as completed Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants