Server error when using the jsonapi.entity.to_jsonapi service

Thanks for the patch!

It would be awesome to get test coverage for this since we already introduced a regression on it once.

+++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -166,6 +166,9 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
     if (empty($context['resource_type'])) {
       $context['resource_type'] = $this->currentContext->getResourceType();
     }
+    else {
+      $this->currentContext->setResourceType($context['resource_type']);
+    }

These are exact mirrors of each other. That is … interesting.

I vaguely remember a discussion in some issue about removing the "context" service altogether. Do you remember where that was, @e0ipso?

Honestly, this sort of bugginess is exactly what I described I feared in #2874509-26: Provide service to simplify generating a JSON API representation of an entity in PHP code.

I think we should change this code completely, and just do a subrequest, so that we don't need to simulate lots of things. Basically, do what #2852272-6: Make it possible to call jsonapi from within templates does.

I think we should change this code completely, and just do a subrequest,

Hmm, I think the reasoning in the parent issue still applies. This is just a normalization of an entity, we don't need to pass it through the http kernel. The fact that there is a bug does not justify a /fliptable and rewrite the approach completely. This service is being used in production by websites often mentioned in DrupalCon keynotes, so I'm confident that it works to an extent.

The patch looks straight forward enough, but I'd like to understand why it stopped working. @a.milkovsky can you provide reprouction steps? Does this work with jsonapi-8.x-1.2?

Hmm, I think the reasoning in the parent issue still applies.

Which reasoning is that? I didn't find any reasoning.

This is just a normalization of an entity, we don't need to pass it through the http kernel.

What is the problem with passing it through the HTTP kernel? A Symfony subrequest doesn't result in an actual HTTP request. It's a purely internal business. This is exactly the sort of thing that subrequests are intended to do.

The fact that there is a bug does not justify a /fliptable and rewrite the approach completely.

Refactoring internals is not "/fliptable" IMO. I'm only proposing to make this service not need to know implementation details anymore (see \Drupal\jsonapi\EntityToJsonApi::calculateContext()).

I've attached a patch that shows the simplification this enables.

#12 simply means fewer code paths to maintain, and thus fewer bugs, fewer edge cases, fewer security vulnerabilities.

Speaking of security vulnerabilities… there is an obvious one right here!

Unpublishing.

Reported: https://security.drupal.org/node/162210.

To make tests pass, you need this… and that's how I noticed this is a security vulnerability.

Republishing this issue after it was decided in the security issue that this could be solved publicly.

This is what I wrote:

This module has a information disclosure vulnerability.

You can see this vulnerability by:

