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

Upgrades dependency xmldom #2900

Closed
wants to merge 2 commits into from
Closed

Upgrades dependency xmldom #2900

wants to merge 2 commits into from

Conversation

karfau
Copy link
Contributor

@karfau karfau commented Dec 25, 2021

Switching from package xmldom to @xmldom/xmldom, which resolves the security issue present in latest xmldom version 0.6.0:
GHSA-5fg8-2547-mr8q

The reason is that the maintainers were forced to switch to a scoped package since 0.7.0:
xmldom/xmldom#271

  • The reference to @types/xmldom can be dropped, since xmldom now comes with types as part of the package.
  • I used node 16 to run npm install which updated the npm-shrinkwrap.json.
  • I didn't attempt to run the project on my machine, but I'm hoping for the CI checks to cover the important things.
  • According to our discussion below I only upgrade to @xmldom/xmldom@0.7.5, so you have more time to resolve the issue that exists with version 0.8.0

I'm one of the xmldom maintainers. Don't hesitate to ask me questions.

Changes in xmldom since 0.6.0 ## [0.8.0](https://github.com/xmldom/xmldom/compare/0.7.5...0.8.0)

Fixed

  • Normalize all line endings according to XML specs 1.0 and 1.1
    BREAKING CHANGE: Certain combination of line break characters are normalized to a single \n before parsing takes place and will no longer be preserved.
  • XMLSerializer: Preserve whitespace character references #284 / #310
    BREAKING CHANGE: If you relied on the not spec compliant preservation of literal \t, \n or \r in attribute values.
    To preserve those you will have to create XML that instead contains the correct numerical (or hexadecimal) equivalent (e.g. 	, 
, 
).
  • Drop deprecated exports DOMImplementation and XMLSerializer from lib/dom-parser.js #53 / #309
    BREAKING CHANGE: Use the one provided by the main package export.
  • dom: Remove all links as part of removeChild #343 / #355

Chore

  • ci: Restore latest tested node version to 16.x #325
  • ci: Split test and lint steps into jobs #111 / #304
  • Pinned and updated devDependencies

Thank you @marrus-sh, @victorandree, @mdierolf, @tsabbay, @fatihpense for your contributions

0.7.5

Commits

Fixes:

0.7.4

Commits

Fixes:

  • Restore ability to parse __prototype__ attributes #315
    Thank you @dsimsonOMF

0.7.3

Commits

Fixes:

  • Add doctype when parsing from string #277 / #301
  • Correct typo in error message #294
    Thank you @rrthomas

Refactor:

  • Improve exports & require statements, new main package entry #233

Docs:

  • Fix Stryker badge #298
  • Fix link to help-wanted issues #299

Chore:

  • Execute stryker:dry-run on branches #302
  • Fix stryker config #300
  • Split test and lint scripts #297
  • Switch to stryker dashboard owned by org #292

0.7.2

Commits

Fixes:

  • Types: Add index.d.ts to packaged files #288
    Thank you @forty

0.7.1

Commits

Fixes:

  • Types: Copy types from DefinitelyTyped #283
    Thank you @kachkaev

Chore:

  • package.json: remove author, maintainers, etc. #279

0.7.0

Commits

Due to #271 this version was published as

  • unscoped xmldom package to github (git tags 0.7.0 and 0.7.0+unscoped)
  • scoped @xmldom/xmldom package to npm (git tag 0.7.0+scoped)
    For more details look at #278

