[regression] Empty response body when user is an administrator and an exception is thrown; some traces cannot be encoded because of recursion detection.

That definitely sounds like a regression :(

Can you provide concrete steps to reproduce this? Is the first sample request sufficient to reproduce it with for example article nodes?

Thanks for confirming! The 2.0 behavior is to get a 422 response, what is the behavior in >=2.1? 422 but just without the useful error message?

Probably won't have time soon to work on this. If you could write a regression test, that'd be helpful.

Since you've done so much for this module, I've put aside my other work for a moment and wrote the test for you 🙂 ❤️

This is passing tests just fine for me, at least for Node. So … I think something else is going on on your site? Or perhaps I misunderstood and this test is not testing what you're saying?

#6 forgot to ensure required fields are present, hence the failures on some entity types. But it does pass for Node. Which shows is why I think something else is going on on your site.

Hopefully this helps you narrow it down!

Are you testing with node--page as the sample request body is showing, or is it actually happening on a custom entity type?

Woah, this is getting very bizarre. You say you can consistently reproduce the problem. But you say that my patch is also passing tests for you.

I don't understand yet how both of those things can happen at the same time. :O

While working on something unrelated to this, I think I encountered the same error. I also realized what's likely causing the discrepancy between the test and manual replication...

@Grimreaper is including authorization for this request and the authorization he is using is for an admin user. This user likely has the access site reports permission; the test user does not.

I was able to replicate this bug by using one user with this permission and one without it.

The failure I encountered is a failure to encode JSON because of a detected recursion. That happens in src/EventSubscriber/ResourceResponseSubscriber.php:

$response->setContent($serializer->encode($jsonapi_doc_object->getNormalization(), $format));

The source of the recursion appears to be in src/Normalizer/HttpExceptionNormalizer.php:

if ($this->currentUser->hasPermission('access site reports')) {
  // The following information may contain sensitive information. Only show
  // it to authorized users.
  $error['source'] = [
    'file' => $exception->getFile(),
    'line' => $exception->getLine(),
  ];
  $error['meta'] = [
    'exception' => (string) $exception,
    'trace' => $exception->getTrace(),
  ];
}

Specifically, the trace array key. Commenting this out, I am able to receive a response body, with it, I am not.

I have not found out the root cause of this regression. I'm not terribly surprised by it, we did make changes to normalization and exceptions in #3022584: Consolidate and simplify NormalizerValue objects: introduce CacheableNormalization, which @Grimreaper identified as the commit which introduced the bug.

EDIT: nope, sorry, this is not at all related. Please ignore, sorry for the noise!

Ran into this too at #3035134-9: Compatibility with upcoming JSON:API 2.4 release: JSON:API Extras 3.5.

After more hours of debugging than I'd like, I discovered the root cause for #3035134: #3038308: Avoid translations DELETE data loss and unintended changes with PATCH and test all methods against entity route parameter translation upcasting removed the basic_auth module as one of the installed-by-default modules. JSON:API Extras was using Basic Auth for authentication. So this is just another occurrence of #3027890: When trying to use `Authorization: Basic …` header and basic_auth module is not installed, warn user.

Is that perhaps also what's going on here, @Grimreaper? I can tell you're using Basic Auth request headers, but perhaps you don't have the Basic Auth module installed?

I suspect that #13 is actually correct, and there's another way to get to a "403 without explanation" case.

Bumping this up to major because I think it's quite common to use the admin role when developing/debugging and this bug makes it nearly impossible to do so. It hides all error messages, whether they're validation errors or access reasons. Worse, HTTP status codes are unaffected thus making it unclear that this is not the intended behavior of the module. That's not good for people that will be trying out JSON:API for the first time now that it's just been added to core.

If we need to fix this quickly and simply, I'd be +1 to commenting out the trace key, adding a todo and filing a lower priority follow up.

Also, if you are in production and an admin, validation errors disappear which could break the functionality of some sites.

Edit: duplicate comment

Attaching quick patch based on @gabesullice's comments in #17.

It helps if you account for the tests as well. Added the same TODO in HttpExceptionNormalizerTest.php. I will also try to trace the actual recursion when I get some cycles between now and Seattle.

#13:

This user likely has the

access site reports permission

; the test user does not.

Based on @Grimreaper's report, I wrote the patch in #6. But even if I change that to first $this->grantPermissionsToTestedRole(['access site reports']);, I still can't reproduce it.

Specifically, the trace array key. Commenting this out, I am able to receive a response body, with it, I am not.

So … there must be some exception that does not offer a trace then, since only commenting out that line means only disabling the $exception->getTrace() call. But: \Exception::getTrace() is defined on PHP's built-in \Exception, so all exceptions must have it. And we know that it's definitely an exception, because the code is in protected function buildErrorObjects(HttpException $exception) {…}, which clearly has a typehint that restricts it to this subset.

Furthermore, \Drupal\Core\EventSubscriber\FinalExceptionSubscriber::onException() also calls ->getTrace(), without problems.

If we need to fix this quickly and simply, I'd be +1 to commenting out the trace key, adding a todo and filing a lower priority follow up.

I don't think we can do that; we need to fix this both in the contrib module and in the committed-to-core module. And we can't just fix this without a regression test in core — I doubt core committers would be willing to accept it.

I did some more digging, and I wonder if this is the cause?

Without concrete steps to reproduce (such as the recursion that @gabesullice reported in #13), it's hard to fix this :(

I have the same issue registering a user, use the following payload twice, and the 2nd request will fail with this error, because validation fails due to the email already being in use.

POST /jsonapi/user/user

{
   "data":{
      "type":"user--user",
      "attributes":{
         "mail":"asdf@example.com",
         "pass":{
            "value":"asdf"
         },
         "name":"email_registration_123",
         "status":1
      }
   }
}

It's the \Symfony\Component\HttpFoundation\Request in the trace that causes the issue, if I unset that then the encoding works, not sure how to go about finding how that object causes the issue but a good guess might be a referenced to one of the violations? \Drupal\Core\Entity\EntityConstraintViolationList

#25: Ah, awesome, concrete steps to reproduce! 🥳 Do you have the bandwidth to add a regression test with that exact sequence of steps to \Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest?

I wrote the test but it passes against Drupal 8 core, further investigation required!

Thanks so much for writing that test! It's very unfortunate that the test passes if the manual testing repeatedly causes this problem. Can you still manually reproduce #25?

Ugh, it's unfortunate that #27 couldn't replicate the error so that this could land before the 8.7 RC. I shouldn't have speculated about which permission caused the error. The simplest thing we can do is run the request as a regular user and another as an admin user to show the issue. This should demonstrate the error.

Moving this to the core queue.

Update:

This proves the bug:

1) Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest::testRecursionDetectedWhenResponseContainsViolationsFrom3042124
Failed asserting that null is not null.

/var/www/html/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php:952

The assertions pass for the regular user, but fail for the admin user (evidenced by the line number).

Great! #29 worked (failed) just as expected.

I created a backport of the test (to the contrib 2.x JSON:API) and used it with git bisect to confirm @Grimreaper's isolation of #3022584: Consolidate and simplify NormalizerValue objects: introduce CacheableNormalization as the culprit.

Investigating the root cause now.

I found the root cause. Prior to #3022584: Consolidate and simplify NormalizerValue objects: introduce CacheableNormalization, HttpExceptionNormalizerValue extended FieldItemNormalizerValue, which had a method called rasterizeValueRecursive. That recursive method would recurse into the trace and normalize all its values. For values it encountered that were non-scalars/non-arrays (like Symfony's Request object, present in the trace as an argument to a function in the call stack), it would attempt to call __toString on them and fall back to NULL otherwise.

After #3022584, we no longer called rasterizeValueRecursive for exceptions. So, when the encoder attempted to encode the "normalized" value. It really wasn't normalized. Full objects still existed in the value to be encoded. The encoder tried to handle this gracefully by serializing the object regardless. However, the request object present in the trace has a reference to the current exception. That exception of course has a trace too, which has the request, which has the exception... ... which was the infinite loop that we encountered in this issue.

The solution is fairly simple. We need to restore the recursive rasterization that we previously had for exceptions and have since lost.

I'm doing that now, but I don't yet have a patch. I'm only recording my thoughts here because it's almost EOD and I may need to pick this up again tomorrow.

And the fix.

I moved the protected rasterizeValueRecursive up into the shared base class between FieldItemNormalizer and HttpExceptionNormalizer and then made sure it was called by HttpExceptionNormalizer::normalize.

The end result doesn't change any internal APIs or change the intended JSON:API output. It only restores the output that should have been there all along (i.e. the missing HTTP response body).

What can I say?

GABE IS BACK!!!!!

Not only did he get to the bottom of this, he's also sprinkling our tests with hilarious references 😆👏

1. +++ b/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php
   @@ -132,42 +132,6 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   -  protected static function rasterizeValueRecursive($value) {
   
   +++ b/core/modules/jsonapi/src/Normalizer/NormalizerBase.php
   @@ -20,6 +21,42 @@
   +  protected static function rasterizeValueRecursive($value) {
   

   So we're moving this into a parent (base) class. That's fine.

2. +++ b/core/modules/jsonapi/src/Normalizer/HttpExceptionNormalizer.php
   @@ -49,7 +49,7 @@ public function __construct(AccountInterface $current_user) {
   -    return new HttpExceptionNormalizerValue(new CacheableMetadata(), $this->buildErrorObjects($object));
   +    return new HttpExceptionNormalizerValue(new CacheableMetadata(), static::rasterizeValueRecursive($this->buildErrorObjects($object)));
   

   This is the fix. We're wrapping an existing function call in one additional function call. That's fine.

3. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -905,4 +905,52 @@ public function testMapFieldTypeNormalizationFromIssue3040590() {
   +    $user = $this->drupalCreateUser(['bypass node access'], 'Sam');
   +    $admin = $this->drupalCreateUser([], 'Gandalf', TRUE);
   

   🤩

#29 is the test only patch.

This looks good to me also, nice work.

Committed and pushed 49a7dfcb47 to 8.8.x and 94fcd54332 to 8.7.x. Thanks!

Ported to the JSON:API contrib module: https://git.drupalcode.org/project/jsonapi/commit/52534b2