Page MenuHomePhabricator
Create Task
Maniphest T226666

RESTBagOStuff client error handling
Closed, ResolvedPublic
Actions

Description

It is currently the case that for any error handled by Kask, a JSON-encoded HTTP problem will be returned. There exists however the possibility that errors occurring lower in the Golang HTTP stack, before control is handed off to application code, can result in text-formatted (Content-Type: text/plain) errors. An excessively long set of headers and/or request URL will return a 431, for example:

HTTP/1.1 431 Request Header Fields Too Large
Content-Type: text/plain
Connection: close

431 Request Header Fields Too Large

RESTBagOStuff seems to parse the body as JSON if $this->extendedErrorBodyFields !== [], which based on the example configuration in the docstrings will be true. I don't expect these errors to happen frequently, and perhaps it already fails gracefully and Does The Right thing if decoding of the body doesn't succeed. Can we confirm whether this is the case?

Related Objects
Search...

Event Timeline

Eevans reassigned this task from Eevans to BPirkle.Jun 26 2019, 8:35 PM
Eevans triaged this task as Normal priority.
Eevans created this task.
BPirkle added a comment.Jun 27 2019, 12:26 AM

Currently, if decoding fails, RESTBagOStuff will simply log the error without any extended information. This would include only:

  • a message describing the operation that was being attempted (ex. "Failed to store XXXX")
  • the HTTP error code (ex. 500)
  • the HTTP response string (ex. "Internal Server Error")

So we would have a record of the error,. But if the body contained error information beyond the HTTP code/string, that information would be lost. In the example from the task description, we would not lose anything.

Are there cases where the body will contain useful text/plain information beyond the HTTP response string?

I considered logging the entire body, but was concerned that was a little loose and unpredictable. However, it would be straightforward to log the entire body if it might be useful.

Eevans closed this task as Resolved.Jun 27 2019, 2:52 PM
In T226666#5287824, @BPirkle wrote:

Currently, if decoding fails, RESTBagOStuff will simply log the error without any extended information. This would include only:

  • a message describing the operation that was being attempted (ex. "Failed to store XXXX")
  • the HTTP error code (ex. 500)
  • the HTTP response string (ex. "Internal Server Error")

So we would have a record of the error,. But if the body contained error information beyond the HTTP code/string, that information would be lost. In the example from the task description, we would not lose anything.
Are there cases where the body will contain useful text/plain information beyond the HTTP response string?

Note that I know of, no.

I considered logging the entire body, but was concerned that was a little loose and unpredictable. However, it would be straightforward to log the entire body if it might be useful.

My expectation (I'd need to spend time spelunking through Go's http module to be sure), is the body would be identical to the status text. I think we can safely assume we're OK here.

Thanks!

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL