Add shorthand way to return non-IntoResponse errors

[Lachstec]

Motivation

There have been some requests for a way to have a shorthand for returning errors as response that themselves do not implement IntoResponse. This is especially handy for development and debugging purposes. Closes #2671

Solution

Add a new struct InternalServerError<T>(pub T) that can wrap anything that implements Error and create a response from it. The response sets 500 as status code and contains the full error chain with descriptions in the response.

As this is potentially undesired behaviour, the provided docs state that this should only be used for debugging purposes.

[yanns]

• I think I'd put this into axum-extra as it's not something that should be used on production
• some users might still use it and expose some sensitive information on real web service. To make this less likely to happen, I'd suggest not to output any information in release mode.

[Lachstec]

I agree with both points. Regarding the second, do you mean via cfg(debug_assertions) to check whether its a debug or release build and only send the error information when it is a debug build and just an empty 500 in release?

[SabrinaJewson]

I’m very sceptical of changing behaviour based on debug_assertions. I know that some people enable debug_assertions for release builds for greater assurance that their service doesn’t break; likewise, you might want to disable debug_assertions for development if there’s a particularly slow code path, for example. It just seems to break the principle of least surprise to do this, as it’s not what the feature is intended for.

I do agree that a sensible default might be to avoid ever emitting 500 errors with bodies. But if this is done I’d like to see it done consistently: currently, the status quo is that we don’t strip 500 bodies, as seen in Json and a number of rejections like MissingRequestExtension, MissingPathParams, MatchedPathMissing, NestedPathRejection, that kind of thing.

Maybe we could switch everything to logging at some point? I don’t think this is a siginificant loss for development purposes: reading logs is about as easy as reading response bodies, and even easiër if you’re sending the request programmatically.

[mladedav]

Personally, I'm also not a fan of different behavior based on debug_assertions. If we wanted to have it configurable, I think going with something similar to RUST_BACKTRACE=1 (which we can also mention in the default response) would be more appropriate.

A few things to create follow up issues from (which are out of scope here) I see right now are:

• Consider moving information from responses to tracing
• This is second newtype for a specific HTTP status code (recently we have added struct NoContent;) so we might want to consider adding more of these in bulk.

[yanns]

I do agree that a sensible default might be to avoid ever emitting 500 errors with bodies. But if this is done I’d like to see it done consistently: currently, the status quo is that we don’t strip 500 bodies, as seen in Json and a number of rejections like MissingRequestExtension, MissingPathParams, MatchedPathMissing, NestedPathRejection, that kind of thing.

I don't think the sensible default should be to avoid emitting 500 errors with bodies, but the sensible default should be not to emit bodies that contain sensible information that can be used by malicious users.
Returning an invalid request with the json path of invalid data should not be an issue. We just give some information that the user already knows.
Returning a stack trace is a sensible information, and should not be emitted, at least in a production environment.

What I was trying to achieve with cfg(debug_assertions) is a different experience in "dev" and in "production" mode. I suggested using that because we're already doing it with the #[debug_handler] macro.
Having different behavior in "dev" and "production" mode can be surprising, but it can also be a nice developer experience. It's what we did with the play framework, and it was working well. But that's more a framework than a library, so it's maybe not suitable for axum.

Maybe my expectations are too high. Just moving this into axum-extra could be sufficient.

[SabrinaJewson]

I get where you’re coming from with regards to sensitive information. I think that users will prefer to use one consistent method of debugging, whether it be via response bodies or via logs. Since logs are more general, it might be useful to guide them toward that? To this end, consistently stripping bodies of 500 responses could be a benefit, as it encourages not relying on 500 bodies (since they’re likely to be omitted).

It also seems a little difficult/opinionated to draw such a line between “sensitive” and “non-sensitive” stuff. But I haven’t put much thought into that. I do agree that 4XX errors should generally be treated as non-sensitive; the difficulty in my eyes is whether non-sensitive 5XX responses make sense as a concept to begin with.

[yanns]

It's perfectly fine to return non sensitive information in 500 errors. For example, at work, we do that.

We can even return some useful information, like: "please contact our support with the id xyz" where the id is being used in all correlated logs.
It's also possible for some inexperienced to return sensitive information in 400 errors. For example if you return the information about which json library you're using in which version.

So you're right, the line between sensitive and non-sensitive information can be difficult to draw, and is not tied to the http status.

We could embrace tracing yes. It might just be difficult for developers to configure the subscribers as they want. But it's something we can iterate on.
We could also have one global function that is being called for any error, letting the developers decide how to deal with that. But I'm not sure if it's easy to introduce for axum as it would be quite a after-though not part of the initial design.

[jplatte]

Having thought a little while about it, I like the always-log (tracing) solution. Idk whether axum has any error-level traces at the moment, but since this looks to be going into axum-extra anyways, I'm not as concerned about keeping existing conventions.

[Lachstec]

After reading through every suggestion here, my personal preference would also be to embrace tracing. It feels cleaner to me. One question that I still have:

When using logging, I see no real point that discourages using this in production, as it simply returns an error and logs it, thus not exposing it to users. Would it still make sense to put it in axum-extra then?

Edit: One possible problem would be if an error contains sensitive data and that data ends up in the logs. But I am torn wether that is a case of using it improperly.

[yanns]

By sensitive information, there are actually two kinds:

• personal data, that only the user should see -> this data can be returned in http responses, but should not be logged
• information helping malicious users -> this data should not be returned in http responses, but can be logged

By looking at what we would log, I don't see any possibility of having personal data in it. So it should be ok to log.

[jplatte]

Suggested change

	/// Convenience response to create an error response from a non-IntoResponse error
	/// Convenience response to create an error response from a non-[`IntoResponse`] error

[jplatte]

Suggested change

	/// the IntoResponse trait itself. This type should only be used for debugging purposes or internal
	/// the `IntoResponse` trait itself. This type should only be used for debugging purposes or internal

[jplatte]

Tracing has built-in support for dyn Errors as values you can capture as fields. I think it would be good to ude that here and add an error field, instead of string-interpolating the error into the message.

[Lachstec]

Thats very nice, I did not know that.

[jplatte]

The % you added bypasses the dyn Error support.. Did it not compile previously?

[Lachstec]

Ah okay, was not aware of that. I did it that way because with error!(error = err), the compiler complains that a bound is not satisfied:

error[E0277]: the trait bound `T: tracing::Value` is not satisfied
  --> axum-extra/src/response/error_response.rs:33:9
   |
33 |         error!(error = err);
   |         ^^^^^^^^^^^^^^^^^^^ the trait `tracing::Value` is not implemented for `T`, which is required by `&T: tracing::Value`
   |
   = note: required for `&T` to implement `tracing::Value`
   = note: required for the cast from `&&T` to `&dyn tracing::Value`
   = note: this error originates in the macro `$crate::valueset` which comes from the expansion of the macro `error` (in Nightly builds, run with -Z macro-backtrace for more info)

I googled about this and found the approach with %

[jplatte]

You need to cast to &dyn Error. This compiles: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b08c7457dc0862f279b3b96daa7fca24

[Lachstec]

That fixes the trait bound, but it then asks me to add a 'static to T. And the CI in cargo hack still fails