getClient called with non-decoded secret/client

[MaximilianGaedig]

Here getClientCredentials takes request.body.client_id and request.body.client_secret without calling decodeURIComponent() on them:

// token-handler.js
TokenHandler.prototype.getClientCredentials = function(request) {
  [...]
  if (request.body.client_id && request.body.client_secret) {
    return { clientId: request.body.client_id, clientSecret: request.body.client_secret };
  }
 [...]
};

The result of this is not the client secret arriving at the getClient method, but the url encoded version of it, which in my case was base64 encoded so it was changed and the getClient method failed.

I think handling it in the model is not problematic but unnecessary so it should be done in the library

"Workaround"

class Model {
  async getClient(clientId: string, clientSecret: string | null): Promise<Client | Falsey> {
    const clientIdDecoded = decodeURIComponent(clientId);
    const clientSecretDecoded = clientSecret ? decodeURIComponent(clientSecret) : null;
    // use clientIdDecoded and clientSecretDecoded in your database logic instead of clientId and clientSecret
  }
}

[jankapunkt]

Hi @MaximilianGaedig since the encoding of the secret is entirely part of the surrounding implementation, so is the decoding, too. We can't make any assumptions beyond the OAuth2 standard, which is why the model is one place where this should could be done. However, note, that your token endpoint could also be a place to handle such.

const server = new OAuth2Server({ model })

// ...other endpoints

// this could be added to express or other middleware
const tokenEndpoint = function (req, res, next) {
  const clientSecret = req.body.clientSecret ?? null;
  req.body.clientSecret = clientSecret ? decodeURIComponent(clientSecret) : null;

  const request = new Request(req);
  const response = new Resonse(res);

  server.token(request, response, options)
    .then(function (code) {
      // ...
    })
    .catch(function (err) {
      // ...
    });
}

[MaximilianGaedig]

Ah okay, maybe documenting it would be a good idea, like including a notice by the getClient method

[jankapunkt]

@MaximilianGaedig sure, I'm adding this to #208