HTTPError Error() avoid nil pointer dereference on nil Cause

[saurori]

The default renderer Error() method does not check for a nil error on the underlying HTTPError type. A nil pointer dereference can be triggered by calling for example on the default renderer: c.Render(500, nil).

This PR changes the HTTPError Error() method to use fmt.Sprint() instead of calling Error() on the underlying error in Cause.

[sio4]

Indeed! Thanks for the fix!

[fasmat]

I'm not a fan of this change. The point of the c.Error method and HTTPError is to give context to the error that triggered the response. The same effect without a change you could achieve at the moment by:

return errors.New("") instead of return c.Error(http.StatusInternalServerError, nil)

[fasmat]

Alternatively DefaultContext.Error should instantiate a cause here: https://github.com/gobuffalo/buffalo/blob/v0.18.5/default_context.go#L189 when the provided error is nil. Something the likes of:

if err == nil {
  err = errors.New("unknown cause")
}
return HTTPError{Status: status, Cause: err}

The reasoning is that there might be code out there that relies on Cause not being nil.

[sio4]

If the purpose of the exported function HTTPError.Error() is the same as its comment, and if there is a possibility that the Cause can be a nil value, the patch is totally meaningful because the current code could make runtime error which is bad and the patched code do the thing the comment described.

// HTTPError a typed error returned by http Handlers and used for choosing error handlers
type HTTPError struct {
	Status int   `json:"status"`
	Cause  error `json:"error"`
}

// Error returns the cause of the error as string.
func (h HTTPError) Error() string {
	return h.Cause.Error()
}

[fasmat]

If there's no cause, then I would expect the Error() function to return nil instead of an empty string. If you take a look at our default error handler the new behavior would return:

{
   "code": 500,
   "error": ""
}

While I think something like this

{
   "code": 500
}

or this

{
   "code": 500,
   "error": "unknown"
}

would be more appropriate for a nil error.

[sio4]

@saurori, I think adding something similar to @famat's commented code could be better in the perspective of usage.

Alternatively DefaultContext.Error should instantiate a cause here: https://github.com/gobuffalo/buffalo/blob/v0.18.5/default_context.go#L189 when the provided error is nil. Something the likes of:

if err == nil {
  err = errors.New("unknown cause")
}
return HTTPError{Status: status, Cause: err}

[saurori]

The point of this PR was to avoid a runtime panic if one passes nil to the Error() method. If a runtime panic is the desired outcome, then this patch is not meaningful.

If a runtime panic is not desirable, then what actually is returned if nil is passed is up for debate. I simplified it by letting Sprint handle that. I agree @fasmat suggestion provides more context vs just having <nil>.

[fasmat]

@sio4, @saurori: The more I think about HTTPError and how it wraps an existing error to add the additional context of which status code should be returned by the API the more I think the change should just be:

// Error returns the cause of the error as string.
func (h HTTPError) Error() string {
	if h.Cause != nil {
		return h.Cause.Error()
	}
	return "unknown cause"
} 

Since the incorrect behavior is clearly this method and not how HTTPError is instantiated. This would also return a nice response via the API without the need of additional changes.

[sio4]

Do you mean HTTPError.Error() function to return the value nil or string "nil"? It cannot return the nil value since the return type is string but will make a runtime error referencing nil.

If there's no cause, then I would expect the Error() function to return nil instead of an empty string. If you take a look at our default error handler the new behavior would return:

By the way, I think @saurori 's approach is not mainly about the usage but a possible runtime error.

Also, I agree to return a string "<nil>" is also not a good thing (the result of this patch). So I think the combination of @saurori 's patch and your quick explanation of adding Cause when context.Error() is called could be the better option.

[fasmat]

Do you mean HTTPError.Error() function to return the value nil or string "nil"? It cannot return the nil value since the return type is string but will make a runtime error referencing nil.

Yes you are right, I forgot for a moment there that string cannot be nil.

[sio4]

Ah, already some more comments in realtime :-) Actually, my typing speed is not good enough, especially for English.

I think we made some consensus now. :-)

[sio4]

Thanks for the quick update!