Skip to content

Fix #207, updated @xmldom/xmldom and xml-crypto packages - #209

Closed
RopoMen wants to merge 1 commit into
node-saml:masterfrom
RopoMen:update-dependencies
Closed

Fix #207, updated @xmldom/xmldom and xml-crypto packages#209
RopoMen wants to merge 1 commit into
node-saml:masterfrom
RopoMen:update-dependencies

Conversation

@RopoMen

@RopoMen RopoMen commented Nov 9, 2022

Copy link
Copy Markdown
Contributor

Updated

  • @xmldom/xmldom
  • xml-crypto

Fixed unit test, because now xmldom detects multiple roots.

@RopoMen

RopoMen commented Nov 9, 2022

Copy link
Copy Markdown
Contributor Author

This fixes also PR #205

@cjbarth

cjbarth commented Nov 12, 2022

Copy link
Copy Markdown
Collaborator

Closed in favor of #213 and #205

@cjbarth cjbarth closed this Nov 12, 2022
Comment thread src/xml.ts
) {
return reject(new Error("Malformed XML; multiple roots detected"));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cjbarth I noticed that you already merged another PR which updated xmldom. That is ok, but I noticed that there was added comment https://github.com/node-saml/node-saml/blob/master/src/xml.ts#L217

My opinion is that that is dead code that should be removed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was thinking about it, then I realized that this code actually protects against a vulnerability and was torn about removing it and then potentially a dependency reintroduces the problem and we are vulnerable again. However, as I type that, I realize they have put in tests to make sure that doesn't happen, so we probably should just remove the old code. I'll create another PR for that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Addressed in #215

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.

2 participants