panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

`Issuer.discover` fails in nodejs process running within an electron window #500

Closed stagefright5 closed 2 years ago

stagefright5 commented 2 years ago

Describe the bug

Proposed Solutions:

  1. Add the below require statement to the top of helpers/request.js module
    const { URL } = require('url')
  2. Or, pass url parameter as string (url.href) instead directly passing url object to the (http || https).request method.
    • This will prevent any interference with nodejs, because, when the url is string, the request method will create a url instance as it needs

To Reproduce

Issuer and Client configuration: (inline or gist) - Don't forget to redact your secrets.

// NOT RELEVENT

Steps to reproduce the behavior:

  1. Run nodejs process within an electron window with nodeIntegration enabled
  2. Use Issuer.discover API
  3. See it fail with the above mentioned error

Expected behavior

Environment:

Additional context Add any other context about the problem here.

panva commented 2 years ago

Would you mind creating a small repro project? And also feel free to open a PR, don't forget tests.

snowflake752 commented 2 years ago

Would you mind creating a small repro project? And also feel free to open a PR, don't forget tests.

Opened this PR https://github.com/panva/node-openid-client/pull/501, and all the tests are passing.

panva commented 2 years ago

Would you mind creating a small repro project? And also feel free to open a PR, don't forget tests.

Opened this PR #501, and all the tests are passing.

And the reproduction project?

cjcooper commented 2 years ago

I am surprised more haven't hit this issue. A call const issuer = await Issuer.discover('http://localhost:4011'); is causing this very issue. simple node & express, typescript application. Using step debugging it is this line: (url.protocol === 'https:' ? https.request : http.request)(url.href, opts);. I read the node documents and that should select the other overload of http but instead its choosing the first overload so opts is being viewed as a callback. the only way i could get it to stop doing that was removing '.href'. That caused node to use the correct overload of the method and it operated as expected. the docs absolutely state the "url" param can be a string or URL object but node is also not adhering to that. this is happening with node v16.16.0 and v16.17.0.