NotFoundResult throws NullReferenceException

[crgolden]

Description

When an id is not found on a GetAsync(id) request, the controller properly returns a NotFoundResult. However, this seems to break after that and what is actually returned to the client is a NullReferenceException.

Most concerning from a security standpoint is that the stack trace is returned.

The most common problem is when detailed internal error messages such as stack traces, database dumps, and error codes are displayed to the user (hacker). These messages reveal implementation details that should never be revealed.

Reference: Improper Error Handling

I updated my sample repo with a simple integration test (NotFound) that reproduces this behavior. Please advise if I am missing something.
...

Environment

• JsonApiDotNetCore Version: 4.0.0-alpha3
• Other Relevant Package Versions: not sure

[crgolden]

the stack trace is returned

I see where there is a method DisableDetailedErrorsIfProduction that is disabling these errors if the "ASPNETCORE_ENVIRONMENT" environment variable equals "Production", but not everyone's production environments set that variable to the same value.

The method is setting the DisableErrorStackTraces and DisableErrorSource properties on the JsonApiOptions class, but when I try to set the same properties on the JsonApiOptions class available in the AddJsonApi method, they aren't there. Could these options be added to setup for people whose production environments set the "ASPNETCORE_ENVIRONMENT" variable to something besides "Production" (like "prod", etc.)?

[maurei]

Hi @crgolden, thanks again for opening up an issue. Your feedback has been pretty helpful.

There's a few things going on here

Stacktraces in error results

It indeed seems that the settings for including stack traces in the error outputs is communicated throughout the framework using static fields on the singleton IJsonApiOptions service. I am not a great fan of that. As you're suggesting, I would prefer to have this configuration exposed as as a normal class members.

Unfortunately right now this static config is accessed in places where DI currently can't reach (extension methods of ModelState, non-DI-registered JsonApiException factories) and there is no easy quick fix for that. I'm going to tackle that later because we want to focus on the full release of v4 before reworking parts of the framework like these.

For now I've exposed the internally used .EnableDetailedErrors() which gives you more control over this option and which should support your usecase of 'prod'. See the startup in the example project

[maurei]

NullReferenceException

Thanks for pointing that out. I've identified the issue and working on a fix. Note that in your test you're expecting a payload, the json:api spec spec doesn't necessarily require the server to return a payload. In our current implementation we do however return an error object.

[maurei]

Alpha4 has been released and should be available in NuGet as a prerelease any minute

[crgolden]

@maurei Thanks so much for your explanations - they are very helpful.

I'll have to wait on trying out the new version. My workplace has not put .NET Core 3.0 into its internal CI pipelines yet. I realize that in order to use features like MVC, ASP.NET Core 3.0 is all-or-nothing now due to the Shared Framework and discontinuation of individual NuGet packages (like Mvc.Core). It may have been impractical for you guys to support ASP.NET Core 3.0 without targeting netcoreapp3.0.

[maurei]

@crgolden Indeed, it wasn't feasible to introduce such backward compatibility.

You can work around the null reference exception you encountered by relying on conventional routing, i.e. don't use [ApiController][Route(" ... ")][DisableRoutingConvention]. If you really need use that, you should be able to fix the null reference exception by overriding the methods of your controller that call NotFound() and replace it with NotFound(null) like I did here

[crgolden]

Ahh... ok. Thanks for the tip.

The reason I'm using the [DisableRoutingConvention] attribute is I'm relying on versioning via the URL path and my controller-level Route attributes with the "api/v{version:apiVersion}/[controller]" template seem to be ignored otherwise.

I have tried setting the namespace on the JsonApiOptions, but when I do that the versioning breaks. Maybe I'm missing something simple there.

Thanks again.

[maurei]

Interesting, I didn't know about the features addressed in that reference. This means you're currently using multiple versions for a particular route, right?

That currently won't work: the value of namespace in JsonApiOptions is added to the controller template "as is" and there is no interaction with ApiVersionAttribute. This is something we should look into, but won't be fixed for v3.1.

In your case I would recommend to stick to attribute-based routing and work around the NullReferenceException as explained in my previous post.

For better configurability of stacktraces in error responses, you can add an extension similar to this one which should be enough for your needs.

[crgolden]

@maurei Thanks for the response.

I did have a chance to try out the v4.0.0-alpha4 release. There were a surprising number of breaking changes. I have been able to work through most of them, but I would like to share a few observations:

1. Entity/Resource splits: I have read the numerous threads in your repo about why you removed this, but it was one of the better aspects of the library. As you know, I use versioning in my APIs. As it happens, the endpoints using this package are all under a separate version from the prior endpoints. Having DTOs that can easily be changed between endpoint versions is a huge benefit - otherwise any DB changes ripple out to all API versions. Being able to plug in AutoMapper in the Service layer was very beneficial to separating these concerns. And, looking at the code, it seemed fairly trivial to simply call a Map() function in that Service layer when appropriate. This level of maturity is something that has become expected with OData, for example (OData Model Configurations). I was very disappointed to see this removed.
2. The StringId property: Please make this virtual. In v3.1.0, I was able to implement IIdentifiable and just use the same implementation for StringId from Identifiable. The reason I did this was so I could add a JsonIgnore attribute - I would prefer the previous versions of the API were not forced to see an irrelevant StringId property in all responses. (If E/R splits were still available this would be a non-issue - see previous point). Now, with this version, the TypeHelper is yet another previously usable class that has inexplicably become internal. So I can't use the same implementation but just add the JsonIgnore attribute. If it was virtual, I could just override and call the base, but add the attribute. (I mean, I could just do exactly what TypeHelper is doing, but come on...)
3. Document.Data: For whatever reason, this property is no longer of type ResourceObject - it is now just an object. I'm sure there's some reason, but it means now when deserializing API responses I have to constantly cast it to ResourceObject to get access to the Attributes dictionary. Not a dealbreaker but puzzling.
4. Cors/Authentication/Authorization: Please add some optional parameters to the UseJsonApi extension like bool useCors = false, bool useAuthentication = false, bool useAuthorization = false - then call the corresponding extensions on IApplicationBuilder if they are set to true. According to the docs, "For most apps, calls to UseAuthentication, UseAuthorization, and UseCors must appear between the calls to UseRouting and UseEndpoints to be effective." I was able to keep them working by simply calling app.UseMiddleware<CurrentRequestMiddleware>() and app.ApplicationServices.CreateScope().ServiceProvider.GetService<IInverseRelationships>()?.Resolve() myself, but it would be nice to allow for these things to work with the extensions your provide (I couldn't call LogResourceGraphValidations because, of course, it's private).
5. Immutable properties: This one was painful. In v3.1.0, if a property used [Attr("", true)], it was just ignored when sent in a PATCH request. Now with v4.0.0-alpha4, if that property exists in a PATCH request, it causes a 400 Bad Request. That seems silly. So.. (1) I do a GET, (2) cast the Data property to a ResourceObject, (3) clear the Attributes dictionary (or manually remove each immutable property), (4) add the updated property, (5) reset the Data property on the Document to the dictionary I cast to earlier and (6) now it's ok to send it to a PATCH. Figuring out why I was suddenly getting 400s when all was good before definitely took some time.

The enhanced Resource Hooks and ASP.NET Core 3.0 support are nice, but using this new version comes at quite a price. Unfortunately, I wasn't able to find any "migration guides" to help overcome the numerous breaking changes. I have more observations but I'll end here as I'm interested to hear your thoughts.