Fixes:

  • Security: Misinterpretation of malicious XML input CVE-2021-32796
  • Implement Document.getElementsByClassName as specified #213, thank you @ChALkeR
  • Inherit namespace prefix from parent when required #268
  • Handle whitespace in closing tags #267
  • Update DOMImplementation according to recent specs #210
    BREAKING CHANGE: Only if you "passed features to be marked as available as a constructor arguments" and expected it to "magically work".
  • No longer serializes any namespaces with an empty URI #244
    (related to #168 released in 0.6.0)
    BREAKING CHANGE: Only if you rely on "unsetting" a namespace prefix by setting it to an empty string
  • Set localName as part of Document.createElement #229, thank you @rrthomas

CI

  • We are now additionally running tests against node v16
  • Stryker tests on the master branch now run against node v14

Docs

  • Describe relations with and between specs: #211, #247

@waldekmastykarz
Copy link
Member

Thank you and we appreciate your help! 👏

@waldekmastykarz
Copy link
Member

Hey @karfau, is the new version backwards compatible? It seems like some of our tests are breaking.

@waldekmastykarz waldekmastykarz marked this pull request as draft December 28, 2021 15:56
@karfau
Copy link
Contributor Author

karfau commented Dec 28, 2021

So what I did to reproduce the error was:

nvm i 16
npm ci
npm run build
npm run test:test

And the output is
image

Which is related to the following breaking change in change in 0.8.0:

  • XMLSerializer: Preserve whitespace character references #284 / #310
    BREAKING CHANGE: If you relied on the not spec compliant preservation of literal \t, \n or \r in attribute values.
    To preserve those you will have to create XML that instead contains the correct numerical (or hexadecimal) equivalent (e.g. 	, 
, 
).

I checked the test source code and found that there is a literal \t part of the expected value, which is no longer possible:

image

From the test code it looks like you are transforming linux path separator / to \(?), but this causes an issue when it is followed by t, n, r or other characters that have a special meaning when they come after a backslash. If you need a literal backslash you should most likely convert / to \\?

(If you intention is to have and preserve a "literal tab" in attribute values you need to put 	 or another entity reference instead of \t.)

We now have two options:
A) I change the PR to only update to 0.7.5 (and you take care of this issue later whenever you have time, before upgrading xmldom further)
B) You change the test to no longer contain tab literals

Let me know which way you prefer.

@waldekmastykarz
Copy link
Member

Thank you for looking into it and I appreciate it your help. If 0.7.5 is backwards compatible, let's use it for now. We'll then do another round of reviews to update our code to work with v0.8. Once again, thank you!

Switching from package `xmldom` to `@xmldom/xmldom`, which resolves the security issue present in latest xmldom version 0.6.0:
GHSA-5fg8-2547-mr8q

The reason is that the maintainers were forced to switch to a scoped package since 0.7.0:
 xmldom/xmldom#271

- The reference to `@types/xmldom` can be dropped, since xmldom now comes with types as part of the package.
- I used node 16 to run `npm install` which updated the npm-shrinkwrap.json.
- I didn't attempt to run the project on my machine, but I'm hoping for the CI checks to cover the important things.
- The package `adaptive-expressions` has a dependency to `@xmldom/xmldom@0.7.5`, so if you prefer I can also change the PR to point to that version in the package.json.
  I didn't find any tools that support this project in keeping dependencies up to date, so I'm not sure which way you would prefer.

I'm one of the xmldom maintainers. Don't hesitate to ask me questions.
@karfau
Copy link
Contributor Author

karfau commented Dec 29, 2021

@waldekmastykarz I rebased onto master and force pushed with the upgrade to 0.7.5
Locally the npm run test:test passed.

@waldekmastykarz waldekmastykarz marked this pull request as ready for review December 29, 2021 18:40
@waldekmastykarz
Copy link
Member

Awesome! I appreciate the effort @karfau. We'll review and merge the PR asap. 👍

@karfau
Copy link
Contributor Author

karfau commented Jan 9, 2022

@waldekmastykarz any update on this?

@waldekmastykarz
Copy link
Member

We've just done a release yesterday and to avoid last-minute changes we'll merge it in the coming few days. Thank you for checking in.

@karfau
Copy link
Contributor Author

karfau commented Jan 10, 2022

I'm assuming you will take care of merging this into your upcoming v5, right?

PS: Since all existing xmldom versions have security issues you might want to evaluate if it's reasonable to deprecate version using that dependency.
It depends on, if the XML can be provided/modified by a potential attacker aka if it has to be treated as insecure user input anywhere.

@waldekmastykarz
Copy link
Member

Yes, we'll merge it into a preview version v4.4, which is backwards compatible and which we'll release this week and which will rollup into v5 at the end of February.

Considering the usage patterns of CLI for Microsoft 365, I don't think there's a way for this vulnerability to be exploited because it's the user running the command who specifies the XML.

I appreciate you thinking together with us.

@waldekmastykarz waldekmastykarz self-assigned this Jan 10, 2022
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Thank you for the patch 👏

@waldekmastykarz
Copy link
Member

Merged manually. Thank you! 👍

@waldekmastykarz waldekmastykarz added this to the v4.4 milestone Jan 10, 2022
@karfau karfau deleted the upgrade-xmldom branch January 11, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants