search

panva / openid-client

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

Not able to set a "state" parameter on the authorize request when using the Passport strategy #316

Closed GertSallaerts closed 3 years ago

GertSallaerts commented 3 years ago

Describe the bug

The passport strategy goes into "callback"-mode whenever a state parameter is found on the current request object. Unless I'm mistaken, this assumption is wrong as the web-application (an SPA in my case) that initiates the "authorize" request is allowed to specify its own state parameter to maintain state between the request and the callback.

To Reproduce This is a demo application to illustrate the problem, it omits the part where state is also sent back to the web application to verify and sync but I think it should be sufficient to show the problem.

const express = require('express');
const bodyParser = require('body-parser');
const session = require('express-session');
const { Passport } = require('passport');
const { Issuer, Strategy } = require('openid-client');

const app = express();
const passport = new Passport();
const issuer = new Issuer({
    issuer: 'https://ambassify.eu.auth0.com/',
    authorization_endpoint: 'https://ambassify.eu.auth0.com/authorize',
    token_endpoint: 'https://ambassify.eu.auth0.com/oauth/token',
    userinfo_endpoint: 'https://ambassify.eu.auth0.com/userinfo',
});

const client = new issuer.Client({
    client_id: 'xxxxxxxxxxxxxxx',
    client_secret: 'xxxxxxxxxxxxxxxxxxxxxx',
    redirect_uris: [ 'http://localhost:8000/callback' ],
    response_types: ['code'],
    usePKCE: true,
    scope: 'openid profile'
});

app.use(session({
    secret: 'keyboard cat',
    resave: false,
    saveUninitialized: true,
}));

app.use(bodyParser.urlencoded({ extended: true }));
app.use(bodyParser.json());

passport.use('oidc', new Strategy({
    client,
}, function verify(tokens, profile, done) {
    done(null, { id: profile.sub });
}));

app.use(passport.initialize());

app.post('/authorize', passport.authenticate('oidc'));

app.get('/callback',
    passport.authenticate('oidc', { session: false }),
    (req, res) => res.redirect('/?user=' + req.user.id)
);

app.get('/', (req, res) => res.send(`
    <!DOCTYPE html>
    <html lang="en">
    <head>
        <meta charset="UTF-8">
        <meta name="viewport" content="width=device-width, initial-scale=1.0">
        <meta http-equiv="X-UA-Compatible" content="ie=edge">
        <title>Demo</title>
    </head>
    <body>
        <h1>Logged in as: ${req.query.user || 'NOT LOGGED IN'}</h1>
        <h1>Login</h1>
        <form action="/authorize" method="POST">
            <div>
                <label for="state">state (this would be hidden and generated by the application): </label>
                <input type="text" name="state" value="my-state" />
            </div>
            <div>
                <input type="submit" value="Login" />
            </div>
        </form>
    </body>
    </html>
`));

app.listen(8000);

Steps to reproduce the behaviour:

  1. Type a value into the "state" box and click the login button to trigger the problem.

Expected behaviour

I would expect the authorization flow to be kicked off using the state set in the web form, instead the Strategy launches into "callback" mode because a state is set on the request and fails because it is not a callback request.

If this is indeed the expected behaviour, I think it would be a simple matter of making this check more specific:

https://github.com/panva/node-openid-client/blob/f1767188dc4aa38121dddf51382a4512851256cb/lib/passport_strategy.js#L90

Environment:

Additional context

panva commented 3 years ago

Hi @GertSallaerts

I would expect the authorization flow to be kicked off using the state set in the web form.

Passing params from a URL straight into the strategy to initiate authorization with is not intended and not signaled anywhere.

Ad-hoc params are to be passed directly into the passport.authenticate call, e.g. shown here.

GertSallaerts commented 3 years ago

Hey @panva

I read that issue and a few related ones and I do not agree that they are addressing the same issue. Maybe my explanation is not clear enough.

I would not expect the strategy to pick up the state I pass in my URL automatically, I am aware that I have to do this manually. I would not consider this a bug as it would probably be dangerous for your strategy to trust what is in the URL.

What I do think might be a bug is that from the moment I add a state query-parameter to my URL the strategy considers the request to be a callback request instead of an authorize request because:

  1. It parses the request object here: https://github.com/panva/node-openid-client/blob/f1767188dc4aa38121dddf51382a4512851256cb/lib/passport_strategy.js#L86
  2. That function extracts all these parameters from the request: https://github.com/panva/node-openid-client/blob/f1767188dc4aa38121dddf51382a4512851256cb/lib/helpers/consts.js#L31-L43
  3. And because state is in there, this test decides to move forward with the callback-related code: https://github.com/panva/node-openid-client/blob/f1767188dc4aa38121dddf51382a4512851256cb/lib/passport_strategy.js#L90

What I am trying to say (perhaps in too many words) is that I think a state query parameter is valid on an authorize request from the consumer as well as on a callback from the identity provider and we should not use it to check which code-path to follow.

panva commented 3 years ago

I get it, but this behaves as designed, if any "oauth" registered callback parameters are detected the function will go into the callback phase. All it takes is that you don't use the registered response parameters for "your stuff".

GertSallaerts commented 3 years ago

IMO the flaw in this design would be that state is not only a registered callback parameter but is also registered as part of an OAuth authorization request. (Here for example.)

Which is exactly why we chose to allow passing state to our authentication gateway (the application that uses the strategy), so we could build it to be OAuth compatible for our customers.

I can provide a PR is you agree with this. If not, no problem, we can easily build our own strategy using the building blocks provided by openid-client. Just let me know!

panva commented 3 years ago

I don't want to change that logic since a state-only including response is a valid OIDC response for response_type=none.

We can easily build our own strategy using the building blocks provided by openid-client.

Sounds like a plan.