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

Wrong currentUrl within passport strategy for v6 #733

Closed
2 tasks done
Alabate opened this issue Nov 27, 2024 · 6 comments
Closed
2 tasks done

Wrong currentUrl within passport strategy for v6 #733

Alabate opened this issue Nov 27, 2024 · 6 comments

Comments

@Alabate
Copy link

Alabate commented Nov 27, 2024

What happened?

I tried to use the passport strategy with Microsoft server. The redirection and login prompt happens properly, but once on the callback endpoint I get this error

ResponseBodyError: server responded with an error in the response body
...
  code: 'OAUTH_RESPONSE_BODY_ERROR',
  error: 'invalid_client',
  status: 400,
  error_description: "AADSTS500112: The reply address 'http://localhost/callback' does not match the reply address 'http://localhost:3001/api/callback' provided when requesting Authorization code. Trace ID: 6fb3c78a-975d-4568-9cb7-d8c9ee407500 Correlation ID: d1a3950e-4dc9-4736-a30b-12278ad1a6a5 Timestamp: 2024-11-27 16:11:00Z"

I may be wrong, but I think it comes from the currentUrl method here:

return new URL(`${req.protocol}://${req.host}${req.url}`)

There is two issue in my case:

  • req.host never contains the port, so if you are not on 80/443, it will not work
  • req.url will not contain the full url if you are using an express.Router() it will only give the last part

I've found a workaround by redefining the currentUrl method:

  strategy.currentUrl = (req) => {
    return new URL(`${req.protocol}://${req.headers.host}${req.originalUrl}`);
  };

And with that, it works as expected

Version

v6.1.4

Runtime

Node.js

Runtime Details

Node v18.19.1

Code to reproduce

import * as client from "openid-client";
import { Strategy } from "openid-client/passport";

import express from "express";
import session from "express-session";
import passport from "passport";

////////////////////////////////////////////////////////////////////////////////
// Prerequisites
let server = null; // Authorization server's Issuer Identifier URL
let clientId = null; // Client identifier at the Authorization Server
let clientSecret = null; // Client Secret
let scope = "openid email";
////////////////////////////////////////////////////////////////////////////////

////////////////////////////////////////////////////////////////////////////////
// Fix for node <19 that doesn't have globalThis.crypto yet
import crypto from "node:crypto";
import { error } from "node:console";
globalThis.crypto ??= crypto.webcrypto;
////////////////////////////////////////////////////////////////////////////////

let app = express();
app.listen(3001, () =>
  console.log("Server ready, go to http://localhost:3001/login")
);

let config = await client.discovery(server, clientId, clientSecret);
app.use(
  session({
    saveUninitialized: false,
    resave: true,
    secret: "secret",
  })
);
app.use(passport.authenticate("session"));

const strategy = new Strategy(
  { config, scope, callbackURL: `http://localhost:3001/api/callback` },
  (tokens, verified) => {
    verified(null, tokens.claims());
  }
);
////////////////////////////////////////////////////////////////////////////////
// Uncomment this block to apply my workaround
// strategy.currentUrl = (req) => {
//   console.log("initial", req.protocol, req.host, req.url);
//   console.log("new", req.protocol, req.headers.host, req.originalUrl);
//   return new URL(`${req.protocol}://${req.headers.host}${req.originalUrl}`);
// };
////////////////////////////////////////////////////////////////////////////////
passport.use(strategy);

passport.serializeUser((user, cb) => cb(null, user));
passport.deserializeUser((user, cb) => cb(null, user));

app.get("/login", passport.authenticate(server.hostname));

///////////////////////////////////////////
// Here we define the callback route via a router, so req.url !== req.originalUrl
const router = express.Router();
router.get("/callback", passport.authenticate(server.hostname), (req, res) => {
  res.send(`Hello ${req.user.email}`);
});
app.use("/api", router);
///////////////////////////////////////////

app.use(function (err, req, res, next) {
  console.error(err);
  res.status(500).send(err);
});

Required

  • I have searched the issues tracker and discussions for similar topics and couldn't find anything related.
  • I agree to follow this project's Code of Conduct
@Alabate Alabate added the triage label Nov 27, 2024
@panva
Copy link
Owner

panva commented Nov 27, 2024

req.host never contains the port, so if you are not on 80/443, it will not work

#713 (comment)

Are you using express@5? Because AFAICT req.host is fixed in express 5 and properly returns port.

req.url will not contain the full url if you are using an express.Router() it will only give the last part

You'll have to overwrite the currentUrl method on the Strategy instance to return the current URL then

@Alabate
Copy link
Author

Alabate commented Nov 27, 2024

Yes, you're right about the .host attribute, sorry I missed that because the version 5 is still marked as next in npm.

Wouldn't it be better to use req.originalUrl instead of req.url as it seems to be precisely made for that use case:

The req.originalUrl property is much like req.url however, it returns the original request URL thereby allowing you to rewrite req.url freely for internal routing purposes.
https://www.geeksforgeeks.org/express-js-req-originalurl-property/

@panva
Copy link
Owner

panva commented Nov 27, 2024

Possibly, yeah. I'll look into it.

@Alabate
Copy link
Author

Alabate commented Nov 27, 2024

Ok. Thanks for your help and your fast answer. You can close this issue if you want, the solution of upgrading express and/or redefining the currentUrl method is enough for me.

For people coming here, the workaround is [edit: NOT WORKING BEHIND PROXY, SEE BELLOW]

strategy.currentUrl = (req) => {
  return new URL(`${req.protocol}://${req.headers.host}${req.originalUrl}`);
};

@panva
Copy link
Owner

panva commented Nov 27, 2024

req.headers.host is not a universally working workaround, as soon as you get your project deployed behind a tls offloading proxy it won't work, it'll be x-forwarded-host. It is much simpler to rely on express@5 proper implementation of req.host and then properly configuring express to trust your proxy.

@panva panva removed the triage label Nov 27, 2024
@panva
Copy link
Owner

panva commented Nov 27, 2024

Wouldn't it be better to use req.originalUrl instead of req.url as it seems to be precisely made for that use case

https://github.com/panva/openid-client/releases/tag/v6.1.5

@panva panva closed this as completed Nov 27, 2024
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

No branches or pull requests

2 participants