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

RaiseError does not work when "boomify" is passed in #468

Closed
virginiam1203 opened this issue May 20, 2024 · 7 comments
Closed

RaiseError does not work when "boomify" is passed in #468

virginiam1203 opened this issue May 20, 2024 · 7 comments

Comments

@virginiam1203
Copy link
Contributor

virginiam1203 commented May 20, 2024

I have an application that throws some custom CodedError messages which extend the Error class. Since updating hapi-auth-jwt2, I am now seeing this custom error messages replaced by an "AssertError: Cannot wrap non-Error object\n at exports.boomify " error. I have traced the problem to this bit of code in internals.authenticate :

catch (validate_err) {
     return {
       error: internals.raiseError(
         options,
         request,
         h,
         'boomify',
         validate_err,
         { decoded }
       ),
       payload: {
         credentials: decoded,
       },
     };
   }

When I debug into raiseError, I see that my error is not being handled correctly.

 internals.raiseError = function raiseError(
  options,
  request,
  h,
  errorType,
  errorOrMessage,
  extraContext,
  isMissingToken
) {
  let errorContext = {
    errorType: errorType,
    message: errorOrMessage.toString(),
    error: typeof errorOrMessage === 'object' ? errorOrMessage : undefined,
  };
  Object.assign(errorContext, extraContext);

  if (internals.isFunction(options.errorFunc)) {
    errorContext = options.errorFunc(errorContext, request, h);
  }
  // Since it is clearly specified in the docs that
  // the errorFunc must return an object with keys:
  // errorType and message, we need not worry about
  // errorContext being undefined

  const error = Boom[errorContext.errorType](
    errorContext.message,
    errorContext.scheme,
    errorContext.attributes
  );

  return isMissingToken
    ? Object.assign(error, {
      isMissing: true,
    })
    : error;
} 

The errorOrMessage parameter is never treated as an error, so when it gets passed to Boomify, that function throws the error that it cannot wrap a non-error object

exports.boomify = function (err, options) {

    Hoek.assert(err instanceof Error, 'Cannot wrap non-Error object');

    options = options || {};

    if (options.data !== undefined) {
        err.data = options.data;
    }

    if (options.decorate) {
        Object.assign(err, options.decorate);
    }

    if (!err.isBoom) {
        return internals.initialize(err, options.statusCode ?? 500, options.message);
    }

    if (options.override === false ||                           // Defaults to true
        !options.statusCode && !options.message) {

        return err;
    }

    return internals.initialize(err, options.statusCode ?? err.output.statusCode, options.message);
}; 

I believe this would also be an issue anywhere else you pass "boomify" into the raiseError method. My suggested fix is to check to see whether errorOrMessage is an error and only call boomify with an error. I am willing to make the necessary changes and open a pull request.

@virginiam1203
Copy link
Contributor Author

virginiam1203 commented May 20, 2024

This change appears to fix it:

internals.raiseError = function raiseError(
  options,
  request,
  h,
  errorType,
  errorOrMessage,
  extraContext,
  isMissingToken
) {
  let errorContext = {
    errorType: errorType,
    message: errorOrMessage.toString(),
    error: typeof errorOrMessage === 'object' ? errorOrMessage : undefined,
  };
  Object.assign(errorContext, extraContext);

  if (internals.isFunction(options.errorFunc)) {
    errorContext = options.errorFunc(errorContext, request, h);
  }
  // Since it is clearly specified in the docs that
  // the errorFunc must return an object with keys:
  // errorType and message, we need not worry about
  // errorContext being undefined
  let error;
  if (errorOrMessage instanceof Error && errorContext.errorType === 'boomify') {
    error = Boom.boomify(errorOrMessage);
  }
  else {
    error = Boom[errorContext.errorType](
      errorContext.message,
      errorContext.scheme,
      errorContext.attributes
    );
  }

  return isMissingToken
    ? Object.assign(error, {
      isMissing: true,
    })
    : error;
};

The function could probably be generally cleaned up a bit further because errorContext.error does not seem to ever be used, so perhaps the check could be moved sooner to determine what is being handled and create two clear paths, one for a message and one for an error.

@virginiam1203
Copy link
Contributor Author

Hello again. I do not know if the thumbs up on my last post was just a general acknowledgement or a "go ahead" indicator for a pull request, but I do not appear to be able to open a pull request on this project without write permissions. Here is the code I would like to submit:

// allow custom error raising or default to Boom if no errorFunc is defined
internals.raiseError = function raiseError(
  options,
  request,
  h,
  errorType,
  errorOrMessage,
  extraContext,
  isMissingToken
) {
  let error;
  if (errorOrMessage instanceof Error && errorType === 'boomify') {
    error = Boom.boomify(errorOrMessage);
  }
  else {
    let errorContext = {
      errorType: errorType,
      message: errorOrMessage.toString(),
      error: typeof errorOrMessage === 'object' ? errorOrMessage : undefined,
    };
    Object.assign(errorContext, extraContext);

    if (internals.isFunction(options.errorFunc)) {
      errorContext = options.errorFunc(errorContext, request, h);
    }
    // Since it is clearly specified in the docs that
    // the errorFunc must return an object with keys:
    // errorType and message, we need not worry about
    // errorContext being undefined

    error = Boom[errorContext.errorType](
      errorContext.message,
      errorContext.scheme,
      errorContext.attributes
    );
  }

  return isMissingToken
    ? Object.assign(error, {
      isMissing: true,
    })
    : error;
}; 

Please let me know any next steps.

@nelsonic
Copy link
Member

Hi @virginiam1203, apologies for the lack of clarity of the "👍 " ... I was reading on my iPhone 📱
If you have a fix that works for you and would prefer it to be integrated into the hapi-auth-jwt2 plugin, 📦
please feel free to create a pull request with the appropriate tests. 👌

@virginiam1203
Copy link
Contributor Author

I have opened the PR at #469

It was a doozy to get this tested because I was sure that the "ASPLODE" error test case should also be impacted, but I could never get it to actually output that error message. It turns out the Hapi server will always wrap any 500 error in a generic "Internal Server Error" message, so I had to write a test case for a 404 boom error instead. It makes sense that I noticed the error in the code I work on because we do use a CodedError class to throw things like 404 errors in our validation method. In this code I used Boom as a shortcut to throw a coded error below 500.

@nelsonic
Copy link
Member

Saw the PR, code & test looks good. 👍

@virginiam1203
Copy link
Contributor Author

Hello, just checking in to ask if I need to do anything else to get that PR reviewed. I see it's been sitting. Is my part done?

nelsonic added a commit that referenced this issue Jun 4, 2024
…ject-passed-into-raiseError

Issue #468 handle error object passed into raise error
asntc added a commit that referenced this issue Jun 4, 2024
PR: Handle Error Object passed into raise error (Boomify) #468 ref: #469
@nelsonic
Copy link
Member

nelsonic commented Jun 4, 2024

@virginiam1203 sorry for the delay. ⏳ 🤦‍♂️
Your code is included in [email protected] 📦 🚀

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