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

Incorrect references to xmldom #637

Closed
trmpowell opened this issue Sep 9, 2021 · 7 comments
Closed

Incorrect references to xmldom #637

trmpowell opened this issue Sep 9, 2021 · 7 comments
Labels

Comments

@trmpowell
Copy link

To Reproduce

npm i --save passport-saml@latest
Search /node_modules/passport-saml for xmldom
You'll find /lib/node-saml/xml.js correctly requires @xmldom/xmldom
but files:

  • /lib/passport-saml/saml.js
  • /lib/passport-saml/xml.js
  • /lib/src/passport-saml/saml.js

are incorrectly requiring xmldom.

When we build our application with this version of passport-saml, we are seeing this error:
Cannot find module 'xmldom'

Expected behavior

This version of passport-saml should have no dependencies on xmldom package.

Environment

  • Node.js version: 14.17.1
  • passport-saml version: 3.1.2
@trmpowell trmpowell added the bug label Sep 9, 2021
@trmpowell trmpowell mentioned this issue Sep 9, 2021
@srd90
Copy link

srd90 commented Sep 9, 2021

It looks as if https://registry.npmjs.org/passport-saml/-/passport-saml-3.1.2.tgz (and quite a few other 3.x.x packages) would have been built and packaged from "non-clean" environment because passport-saml directory contains e.g. xml.js which was moved to node-saml directory. I.e. its as if package would contain 2.x.x stuff also (which would explain why there are few files which reference to unscoped xmldom package) .

Here is content of passport-saml-3.1.2.tgz at npmjs:

.
├── CHANGELOG.md
├── lib
│   ├── node-saml
│   │   ├── algorithms.d.ts
│   │   ├── algorithms.js
│   │   ├── algorithms.js.map
│   │   ├── index.d.ts
│   │   ├── index.js
│   │   ├── index.js.map
│   │   ├── inmemory-cache-provider.d.ts
│   │   ├── inmemory-cache-provider.js
│   │   ├── inmemory-cache-provider.js.map
│   │   ├── saml.d.ts
│   │   ├── saml.js
│   │   ├── saml.js.map
│   │   ├── saml-post-signing.d.ts
│   │   ├── saml-post-signing.js
│   │   ├── saml-post-signing.js.map
│   │   ├── types.d.ts
│   │   ├── types.js
│   │   ├── types.js.map
│   │   ├── utility.d.ts
│   │   ├── utility.js
│   │   ├── utility.js.map
│   │   ├── xml.d.ts
│   │   ├── xml.js
│   │   └── xml.js.map
│   ├── passport-saml
│   │   ├── algorithms.d.ts
│   │   ├── algorithms.js
│   │   ├── algorithms.js.map
│   │   ├── index.d.ts
│   │   ├── index.js
│   │   ├── index.js.map
│   │   ├── inmemory-cache-provider.d.ts
│   │   ├── inmemory-cache-provider.js
│   │   ├── inmemory-cache-provider.js.map
│   │   ├── multiSamlStrategy.d.ts
│   │   ├── multiSamlStrategy.js
│   │   ├── multiSamlStrategy.js.map
│   │   ├── saml.d.ts
│   │   ├── saml.js
│   │   ├── saml.js.map
│   │   ├── saml-post-signing.d.ts
│   │   ├── saml-post-signing.js
│   │   ├── saml-post-signing.js.map
│   │   ├── strategy.d.ts
│   │   ├── strategy.js
│   │   ├── strategy.js.map
│   │   ├── types.d.ts
│   │   ├── types.js
│   │   ├── types.js.map
│   │   ├── utility.d.ts
│   │   ├── utility.js
│   │   ├── utility.js.map
│   │   ├── xml.d.ts
│   │   ├── xml.js
│   │   └── xml.js.map
│   ├── src
│   │   └── passport-saml
│   │       ├── algorithms.d.ts
│   │       ├── algorithms.js
│   │       ├── algorithms.js.map
│   │       ├── index.d.ts
│   │       ├── index.js
│   │       ├── index.js.map
│   │       ├── inmemory-cache-provider.d.ts
│   │       ├── inmemory-cache-provider.js
│   │       ├── inmemory-cache-provider.js.map
│   │       ├── multiSamlStrategy.d.ts
│   │       ├── multiSamlStrategy.js
│   │       ├── multiSamlStrategy.js.map
│   │       ├── saml.d.ts
│   │       ├── saml.js
│   │       ├── saml.js.map
│   │       ├── saml-post-signing.d.ts
│   │       ├── saml-post-signing.js
│   │       ├── saml-post-signing.js.map
│   │       ├── strategy.d.ts
│   │       ├── strategy.js
│   │       ├── strategy.js.map
│   │       ├── types.d.ts
│   │       ├── types.js
│   │       └── types.js.map
│   ├── test.d.ts
│   ├── test.js
│   └── test.js.map
├── LICENSE
├── package.json
└── README.md

5 directories, 85 files

Reference content from clean env

mkdir -p /tmp/foo
cd /tmp/foo
git clone https://github.com/node-saml/passport-saml.git
cd passport-saml
git checkout v3.1.2
npm ci
npm run build
npm pack
# and content of packed passport-saml-3.1.2.tgz is:
.
├── CHANGELOG.md
├── lib
│   ├── node-saml
│   │   ├── algorithms.d.ts
│   │   ├── algorithms.js
│   │   ├── algorithms.js.map
│   │   ├── index.d.ts
│   │   ├── index.js
│   │   ├── index.js.map
│   │   ├── inmemory-cache-provider.d.ts
│   │   ├── inmemory-cache-provider.js
│   │   ├── inmemory-cache-provider.js.map
│   │   ├── saml.d.ts
│   │   ├── saml.js
│   │   ├── saml.js.map
│   │   ├── saml-post-signing.d.ts
│   │   ├── saml-post-signing.js
│   │   ├── saml-post-signing.js.map
│   │   ├── types.d.ts
│   │   ├── types.js
│   │   ├── types.js.map
│   │   ├── utility.d.ts
│   │   ├── utility.js
│   │   ├── utility.js.map
│   │   ├── xml.d.ts
│   │   ├── xml.js
│   │   └── xml.js.map
│   └── passport-saml
│       ├── index.d.ts
│       ├── index.js
│       ├── index.js.map
│       ├── multiSamlStrategy.d.ts
│       ├── multiSamlStrategy.js
│       ├── multiSamlStrategy.js.map
│       ├── strategy.d.ts
│       ├── strategy.js
│       ├── strategy.js.map
│       ├── types.d.ts
│       ├── types.js
│       └── types.js.map
├── LICENSE
├── package.json
└── README.md

3 directories, 40 files

@cjbarth @markstos

@markstos
Copy link
Contributor

markstos commented Sep 9, 2021 via email

@trmpowell
Copy link
Author

@cjbarth @markstos, any idea when this might be resolved?

@cjbarth
Copy link
Collaborator

cjbarth commented Sep 20, 2021

I hesitate to just repackage as-is. I'll update all the dependencies, make sure I do a git clean -xfd, and then package. Will that work?

@cjbarth
Copy link
Collaborator

cjbarth commented Sep 20, 2021

I've created a PR for this: #640

Once this lands, I'll call it a 3.2.0 release and get it out.

@cjbarth
Copy link
Collaborator

cjbarth commented Sep 25, 2021

Version 3.2.0 was released. This should fix the issue.

@cjbarth cjbarth closed this as completed Sep 25, 2021
@trmpowell
Copy link
Author

Thanks, @cjbarth ! We will give it a shot!

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

4 participants