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
chore: Bump xmldom to 0.8.0 #660
Conversation
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 No matter what version of node I used to try and run the normal `npm install`, I always received a warning about either old package lock file format or newer lock file version format. So to avoid to many unrelated changes, I disabled the `postinstall` step locally and installed the root level using node v12 and the `mbTest` folder using node v16. - When running the `npm run test` on the root level using node v12, there is a single failing test, but running them with node v16 works.So let's see what happens on CircleCI. I'm happy to fix any regression we introduced, I just need help to understand how exactly the xmldom upgrade influences the failing test. I'm one of the xmldom maintainers. Don't hesitate to ask me questions.
Thanks! Sorry for a slow response, been busy but should free up shortly and will respond. |
First, thank you so much for creating a PR like this. It's pretty amazing that you not only updated your package but made an attempt at updating the ecosystem around it. The failing test is causing me some head-scratching so I'll describe it here in case you have an idea based on a deeper understanding of the change. The cause appears to be an assumption in another npm package (xpath), which sets a case sensitivity flag based on an check that I'm struggling to wrap my brain around (
Any thing strike you as obvious about the change that would flip that feature between versions? |
After a bit more research, I suspect it's a legacy check in xpath, but I don't understand the issue well enough to make a PR there, so I ended up patching xmldom. Works as far as I can tell, but LMK if you think there's something wrong with this code:
And, thanks again for all your effort! |
Yes, before 0.7.0 I'm very close to finish a change that will allow HTML detection again starting from 0.9.0. (Of course you could simplify your patch to just always return |
O and goto100/xpath#27 (comment) suggests there is an |
Not a problem, thanks again for your help,
…On Thu, Feb 24, 2022 at 1:14 PM Christian Bewernitz < ***@***.***> wrote:
I will let you know when it's available, so you can stop patching xmldom :)
Sorry that was an offer I have to take back. I have to many things to keep
track of already. Feel free subscribe to the related issue
<xmldom/xmldom#203> or PR
<xmldom/xmldom#338> or to watch the xmldom
releases <https://github.com/xmldom/xmldom/releases>, so I don't have to
remember to notify you 🤝
—
Reply to this email directly, view it on GitHub
<#660 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAARFP4UVSRJ3HM3WGIKU2DU4Z7TFANCNFSM5KYBC7EA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
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
No matter what version of node I used to try and run the normal
npm install
, I always received a warning about either old package lock file format or newer lock file version format.So to avoid to many unrelated changes, I disabled the
postinstall
step locally and installed the root level using node v12 and thembTest
folder using node v16.npm run test
on the root level using node v12, there is a single failing test, but running them with node v16 works.So let's see what happens on CircleCI.cb mbTest && npm run test
(using node 16) The first listed test fails. When addingMB_AIRPLANE_MODE=true
there are quite a bunch of tests failing... not sure if this is really related to the xmldom update.I'm happy to fix any regression we introduced, I just need help to understand how exactly the xmldom upgrade influences the failing test(s).
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
BREAKING CHANGE: Certain combination of line break characters are normalized to a single
\n
before parsing takes place and will no longer be preserved.#303
/#307
#49
,#97
,#324
/#314
#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.
	
,

,
).DOMImplementation
andXMLSerializer
fromlib/dom-parser.js
#53 /#309
BREAKING CHANGE: Use the one provided by the main package export.
removeChild
#343
/#355
Chore
#325
#111
/#304
Thank you @marrus-sh, @victorandree, @mdierolf, @tsabbay, @fatihpense for your contributions
0.7.5
Commits
Fixes:
#319
/#321
Thank you @lupestro
0.7.4
Commits
Fixes:
__prototype__
attributes#315
Thank you @dsimsonOMF
0.7.3
Commits
Fixes:
#277
/#301
#294
Thank you @rrthomas
Refactor:
#233
Docs:
#298
#299
Chore:
#302
#300
#297
#292
0.7.2
Commits
Fixes:
#288
Thank you @forty
0.7.1
Commits
Fixes:
#283
Thank you @kachkaev
Chore:
#279
0.7.0
Commits
Due to
#271
this version was published asxmldom
package to github (git tags0.7.0
and0.7.0+unscoped
)@xmldom/xmldom
package to npm (git tag0.7.0+scoped
)For more details look at
#278
Fixes:
CVE-2021-32796
Document.getElementsByClassName
as specified#213
, thank you @ChALkeR#268
#267
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".
#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
localName
as part ofDocument.createElement
#229
, thank you @rrthomasCI
Docs
#211
,#247