Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Fragile data in response #1

Merged
merged 9 commits into from
Oct 5, 2017
Merged

Fragile data in response #1

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
314f73f
Falling test
snapshotpl Aug 1, 2017
e733264
Detailed test name
snapshotpl Aug 1, 2017
2ce4dbc
Fix falling test and add posibility to show exception message in detail
snapshotpl Aug 1, 2017
470eeed
CS fixes
snapshotpl Aug 1, 2017
a82dc24
Remove unused const
snapshotpl Aug 1, 2017
34b0b8b
Add detail flag
snapshotpl Aug 6, 2017
6a82d85
Normalize status code in nondebug mode
snapshotpl Aug 6, 2017
b50c9d7
Short var name
snapshotpl Aug 6, 2017
bf6e750
Remove nb settings
snapshotpl Aug 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion test/ProblemDetailsResponseFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@
use RuntimeException;
use Zend\ProblemDetails\Exception\InvalidResponseBodyException;
use Zend\ProblemDetails\Exception\ProblemDetailsException;
use Zend\ProblemDetails\ProblemDetailsResponse;
use Zend\ProblemDetails\ProblemDetailsResponseFactory;

class ProblemDetailsResponseFactoryTest extends TestCase
{
use ProblemDetailsAssertionsTrait;

private $request;
private $factory;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF we're going to declare this explicitly, then we should also include their types via a docblock.


protected function setUp() : void
{
$this->request = $this->prophesize(ServerRequestInterface::class);
Expand Down Expand Up @@ -176,4 +178,14 @@ public function testFactoryRendersPreviousExceptionsInDebugMode() : void
$this->assertEquals(101010, $payload['exception']['stack'][0]['code']);
$this->assertEquals('first', $payload['exception']['stack'][0]['message']);
}

public function testFragileDataInExceptionMessageShouldBeHiddenInResponseBodyInNoneDebugMode()
{
$fragileMessage = 'Your SQL or password here';
$exception = new \Exception($fragileMessage);

$response = $this->factory->createResponseFromThrowable($this->request->reveal(), $exception);

$this->assertNotContains($fragileMessage, (string) $response->getBody());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ProblemDetailsResponseFactory to generate a custom response, via the createResponse() method.
  • Creating a custom ProblemDetailsException type 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway I think is better to show Internal server error even useful details for api user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}