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

bump xmldom to 0.5.x since all lower versions have security issue (#551) #552

Closed
wants to merge 1 commit into from

Conversation

forty
Copy link
Contributor

@forty forty commented Mar 15, 2021

Description

This is a cherry-pick of the commit introduced by #551 on tag v2.0.5, so a v2.0.6 can be released with the security fix (while waiting for a 3.x release).

I think a maintainer will have to create a v2.x branch on the main repo so I can target it with this PR (currently targeting master, which does not make any sense, I know ^^)

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 15, 2021

This PR needs to be closed and a new one opened with the requested changes against the latest v2.x tag.

@cjbarth cjbarth closed this Mar 15, 2021
@forty
Copy link
Contributor Author

forty commented Mar 15, 2021

@cjbarth don't you need to create a branch from tag v2.0.5 first? that's what I'm asking in my PR description, I'm not really sure what a PR against a tag would be otherwise.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 15, 2021

You would make your PR screen look like this, where you've selected a tag, not a branch:
image

@forty
Copy link
Contributor Author

forty commented Mar 15, 2021

@cjbarth I can watch the diff this way, but not open a PR. That makes sense to me, since I think a branch would be required to point to the result of the merge.

@markstos
Copy link
Contributor

@forty I checked-- the "New Pull Request" page does allow selecting a tag as the "base" to submit the PR against. It looks like the screenshot @cjbarth posted.

@forty
Copy link
Contributor Author

forty commented Mar 15, 2021

@markstos I can select it indeed, but then the button to open the PR disappears

ezgif-1-6be108cfb511

@forty
Copy link
Contributor Author

forty commented Mar 15, 2021

(Regardless of the Github UI, I'm not really sure what opening a PR on a tag would mean in term of Git? Where would the merge commit go once the PR is merged? tag do not follow commits the same way branches do)

@markstos
Copy link
Contributor

@forty You are right-- it's a bad UI. I've created a "2.x" branch based on the v2.0.5 to submit your PR against. Thank you.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 15, 2021

We actually already had a v2 branch. I was just going to bring it up to date :)

@forty
Copy link
Contributor Author

forty commented Mar 15, 2021

Ah yes, I did notice that v2 branch, but it did seem very old :) Let me know which one I should use (v2 once @cjbarth has updated it or 2.x)

@forty
Copy link
Contributor Author

forty commented Mar 15, 2021

@cjbarth I just saw the v2 branch is now updated. I would recommend resetting --hard it to v2.0.5 instead, so that it points to the same commit (or I can use that 2.x branch if that's simpler for everyone)

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 15, 2021

I've brought the branch up to date. It now equals the tag v2.0.5. I have a neat little script that is like git merge -s theirs to make a merge commit that makes one branch look identical to another and handles all merge conflicts transparently :)

@markstos, do you have an opinion on this. I don't mind resetting --hard, but I worry about history. It seems that we should probably just go with the 2.x branch and if we need even more granularity, we can create a 2.1.x branch and so on. This way our pattern will be a v prefix for tags and no prefix for branches. Thoughts?

@forty
Copy link
Contributor Author

forty commented Mar 15, 2021

Thanks @cjbarth @markstos and @eero3 to have made it happen.

Note that it seems passport-saml still depends on a vulnerable xmldom version, indirectly via xml-encryption. A patched release has not yet been released to NPM, so we need to wait for xml-encryption upstream to do it. I opened an issue there.

@mhassan1
Copy link
Contributor

@forty I've just opened issue #556 for bumping this package's dependency to xml-encryption@1.2.2.

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.

5 participants