fix(rest): pass a better detailed error message into the response body #1598

shimks · 2018-08-07T20:50:54Z

This PR configures writer.ts to use strong-error-handler to transform the thrown error.
Please take a look at the modified test cases to see what the response body would now look like.

fixes The error response for a request with malformed JSON body provides too little information #1434
depends on add types and introduce writeErrorToResponse strong-error-handler#77

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2018-08-07T21:49:16Z

packages/rest/src/writer.ts

@@ -6,6 +6,7 @@
 import {OperationRetval, Response} from './types';
 import {HttpError} from 'http-errors';
 import {Readable} from 'stream';
+const buildResponseData = require('strong-error-handler/lib/data-builder');


This is a bit hacky. Would it be better to have strong-error-handler module to export the data-builder?

I think it would be. I'll make a PR on strong-error-handler to export the function

virkt25 · 2018-08-08T15:37:08Z

packages/rest/src/writer.ts

-  const errObj: Partial<HttpError> = {
-    statusCode,
-  };
+  const errObj = buildResponseData(e, {});


Should we still keep the type? Partial<HttpError>

I think yes. Let me verify if the given data does in fact fit the HttpError type

bajtos · 2018-08-13T09:51:54Z

Cross-posting from #1434:

Ideally, error responses should contain a single top-level error property that contains an object with additional details. This makes it easier to consume the response in places where the response body is available only (no access to response status code).

I am proposing to rework our error handling to delegate all work to strong-error-handler module.

As I am envisioning this part of LB4, we should:

Obtain error-handler configuration via dependency injection - see RejectProvider. At minimum, we need to support debug and safeField options - see https://github.com/strongloop/strong-error-handler#options.
Modify the obtained error-handler config to always disable error logging to console (we have our own error logger in place).
Call strong-error-handler to produce the response for the given error and config.

The last step requires a small refactoring in strong-error-handler, I think we need to extract the following block of code into a new function that should be available to strong-error-handler consumers.

https://github.com/strongloop/strong-error-handler/blob/8b35494da09f945c6a7b1b3c38191d0b014a0872/lib/handler.js#L39-L60

     if (res._header) {
       debug('Response was already sent, closing the underlying connection');
       return req.socket.destroy();
     }
 
     // this will alter the err object, to handle when res.statusCode is an error
     if (!err.status && !err.statusCode && res.statusCode >= 400)
       err.statusCode = res.statusCode;
 
     var data = buildResponseData(err, options);
     debug('Response status %s data %j', data.statusCode, data);
 
     res.setHeader('X-Content-Type-Options', 'nosniff');
     res.statusCode = data.statusCode;
 
     var sendResponse = negotiateContentProducer(req, warn, options);
     sendResponse(res, data);
 
     function warn(msg) {
       res.header('X-Warning', msg);
       debug(msg);
     }

Assuming the new function is called writeError, here is a mock-up usage in writeErrorToResponse to illustrate my point:

export function writeErrorToResponse(
  {request, response}: HandlerContext, 
  error: Error,
  options: ErrorHandlerOptions,
) {
    strongErrorHandler.writeError(error, request, response, options);
}

I think we can remove writeErrorToResponse entirely and call strong-error-handler from the action method of RejectProvider.

Thoughts?

bajtos · 2018-08-13T09:55:35Z

packages/rest/src/writer.ts

@@ -6,6 +6,7 @@
 import {OperationRetval, Response} from './types';
 import {HttpError} from 'http-errors';
 import {Readable} from 'stream';
+const buildResponseData = require('strong-error-handler').buildResponseData;


Let's add a .d.ts file to strong-error-handler so that we can use it in a type-safe way here.

import {buildResponseData} from 'strong-error-handler';

bajtos · 2018-08-13T09:57:24Z

packages/rest/src/writer.ts