1. Enabling the module
2. As a developer, write code that uses the jsonapi.entity.to_jsonapi service, and whose output is visible at some URL. For example:
   

   var_dump(\Drupal::service('jsonapi.entity.to_jsonapi')->serialize(Node::load(1));
   var_dump(\Drupal::service('jsonapi.entity.to_jsonapi')->serialize(User::load(1));
   

3. As an anonymous user without any permissions, go to the URL that exposes that output.
4. Note that this anonymous users is allowed to see the full details of all entities serialized to their JSON API representation. Entity access it not respected. Field access is not respected.

#2874509: Provide service to simplify generating a JSON API representation of an entity in PHP code introduced this API. No mentions of "access" or "security" in there. This is understandable, because it's an API targeted at developers. But it's likely that most developers will assume that access checking will happen automatically.

We should either:

1. Document explicitly that the service is not checking access. Preferably also make this explicit in the service name (similar to core's router.no_access_checks service)
2. Update the existing code to respect access. In fact, that's what I did by accident, in refactoring the code to be simpler: #12 — and that's how I found this security vulnerability.

to which @dawehner responded:

https://www.drupal.org/project/jsonapi/issues/2874509 introduced this API. No mentions of "access" or "security" in there. This is understandable, because it's an API targeted at developers. But it's likely that most developers will assume that access checking will happen automatically.
Do you really think this is a security issue which should be dealt in private? For me this is rather security hardening.

to which I responded:

I'm not on the security team, so I defer to them. Hence Needs triage.

to which @mlhess responded:

I am ok with this being in public, if no one objects in 2 weeks, let move it there.

We're now nearly two months further, with zero responses since that last one, so definitely okay to solve publicly.

To clarify #18: field access is checked while normalizing fields, that still happens. It's entity access checking that does not happen during normalizing, that happens before (in \Drupal\jsonapi\Controller\EntityResource::getIndividual()).

Rebased #17.

Fix a failure.

Still 13 failures, but it used to be 2 failures within each of those 13 failing test classes, now it's just 1.

Closed #2948630: Error using EntityToJsonApi serialize method as a duplicate of this.

To clarify: the security problem here lies in the fact that entity access is checked NOT during normalization (in \Drupal\jsonapi\Normalizer\EntityNormalizer), but before normalization, in the controller (in \Drupal\jsonapi\Controller\EntityResource::getIndividual()).

Unfortunately, if we fix this the way I'm currently proposing, then we need to also update the test coverage I added in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and that we're still expanding.

That means we need to do nasty things with mocking requests, which is almost as bad as the problem we're fixing. But that complexity would live in tests only, so that's definitely still an improvement.

However, for one of the bits of test coverage, we need to normalize an in-memory-but-not-stored modified entity. And the direction that I set out with this patch actually doesn't allow for that.

It'd seem that the easiest way forward is to do #2941685: Port parameter based resource config from REST module first, because it removes the whole CurrentContext concept, which will allow the test coverage to not use the jsonapi.entity.to_jsonapi service, at which point the direction in my patches above is completely doable!

IOW:

1. the original reported bug/fix I'm proposing solves the problem by no longer mocking lots of things, but instead just performing a subrequest
2. however, in doing so, we require that the test coverage starts mocking a request, which is also not great; but worse yet, it means we can no longer use the jsonapi.entity.to_jsonapi for at least one assertion.

#2941685: Port parameter based resource config from REST module landed. I believe that was the blocking issue.

TBH, @Wim Leers, I don't fully understand the gist of this issue. I've tried to read the issue summary and your comments to get a sense of it, but I'm afraid I'm still not completely following.

I'm pretty certain my lack of understanding results from a different perspective on why this service exists.

I think your view on it is that: it's a way to get the proper JSON API response for a given entity. Is that accurate?

My perspective on it is that it's a way to get a JSON API spec compliant serialization of an entity.

At first glance, those two perspectives probably seem like a distinction without a difference. But I think they the end up informing a lot of decisions.

For example, this service was introduced so that one site could create a representation of an entity and POST it to another JSON API server. That entity could be in-memory only. Thus, one can't do a subrequest because the entity doesn't exist in the database on either server.

IOW, it's more about our funky serializer than it is about providing a service to synthesize responses.

That difference also informs the aforementioned security issue. If this is about getting a JSON API response then one would expect access to be checked because for a given user the response might be a 403 error document and for another user it might be 200. However, if this is about normalization/serialization of any given entity, it's a non-issue because the service's intended purpose is just to perform a data transformation.

For example, this service was introduced so that one site could create a representation of an entity and POST it to another JSON API server.

That's indeed what #2874509: Provide service to simplify generating a JSON API representation of an entity in PHP code said in the very start.

That entity could be in-memory only.

I don't see this mentioned anywhere in that issue. AFAICT they're trying to send existing content to a 3rd party server.

Thus, one can't do a subrequest because the entity doesn't exist in the database on either server.

… which would make this untrue. And this then leads me to one of my key concerns with jsonapi.entity.to_jsonapi: we implement the same logic twice. Rather than reusing what already exists! Implementing this using a subrequest instead allows us to reduce the maintenance burden and completely eradicate subtle differences.

IOW, it's more about our funky serializer than it is about providing a service to synthesize responses.

Let's assume this is true. Then we ought to remove the links portion of the generated JSON API representation, because it currently is invalid, which also means it's an invalid JSON API representation. As #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only discovered:

      $created_entity = $this->entityStorage->loadUnchanged(static::$firstCreatedEntityId);
      $created_entity_document = $this->entityToJsonApi->normalize($created_entity);
…
       // @todo Remove the two lines below once https://www.drupal.org/project/jsonapi/issues/2925043 lands.
        unset($created_entity_document['links']);
        unset($decoded_response_body['links']);
        $this->assertSame($created_entity_document, $decoded_response_body);

(This is an example of such a subtle difference.)

That difference also informs the aforementioned security issue. If this is about getting a JSON API response then one would expect access to be checked because for a given user the response might be a 403 error document and for another user it might be 200. However, if this is about normalization/serialization of any given entity, it's a non-issue because the service's intended purpose is just to perform a data transformation.

I'm fine with that reasoning, if it is applied consistently. Currently, the jsonapi.entity.to_jsonapi service does not apply entity access checks, but it does apply field access checks (see \Drupal\jsonapi\Normalizer\FieldNormalizer::normalize()). Which of course is misleading. If it's meant as a pure "transform this to that, regardless of access control", then we need to be much more explicit about it, and have test coverage for it.

The above is my POV on A) the jsonapi.entity.to_jsonapi service, B) how that service would work ideally.

Since this service is available, I started using it in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only. And in doing so, I ran into the "in-memory entity" problem (described in #27, and you cited it in your comment). But to be perfectly honest, I think that the test coverage should NOT use the jsonapi.entity.to_jsonapi service. Especially because AFAIK we're all agreed in #2952293: Branch next major: version 2, requiring Drupal core >=8.5 that this service should be moved to jsonapi_extras. Therefore the test coverage should not rely on it either.

Of course, if people want to continue using jsonapi.entity.to_jsonapi despite the flaws I've pointed out above, they're free to do so.

So, recapping to determine next steps:

1. #2941685: Port parameter based resource config from REST module has removed CurrentContext altogether, hence the bugfix in #3 is no longer relevant — the original bug has been fixed!
2. I shifted the scope in #12: from fixing a bug in jsonapi.entity.to_jsonapi + CurrentContext to fundamentally changing jsonapi.entity.to_jsonapi's implementation to address the flaws I've described. But per #2952293: Branch next major: version 2, requiring Drupal core >=8.5, doing that is no longer relevant.
3. Which means this issue has pretty much served its purpose. We could rescope it again, to make the test coverage introduced in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only no longer rely on jsonapi.entity.to_jsonapi, and hence paving the path for jsonapi.entity.to_jsonapi being moved to jsonapi_extras. But I think it'd be better to do that in a separate issue… done: #2958504: Stop using jsonapi.entity.to_jsonapi service in ResourceTestBase!

… which means we can close this too. Tentatively doing so!