Design an error details protocol for NuGet HTTP client-server communication

[joelverhagen]

Today

Today, we have a very simplistic approach for error messages in the NuGet HTTP API. There are two facets today:

1. HTTP status code - this is the only documented indicator of HTTP failure. For example, in the push and delete resource only 201, 202, 400, and 409 are document indications of failures.

2. Reason phrase, a.k.a status description. This is an undocumented way that some NuGet servers (such as NuGetGallery) pass an error message to the user. The official NuGet client depends on the implementation of HttpResponseMessage.EnsureSuccessStatusCode which includes both the status code and the reason phrase in an exception message.

For example, this clearly wrong push command to NuGet.org yields this client output:

> nuget.exe push Package.1.0.0.nupkg -source https://api.nuget.org/v3/index.json -apikey "foo"

Pushing Package.1.0.0.nupkg to 'https://www.nuget.org/api/v2/package'...
  PUT https://www.nuget.org/api/v2/package/
  Forbidden https://www.nuget.org/api/v2/package/ 500ms
Response status code does not indicate success: 403 (The specified API key is invalid, has expired, or does not have permission to access the specified package.).

The client output is pretty good. It states the problem and allows the user to self resolve.

We can do better

1. The reason phrase thing is undocumented. The API docs only talk about status codes.

2. The reason phrase has a maximum length. I think it's related to the maximum header size, which varies per client and server.

3. The reason phrase is just a string. We don't have a way to return an error code that provides anything more granular than the HTTP status code. We have good error codes on the client side, couldn't we have the same for server communications?

4. It's unclear about how this works with localization.

5. And most importantly, reason phrases has been removed from (or made nullable?) HTTP 2.0. Thanks for the heads up, @satamas, https://github.com/NuGet/Home/issues/6844.

For this reason, we should design a schema for an error response body. For example,

{
    "statusCode": 403,
    "errorCode": "NU4031",
    "message": "The specified API key is invalid, has expired, or does not have permission to access the specified package.",
    "locale": "en-US"
}

This also synergizes with https://github.com/NuGet/Home/issues/6333, which has one potential solution of making an official response body for even successful push requests to include extra information, such as the gallery details URL.

Having an document error response body will allow servers to align their error modes so that users can have a more consistent experience across package sources.

This doesn't solve everything and there are some issues with this, such as the potential lack of flexibility in Azure Blob Storage, CDN, and APIM response bodies.

[joelverhagen]

/cc @maartenba, @xavierdecoster. What do you guys think?

[maartenba]

All for it. Going to be some indexing on what is out there today but the idea is great!

[shishirx34]

@karann-msft, @anangaur - thoughts on this feature request?

[reybard]

:wave: I'm a developer for the GitHub package registry and this has bitten us badly. Our primary registry application is a Golang service using the core net/http library (version 1.13 at the time of writing).

The Go maintainers have no plans to allow for overriding the status text each http status code already has assigned: https://github.com/golang/go/issues/8990

For now we get around this by setting an X-NuGet-Warning response header with the actual error we want users to see. Without this, the only output the nuget CLI provides is the very ambiguous status text Go has internally defined for each code.

For example, perhaps a user is attempting to upload a package where they have set their repository in their nuspec to some other external site that isn't GitHub. We have a validation in place on uploads that disallows this, and if I try to do such a push right now anyway this is the current output I get from GPR:

$: nuget push -Verbosity detailed -Source "GitHub local nuget registry" example-nuget-package-2.1.0.3.nupkg
NuGet Version: 5.1.0.6013
WARNING: No API Key was provided and no API Key could be found for 'http://nuget.pkg.github.localhost/monalisa'. To save an API Key for a source use the 'setApiKey' command.
Pushing example-nuget-package-2.1.0.3.nupkg to 'http://nuget.pkg.github.localhost/monalisa'...
  PUT http://nuget.pkg.github.localhost/monalisa/
WARNING: invalid repo host 'foobar.com', only github (github.com) repositories allowed
  BadRequest http://nuget.pkg.github.localhost/monalisa/ 43ms
Response status code does not indicate success: 400 (Bad Request).
System.Net.Http.HttpRequestException: Response status code does not indicate success: 400 (Bad Request).

WARNING: invalid repo host 'github.com', only github (github.localhost) repositories allowed is the error we want users to see, but they only see it because of this undocumented header.

The actual error according to the CLI is 400 (Bad Request) where Bad Request is the hardcoded status text for a 400 status in Go.

It's nice that we have the header as an option but it is not ideal because it displays as yellow warning text and because we use it for errors, it can't easily be used for deprecation notices or things we'd actually want to just warn the user about.

I second @joelverhagen's proposed response body. For our purposes a simple JSON object with a status code and error string would suffice as long as the client displayed it back properly to users.

[johnterickson]

I love the idea of making errors more actionable!

What do the other package managers do? For Cargo, they just display the body to stderr. I'm leaning towards something like that because - as you mention - it will better handle CDN errors (and their correlation IDs)?