Breaking change in v5.0.0, docs not updated (TypeError: callbackUrl is required)

[nicokaiser]

With the update to v5.0.0, the path configuration option does not seem to work anymore:

new SamlStrategy(
    {
      path: "/login/callback",
      entryPoint:
        "https://openidp.feide.no/simplesaml/saml2/idp/SSOService.php",
      issuer: "passport-saml",
      cert: "fake cert", // cert must be provided
    },
    ...

(This is the example from README.md)

This throws "TypeError: callbackUrl is required". There is no migration guide from passport-saml 4.x to 5.0.0, so I assumed, apart from the Node.js 18 requirement nothing changed.

• There should be a migration guide which explains how to set a relativ callback URL now (callbackUrl is relative, so we used to use path, like in the example)
• README.md should be updated to reflect these changes

[cjbarth]

You are right, path no longer works because it was removed from node-saml on which passport-saml depends. The README should have its example updated. Please submit a PR. Also, note in the README how it does reference node-saml, so that might help you see other changes that are noteworthy during migration.

[nicokaiser]

This is related to node-saml/node-saml#214

While I completely agree that removing complexity is a good thing, I now miss the possibility to dynamically configure the callbackUrl: in an environment where I do not know my public hostname, I could just use a relative path. This is not possible anymore, and I cannot even guess my hostname using the current request, since configuration takes place outside of a request context. Or am I missing something?

[nicokaiser]

So the "new" way when you can only use path (but do not know your own host) is this:

passport.use(
    new SamlStrategy(
        {
            callbackUrl: 'http://some-random-host/some-random-url/', // required but not used, see below
            ...
        }
    )
);

app.get(
    "/login",
    (req, res, next) => {
        path = '/login/callback';
        callbackUrl = (req.protocol || 'http').concat('://') + (req.headers?.host || 'localhost') + path;
        passport.authenticate("saml", { callbackUrl })(req, res, next),
    }
);

Right?

[cjbarth]

I would probably use const path = ..., etc. I would probably also use a URL builder API instead of string concatenation like https://nodejs.org/api/url.html#url_url_format_urlobject, but yes, you have the right idea. Another approach would be to use a pattern like passport-saml has for multi-SAML strategy.

Basically, the node-saml library isn't the in the business of building URLs anymore. There were problems with the implementation, and to make the problems go away it was decided that we'd simplify the code and offload that work to the consumer (who could use another library). You're case does seem a little strange, and it isn't always wise to trust req.headers.host; see node-saml/node-saml#214, https://portswigger.net/web-security/host-header, and https://cqr.company/web-vulnerabilities/http-host-header-attacks/

[nicokaiser]

library isn't the in the business of building URLs anymore

I agree. However there are legitimate cases where the service does not know its hostname, e.g. when it is deployed in staging branches, etc. I cannot hard-code a callbackUrl there, otherwise we'd risk surprises when redirecting back to the wrong host. So there should be a "best practice" guide on how to do this in case other users also need to rely on the previous path behaviour.

I'll try to make a sensible README PR...

[cjbarth]

Our official suggestion is to pass in a constructed URL, like you're currently doing. We'd be open to a PR that would take a function for callbackUrl, so that you can build it on invocation.

[nicokaiser]

Ok I need to withdraw my solution: callbackUrl is not used at all in the authenticate method. So the solution needs to be a function for callbackUrl.

[cjbarth]

Honestly, I don't know why you don't have some parameters for your different environments that you can use to build a URL specific to the environment. An environment variable could be used to switch which URL is passed in. Or, you could use the multi-SAML functionality. I'm open to the function idea still, but it seems that this might be pointing to a more endemic situation. How do you change your DB config or other such-like things per environment?

[srd90]

... Or, you could use the multi-SAML functionality. I'm open to the function idea still, but it seems that this might be pointing to a more endemic situation. ...

@nicokaiser as @cjbarth is trying to say: Multisaml approach provides access to request (to extract whatever request specific information) and it provides mechanism to return e.g. fixed set of other config variables and change callbackUrl config per request (which would solve this issue reporter's case).

This comment entry's actual point is: why even consider new round of fine-grained configuration stuff because (and especially in this case/for this case) there is goarse-grained approach (from passport-saml pov) already available which provides mechanism to change about everything per request.

Sidenote: host parameters of various methods at node-saml are not used anymore https://github.com/node-saml/node-saml/blob/v5.0.0/src/saml.ts

Seems that those were missed during node-saml/node-saml#214 review. Unfortunately removing those is breaking change.