+972 52-548-6969
Exception.Message property considered harmful
I just went over the following pull request, where I found this nugget:
I have an admittedly very firm views on the subject of error handling. The difference between good error handling and the merely mediocre can be ten times more lines of code, but also hundred times less troubleshooting in production.
And a flat out rule that I have is that whenever you have a usage of the Exception.Message property, that means that you are discarding valuable information aside. This is a pretty horrible thing to do.
You should never use the Message property, always use the ToString(), so you’ll get the full details. Yes, that is relevant even if you are showing to the user. Because if you are showing an exception message to the user, you have let it bubbled up all the way, so you might as well give full details.
Comments
How do you consider error response in API design? I mean web based, either RESTful, SOAP etc. Will you return full stacktrace?
I have seen people talking about security concern over stacktrace. Personally, full exception message can definitely help trace and address issue. The only security concern I can see is people could figure out which library you are using. Then, go over each security bug version to try attach the system.
On the other hand, if keep everything up to date, then attack surface is kind of minimum.
I think the full stack trace could be also harmful for most of the situations but surely useful when doing one on one debugging or analysis. End users shouldn't be shown the details of an exception rather than "where the error was happened" (i.e. storing data, loading data, sending or receiving commands).
You can expose worlds of information about your system and topology just by pushing full stack trace to the end user - which, not always, is with good intentions.
Tbh I think Security considerations are mostly overblown, except in very narrow circumstances. Especially for RavenDB, where the source is available. In general, hiding stacktraces is like security thorugh obscurity, which has a poor track record.
Yes it builds a respect among users if you show them full stack trace instead of error message. They will feel unworthy and stupid when calling the helpdesk, so 50% of your work is done already :)
Jason He,
In almost all cases, I would prefer to return the full exception message to the user.There is a UX issue here, that you don't want to show a lot of potentially scary details to the user, but the details should be visible.
I understand the security issue, and I don't agree. If you can get hacked by the error message, that is not a secured system.
Tommi,
Not showing the user means that you won't have those details, period. And "where it happened" doesn't mean anything to the user. And the issue isn't just the stack trace, also the nested exception stack is critical.
Chrisitan,
Even without having the source available, if a stack trace showing up is risky that is a huge issue. Not a security practice.
Actual both Message and ToString (recursive messages + stacktrace) are never safe to send to the user. You will never know what the Exception message exposes (also consider that C# hasn't checked exceptions, and even Java has RuntimeExceptions). Do you know all Exceptions from your 3. part libraries? If security should be considered, you should always map your Exceptions to a controlled string. I don't think that the Stacktrace is an extra concern.
I don't think the point of this article is whether we should or not return the exception details of the exception but what part of the exception should be used for diagnosis purpose.
However, I can see use-cases where I would use .Message over .ToString() : for the case of Business treatment exception where the .Message is intended to be shown to the end user in an API (Eg: "A XXX with this name already exists" or "No more stock for this item".
Regards,
Alexandre
Stig,
The whole exception should not be considered sensitive information. And yes, I'm aware of pretty much all the details that would be exposed.
Exceptions are logged, transmitted and reported. From the get go, they are designed to assume that they are not containing sensitive information.
Doing something like:
new Exception("Failed to connect to " + conStrWithPass);is a REALLY bad idea, but it doesn't happen in general. And if it would, the sensitive information is already leaking all over the place anyway. (logs, traces, etc)Alexandre,
At a minimum, you need to show the whole hierarchy.
Failed to save XXX->Error saving to database->Constraint violation, name already existsAyende,
Why ? I talked about business exceptions returned from an API. The end user doesn't care at all that it was a database issue or a pre-emptive check. I fell that all that matters to her/him is that he/she should check the existing item or change the name before insertion.
In the case of technical exception, I completely agree that the whole .ToString() has to be used to log the exception.
However, I join the other guys here about the control of what is send in the pipes to the end user. I saw a common scenario about giving the user an ID to call the support and searching this ID in the exception log for having more details. This allows to give enough information to the end-user so that the support is still efficient without giving away any of the technical details to outside guys
Regards
Alexandre, agree. I often implement a Base BussinesException so I know i can sent the Message (and only this) to the client.
Ayende, but you don't know the source code of all 3. part libraries, especially not after a NuGet update.
Alexandre,
There is no such thing as a business exception vs. technical exception. There are just exceptions. There is validation, which is a business concern. Likely shouldn't be reported as an exception, but that is a singular focus, and pretty rare in general. I'm talking about something like: "failed to do X", how do you report and show that?
I'm sort of okay with giving the user an ID that the support team can lookup in the logs, but even that is often PITA and would cause breakage. But that is as far as I think we should deal with it.
Even so: "An error happened" is pretty useless thing to do, and "call support" is something that incurs a HUGE expense for the company. You want to provide enough details that they can fix that themselves.
Stig,
I do know what kind of information it is going to expose as exceptions, however. And if the exception message shows sensitive information, that is a bug, not something that should cause me to degrade error reporting.
Ayende,
Well, an exception already hold in itself a valuable information to the user: "your command couldn't complete" be it for a business reason or a technical reason matters less for an API consumer. We agree in communicating the information to the user so that it may solve her/his issue. In APIs it's the "business" information that I talked previously that hold this value and therefore should be returned ("No more stock for the product", "item already exists", etc.).
I think we have some different constraints in mind : running an API to serve the end user vs making users operate a highly technical product consumed by applications.
I think in your business the product is intended to be operated and maintained by the tech guys where any information she/he can have will increase the probability that she/he may be able to solve the issue without resorting to the support.
However for non-technical consumers, most of the time, technical information don't give them value at all. All that matters for them is "Is my operation successful ? If not, why ?". In case of business related issue, we can provide them a descriptive information or the error so that they can fix it. In case of technical issue (and maybe temporary), and the only valuable thing we can tell them is "Sorry, we couldn't do what you wanted, please retry later" (which already is valuable in the sense that they know that their operation failed).
Regards
Ayende, I do know how to concatenate safe SQL strings, and if I expose SQL injection vulnerability, that is a bug. :)
I guess it depends on the code base.
Stig,
Yes, if you have a SQL Injection issue, that is a serious issue. If you are making your life harder to avoid SQL Injection "just because", that is a problem.
Furthermore, if you actually have such an issue, you need to fix that, not paper over it.
Alexandre,
A non-technical person may not understand the error, but they can recognize it. They can tell that they have seen it before, what was done to fix it, etc.
Also, you may call it non-technical, but many users are domain experts, and as such, know what needs to be done, if only you'll give them enough information.
Looks like I started a debate on this topic. Personally I am stand with Oren, where I don't think there is any security concern, which is why I struggle to understand what other people says and try to raise this question to see if Oren have his opinion on that.
From UI/UX point of view, if you design your API error response well, stacktrace won't be viewable by ordinary users. First, format well, separate message and stacktrace to different properties. Inner exception too. While at UI side, depends on audience, show only enough and let support to view the network response.
Also, as Oren said, there is no business exception there. Any business related error will be around HTTP status code of 401, 403, 404 etc. None will reach 5xx. Anything reach 5xx is unexpected exception. Where it has little to no value to end user unless they are technical. None of them will contain any sensitive information. (400 is generated by service model validator, maximum issue is it might return the value from the API request. Like echo back through same tunnel, even that is user rare, since UI handles majority of validation already).
Where 4xx exception, won't have stacktrace, since business logic related errors are all hand crafted. They are not exception but error response. This is something we should be clear.
In ASP.NET 3, 4, they had really bad practice, where they use exception to drive error response. Thus, many people use exception to raise 4xx response. Where in ASP.NET core and ASP.NET 5, they changed approach. By drive people to use
IApiResult<T>for business logic related response.Oren, while I do agree with the need for capturing more detailed error information, sending it out to the user is not the best idea. From my experience, what works best is logging out the detailed error information (including full stack trace), but correlating this with a newly generated uuid. Then, return that error ID to the end user, displaying a user-friendly message and asking them to pass on the error reference to the support team. Looking up the error details is then super easy, as you just need to query (with whatever logs storage you're using) that ID. It's a win/win situation, because you keep your backend secure and don't leak any internal info, yet you can still quickly identify the problem.
Exception.Message to show a readable message. We show the nitty gritty details on a 2nd tab of the error window and include in a "copy to clipboard ".
That's good, appropriate usage.
Comment preview