Fragile data in response #1
Conversation
| use Zend\ProblemDetails\ProblemDetailsResponseFactory; | ||
|
|
||
| class ProblemDetailsResponseFactoryTest extends TestCase | ||
| { | ||
| use ProblemDetailsAssertionsTrait; | ||
|
|
||
| private $request; | ||
| private $factory; |
There was a problem hiding this comment.
IF we're going to declare this explicitly, then we should also include their types via a docblock.
|
|
||
| $response = $this->factory->createResponseFromThrowable($this->request->reveal(), $exception); | ||
|
|
||
| $this->assertNotContains($fragileMessage, (string) $response->getBody()); |
There was a problem hiding this comment.
This is a tough call.
Generally, we want to include some usable detail here. If we never include the exception message, then what do we include in order to provide useful details to the end-user consuming the API?
I would argue that if you have exceptions that have sensitive details in them, you should be catching them, and doing one of the following:
- Using the
ProblemDetailsResponseFactoryto generate a custom response, via thecreateResponse()method. - Creating a custom
ProblemDetailsExceptiontype and re-throwing that type with the details you're interested in.
Alternately, we could have a flag for the ProblemDetailsResponseFactory to disable using the exception message for the detail; if we do that, we likely should have a default detail message to use, however.
There was a problem hiding this comment.
If we want to provide details you should use ProblemDetailsException. User should see only details of 4xx. 5xx are internal and api developer should only know about this. For others should be Internal server error provided or custom ProblemDetailsException if we really want to say something.
Show exception details out of box is dangerous and we should care about this in most common cases by serve safe message.
There was a problem hiding this comment.
Anyway I think is better to show Internal server error even useful details for api user.
There was a problem hiding this comment.
Let's make it a flag, then, and have it default to not use non-ProblemDetailsException messages; additionally, there should be a constructor argument for specifying the default message to use with such exceptions.
There was a problem hiding this comment.
👍 However I worried about number of constructor parameters:
- bodyFactory - is inject response not enough?
- jsonFlags - it's necessary however maybe we should add possibility to add/manage generate json/xml/other response?
There was a problem hiding this comment.
safely can you expand this? For me if you want to provide/change stream, you can do that by inject response with given stream.
The underlying stream is generally backed by a PHP resource. We cannot guarantee that something else has not written to that, particularly if you are working in an async environment; as such, writing directly to it is unsafe. That's what the "body factory" is for: to generate a new stream that we can inject into a new response, which we can then safely write to.
about rendering: yes number of args is same but detail of implementation is much lower, we don't depend on json stuff.
Okay, so we should separate that out. Do you want to do that?
Anyway, what about this pr? Ready to merge?
No: we need a default message to include when the flag is disabled; additionally, it should be configurable. (MOAR CONSTRUCTOR ARGS!)
There was a problem hiding this comment.
Which message? detail or title? Now title always depends on status. detail field is not required, so if it's disable we don't need to provide any default message.
I don't like how status logic is implemented (and affects on title, more here weierophinney/problem-details#3) but it problem to anoter discussion.
There was a problem hiding this comment.
The detail field; that's what we were populating with the exception message previously. Yes, it's technically optional, but I'd rather provide something there than omit it or have it empty, which is what you currently do.
Title doesn't always depend on status; we simply populate the title and type based on the status if they are not provided.
There was a problem hiding this comment.
But question is: when you want to hide details, that user need to see something more? Internal server error is enough. It's no problem to add this parameter, but it complicate this class and I don't see any value to have it
There was a problem hiding this comment.
Okay, let me put it another way: if you make it empty and/or nullable, you'll have to change the signature of createResponse(), as it's currently a required argument, or you'll have to change the internals of that method to test for an empty value and omit it from the payload.
That's a subject for another pull request entirely at that point: should detail be required or not?
Yes, the spec says it can be omitted. My argument is that detail is the one field that provides human readable information that can help them understand what they need to correct, which is why I've made it a required argument.
In the case of exceptions, if we do not include the exception message, even something like "An error occurred on the server preventing processing of your request" is helpful, as then you know it's not likely something you did, but a server-side issue or a bug in the API you're hitting.
|
Thanks, @snapshotpl |
I wrote about this here
I have same problem with apigility and I need to extend it by my own. Unfortunately after data leaks on production enviroment...
Here it's falling test to show how it's dangerous and should be fixed. If we aggre about that I can provide fix for that.