Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions lib/grant-types/authorization-code-grant-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ const InvalidRequestError = require('../errors/invalid-request-error');
const ServerError = require('../errors/server-error');
const isFormat = require('@node-oauth/formats');
const pkce = require('../pkce/pkce');
const { isDefined, toString, isInTypes } = require('../utils/param-util');
const isStringOrNumber = isInTypes('string', 'number');
const isString = isInTypes('string');

/**
* @class
Expand Down Expand Up @@ -73,14 +76,22 @@ class AuthorizationCodeGrantType extends AbstractGrantType {
*/

async getAuthorizationCode(request, client) {
if (!request.body.code) {
if (!isDefined(request.body.code)) {
throw new InvalidRequestError('Missing parameter: `code`');
}

if (!isFormat.vschar(request.body.code)) {
if (!isStringOrNumber(request.body.code)) {
throw new InvalidRequestError('Invalid parameter: `code`');
}

// normalize string|number to string
const requestCode = toString(request.body.code);

if (!isFormat.vschar(requestCode)) {
throw new InvalidRequestError('Invalid parameter: `code`');
}

// XXX: still passing the original value from request to model
const code = await this.model.getAuthorizationCode(request.body.code);

if (!code) {
Expand Down Expand Up @@ -163,7 +174,7 @@ class AuthorizationCodeGrantType extends AbstractGrantType {

const redirectUri = request.body.redirect_uri || request.query.redirect_uri;

if (!isFormat.uri(redirectUri)) {
if (!redirectUri || !isString(redirectUri) || !isFormat.uri(redirectUri)) {
throw new InvalidRequestError('Invalid request: `redirect_uri` is not a valid URI');
}

Expand Down
10 changes: 6 additions & 4 deletions lib/grant-types/password-grant-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const InvalidArgumentError = require('../errors/invalid-argument-error');
const InvalidGrantError = require('../errors/invalid-grant-error');
const InvalidRequestError = require('../errors/invalid-request-error');
const isFormat = require('@node-oauth/formats');
const { isDefined, isInTypes } = require('../utils/param-util');
const isString = isInTypes('string');

/**
* Constructor.
Expand Down Expand Up @@ -58,19 +60,19 @@ class PasswordGrantType extends AbstractGrantType {
*/

async getUser(request, client) {
if (!request.body.username) {
if (!isDefined(request.body.username)) {
throw new InvalidRequestError('Missing parameter: `username`');
}

if (!request.body.password) {
if (!isDefined(request.body.password)) {
throw new InvalidRequestError('Missing parameter: `password`');
}

if (!isFormat.uchar(request.body.username)) {
if (!isString(request.body.username) || !isFormat.uchar(request.body.username)) {
throw new InvalidRequestError('Invalid parameter: `username`');
}

if (!isFormat.uchar(request.body.password)) {
if (!isString(request.body.password) || !isFormat.uchar(request.body.password)) {
throw new InvalidRequestError('Invalid parameter: `password`');
}

Expand Down
13 changes: 10 additions & 3 deletions lib/grant-types/refresh-token-grant-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const InvalidRequestError = require('../errors/invalid-request-error');
const ServerError = require('../errors/server-error');
const isFormat = require('@node-oauth/formats');
const InvalidScopeError = require('../errors/invalid-scope-error');
const { isInTypes, toString, isDefined } = require('../utils/param-util');
const isStringOrNumber = isInTypes('string', 'number');

/**
* Constructor.
Expand Down Expand Up @@ -64,13 +66,18 @@ class RefreshTokenGrantType extends AbstractGrantType {
/**
* Get refresh token.
*/

async getRefreshToken(request, client) {
if (!request.body.refresh_token) {
if (!isDefined(request.body.refresh_token)) {
throw new InvalidRequestError('Missing parameter: `refresh_token`');
}

if (!isFormat.vschar(request.body.refresh_token)) {
if (!isStringOrNumber(request.body.refresh_token)) {
throw new InvalidRequestError('Invalid parameter: `refresh_token`');
}

const refreshToken = toString(request.body.refresh_token);

if (!isFormat.vschar(refreshToken)) {
throw new InvalidRequestError('Invalid parameter: `refresh_token`');
}

Expand Down
12 changes: 8 additions & 4 deletions lib/handlers/authorize-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ const tokenUtil = require('../utils/token-util');
const url = require('url');
const pkce = require('../pkce/pkce');
const { parseScope } = require('../utils/scope-util');
const { isDefined, isInTypes } = require('../utils/param-util');
const isString = isInTypes('string');
const isStringOrNumber = isInTypes('string', 'number');

/**
* Response types.
Expand Down Expand Up @@ -160,17 +163,18 @@ class AuthorizeHandler {
const self = this;
const clientId = request.body.client_id || request.query.client_id;

if (!clientId) {
if (!isDefined(clientId)) {
throw new InvalidRequestError('Missing parameter: `client_id`');
}

if (!isFormat.vschar(clientId)) {
if (!isStringOrNumber(clientId) || !isFormat.vschar(clientId)) {
throw new InvalidRequestError('Invalid parameter: `client_id`');
}

const redirectUri = request.body.redirect_uri || request.query.redirect_uri;

if (redirectUri && !isFormat.uri(redirectUri)) {
// XXX: why is there no 'Missing parameter: `redirect_uri`' error?
if (isDefined(redirectUri) && (!isString(redirectUri) || !isFormat.uri(redirectUri))) {
throw new InvalidRequestError('Invalid request: `redirect_uri` is not a valid URI');
}

Expand Down Expand Up @@ -236,7 +240,7 @@ class AuthorizeHandler {

getState (request) {
const state = request.body.state || request.query.state;
const stateExists = state && state.length > 0;
const stateExists = isString(state) && state.length > 0;
const stateIsValid = stateExists
? isFormat.vschar(state)
: this.allowEmptyState;
Expand Down
4 changes: 3 additions & 1 deletion lib/handlers/token-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ const UnsupportedGrantTypeError = require('../errors/unsupported-grant-type-erro
const auth = require('basic-auth');
const pkce = require('../pkce/pkce');
const isFormat = require('@node-oauth/formats');
const { isInTypes, toString } = require('../utils/param-util');
const isStringOrNumber = isInTypes('string', 'number');

/**
* Grant types.
Expand Down Expand Up @@ -123,7 +125,7 @@ class TokenHandler {
throw new InvalidRequestError('Missing parameter: `client_secret`');
}

if (!isFormat.vschar(credentials.clientId)) {
if (!isStringOrNumber(credentials.clientId) || !isFormat.vschar(toString(credentials.clientId))) {
throw new InvalidRequestError('Invalid parameter: `client_id`');
}

Expand Down
54 changes: 54 additions & 0 deletions lib/utils/param-util.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
'use strict';

/**
* @module ParamUtil
*/

/**
* Create a function to check, whether a value is one of the given types.
* @param types {...string[]}
* @return {function(*): boolean}
*/
function isInTypes (...types) {
return function (value) {
return types.includes(typeof value);
};
}

/**
* Check if a value is defined (not missing)
* @param value
* @return {boolean}
*/
function isDefined (value) {
// TODO: in future versions, consider changing this to `value !== undefined && value !== null && value !== ''`,
// which might be a breaking changes for some users
return !!value;
}

/**
* Safely converts a value to a string in the following order:
* - If the value is already a string, return it.
* - If the value has a `toString` method, call it and return the result.
* - If the value is `null` or `undefined`, return an empty string.
* - Otherwise, use the global `String` function to convert the value.
* @param value {*}
* @return {string}
*/
function toString(value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is too permissive because it will allow objects, arrays, etc to be stringified in a way that we don't want (eg: {} => [object Object], [] => "" and so on).

Also toString(undefined) causes an error to be thrown.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhensby let's collect a little spec, before further coding:

  • undefined, null'' (empty string)
  • stringstring
  • numbernumber.toString() (what to do with NaN and Infinity?
  • BigIntbigInt.toString()
  • Object → JSON stringify? Throw error?
  • Array → array.join ? JSON stringify? throw error?
  • Symbol → JSON stringify? Throw error?
  • Boolean → JSON stringify? Throw error?
  • function → toString() ? JSON stringify? Throw error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO: If it's not a scalar value I'd throw, plus boolean should probably throw. Numbers that are NaN or Infinity should also be rejected. For undefined or null, I'm not sure what the behaviour should be, I suppose that depends on context. If we have a body that does not have a value (eg: body.client_secret) and that is undefined I don't think we should be coercing that into a string, but then again maybe that's a check that should be upstream of this utility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For null and undefined I would personally throw, because these values are not expected to reach this method as they should always be handled prior with respective "Missing xyz" error.

if (typeof value === 'string') {
return value;
}

if (Object.prototype.hasOwnProperty.call(value, 'toString')) {
return value.toString();
}

if (value === null || value === undefined) {
return '';
}

return String(value);
}

module.exports = { isInTypes, isDefined, toString };
4 changes: 3 additions & 1 deletion lib/utils/scope-util.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
const isFormat = require('@node-oauth/formats');
const InvalidScopeError = require('../errors/invalid-scope-error');
const { isInTypes } = require('../utils/param-util');
const whiteSpace = /\s+/g;
const isString = isInTypes('string');

/**
* @module ScopeUtil
Expand All @@ -22,7 +24,7 @@ function parseScope (requestedScope) {
return undefined;
}

if (typeof requestedScope !== 'string') {
if (!isString(requestedScope)) {
throw new InvalidScopeError('Invalid parameter: `scope`');
}

Expand Down
Loading