@@ -59,17 +60,16 @@ export function writeResultToResponse(
 */
 export function writeErrorToResponse(response: Response, error: Error) {
  const e = <HttpError>error;
-  const statusCode = (response.statusCode = e.statusCode || e.status || 500);
  if (e.headers) {


By moving to strong-error-handler, we will loose this feature. However, I believe were not using error.headers field so far (LB 3.x, 4.x), thus it should not be a problem. We can always add support for e.headers later in strong-error-handler if needed.

bajtos · 2018-08-13T09:59:43Z

packages/rest/src/writer.ts

-  };
+  const errObj: Partial<HttpError> = buildResponseData(e, {});
+  response.statusCode = errObj.statusCode!;
+
  if (e.expose) {


strong-error-handler is intentionally using a different approach. Instead of asking the code throwing an error to decide whether the error is safe or not to be shown to the client, strong-error-handler asks the app developer/environment configuration to decide whether it's ok to show sensitive information.

I believe the approach used by strong-error-handler makes more sense, because the same error (e.g. "file /etc/passwd" cannot be found) usually needs different treatment in production (don't show any details) vs. in development (show all details to make debugging easier).

Let's remove e.expose please.

bajtos

☝️

shimks

There is a question to answer as to how we want to deal with expose property from errors generated through HttpError. The default behavior for setting expose is if statusCode is < 500, it's set to true. If it's >= 500, it's set to false. This can also be manually set through the HttpError constructor as well.

I'm concerned that users would expect expose to work as intended, and I don't know how to discourage users from using it. Would documentation on this would be enough?

On a side note, please remind me to add in documentation

bajtos · 2018-08-14T09:17:47Z

packages/rest/src/providers/reject.provider.ts

@@ -24,7 +31,8 @@ export class RejectProvider implements Provider<Reject> {
  action({request, response}: HandlerContext, error: Error) {
    const err = <HttpError>error;
    const statusCode = err.statusCode || err.status || 500;
-    writeErrorToResponse(response, err);
+    const writeErrorToResponse = errorHandler(this.errorHandlerOptions);
+    writeErrorToResponse(err, request, response);

    // Always log the error in debug mode, even when the application
    // has a custom error logger configured (e.g. in tests)


I think this debug log is no longer needed because strong-error-handler is already printing a debug log. Thoughts?

There isn't a strict equivalent of this debug statement (the request method/url information is lost here) but I don't think it's a big deal to remove the debug log here.

bajtos · 2018-08-14T09:19:06Z

packages/rest/src/writer.ts

-  response.write(JSON.stringify(errObj));
-  response.end();
-}
+export const defaultErrorHandlerOptions: ErrorHandlerOptions = {log: false};


Let's move this to RejectProvider please. Now that writeErrorToResponse is gone from writer.ts, it feels weird to be to see defaultErrorHandlerOptions defined here.

Should we move defaultErrorHandlerOptions to the provider as a property or outside of it as a constant? A problem I see with it being a property is that binding REJECT_OPTIONS to the object may become more awkward; app.bind(RestBindings.SequenceActions.REJECT_OPTIONS).to(RejectProvider.defaultErrorHandlerOptions); does not seem ideal when we want users to register providers separately.

I would keep defaultErrorHandlerOptions as a constant outside of the class.

bajtos · 2018-08-14T09:19:58Z

packages/rest/src/rest.component.ts

@@ -48,6 +49,9 @@ export class RestComponent implements Component {
    @inject(RestBindings.CONFIG) config?: RestComponentConfig,
  ) {
    app.bind(RestBindings.SEQUENCE).toClass(DefaultSequence);
+    app
+      .bind(RestBindings.SequenceActions.REJECT_OPTIONS)
+      .to(defaultErrorHandlerOptions);


A question to consider: should REJECT_OPTIONS be part of RestBinding.CONFIG, alongside port etc.?

If we decide to keep it as a top-level binding, then please move the key from RestBinding.SequenceActions to RestBindings namespace. IMO, options are not a sequence action.

It does seem awkward that REJECT_OPTIONS is a part of SequenceActions

bajtos · 2018-08-14T09:21:01Z

packages/rest/src/providers/reject.provider.ts

@@ -24,7 +31,8 @@ export class RejectProvider implements Provider<Reject> {
  action({request, response}: HandlerContext, error: Error) {
    const err = <HttpError>error;
    const statusCode = err.statusCode || err.status || 500;
-    writeErrorToResponse(response, err);
+    const writeErrorToResponse = errorHandler(this.errorHandlerOptions);


Please note that we should always set log: false, otherwise we may end up with two logs for each failed request (see this.logError below).

Creating a new handler function every time an error is handled is suboptimal performance wise. V8 has to allocate a new closure and create a new dynamic function, which puts more pressure on the garbage collector. Additionally, V8 may not be able to optimize the handler function when it's always created anew (it definitely used to be a problem back in CrankShaft days, before Node.js 8.3 upgraded to a TurboFan-enabled version of V8).

That's why I proposed to modify strong-error-handler to export a function accepting (err, req, res, options).

Another reason is that technically, the error handler middleware accepts also a next callback and I am not sure if it's a good idea to rely on the fact that the middleware never calls next.

Sorry for not providing a detailed explanation from the beginning.

Please note that we should always set log: false, otherwise we may end up with two logs for each failed request (see this.logError below).

On the second thought, if we refactor strong-error-handler to export a new function that can be used directly from LB4, then I think a better alternative is to leave logging out of this newly exported function. That way we don't have to worry about log flag because it will be always ignored.

Thoughts?

bajtos · 2018-08-14T09:34:18Z

There is a question to answer as to how we want to deal with expose property from errors generated through HttpError. The default behavior for setting expose is if statusCode is < 500, it's set to true. If it's >= 500, it's set to false. This can also be manually set through the HttpError constructor as well.

Oh, I didn't realize expose property is coming from http-errors package. We need to eventually improve strong-error-handler to honor this property then. Can you create a new issue in strong-error-handler to keep track of that work please?

I'm concerned that users would expect expose to work as intended, and I don't know how to discourage users from using it. Would documentation on this would be enough?

This is a fair point, thank you for raising it. For the scope of this pull request, let's describe this limitation in our docs and point users to the strong-error-handler issue tracking support for expose.

bajtos

Almost there :)

bajtos · 2018-08-23T14:28:07Z

packages/rest/src/keys.ts

+   * options
+   */
+  export const ERROR_HANDLER_OPTIONS = BindingKey.create<ErrorHandlerOptions>(
+    'rest.sequence.actions.reject.options',


Can we remove a reference to sequence actions from the key name too? E.g. rest.errorHandlerOptions.

bajtos · 2018-08-23T14:28:43Z

packages/rest/src/providers/reject.provider.ts


-const debug = require('debug')('loopback:rest:reject');
+export const defaultErrorHandlerOptions: errorHandler.ErrorHandlerOptions = {
+  log: false,


Do we still need log considering that writeErrorToResponse is never logging anything?

Oops, can't believe I missed that

bajtos · 2018-08-23T14:29:49Z

packages/rest/src/providers/reject.provider.ts


 export class RejectProvider implements Provider<Reject> {
  constructor(
    @inject(RestBindings.SequenceActions.LOG_ERROR)
    protected logError: LogError,
+    @inject(RestBindings.ERROR_HANDLER_OPTIONS)
+    protected errorHandlerOptions?: errorHandler.ErrorHandlerOptions,


If the options are optional (as you specified in the type annotation), then you also need to configure @inject to be optional. In which case it's better to use property-level injection instead of constructor-level injection.

Alternatively, keep the options required and fix the typescript signature (remove the question mark).

bajtos · 2018-08-23T14:30:34Z

packages/rest/src/providers/reject.provider.ts

 import {RestBindings} from '../keys';
+import * as errorHandler from 'strong-error-handler';


Any reason for not using the following?

import {ErrorHandlerOptions, writeErrorToResponse} from 'strong-error-handler';

bajtos · 2018-08-23T14:32:31Z

packages/rest/test/integration/http-handler.integration.ts

@@ -437,6 +482,9 @@ describe('HttpHandler', () => {
      .to(createUnexpectedHttpErrorLogger());
    rootContext.bind(SequenceActions.SEND).to(writeResultToResponse);
    rootContext.bind(SequenceActions.REJECT).toProvider(RejectProvider);
+    rootContext
+      .bind(RestBindings.ERROR_HANDLER_OPTIONS)
+      .to(defaultErrorHandlerOptions);


I think it would be nicer to have RestBindings.ERROR_HANDLER_OPTIONS as an optional binding, so that the tests don't have to provide it. Not a big deal though.

jannyHou · 2018-08-30T02:53:48Z

packages/rest/test/integration/http-handler.integration.ts

+    });
+
+    it('respects error handler options', async () => {
+      rootContext.bind(RestBindings.ERROR_WRITER_OPTIONS).to({debug: true});


a question: how is the option {debug: true} tested?

When the error handler is set to debug mode, it returns details of 500 errors in the HTTP responses. By default, debug is disabled and HTTP responses show only a generic HTTP status message.

This is important for security to prevent the error handler from leaking sensitive informations like filesystem paths or hostnames & ports. This information is often included in the error message and/or error properties.

$ node > fs.readFileSync('/etc/passwords') Error: ENOENT: no such file or directory, open '/etc/passwords'

See also the assertion on lines L434-438 above, where the error handler in production mode returns the following output:

error: { message: 'Internal Server Error', statusCode: 500, },

Compare it with debug mode:

error: { message: 'Bad hello', statusCode: 500, },

@bajtos 👍 Thanks for the detailed answer, got it. Flag debug controls the security level which affects the returned error message.

bajtos · 2018-08-30T10:47:29Z

packages/rest/src/keys.ts

+   * Binding key for setting and injecting Reject action's error handling
+   * options
+   */
+  export const ERROR_WRITER_OPTIONS = BindingKey.create<ErrorHandlerOptions>(


Should be ErrorWriterOptions after recent changes in the strong-error-handler pull request.

bajtos · 2018-08-30T10:50:40Z

packages/rest/test/integration/http-handler.integration.ts

+    });
+
+    it('respects error handler options', async () => {
+      rootContext.bind(RestBindings.ERROR_WRITER_OPTIONS).to({debug: true});


When the error handler is set to debug mode, it returns details of 500 errors in the HTTP responses. By default, debug is disabled and HTTP responses show only a generic HTTP status message.

This is important for security to prevent the error handler from leaking sensitive informations like filesystem paths or hostnames & ports. This information is often included in the error message and/or error properties.

$ node > fs.readFileSync('/etc/passwords') Error: ENOENT: no such file or directory, open '/etc/passwords'

See also the assertion on lines L434-438 above, where the error handler in production mode returns the following output:

error: { message: 'Internal Server Error', statusCode: 500, },

Compare it with debug mode:

error: { message: 'Bad hello', statusCode: 500, },

jannyHou

…e body

shimks requested review from bajtos and raymondfeng as code owners August 7, 2018 20:50

shimks force-pushed the better-error branch from 61c2c6b to bd5f060 Compare August 7, 2018 20:51

raymondfeng reviewed Aug 7, 2018

View reviewed changes

bajtos mentioned this pull request Aug 8, 2018

Monthly Milestone - August 2018 🍉⛱ #1581

Closed

31 tasks

virkt25 reviewed Aug 8, 2018

View reviewed changes

shimks force-pushed the better-error branch from ff9048a to a68a6d2 Compare August 8, 2018 16:04

shimks mentioned this pull request Aug 8, 2018

add types and introduce writeErrorToResponse loopbackio/strong-error-handler#77

Merged

2 tasks

bajtos reviewed Aug 13, 2018

View reviewed changes

shimks force-pushed the better-error branch 2 times, most recently from a7270af to 57146cd Compare August 13, 2018 21:20

shimks commented Aug 13, 2018

View reviewed changes

bajtos reviewed Aug 14, 2018

View reviewed changes

shimks force-pushed the better-error branch from 6755841 to 33a702d Compare August 14, 2018 21:22

shimks mentioned this pull request Aug 22, 2018

Support expose property of http-errors loopbackio/strong-error-handler#78

Open

bajtos reviewed Aug 23, 2018

View reviewed changes

shimks force-pushed the better-error branch from 33a702d to 4e180b2 Compare August 24, 2018 16:37

b-admike approved these changes Aug 29, 2018

View reviewed changes

jannyHou reviewed Aug 30, 2018

View reviewed changes

bajtos approved these changes Aug 30, 2018

View reviewed changes

shimks force-pushed the better-error branch 3 times, most recently from f95fa57 to ba67cb4 Compare August 30, 2018 17:04

jannyHou approved these changes Aug 30, 2018

View reviewed changes

fix(rest): use strong-error-handler for writing errors to the respons…

34b7882

…e body

shimks force-pushed the better-error branch from ba67cb4 to 34b7882 Compare August 30, 2018 19:22

shimks merged commit ac011f8 into master Aug 31, 2018

shimks deleted the better-error branch August 31, 2018 02:59

bajtos mentioned this pull request Aug 31, 2018

docs(rest): describe HTTP error responses & handler options #1662

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rest): pass a better detailed error message into the response body #1598

fix(rest): pass a better detailed error message into the response body #1598

shimks commented Aug 7, 2018 •

edited by bajtos

Loading

raymondfeng Aug 7, 2018

shimks Aug 8, 2018

virkt25 Aug 8, 2018

shimks Aug 8, 2018

bajtos commented Aug 13, 2018

bajtos Aug 13, 2018

bajtos Aug 13, 2018

bajtos Aug 13, 2018

bajtos left a comment

shimks left a comment •

edited

Loading

bajtos Aug 14, 2018

shimks Aug 14, 2018

bajtos Aug 14, 2018

shimks Aug 14, 2018

bajtos Aug 14, 2018

bajtos Aug 14, 2018

shimks Aug 14, 2018

bajtos Aug 14, 2018

bajtos Aug 14, 2018

bajtos Aug 14, 2018

bajtos commented Aug 14, 2018

bajtos left a comment

bajtos Aug 23, 2018

bajtos Aug 23, 2018

shimks Aug 24, 2018

bajtos Aug 23, 2018

bajtos Aug 23, 2018

bajtos Aug 23, 2018

jannyHou Aug 30, 2018

bajtos Aug 30, 2018

jannyHou Aug 30, 2018

bajtos Aug 30, 2018

bajtos Aug 30, 2018

jannyHou left a comment

		import {RestBindings} from '../keys';
		import * as errorHandler from 'strong-error-handler';

fix(rest): pass a better detailed error message into the response body #1598

fix(rest): pass a better detailed error message into the response body #1598

Conversation

shimks commented Aug 7, 2018 • edited by bajtos Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

shimks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Aug 14, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

shimks commented Aug 7, 2018 •

edited by bajtos

Loading

shimks left a comment •

edited

Loading