[DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities

FWIW, I'm strongly +1 to letting sites have some say over what gets exposed to JSON:API. I don't yet know exactly how that should work, but I'd *love* to use it once it does. ;)

Thanks for opening this discussion and being willing to move quickly on it!

Cheers,
-Derek

p.s. Escalating and tagging appropriately.

So some of the pros of this design choice include that it's philosophically "API-first" (i.e., there's nothing special about accessing something over JSON:API vs. in a browser), it's better for UX and maintainability, and it eliminates the need for any kind of upgrade path now or in the future.

The cons are that this configuration results in vastly increased attack surface, and no option for mitigating any particular issue beyond turning off the entire module (which, for decoupled sites, is basically turning off the whole site). This resulted in https://www.drupal.org/sa-core-2019-003 being exposed at higher severity for JSON:API than for core REST (originally much higher, although the gap closed somewhat when we confirmed anonymous exploits in both).

If we do want to give JSON:API in core additional mitigation or hardening strategies, we have a few choices:

1. No configuration at all; just trust the Entity API and routing system. (current behavior)
2. Allow disabling specific resources and/or methods in settings.php.
3. Configuration settings for resources and/or methods, not exposed in the UI, but everything on by default. (Similar to REST in HEAD, except with the default behavior inverted.)
4. Configuration exposed in the UI.

The decision to make JSON:API zero configuration was taken with the caveat that it could be optionally configured using JSON:API Extras. I don't see JSON:API Extras mentioned here. Maybe that's because if we include this into core we cannot rely on a sidekick contrib module, I can see that reasoning.

The cons are that this configuration results in vastly increased attack surface, and no option for mitigating any particular issue beyond turning off the entire module (which, for decoupled sites, is basically turning off the whole site).

In this case, for instance, you could install JSON:API Extras and mitigate the risk. With JSON:API in core we cannot assume / ask JSON:API Extras to be installed, hence the attack surface is the same as if Extras didn't exist. However, if there is a known concern we can leverage Extras as a mitigation tool.

As a former security team member, and maintainer of restws (which uses the same approach as JSON:API) says:

Yep, direct conflict of interest between Zero config and Security defense in depth

Difficult conversation. Looking forward to it!

Drupal, especially if you add a bunch of contrib modules has tons of entity types that are not *directly* exposed in the UI. Luckily, entity types are by default access restricted, but fields are not. I personally find it super-scary to just install jsonapi, and then looking all the things that are exposed on /jsonapi. One difference between jsonapi and the UI is that the UI doesn't give any anonymous visitor a list of all the things that are there.

I would argue that 100% decoupled sites are going to be the exception, a majority will IMHO expose only some data over jsonapi, even if the frontend is fully decoupled then there still a ton of things that are only used in the backend.

IMHO, a configuration could look like this:

( x ) Include ( ) Exclude

Node
[x] Article
[x] Page
[ ] ...
Terms
[ ] Tags

If you want to expose all-the-things, it is one radio-switch away, I'd even be fine if we make that the default, or if we'd update existing installations to that default in an update function. I guess if we'd also make the HTTP methods configurable like rest.module then the UI would get too complex (like the translation settings basically). No strong opinions on whether there is a UI for this in core, could be like rest where the config is in core but the settings are in contrib. A contrib module could even have a feature that remembers which parts were actually used, then you can develop with everything open and it has a button that says "Limit to accessed API's" or something as a starting point when you go into production.

#3 + #8:

One difference between jsonapi and the UI is that the UI doesn't give any anonymous visitor a list of all the things that are there.

This makes it sounds worse than it is.

• If by "things" you mean which entity types are enabled: every Drupal site has certain entity types enabled, that list can easily be largely constructed by inspecting the generated HTML, but is indeed easily available with JSON:API installed. _{Note: if this is going to be the main concern, then we can just remove the /jsonapi route. This is the one and only route that isn't part of the JSON:API spec, but which has proven to be a massive DX improvement.}
• If by "things" you mean which entities of a given type are available: only if the user has the necessary permissions to view an entity can they see the list of entities of a given type. If you can read the data via JSON:API, you can already read it somewhere else today (albeit probably less structured).

letting sites have some say over what gets exposed to JSON:API.

No strong opinions on whether there is a UI for this in core

Such a UI already exists and is intentionally separate from the JSON:API module (see #7). This has proven to be the right trade-off: 80% of sites using JSON:API don't use JSON:API Extras.

"Limit to accessed API's" or something as a starting point when you go into production.

I'd like that! But that requires #1537198: Add a Production/Development toggle for it not to be painfully confusing. _{I worked on that many years ago, but it didn't get anywhere.}

I would argue that 100% decoupled sites are going to be the exception

Agreed. But "empowering fully decoupled Drupal sites" is not the sole goal of JSON:API. The goal is to also enable:

• https://github.com/jsdrupal/drupal-admin-ui
• decoupled UI components that ship with modules and theme _{(think better commenting UX, better revision/diff UI, Quick Edit refactored to use JSON:API instead of Form API hacks …)}

In other words: the goal is to enable the ecosystem to actually use these APIs with one simple, crystal-clear dependency: module:jsonapi. As soon as that dependency changes to module:jsonapi + setting:jsonapi_foo=Y setting:jsonapi_bar=Z, we've effectively killed JSON:API's ability to empower the ecosystem, to enable a new wave of innovation.

Let's do a thought experiment. Fast forward a few years, assume https://github.com/jsdrupal/drupal-admin-ui has been completed, is part of Drupal core and is widely used. Imagine there's a security vulnerability for a widely used entity type, for example Node or User. What this issue is saying, is that we should have a way to mitigate the risk by temporarily disabling that entity type's API routes. But since the admin UI cannot function without these APIs, that also means these entities cannot be administered anymore. Do we want that?

What this issue is saying, is that we should have a way to mitigate the risk by temporarily disabling that entity type's API routes. But since the admin UI cannot function without these APIs, that also means these entities cannot be administered anymore. Do we want that?

That sounds good to me from a mitigation point of view - i.e. you're able to provide a read-only site while you fix the vulnerability, rather than either getting hacked or having to completely shut down.

One possibility might be to have a route listener or an entity access hook that looks at an array in $settings and does return AccessResult::forbidden() if a particular route/entity method/operation combination is in there. Could be in system since it doesn't only apply to JSON:API.

#10: If we go that direction, we might as well protect not just JSON:API but also REST and GraphQL. We already have the necessary infrastructure for that: \Drupal\Core\Entity\EntityTypeInterface::isInternal(), introduced by #2936714: Entity type definitions cannot be set as 'internal'.

So: have something in settings.php trigger the addition of internal = TRUE to the annotation of an entity type.

There are a couple of points that I'm having a hard time processing positively. I'm not saying these are the intentions of anyone, I'm just saying this is how I'm feeling (note that I don't speak for the other maintainers):

1. I feel a bit strongarmed to comply with whatever demand that is holding the merge into core. Otherwise we miss the deadline and JSON:API slips again.
2. This was an open decision. I even invited people from outside the project (like Wim at the time) when we made this key design decision. This was praised in more occasions than I can count. I cannot remember this being challenged in the +2 years we've been proposing JSON:API for core.
3. Walking back on this key decision last minute will likely break a full ecosystem of modules that I personally will have to fix. This will also trigger a re-check of the BC core gate with uncertain expected results.

Again, I'm not adding any technical reasoning here, I may do it in another post. However, I wanted to share this because I'll probably sound frustrated at some point and I want to apologize in advance.

I have no particular stake in this, and no strong opinion one way or the other, but I wanted to add a couple thoughts to consider. I've been thinking about some of my enterprisey type sites and security and what I might do if was using this module.

The cons are that this configuration results in vastly increased attack surface, and no option for mitigating any particular issue beyond turning off the entire module

This isn't necessarily true, depending on hosting. I can disable paths at the loadbalancer, ingress, proxy, webserver, or caching layers. If I were deploying JSON:api in production, I think I would automatically block any endpoints I wasn't using in an nginx config. This is, of course particular to my setup, but it is a place in the stack where someone could mitigate an issue caused by a particular endpoint or set of endpoints temporarily. In the more common LAMP setup, you could do the same thing in a .htaccess as well. My point is that options 2 or 3 above (allowing a way to disable it without exposing a UI for it) are only slightly better than just directly blocking endpoints in your server settings/.htaccess. It's still something that will require a technical user to manage.

If the issue is about having the setting so that in the case of a SA that affects JSON:api only and only a subset of endpoints that there is a way to disable just those endpoints temporarily, I feel like its a lot of things that would have to line up in order to have a situation where that is useful.

If we are talking about having a way to disable endpoints because you aren't using them and want to reduce potential attack surface area preemptively, then I support having the ability to do that, but I feel like if we don't provide a UI for it, then we might as well just provide documentation on how to block some endpoints in .htaccess/apache/nginx and let people manage that side of it on their own.

Edit: I want to add that I don't personally use any of the managed hosting providers. My comments above may not apply to them, as I'm not sure how easy it is to block routes elsewhere in the stack with some of them. I know it's not always possible or easy to adjust server settings on managed hosting, so my comments above are only referring to self hosted or semi-self managed deployments.

I was not aware of the Zero-config goal of JSON:API, and now that I've read this issue and understand it a bit I certainly see the merits of that strategy. Had I known that was a design goal when it was originally decided I'm not sure I would have been aware of the potential issues to really give good feedback about it. I agree it's a good design decision. It also has to come at the right time. I'm not sure this is quite the right time.

I'm thinking about this issue mostly from the perspective of the security risk calculator (more details here) and the risk for wormable security vulnerabilities. Within security issues, it's very damaging is when there is a security vulnerability that can be exploited in an automated fashion or, worse, where it can self-propagate (i.e. an infected site turns around and infects other sites). If the vulnerability is below a certain threshold it tends not to ever get exploited. Having a feature defaulted off, or default to on in a reduced mode will reduce the likelihood of the issue being exploited on a wide scale. It's also more likely to get exploited if there is no easy way to mitigate the risk with a configuration change: many sites are slow to ship code, but can ship config quickly. If config that is already supported on the site can mitigate the issue then that's great.

The various API improvements of Drupal 8 are really exciting and this is another step on that exciting path. They are also shining light on some code paths that are not as well hardened as the rest of Drupal. As a community of developers and site builders our sophistication around writing and testing these kinds of sites is advancing.

While the end goal of having it be zero config is a good one, I think it's worth considering whether that's the right strategy as the module is added to core and therefore available on a lot more sites.

Among the options in comment #4, I'm in favor of ideas #3 or #4 at this point from the perspective of reducing the impact of any future vulnerabilities.

I wouldn't block going ahead with strategies #1 or #2 from comment #4. This represents only my own perspective.

Thanks, Gabe.

I see an alternative perspective on 2 points you made, so I want to get into them a bit more.

Is this a chicken and egg problem? I don't think so. More eyes are landing on these code paths and on the various API modules already, even before it's in core. Moving it into core will bring yet more, regardless of which plan from #4 is followed. It's true that the more enabled it is, the more it will get attention and testing and, yes, hardening. I don't see it as a binary "chicken and egg" situation, but a spectrum of attention that can be gradually turned up.

There's another dynamic here that's worth considering that gets at your last paragraph:

What is being proposed here is simply outsourcing the risk from the Drupal project to Drupal's users by requiring them to carefully configure their sites.

An attackers ideal scenario is that in a single request by an anonymous user to a predictable path they can guarantee arbitrary code execution. The more that Drupal has predictable urls that are enabled by default and that are vulnerable, the more an attacker is going to go after it.

Let's consider a scenario where there is a vulnerability that allows RCE but it's only related to uploading files via the API, which is often possible via the profile picture. If jsonapi requires site builders to enable each resource/method via configuration in the API it seems likely that many sites would not enable the registration api which would dramatically reduce the number of vulnerable sites. That could dramatically reduce the value to an attacker in developing an exploit, perhaps to the degree that the exploit is not developed/automated. In this scenario, requiring site builders to opt-in on individual resources/methods would makes all sites safer (since an exploit is never automated).

I also think it's worth considering option #3 (a config in Drupal to disable certain routes/methods). We all always say "upgrade quickly!" but that's not always practical. In those cases, having a way to easily turn off the feature would reduce the risk for site-owners and, again, perhaps reduce the value to an attacker so much that attackers don't develop/automate an exploit.

We all always say "upgrade quickly!" but that's not always practical.

I agree. This issue isn't currently prioritized as Critical, and I don't know if it needs to be, but IMO, if any part of it is Critical, it's this part. If a site is running a bunch of core patches, or if there's some other reason that applying a security update can't happen in a matter of minutes/hours, it would be very helpful for a site owner to have a way to mitigate in the meantime.

I can disable paths at the loadbalancer, ingress, proxy, webserver, or caching layers... In the more common LAMP setup, you could do the same thing in a .htaccess as well.

I agree it's worth considering whether this is sufficient, or whether there's additional value to adding a mechanism in settings.php.

I also think it's worth considering option #3 (a config in Drupal to disable certain routes/methods).

If we decide that there is value in adding a mechanism in settings.php, here's a proof of concept patch for that. With this patch, I can add the following to settings.php:

$settings['routes_exclude'] = '/^jsonapi\./';
$settings['routes_include'] = '/^jsonapi\.(node--article\.|comment--)/';

Then, lo and behold, I can use JSON:API for Article nodes and all comments (regardless of bundle), but all other JSON:API routes return a 403.

Nothing in this patch is bound to JSON:API. Different values for those regular expressions could mitigate vulnerable routes elsewhere in Drupal.

If we do something like this, would that be sufficient to unblock security concerns with respect to committing JSON:API to core? And then perhaps additional discussion beyond that (such as whether to make it configuration-based, with a UI) could continue to happen in non-Critical followups?

One difference between jsonapi and the UI is that the UI doesn't give any anonymous visitor a list of all the things that are there.

if this is going to be the main concern, then we can just remove the /jsonapi route

I don't think we need to go as far as removing the /jsonapi route, but I do wonder if we should introduce a permission for it.

Inevitably, some will recommend that users install JSON:API, then promptly disable dozens of resources to mitigate risk.

Do you think that's a problem with #17? I doubt people will recommend adding a regex in settings.php as a way to achieve that. Wouldn't they just point people to JSON:API Extras instead? Which they can do regardless of what we do here. Because JSON:API Extras exists, I think #17 can remain purely a security mitigation switch for use when we issue SAs rather than as a proactive way of creating a minimal API. For people who proactively want to create a minimal API, use JSON:API Extras.

Speaking of which,

FWIW, I'm strongly +1 to letting sites have some say over what gets exposed to JSON:API. I don't yet know exactly how that should work, but I'd *love* to use it once it does. ;)

@dww: Why aren't you already using JSON:API Extras to achieve that?

Let's consider a scenario where there is a vulnerability that allows RCE but it's only related to uploading files via the API, which is often possible via the profile picture. If jsonapi requires site builders to enable each resource/method via configuration in the API it seems likely that many sites would not enable the registration api which would dramatically reduce the number of vulnerable sites.

every entity type in Drupal has an AccessControlHandler, which handles access to view, update and delete

Right. And also "create". And jsonapi does not come with a user registration resource. You can only POST a new user entity if you have permission to create User entities, which requires "administer users" permission.

I think the user registration example is a good one in that it points to that if an HTTP API module exposes non-entity resources, then it needs to be careful with getting the access control correct for that resource, and because it's easy to get that access control wrong, it's a good idea to apply defense in depth by requiring configuration to enable the resource. However, since jsonapi only exposes entities as resources, I think we can have much more confidence (and tests!) that the resources honor the access control of the entities, and because of this, I don't think defense in depth buys us enough additional confidence to be worthwhile.

I think we're still coming from a place of shock about https://www.drupal.org/sa-core-2019-003, where some GET requests were vulnerable, and it's natural that we want to learn from that and be cautious. However, 1) we fixed that, and 2) if it were to happen again that GET requests became vulnerable to something, how much would proactive API limiting really help? For example, using the above example, if the vulnerability were with a file field, then nodes and users and other entity types that are very likely in-use by the decoupled site and therefore their JSON:API routes are exposed, are also affected. Similarly, if there turns out to be a vulnerability with a particular non-internal contrib/custom content entity type, aren't the chances pretty high that if a site has enabled anonymous access to that entity type, that those entities are part of the decoupled site and therefore even if jsonapi were to allow per-resource configuration, that those resources would be enabled? And then what value would be achieved in a mitigation that disables that resource as compared with a mitigation that disables anonymous permissions to the entity type. Both would equally result in both securing the site and breaking the exact same features of the site in the exact same way.

even if the frontend is fully decoupled then there still a ton of things that are only used in the backend

By "things", do you specifically mean config entities, or something else? For config entities, by default, anonymous doesn't have access to anything. Some config entites do allow unrestricted (including anonymous) access to view the label, but if a decision was made to allow that for HTML access, then what's the risk/harm in exposing it to JSON:API?

In summary, #22 is my current thinking for why proactive API reduction isn't much help for most sites, but might be helpful for some sites, and those sites should use JSON:API Extras. But I think reactive mitigation once a vulnerability is disclosed is valuable, so something like #17 might still make sense, unless we decide we agree with #13 that reactive mitigation can be just as easily performed elsewhere in the infrastructure stack.

@effulgentsia re: #21:

@dww: Why aren't you already using JSON:API Extras to achieve that?

Because I didn't read the fine print on https://www.drupal.org/project/jsonapi and didn't realize jsonapi_extras even existed until it was mentioned in here. ;)

(The site I'm building where I care about JSON:API is still in the build stage, and the JSON:API integration is totally long term wish-list stuff for them, so I'm just starting to learn my way around all of this code in preparation for some day needing to use it).

/me blushes sheepishly...

It's been fascinating to read this conversation as it unfolds.

Thanks,
-Derek

I'd like to get candid clear clarification on something. What scenario is this issue talking about:

1. A break this glass in case of emergency solution.
2. A new feature to ease configuration of the available resources (since JSON:API already offers that configurability via internal).

I am not one that cares too much about the technical solution. However, I'd hate to have this feature publicly announced as the proper way to configure JSON:API. Future tutorials should not start with: Enable JSON:API (in core now!) and then edit settings.php to disable foo and bar…. This is what I mean by scenario 2. Alternatively in Gabe's words:

Inevitably, some will recommend that users install JSON:API, then promptly disable dozens of resources to mitigate risk. Blog posts just like the one I quoted in #7 won't have that same enthusiasm and appreciation for JSON:API's ease-of-use. Instead, they'll become longer and more tedious.

Scenario 1 would be something we add (to core or JSON:API) and only uncover when the security team deems necessary as part of a SA (I'm still a bit shocked about the necessity of this, but I don't want to mix concerns in this post).

Why the distinction? Because if this is going to become the way to configure JSON:API then the API-First coordinators / JSON:API maintainers should own this feature and build it with the delicate care we've put into JSON:API. If this is only break this glass in case of emergency, then … I don't feel strongly.

The direction we go about in this issue is directly linked to the intentions and motivations. In other words, weather we are going to add public documentation about this or not will define my recommendation.

I fully agree with:

In summary, #22 is my current thinking for why proactive API reduction isn't much help for most sites, but might be helpful for some sites, and those sites should use JSON:API Extras.

I see some technical problems on the patch for #17 and the suggestion in #13, but I'd like to hold off those thoughts until I get some responses to #25 if that's OK.

@effulgentsia in #17: I think a generic mechanism to disable certain routes for security reasons is fine, because it's not JSON:API-specific. This will be useful for many potential future security vulnerabilities/advisories. 👍

@effulgentsia in #22: wow, excellent breakdown. I wish I had written that. 😀 I think it even makes for an excellent issue summary. _{Just one small clarification: unlike REST, JSON:API was not vulnerable to deserialization-on-GET.}

I think #22 explains very well why zero config for entities is reasonable (because: high confidence), but riskier for other things (such as user registration). I think that #17 is reasonable to do, because it doesn't require sacrificing JSON:API's benefits and is a generic vulnerability mitigation mechanism.

So for me I think a reactive mitigation (similar to #17, not exactly that patch) should be enough to call this issue done in a minimal way - in that it doesn't add a UX burden and could help if an automated exploit appears in the first few minutes/hours after an SA. Whether it's actually the better solution here or not is a different question though.

Does JSON:API not offer a user registration resource yet or will it never do that?

@catch No user registration in JSON:API. I don't think there's much value in the adding explicit support for it in the future.

The portrayal of balancing security and DX makes some sense to me and it's a tough one to strike, for sure.

In #19 you provided some detail about how api endpoints can be disabled, either by changing an annotation (shipping code) or a permission change (which would also disable other paths to the entity). IMO those are not an effective, viable solution to the situation of a vulnerability that is only accessible via jsonapi and a site that cannot ship new code.

Removing or restricting access to this route is purely security by obscurity..

Security by obscurity is not a reliable solution to a vulnerability. However, to choose a random example, someone who has moved their ssh port from 22 to something higher will quickly tell you that obscurity has a great benefit for the readability of your auth log (which has a huge benefit for security). We should not rely on security by obscurity, but it is a very effective tool for both the ecosystem and an individual site owner on the blue team. More specifically to our situation, consider the practice of prefixing database tables with a random string, suggested in this fine blog post That is also security by obscurity, isn't it? And is it effective? Well, as this answer succinctly puts it it depends what kind of attack you want to protect against. Security by obscurity, when cheap to perform and done broadly, can help the both individual sites and the whole group of sites avoid attacks.

All of JSON:API's routes are quite predictable. The entrypoint is an essential starting point for HATEOAS based consumers and for developers exploring/learning their site's API.

Maybe many are predictable, but if they are not absolutely trivial to find (whether because it's easy to disable them or they are disabled by default) then it can offer a site extra days or weeks of delay against broad automated attacks.

Considering the perspective of "we'll get blog posts like X" - what about blog posts like "JSONAPI is insecure by design"? Or "Use the REST api in core, but skip jsonapi as it opens your site far too much far too quickly." That second one is something I'd be tempted to write, honestly. There are also big-name security bloggers/news outlets that love to criticize Drupal and, to the extent this helps a vulnerability become Drupalgeddon 4, they will point it out.

IMO those are not an effective, viable solution to the situation of a vulnerability that is only accessible via jsonapi and a site that cannot ship new code.

I'm curious what your opinion is of @effulgentsia and @catch's thoughts about a solution like #17.

I also just discovered https://www.drupal.org/blog/regarding-critical-security-patches-we-hear-.... Which proposes Drupal Steward which will offer sites a form of mitigation through the implementation of web application firewall rules to prevent mass exploitation of some highly critical vulnerabilities.

Don't they both address your concern?

@xjm I'm starting to be concerned about timing for this discussion. If there is anything mid level of effort to implement as the outcome of this we'll be running against the clock for the alpha tag. Unless you feel that we can implement after the commit into core -dev happens.

In other words, can we set a time frame for this?

> My suspicion is that since @Berdir's comment in #8, his opinion might have shifted to 1 as well.

Berdir isn't quite sure :)

My main goal was to have a discussion around my concerns, and this issue does that. I think there are enough people involved here to make a decision on that, I will definitely not block this in any way.

Personally, I do still have some concerns about the approach, and in case I will be able to use jsonapi in a project, I will most likely only do so in combination with the extras module that allows to only expose what I want.

The only thing that's a bit weird about that is that it explicitly relies on @internal things to do its job. If we "officially" suggest to use that module, then it is IMHO a bit strange that such a module or similar custom code must rely on @internal things. It might be harder to keep the integration in sync between core and contrib than between two contrib modules. For example, if 8.8 will make an change there that breaks the extras module, then it would need to do a new release, but 8.7 will still be supported for another 6 month too.

I will most likely only do so in combination with the extras module that allows to only expose what I want.

Me too. However, I think that not enforcing that delivers superior DX for many people that don't have any API design background.

The only thing that's a bit weird about that is that it explicitly relies on @internal things to do its job. If we "officially" suggest to use that module, then it is IMHO a bit strange that such a module or similar custom code must rely on @internal things. It might be harder to keep the integration in sync between core and contrib than between two contrib modules. For example, if 8.8 will make an change there that breaks the extras module, then it would need to do a new release, but 8.7 will still be supported for another 6 month too.

We had a meeting fully dedicated to this topic. Wim and Gabe felt like Extras is the second use of those internal APIs, and we will create PHP APIs when there is a third one (for better abstraction). In the mean time they agreed to share the burden to play catch up in JSON:API Extras every time we break "meaningful internal APIs". I'm OK with that (given that there is no ideal solution).

For the official record: I definitely do not want to block anything over my concerns (and ignorance). No one should await my "sign-off" on this. Yes, I'm happy to use jsonapi_extras if I want to be careful about what's getting exposed.

However, #35 is worrisome for that strategy. ;) I don't know the details, but I don't think jsonapi_extras can be using @internal to do what it does. If there's API needed for jsonapi_extras to provide this additional functionality, I don't believe that can be @internal anymore. Maybe that's a viable compromise? I know the JSON:API maintainers here love the idea that the whole module is @internal and that "there is no PHP API", but it's sort of hand-waving and even misleading if they then point to jsonapi_extras as a solution to these security concerns and that module uses "internal" API. If jsonapi_extras needs "hooks" (for lack of a better term, not knowing the specifics) to work, those hooks should be "public" and core-ified jsonapi.module needs to be rigorous / diligent about supporting that API, not changing it at will as "implementation details", etc.

x-post with #36. The "Rule of 3" is well-known, but also somewhat arbitrary. Just because we want to follow that doesn't mean we have to... ;) *shrug* There's no "rule of 3" core gate. It's just a convention.

Meanwhile, I have no doubt that the jsonapi_extras maintainers will be responsive!

But I still think it's sort of hand-waving and misleading to call this @internal if we know we need it. *shrug*

I'm happy to keep discussing if a PHP API is appropriate or not, and then what to mak it an API. However that can happen after we land JSON:API into core.

I'd like to get candid clear clarification on something. What scenario is this issue talking about:

1. A break this glass in case of emergency solution.
2. A new feature to ease configuration of the available resources (since JSON:API already offers that configurability via internal).

Correct me if I'm wrong, but I think we're converging on a consensus (maybe?) to:

1. Support #1 in core (potentially via settings.php).
2. Support #2 via a public API. This API does not need to provide the config model, it could just expose the PHP hook or interface, and leave configuration implementation to JSON:API Extras.

For #1, I'm updating my proposed PoC from #17 to this patch. No interdiff, because it's an entirely different approach. Rather than intervening during routing, it intervenes in Entity/Field/TypedData isInternal(). This provides more security, because it prevents undesired routes from even existing, and it also allows for the emergency removal of vulnerable field and property types without needing to remove the entire entity type.

Locally, I tested this with the following in my settings.php:

$settings['typed_data_internalize'] = [
  'internalize' => [
    \Drupal\Core\Config\Entity\ConfigEntityBase::class,
    \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::class,
  ],
  'except' => [
    \Drupal\Core\Datetime\Entity\DateFormat::class,
    \Drupal\image\Plugin\Field\FieldType\ImageItem::class,
  ],
];

This removed all config entity types except DateFormat from JSON:API (which you can see by inspecting the links section of the output of the /jsonapi page) and all ER fields except for image fields (which you can see by viewing the JSON:API output of an article node, and seeing field_image, but not field_tags or anything else, in data.relationships).

For #2, I opened #3037039: Create a public API for indicating resource types should not be exposed. Per #39, I know the JSON:API maintainers are open to discussing this, but I don't yet know if they agree with actually doing it. E.g., they might still have concerns with the principle of a JSON:API-specific API for this, as opposed to requiring that the only public API be the entity type's isInternal() method. So let's discuss that more in that issue?

We still need a security team and release management decision on if these two would be sufficient, and what if anything needs to be done prior to commit (e.g., merely a decision to do these things, or a code commit of one or both of them, or something else).

#40 looks like a very elegant solution to this! Bravo.

A few nits, then let's see a full testbot run on it. I can't imagine it'll fail, but it'd be nice to know for sure.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -370,6 +372,11 @@ public function set($property, $value) {
   +    if ($this->internalizeViaSettings($this->getClass())) {
   

   Wonder if the method wants to be called "isInternalViaSettings()" to be a bit more clear that we're basically testing a bool. "internalize" as a verb sounds like it might take some action or have a side effect or something, while "is" is very clear. ;)

2. +++ b/core/lib/Drupal/Core/TypedData/InternalizeViaSettingsTrait.php
   @@ -0,0 +1,33 @@
   +/**
   + *
   + */
   

   todo

3. +++ b/core/lib/Drupal/Core/TypedData/InternalizeViaSettingsTrait.php
   @@ -0,0 +1,33 @@
   +  private function internalizeViaSettings($class) {
   

   This also needs a docblock.

4. +++ b/core/lib/Drupal/Core/TypedData/InternalizeViaSettingsTrait.php
   @@ -0,0 +1,33 @@
   +      foreach ($setting['internalize'] as $test_class) {
   +        if (is_a($class, $test_class, TRUE)) {
   

   "test_class" might be misunderstood on a quick glance, or via grep, since we have a metric crapton of "test classes" in core. ;) Perhaps "settings_class"? Or maybe "internal_class" here?

5. +++ b/core/lib/Drupal/Core/TypedData/InternalizeViaSettingsTrait.php
   @@ -0,0 +1,33 @@
   +      foreach ($setting['except'] as $test_class) {
   

   ... and "except_class" here? Or stick with "settings_class" in both?

6. Needs tests (if possible). I don't know if/how we could mock the Settings class since it's so low-level. :/ A functional test of some kind to verify that the thing we said is internal is no longer accessible might also be nice for peace-of-mind that this actually locks down something (although I'm sure we've got other access tests for things where isInternal() returns true. But it'd be nice to make sure we don't accidentally forget to call the "internalize" method in the right spots, or refactor/remove it without noticing, etc.

Otherwise, I love it. ;) Meanwhile, I think this could be a follow-up security hardening "task" that could be done after alpha1 (and perhaps back-ported to 8.6.x), so I don't think we should block jsonapi itself on this. Perhaps we should submit that separately now so as not to bog down this discussion with nitpicky reviews like I've just done. ;)

Otherwise, some kind of agreement/commitment on #3037039: Create a public API for indicating resource types should not be exposed would be nice before we commit #2843147: Add JSON:API to core as a stable module but I don't think we need to *fix* #3037039 first, just agree on the plan.

Thanks!
-Derek

Just responding all the way back to comment #33 and earlier after I didn't post this on Friday:

I have a lot of hope for Drupal Steward (I've helped in the creation of it), but I wouldn't want to rely on its existence.

The solution from #17 seems great to me - thanks for drawing my attention to it. It's a way to do plan #3 from comment #4 that also helps in other ways, which seems great.

I do think approach #4 from comment #4 has a lot of merit in reducing mass attacks in addition to the solution in comment #17 (they don't seem mutually exclusive to me).

It seems a bit odd to me that a few folks are saying "I would deploy this in a much more conservative way" and also "It's important the default is completely open." That feels like we're putting the default user of Drupal at more risk than is recommendable. Maybe I'm misunderstanding something nuanced here?

While #42 was posted, @e0ipso and I were talking about this issue and the proposed solutions during the weekly API-First Initiative meeting.

@e0ipso was concerned that if there is a vulnerability related to GETting information, #17 won't mitigate it, since JSON:API allows following relationships, for example from a Node to the User that created it. So removing the User-related JSON:API routes will still not prevent access to JSON:API.

That is what #40 solves. @e0ipso is concerned that mechanism may end up being used by many sites, even though it is intended to be used only for security purposes. I am less concerned about that, because it's "so deep in Drupal" that I think few sites will use it (just like very few sites have, say, alternative container implementations even though that is also technically possible). Also, if many sites would do that, it'd be more difficult for them to follow the instructions of the SA, because they'd need to reconcile what the SA is telling them to do with what they already have.

We thought that both #17 and #40 have merit, they both are useful tools in mitigating security vulnerabilities, and neither is JSON:API-specific. Perhaps it makes sense to implement both, in separate issues?

_{I tried to remember what @e0ipso said, but after he left the call, the call still went on for a while, so I'm foggy on details :()}

#42:

Glad to read you like #17 :) But note the caveats for #17 I just called out.

What are your thoughts on #40?

I do think approach #4 from comment #4 has a lot of merit in reducing mass attacks in addition to the solution in comment #17 (they don't seem mutually exclusive to me).

Correct, they're not mutually exclusive. But #4 is would destroy DX, which has been said many times now, so I won't go into detail about it.

It seems a bit odd to me that a few folks are saying "I would deploy this in a much more conservative way" and also "It's important the default is completely open." That feels like we're putting the default user of Drupal at more risk than is recommendable. Maybe I'm misunderstanding something nuanced here?

What comments are you referring to when you say I would deploy this in a much more conservative way?

Thanks Wim. That captures very well the spirit of my side of the conversation.

To reiterate: we have an existing HTTP API surface that JSONAPI is increasing significantly. 17 and 40 close down that surface using independent techniques. We have a use for BOTH techniques, for mitigation purposes, regardless of JSON:API.

Hence, strong agreement with

Perhaps it makes sense to implement both, in separate issues?

One thing that we need to be aware of when we promote the internal-based approach from #40 is that it could have all kinds of side effects, because that is an API that we are promoting and telling everyone they should respect it if it makes sense for their context. Imagine content_moderation wouldn't allow to moderate internal entity types, or content_translation wouldn't allow to translate internal entity types (doesn't matter if that makes sense or not, just as an example), and then you make Node internal...

#45: good point! I'm not worried based on \Drupal\Core\Entity\EntityTypeInterface::isInternal()'s documentation:

  /**
   * Indicates whether the entity data is internal.
   *
   * This can be used in a scenario when it is not desirable to expose data of
   * this entity type to an external system.
   *
   * The implications of this method are left to the discretion of the caller.
   * For example, a module providing an HTTP API may not expose entities of
   * this type or a custom entity reference field settings form may deprioritize
   * entities of this type in a select list.
   *
   * @return bool
   *   TRUE if the entity data is internal, FALSE otherwise.
   *
   * @see \Drupal\Core\TypedData\DataDefinitionInterface::isInternal()
   */
  public function isInternal();

But #4 is would destroy DX, which has been said many times now

IMO it's been overstated many times now. I've tried to avoid hyperbole. Reading that DX is "destroyed" by requiring UI configuration seems extreme.

To the question:

What comments are you referring to when you say I would deploy this in a much more conservative way?

I mean berdir in #35:

I will most likely only do so in combination with the extras module that allows to only expose what I want.

And e0ipso agreeing in #36:

Me too.

Do I misunderstand something?

#46: I would be very worried though precisely based on that documentation, specifically the "The implications of this method are left to the discretion of the caller." part.

For example, like @Berdir hinted in #45, Content Moderation doesn't allow internal entity types to be moderated (#2971779: Disallow moderation of internal entity types) and Workspaces will not support workspace-specific revisions for internal entity types (#3027598: Omit workspaces entity presave and predelete hooks for internal entities). For a community site that relies on Content Moderation and/or Workspace workflows, this would mean that instead of mitigating a possible attack vector you would be disabling all of your site's functionality as well.

#48: Thanks, I didn't realize that. Given that, I agree that #40 is not an option.

Pair-written by @gabesullice and I.

#47:

Reading that DX is "destroyed" by requiring UI configuration seems extreme.

We want Drupal to be an API-First CMS/platform. That's why it was made an initiative.

The API-First Initiative has gone through lots of iterations, validation, processes and numerous discussions to arrive at jsonapi.module.

As an API-First platform, Drupal's data model is the configuration. Adding another layer of configuration is merely another layer of indirection that is going to make API client developers rule out Drupal as a valid choice at worst, a second-class citizen at best.

If we're going to add configuration separate from the data model, then it's not API-First, and hence diminishes Drupal's chances of being a successful API-first platform.

It seems a bit odd to me that a few folks are saying "I would deploy this in a much more conservative way" and also "It's important the default is completely open." That feels like we're putting the default user of Drupal at more risk than is recommendable. Maybe I'm misunderstanding something nuanced here?

Great question, and thanks for clarifying it in #47!

The nuance is that we do agree that for quite a few entity types that are currently exposed by default via JSON:API, it would make sense to not have these exposed by default (think Drupal plumbing like Action, ImageStyle, View …). That would already address part of the concern raised here.

Summary

Since the beginning of this issue, we've been agreeing that the surface area can be reduced. The disagreement is in where the appropriate place for that to happen is. If Drupal is to be API-First, we believe that place is not in the JSON:API module.

@amateescu: Core only marks 2 entity types as internal = TRUE: ContentModerationState and WorkspaceAssociation.

I think I understand the rationale for ContentModerationState: because its info is gettable and settable via the 'moderation_state' field of the host entity.

However, why is WorkspaceAssociation internal? That was added in #2784921-134: Add Workspaces experimental module's interdiff, but with no explanation.

I'm asking in an effort to understand what the intended semantics of "internal" is with respect to "a Drupal site's data model". Why, for example, should the workspace that an entity/revision is in not be considered as part of a site's data model? Is it because HTTP APIs are always expected to query from within a particular workspace (and then only see entities that are in that workspace), and therefore have no legitimate reason to query workspace association in any other way?

Content Moderation doesn't allow internal entity types to be moderated and Workspaces will not support workspace-specific revisions for internal entity types

a module providing an HTTP API may not expose [internal entity types] or a custom entity reference field settings form may deprioritize [internal entity types] in a select list

it would make sense to not have [internal entity types] exposed [to HTTP APIs] by default (think Drupal plumbing like Action, ImageStyle, View …)

I wonder if we're trying to overload "internal" with too many meanings. Seems like "internal with respect to content moderation and workflow", "internal with respect to HTTP APIs" and "internal with respect to entity referencing" are not identical concepts. Maybe there needs to be global "internal" concept that would cause all of those subconcepts to be true, but maybe there also needs to be separate concepts of internal with respect to each of those dimensions?

@effulgentsia,

Is it because HTTP APIs are always expected to query from within a particular workspace (and then only see entities that are in that workspace), and therefore have no legitimate reason to query workspace association in any other way?

That's 90% precise :) The only "correction" is that, when querying from within a particular workspace, you don't see only entities that are in that workspace, but all entities from the site with some possible workspace-specific "overrides".

Maybe there needs to be global "internal" concept that would cause all of those subconcepts to be true, but maybe there also needs to be separate concepts of internal with respect to each of those dimensions?

Remembering how hard it was and how long it took to introduce the initial internal concept, I feel that trying to come up with a superset of it would burn all of us out in a heartbeat. But maybe I'm just pessimistic lately...

Anyway, going a bit more on-topic for the broader discussion here, I would like to point out that Drupal provides an UI in core for configuring which entity fields are being shown to anonymous visitors and which fields are editable in an entity form (via the much dreaded Field UI), and that the options to customize the way a field is shown/edited are presented immediately after adding it, in the 'Manage display' and 'Manage form display' tabs.

IMO, the decision to not provide similar UI configuration capabilities in core for JSON:API and expose everything by default is really strange. We seem to be going from one extreme: you need REST UI from contrib to configure the output of your REST endpoints (with everything OFF by default) to another: you need JSON:API Extras from contrib for similar configuration, but with everything ON by default.

Wouldn't it be better to get to a middle ground and provide the configuration UI for JSON:API in core, while still exposing all (or most?) entity types by default? Note that I don't think adding this UI should block the commit of #2843147: Add JSON:API to core as a stable module, but it could be a high priority task that would hopefully land before 8.7.0-rc1. I think @Berdir's suggestion from #8 is a very sensible starting point.

#55 sounds completely right to me. Let's move this issue to core, call it a major task, and re-scope as described. This shouldn't be specific to JSON:API.

#40 also doesn't belong in the JSON:API queue, and should be done as a separate core task.

IMHO both are worth doing and neither should block JSON:API.

Thanks!
-Derek

I think we're arguing for exactly what you think we're arguing against. We are saying that core should have a way to make entity types internal. We're just saying that that facility should not be in JSON:API.

Okay, I think I see your point :) To be fair though, core had a facility to mark entity types as "internal" (as in: not exposing a rendered/normalized version of it to site visitors by default) way before we introduced the internal definition flag. Such an entity type simply doesn't provide a canonical link template in its definition, with \Drupal\file\Entity\File and \Drupal\aggregator\Entity\Item as examples in core.

Another two interesting data points are \Drupal\menu_link_content\Entity\MenuLinkContent and \Drupal\shortcut\Entity\Shortcut, which have their canonical link relation set to be the same as the edit one, which means something like "this entity does not make sense to be 'exposed' on its own, but possibly as part of a group of similar entities". Not sure how that would translate for JSON:API though..

In essence, if the issue title was:

[DISCUSSION] Should a configurable facility be added to Drupal core to limit automatic exposure of all entities

and this were not in our issue queue, but core's, I would be vehemently cheering it on!

TBH, I only realized this issue was not in the core issue queue after I posted #54 :)

IMO, the middle ground is: go through core's entity types and aggressively mark their annotations internal.

I have to say that I think we completely missed the boat for using the internal flag for this, because its behavior is so loosely defined that it can lead to problems like the one I mentioned in #48. So @effulgentsia's proposal for introducing a new concept is the better path to take IMO.

It's just the idea that this configuration should live under something like /admin/structure/jsonapi that bothers us so much.

I'm not sure why this idea is so problematic, having a centralized UI at /admin/config/services/jsonapi (note the different path) that would use the "new internal concept" to provide the default values, while also allowing site builders to override those (in-code) defaults is exactly what I would expect from any core module..

In case a site was using both JSON:API and REST, or trying out both to decide which to use, or maintaining both for legacy vs. new APIs or whatever, I don't see why they'd possibly want to configure this in 2 separate places. Hence a single UI (part of the entities themselves) for this, instead of dedicated/separate UIs + storage for these settings in both REST and JSON:API.

Same goes for the "non-API" world. If I could mark on a per entity type (+ bundle for entity types that have them) basis if a given thing was internal, that would mostly make something like rabbit_hole unneeded. #3017935: Media items should not be available at /media/{id} to users with 'view media' permission by default also might not have been needed, right?

At the same time, I see your concern in #48 and #57. However, re:

I have to say that I think we completely missed the boat for using the internal flag for this, because its behavior is so loosely defined that it can lead to problems like the one I mentioned in #48.

Wearing an API-first hat, that sounds like we have bugs in core that we should fix, not something JSON:API should have to hack around. :/

*shrug*

Seems like we all agree #40 is worth pursuing in a new core issue, right? Insofar as this issue as a security hardening concern is blocking JSON:API (see @xjm in #2843147-153: Add JSON:API to core as a stable module), #40 seems like the easiest and fastest solution in the short term, right? It's basically done except for a few minor nits from #41.

We can hash out the rest of this during alpha, right?

Trying to keep this moving towards a viable solution...

Thanks,
-Derek

I'm still reading up on the above discussion, but just wanted to point out that this is still blocking for JSON:API in core until it's had committer and probably security signoff, even if the solution we pick involves an upstream change in core rather than in the module itself. (Such is always true of core feature additions; e.g. there are limitations of the entity API itself that are blockers for Layout Builder even though they're not the fault of any code in Layout Builder.)

One thing I'm wondering about based on some of the above discussion is if we could put the "list all the everything" /jsonapi route behind some manner of permission? How important is that functionality in production or for unprivileged users? (This wouldn't address the central issue but it could provide added mitigation.)

#59: RE: the one thing you're wondering about: see #20.

Moved #40 and #41 to #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php as the potentially JSON:API-blocking security hardening task. Is that sufficient? Can we unblock JSON:API on the rest of this, and agree to work on making it better before beta1, not alpha1?

An IS update with specific proposed solutions and their pros/cons might help here.

Commenting on a few of the specific solutions above:

1. "Install JSON:API Extras" isn't really a mitigation strategy we can use for core. :)
2. I share concerns above about overloading internal and about the risks and side effects of adopting that solution. Exploring and validating that is not a two-week task. It's more like a two-month task, or even a two-year task... =/
3. Sounds like the listing permission idea is strongly rejected by maintainers as they deliberately removed it already. I've mixed feelings about that; I still kind of think disabling the route would be a valuable option for core to have.
4. Did we have any further discussion of #17?

@dww, re: #61: No, unfortunately we can't just defer the issue to try to fix it between alpha and beta. Security concerns are commit-blocking for new features. If we committed things with outstanding critical technical debt, that would add release blockers, which is incompatible with D8's fixed release schedule. Also, if we committed JSON:API to core but then this weren't resolved in time, we'd have to revert JSON:API from core before beta1, which would be hugely disruptive (and also even sadder than not landing it in 8.7 to begin with). :)

That said, I think we'd be open to commit the main JSON:API patch itself during the alpha phase (giving us two more weeks), if this issue were the only outstanding concern as of the alpha deadline. (There are several other commit-blockers ATM that we're actively discussing.) https://www.drupal.org/sa-core-2019-003 set us back basically an entire month, and I'm sensitive to the fact that all the JSON:API contributors (as well as myself) have been burning the candle at both ends for weeks especially because of that disruption.

I understand and support the goal and ideal of JSON:API having nothing special or additional over what core APIs provide -- I really do. But we're in an intermediate stage here, where core APIs are still catching up to what the UI controls, and in such an intermediate stage, mightn't a short-term workaround in JSON:API itself be OK? Something we'd have a followup to deprecate in favor of upstream fixes.

I think what we want is a simple way for a site owner to toggle read-only mode or turn off specific resources. Let's say the comment approval bypass from SA-CORE-2017-004 had just been released. As a site owner, it'd be really valuable to me to have a quick way to just turn off the darn comment resource, without disabling the commenting functionality on the main site.

Yes, ideally there'd be no tension between "Secure by default" and "API-first by default". Pragmatically, though, core APIs are still catching up. There's still many gaps between the robustness of the UI and what we support in the API. We've had at least five vulnerabilities in the past two years where the attack vector was REST or JSON:API. In almost every case the fix had to be made in the entity system or to particular entity types -- REST and JSON:API just exposed underlying problems. But these were still vulnerabilities that simply didn't exist through the UI. With REST, most of them (aside from that horrible GET object injection) were not a big deal, because the default configuration of the module didn't enable the vulnerable resources. All these issues would have been more severe if JSON:API were in core -- the difference between moderately critical and critical, or critical and highly critical. That's what we want to mitigate, and it'd be really good if we could find a compromise to do that while we continue to improve underlying APIs so that Drupal can be really API-first.

Hm -- I've been drafting and redrafting a comment for three hours so I'm going to leave it there and go to sleep since it's after midnight. :)

#62

I will respond to everything for the sake of addressing all questions, but please let us focus on this:

As far as I can tell, nobody is opposed to #17? I think it's very easy to keep this discussing for many more hours, days and even months. If we believe #17 is a mitigation that reduces risk sufficiently, can we not just do that? Created #3037942: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily for this.

#62 — addressing all questions. Again, I hope we can just do #17 (in #3037942: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily) and stop spending so much time on this discussion.

1. Agreed!
2. The meaning of internal is not being overloaded. The purpose we want to use it for here is the very reason it was added to core! See #2936714: Entity type definitions cannot be set as 'internal'. I don't think that issue summary leaves any doubt.
   _{Apparently Content Moderation decided to interpret this in a vastly different way. In #57, @amateescu says there was a heuristic already available for this. I suggest Content Moderation then use that instead. The heuristic in #57 is insufficient for the HTTP API use case, because A) contrib modules could be adding/altering entity link relations to provide additional functionality (think https://www.drupal.org/project/contact_storage), B) the absence of canonical doesn't signal internal to HTTP APIs: File entities still need to be HTTP API-accessible.}
3. […] I still kind of think disabling the route would […] — I think you're talking about the /jsonapi route (route name: jsonapi.resource_list).
   With #17 core would have the option to disable that route, just like any other route. Also, nothing prevents core from adding a new route requirement in the future. But there is no sensitive information there. #20 already explained this. #32 pointed out that some obscurity can be helpful. I agree! But even with this route gone, JSON:API routes/URLs still are absolutely predictable (that's the point) and hence it would not protect against automated attacks.
4. I +1'd #17 in #27, @catch +1'd the concept in #28, @greggles in #42, @e0ipso in #43/#44. I think we all agree this is a useful security mitigation tool to have in core in general.

[…] mightn't a short-term workaround in JSON:API itself be OK?

No, because:

1. AFAICT it cannot be a "short-term work-around", since removing this would break BC?
2. Like @gabesullice argued many times, most recently in #55: this cannot be JSON:API-specific. As https://www.drupal.org/sa-core-2019-003 demonstrated, vulnerabilities deep in Entity/Field API affect not just REST, but also JSON:API (and GraphQL if it were stable). We want to be able to protect/mitigate all of these simultaneously.

I think what we want is a simple way for a site owner to toggle read-only mode or turn off specific resources.

We've had this discussion already. That would mean JSON:API no longer is zero-configuration. It also makes it impossible for an install-and-it-works ecosystem to be built on top of JSON:API. See #9.

REST and JSON:API just exposed underlying problems.

Correct. Note: the same is true for GraphQL, but we aren't talking about that because https://www.drupal.org/project/graphql doesn't yet have a stable release.

But these were still vulnerabilities that simply didn't exist through the UI.

… but those vulnerabilities also exist in contrib/custom modules using Entity/Field API directly, and we chose not to protect those (because we chose not to fix the root cause) and instead modify REST/JSON:API. See @effulgentsia's very clear comment about this in #22/#23.

That's what we want to mitigate, and it'd be really good if we could find a compromise […]

Yep. I believe that compromise has already been found: #17 and maybe also #40.

As far as I can tell, nobody is opposed to #17? I think it's very easy to keep this discussing for many more hours, days and even months. If we believe #17 is a mitigation that reduces risk sufficiently, can we not just do that?

I like 17 but would prefer 17 and more.

I like 17 but would prefer 17 and more.

What does "17 and more" mean?

Re: #62:

@dww, re: #61: No, unfortunately we can't just defer the issue to try to fix it between alpha and beta. Security concerns are commit-blocking for new features.

To be clear, I was proposing:
- Use #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php as the critical blocking security hardening issue to give us a way to quickly mitigate if needed without deploying new code.
- Use this issue to continue hashing out a UI and any other fall-out of marking entities and fields internal per #55.

JSON:API-in-core would be blocked on the first, but not the second. I was not proposing to commit JSON:API without any mitigation strategy. I was proposing to sort out the UI and extended philosophical debates as a "should-have" for 8.7.0. If the UI ends up having to wait for 8.8, so be it. We'd still have a settings.php-based mitigation strategy, which I believed the security team (current and former) members in this issue agreed was sufficient.

Meanwhile, if internal is "overloaded" in core to mean something other than "not external", that is the critical technical debt y'all already added. That's not JSON:API's fault, and shouldn't hold this up. If Content moderation or whatever thinks "internal" actually means "something we can't deal with, punt", please link those critical bug reports here (or in a new "[META] Make @internal internal" plan or something) for interested parties to deal with.

If there has to be some "JSON:API-specific" compromise about this all, then perhaps it could be summarized as:

"JSON:API in core always believes that @internal means that thing should be internal, and doesn't expose any end points for it (even if the rest of core is broken about this in some way)."

I believe the JSON:API maintainers would agree with that approach, and my understanding is that's how the code already works. ;)

In other news, I'd also be okay with #3037942: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily instead of #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php as the critical mitigation blocker, although I think it's going to give us less incentive to fix these @internal tech debt bugs already in core.

Ok, going to attempt to read this entire thread and write an issue summary. Wish me luck.

I think whether #17 could work would depend on how easy it was for other sites. Are we going to have example rules for them to uncomment? How does the site owner which endpoints to disable and how do they know how to capture that in their settings.php? The list is ever-expanding and dependent on what content their site actually provdes. Does it provide us a way to put the entire site in read-only mode, for a situation where multiple entity types are affected or the security team has not yet disclosed which entity types are affected?

Up through comment #20.

Re: #67:

Meanwhile, if internal is "overloaded" in core to mean something other than "not external", that is the critical technical debt y'all already added. That's not JSON:API's fault, and shouldn't hold this up.

Unfortunately, even if it's not JSON:API's fault, JSON:API is still the thing exposing the vulnerabilities in this way and therefore to add JSON:API to core it's on the initiative to figure out how to harden it. Let's not continue to raise arguments about things not blocking JSON:API because of otherwise unexposed (or only minimally exposed) technical debt in core. If your module exposes a critical problem for the first time, it blocks core inclusion of your module to fix that, even if the fix is in code outside your module. This a release management requirement and we're not going to change that here, so I'd really appreciate it if we'd stop repeating that on this issue and focus on what mitigations we can agree on.

I also just don't think internal is the right solution for this, and it's certainly not something I'd want to rush on right now when all the entity and field subsystem maintainers have expressed concern.

And that busted my brain wide open, so taking a break for a bit, if anyone else wants to take over. :) Else I'll be back later this afternoon.

AFAICT it cannot be a "short-term work-around", since removing this would break BC?

We would deprecate and remove it in Drupal 9 or 10. Core does this all the time, already, everywhere.

It is also not a requirement to implement a solution that works for REST and GraphQL and etc, because:

• REST already has the requested mitigations in place: Most resources are off by default and any can be disabled with a config change.
• GraphQL isn't in core and doesn't have a stable release to receive security team coverage, so it's not really in scope for the discussion.

Ideally, we would have a long-term solution that makes disabling resource routes or whatever across the board a cinch for site builders. But pragmatically, that's going to take a lot longer to build than just putting in a simple mitigation, and also is risky because it could break other things. Many, many modules have had to implement workaround and then deprecate those workaround when the functionality was later finalized in core APIs. JSON:API would not be the first.

I think what we want is a simple way for a site owner to toggle read-only mode or turn off specific resources.

We've had this discussion already. That would mean JSON:API no longer is zero-configuration. It also makes it impossible for an install-and-it-works ecosystem to be built on top of JSON:API. See #9.

It was discussed and maintainers pushed back, but other contributors did not agree. All the security team members still think a read-only mode is desirable. We have a need for this (from my comment in #69):

The list [of routes] is ever-expanding and dependent on what content their site actually provides [so therefore a lot for a site builder to manage]. Does it provide us a way to put the entire site in read-only mode, for a situation where multiple entity types are affected or the security team has not yet disclosed which entity types are affected?

#69:

How does the site owner which endpoints to disable and how do they know how to capture that in their settings.php?

As I imagine it, the security team will share the code snippet to put in settings.php.

Does it provide us a way to put the entire site in read-only mode, for a situation where multiple entity types are affected or the security team has not yet disclosed which entity types are affected?

Yes, #17 covers that. I think it would look like

$settings['routes_exclude'] = '/^jsonapi\./';

#71:

I also just don't think internal is the right solution for this, and it's certainly not something I'd want to rush on right now when all the entity and field subsystem maintainers have expressed concern.

I agree with this.

#73:

Ideally, we would have a long-term solution that makes disabling resource routes or whatever across the board a cinch for site builders.

That sounds like a new issue Add JSON:API Extras into core. I'm not ready for that just yet (shivers terrified).

Now seriously, this is exactly what I wanted to gather early on in this discussion (see #25). I'm very concerned if this is a goal for this issue. I think you are still talking about an ideal that we won't implement just now. We are still treating this as a break this glass in case of emergency scenario. Can you please confirm this?

#65:

I like 17 but would prefer 17 and more.

@greggles, like #66 I am hoping for more concrete concerns. I think #17 is a great way to get that Read Only Mode with some granularity even.

Your voice is extremely influential in this discussion, and we are on a very very tight deadline at the moment. Any further insights will be greatly appreciated.

Commenting since I've been asked to, but I don't have any new information to share.

As I've said previously, I'm commenting as myself and not in any kind of official capacity.

Re: #75:

Ideally, we would have a long-term solution that makes disabling resource routes or whatever across the board a cinch for site builders.

That sounds like a new issue Add JSON:API Extras into core. I'm not ready for that just yet (shivers terrified).

I think my remark there was unclear. In the context of my comment, I was replying to @dww and meant that a potential goal would be an agnostic way to control this that would work for REST, JSON:API, GraphQL, or whatever else as @dww was suggesting, but was trying to clarify that completing that is not a requirement here. I.e., the only requirement in scope here is to solve this hardening issue for JSON:API alone. From core's perspective, it doesn't have to be a global solution upstream in the Entity or routing systems, and it doesn't have to also work for REST or anything else, because REST really doesn't have issue in the same way. Sorry for the confusion! (Sometimes I wish our issue queue supported threading.)

That sounds like a new issue Add JSON:API Extras into core. I'm not ready for that just yet (shivers terrified).

I hadn't considered this as an actual possibility -- it something you would never consider, or does this mean you might consider it at some point in the future? If it were considered for core inclusion I would probably have advocated for putting it in the same module, which it's very clear the maintainers reject. (Reminds me of the Views vs. Views UI split which is a pattern that is repeated several times with core modules.)

Now seriously, this is exactly what I wanted to gather early on in this discussion (see #25). I'm very concerned if this is a goal for this issue. I think you are still talking about an ideal that we won't implement just now. We are still treating this as a break this glass in case of emergency scenario. Can you please confirm this?

I think that, at a minimum, it needs to be something a site owner can do easily and without much help. UIs are obviously the easiest way for a site owner to do something without help, so I'm saddened by the vehement rejection of giving nontechnical site owners control over this aspect of their site. I disagree that it would be such a horrible thing, but since it looks like the JSON:API maintainers definitely will not bring consensus on that, I want to look at how else we can make it easy.

I'm not sure we can expect site owners to write regexes in their settings.php in response to a security issue. We might however have more success letting them set a line like:

$json_api_readonly = TRUE;

If I'm not mistaken, this example:

$settings['routes_exclude'] = '/^jsonapi\./';

Wouldn't that disable GET endpoints as well, and at that point, an easier solution is to turn off the whole module?

I'm also just leery of any mitigation strategies that require regexes. It's so easy for a stray character to break the mitigation with no indication to the site owner. It'd be really good if we could wrap that into something cleaner and declarative.

Back on this summarizing crapola again.

#78 + #79: you're not alone, which is why as of #3037942-3: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily that proposal is not using regular expressions anymore at all. 🙂

Up thru #40.

Thru #61, but running out of steam again. That was a nice little loop-de-loo on #17 over to #40 back to #17. ;)

$json_api_readonly = TRUE;

With the proposal in #3037942-9: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily that could be achieved with:

$settings['security_mitigations']['routes'] = [
  'disable' => [
    'jsonapi.*.post',
    'jsonapi.*.patch',
    'jsonapi.*.delete',
  ],
];

Because it just so happens that JSON:API names its routes nicely like that. The above wouldn't work for modules that don't structure their route names similarly.

JSON:API currently does not add a "get" suffix to the names of GET routes. But we could maybe change it to do that? In which case it would allow the above to be alternately implemented as:

$settings['security_mitigations']['routes'] = [
  'disable' => [
    'jsonapi.*',
  ],
  'except' => [
    'jsonapi.*.get',
  ],
];

#40 allows intervening in Entity/Field/TypedData isInternal() which provides more security, because it prevents undesired routes from even existing, and it also allows for the emergency removal of vulnerable field and property types without needing to remove the entire entity type. #3037039: Create a public API for marking resource types internal However, this can have unintended consequences; for example, Content Moderation doesn't allow internal entity types to be moderated, and for a community site that relies on Content Moderation and/or Workspace workflows, this would mean that instead of mitigating a possible attack vector you would be disabling all of your site's functionality as well. [#45, #48], so was ruled out as a viable solution. [#49])

Remembering how hard it was and how long it took to introduce the initial internal concept, I feel that trying to come up with a superset of it would burn all of us out in a heartbeat.

Hm, maybe this'll be shot down, but FWIW, here's an idea: maybe everyone will like it and it won't burn anyone out :)

What if we add an EntityTypeInterface::isApi() method? Perhaps even with a $bundle optional parameter? Perhaps its initial implementation could be:

function isApi($bundle = NULL) {
  return ($this->api !== FALSE) && (!$this->isInternal()) && (!SOMEHOW_DISABLED_VIA_SETTINGS_PHP());
}

Then #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php could be refactored to affect isApi() rather than affecting isInternal(), and JSON:API (and other HTTP API modules) could be updated to create resource types based on isApi() rather than based on !isInternal().

A module or future core patch could then create a configuration UI for affecting isApi() (e.g., via altering the 'api' definition key).

This would all be in the spirit of #83.

The concept would then be that isInternal() means "internal to lots of things, including Content Moderation, HTTP APIs, and other stuff" and isApi() would just be about HTTP APIs.

Thoughts?

Note to all: please read #3037942: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily before commenting on any of the specifics of that mitigation strategy in here. @effulgentsia and I are sorting out a bunch of details, making it easier to use, etc. We're not spamming both issues for it, but I wanted to mention it here. @Wim Leers already pointed out it's no longer regexp-based in #81, but even the example code in #85 doesn't reflect our current thinking on how it should work. If folks care about the details, please participate there, not here.

Useful comments here would be: "yes, #3037942 addresses our concerns and when that lands in core, JSON:API can, too." ;)

Thanks,
-Derek

p.s. Adding links to the child issues to "Proposed resolution" point 2.

Sorry, the summary is confusing. I see I put the links in the wrong spot(s). Now added the link to #3037942: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily in point #7 of proposed resolution.

Also, points 9 and 10 were duplicate. Merged all that into a single point. They're all talking about the same approach, which is at #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php (and which is probably headed for "won't fix").

Attempting to further clarify the proposed resolutions and restoring the link to #3037039: Create a public API for indicating resource types should not be exposed for completeness.

Thanks @gabesullice. #3037039: Create a public API for indicating resource types should not be exposed seems an interesting approach to me, although I might call the resources something other than "internal". One other difficulty is that there are entity types and bundles that don't have a page in the core UI like the node article in the screenshot does. A central configuration page OTOH could display all those random things we've discussed above on the thread.

I think this could be implemented in JSON:API (since REST stuff is off by default) and then deprecated once the core API is complete. I understand the desire to have core provide the API, but the fact is that JSON:API has a limitation here in a way that REST currently doesn't is important in the messy intermediate state core is in.

What I find important is that every person on this issue (as far as I've seen) has said "I wouldn't install JSON:API without JSON:API Extras" (because of this issue and things related to it). But less knowledgeable site owners aren't going to know they need a contrib module, and they shouldn't need a contrib module if users consider it an essential feature of the core module. I think what's strategically important for JSON:API is that you just turn it on and it works without complicated setup -- but you should also be able to turn stuff off.

I think this be implemented in JSON:API (since REST stuff is off by default) and then deprecated once the core API is complete.

Here's one more attempt at a proposal to try to avoid going down that route.

System.module already defines a admin/config/services path (with a admin/config/services/rss-publishing child). Let's have it add a admin/config/services/data-model path with a configuration form that writes to a system.http_api_data_model.yml config file (or, if we're allowed to put that into the core namespace, then core.http_api_data_model.yml).

The configuration form would contain a checkbox for each entity type whose isInternal()is false. The title for the checkboxes could be something like "Restrict Web APIs to the following entity types". If no checkboxes are checked, then all entity types are allowed. If some checkboxes are checked, then JSON:API only exposes the ones that are.

We can then make the EntityTypeInterface::isApi() method proposed in #86 return true or false based on that configuration. Or, if we don't want to add a method to EntityTypeInterface for whatever reason, then perhaps we could add a Drupal\Core\Http\ApiManager class/service with a mayExposeEntityType($entity_type) method (or better name).

And we punt on per-bundle config for now. REST doesn't support per-bundle config, so doesn't seem urgent to do so for JSON:API. But, I do think we could eventually add the per-bundle support.

And we punt on per-bundle config for now.

See #3037039-8: Create a public API for indicating resource types should not be exposed for why.

I just got off the phone with Wim and Gabe, and we came up with another proposal that I'll write up in an hour or so.

Also, I forgot to mention in my earlier comments that the iterations of #86, #93, and #95 are all about solving the "and more" part of #65. I'm still in favor of also doing #3037942: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily to cover the "I like 17" part.

I think it's odd that we're talking about adding a new core API (settings.php functionality) to protect against predictable future security vulnerabilities in a new core module, instead of making the module more secure by default. Enabling JSON:API and visiting one configuration screen to enable the resources you need doesn't seem that bad to me, compared to the known risk of turning on all entity resources by default when JSON:API is enabled.

What developers will be turned away from JSON:API because they have to complete one configuration form? Is the known security risk of enabling all entity resources by default worth minor inconvenience?

I'm also not sure if this has been explicitly mentioned, but I'm not aware of any members of the security team that support enabling all entity resources by default in JSON:API.*

*Edit: Was just made aware that @webchick supports this, who is a member of the team - just based my comment on all of the security Slack conversations for the past few weeks.

Here's the writeup promised in #95.

I'm not aware of any members of the security team that support enabling all entity resources by default

Per #55 and #83, the JSON:API maintainers aren't demanding that all entity resources be enabled by default either. The sticking points have been:
1) Should the marker of whether an entity type is JSON:API-enabled be a JSON:API-specific marker (whether a hook or config)?
2) Should the marker be controlled through configuration or through code?

The JSON:API maintainers have argued for making it a marker that's independent of JSON:API. In other words, the decision shouldn't be "is this enabled for JSON:API", but rather "is this enabled for HTTP APIs in general, with JSON:API being one implementation of an HTTP API".

Additionally, during the phone call in #95, Gabe made a good case for the marker to be set at the same place as the thing being marked. In other words, entity types are defined in code, therefore they should be marked for HTTP API consumption or not in code. Bundles are (usually) created in config, therefore they should be marked for HTTP API consumption or not in the same config.

For security parity with REST module, we don't need to implement the per-bundle config yet, so let's punt that to #3037039: Create a public API for indicating resource types should not be exposed.

For entity types, here's a patch that:

• Adds a EntityTypeInterface::isApiConsumable() method. It's reclaiming what the JSON:API maintainers believed !isInternal() was prior to learning about #48.
• Defaults it to TRUE for content entity types, FALSE for config entity types.
• In some cases, config entity types are intended for API usage and content entity types are not. To demonstrate each of these, this patch annotates it to TRUE for Vocabulary and to FALSE for aggregator Item.

The proposal here is then for contrib modules to be the ones to take action when the most logical choice is different than the above defaults. For example, Commerce module might:

• Annotate all/some of its entity types with api_consumable=FALSE.
• Have a commerce_api module that alters those to TRUE.

Meanwhile, a different contrib module might have its own configuration page (or checkbox on existing pages) for deciding whether API support is wanted for them. While other contrib modules could just do nothing and allow all their content entity types to automatically be available for API consumption.

I realize some people have argued for this to be UI-configurable, especially for the case of quickly allowing mitigation following a security disclosure. However, if we set better defaults in code, as in this patch, then would #3037942: Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily be sufficient for remaining mitigation needs?

@effulgentsia Since api_consumable defaults to TRUE for content entity types, you're expecting maintainers to update all their entity type annotations to add extra security, which they're already not doing with the APIs (internal annotation) and information (testing their code in the context of JSON:API and REST) available to them.

Real quick (I can respond more in depth later), if your target is a non-technical user for a "holy shit break glass emergency" type of problem, what you want is a one button "turn on read-only mode of my site." Not them screwing around with understanding what a "node entity endpoint" is nor how to edit PHP configuration files.

I'm also in favour of read-only mode, being able to disable these resources is going to make life extremely difficult for the Admin UI team.

From the point of view of building sites that actually consume these APIs, in my experience the only practical way to do this is what Gabe mentioned in #91 or having a layer in between Drupal to serve the API*, and so that's where all the bits we don't care about get filtered.

* The reason for this is the way Drupal's content model can shift makes it impossible to provide a stable API for a consumer without some middleware processing it. It also becomes very slow for queries with non-trivial relationships, which the middleware also mitigates somewhat.

Yes!

What Angie said in #100 and then Sally agreed on #101! I was talking about that yesterday with the other API-First coordinators.

e0ipso [8:51 PM]
@wimleers you around?
@gabesullice ^
Why is this configurability granular to resource type?
Why not, <code>$settings['read_only_routes'] = ['jsonapi.*', 'other.affected.routes']

and we're done with it?

This will massively reduce the API surface (only allow GET/HEAD/OPTIONS) and even more the vectors of attack (since the mutating methods are orders of magnitude more vulnerable).

I want to note that the API-First Initiative coordinators are (speaking my mind here):

1. Very open to collaboration to reduce the potential vectors of attack. See the read-only proposal, the entity type annotation proposal, the route deletion proposal, …
2. Very knowledgeable in our area of expertise.

We are not making shit up. We are not saying NO to site-builder configurability so we can miss important stuff with our families and have a chance to keep dealing with this last minute crisis. The only person that is relying on the JSON:API features that has commented here validates this by saying:

being able to disable these resources is going to make life extremely difficult for the Admin UI team

Personally I've been dealing with decoupled architectures for the last +5 years. In parallel with that I have been maintaining a good number of modules on the decoupled front, which has given me an insight to many other architectures people use through the issue queues.

On top of that, I even wrote and maintained the configurable approach in https://www.drupal.org/project/restful. And after that experience I decided to write https://www.drupal.org/project/jsonapi from a zero-configuration approach (and then optional refining via an additional module). That was not a coin-toss decision, it was an intentional & meditated decision backed by years of experience in using API generators in Drupal and node, maintaining API generators, and analyzing support requests of other people having these problems.

On top of that, I am the author and maintainer of JSON:API Extras. This module provides site-builder configurability. I know that we cannot add this as a feature last minute because I have first-hand experience about the potential for site-breaking configurations this will open. It would be irresponsible to put this feature in core now.

Again, I'm also very empathic about reducing attack vectors, as highlighted by the proposals above. But I will not support a JSON:API in core with site-builder configurability added last minute. When we add that configurability it will be a feature that the JSON:API team and the API-First collaborators will support and feel proud about.

Cross-linking https://www.drupal.org/project/readonlymode and #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released.

FWIW though, #98 seems like a pretty decent compromise on the security vs. usability/DX paradox:

1) It vastly decreases the overall attack surface, by only enabling content entities by default. (Which is a reasonable assumption; and when it's not, there's the ability to override it, as indicated in the patch.)
2) It passes the burden of creating a secure HTTP API to module developers vs. less-technical site builders.
3) If, despite this, people need/want a UI for more fine-grained access tweaking, hhttps://www.drupal.org/project/jsonapi_extras exists.

And I agree strongly with @e0ipso that this is not the kind of UI you want to rush. Clicking the wrong thing could break huge swaths of your site, and it's not necessarily clear how to get it back unless you have a very firm grasp of Drupal terminology (which we generally go to great lengths to keep out of core UIs).

I do see Sam's concerns in #99, but it's a one-line fix if we find something untoward. And that's way less risky, to me at least, than asking site builders to understand and grapple with the security implications of having this endpoint turned on vs. that one.

#98 would only protect one of the API related vulnerabilities I've worked on: https://www.drupal.org/sa-contrib-2018-057

https://www.drupal.org/sa-core-2019-003, https://www.drupal.org/SA-CORE-2017-002, and https://www.drupal.org/SA-CORE-2017-003 would have needed something more like the settings.php proposal(s) to be mitigated.

#98 also seems a bit impractical for the site builders we're trying to protect here, because modules like Paragraphs aren't going to work out of the box anymore.

So now it

So now it

So now it

So now it

#111 is missing some covfefe

Oookay. Coming at this wearing my "I am trying to listen hard and balance all the concerns" hat, not my security team hat, not my product manager hat, not my "help, all my friends are disagreeing and I want to help" hat (ok, maybe a little of that one ;)).

I spent some time today in the #security channel trying to distill down a "short list" of the concerns expressed by the team. With the caveats that these are all individuals, not speaking on behalf of the entire team, and therefore this list may not yet be complete, here's what this essentially boiled down to:

1) Do not indiscriminately turn on invisible stuff, because it vastly increases the attack surface of an average Drupal site that has JSON:API installed with no mechanism (in core) to turn it off.



2) Do not rely on entity access + permissions alone to mitigate against attacks, because although theoretically that should be fine, both recent core and certainly contrib SAs have shown that module developers are not currently viewing things with an “API-First” mentality. An "opt-in" mechanism works, but also works directly against the module developers' design goals of "zero configuration" / "it just works." (Which, it should be said, is also a key product management goal, though not one that should take priority over security.)



3) We need some way in case of emergency to shut down an API endpoint (including GET access), because sometimes shit happens.

 The optimal case for this would be a checkbox (or similar) in the UI, because anything that requires code changes (even as simple as a settings.php toggle) can introduce additional delays due to overhead/process.

4) The /jsonapi path makes some folks very nervous, because it effectively gives attackers the "answer key" of what available paths they can try and attack (even though these are guessable/scriptable as-is). It's more concerning because of the recent rise in API-based attacks as a general security trend, esp. "Zombie" ones that turn the site itself into something that can attack other sites.

(I'd like to take security concern #4 to a separate issue, since there was what sounded like reasonable pushback from the JSON:API maintainers on that one.)

With that in mind, we should also discuss a few concrete use cases for JSON:API (I'm sure there are others):

A) The "Fancy Comments" module. It (theoretically) provides a nice, responsive, swooshy-wooshy JavaScript-based front-end experience for comments (so, for example, new replies show up without needing to refresh) and uses JSON:API to pull and push comment data. Installed by a site builder, who has no clue how the underpinnings work, they just want fancy comments.

B) The "Commerce API" module. This is installed by a developer who expects to be able to then immediately begin work on their decoupled front-end for Commerce, without a lot of hassles.

C) The Drupal Admin UI initiative. This is intending to replace the entirety of the Drupal admin UI with a fully decoupled React application. As such, it needs full access to every part of Drupal, so that it can expose administration screens for things like Image Styles, Views, and so on. (Also a variation of A), in that the person enabling this is not going to necessarily know/care what's happening under the hood).

D) The "I'm a JavaScript developer who's using Drupal as a backend for my decoupled front-end" person. Their initial foray into our API-first experience is going to determine their impressions of Drupal, and 97 of their closest friends' impressions of Drupal, and so the evaluator experience is key here, especially given the general trends toward headless CMS + JavaScript and the rise of other formidable competitors in this space, like Contentful. (TL;DR: Drupal definitely ain't the only thing providing a UI and API for structured content anymore.) They do expose the entire data model via the API, and Drupal not doing this would be a significant handicap — for this use case.

Going to stop here and move everything else that happened today to separate comment(s), since this part might be useful for future reference to help build alignment on what problems we're trying to solve and who we're solving them for.

Next, another round of solution-eering.

Talking with @Wim Leers and @effulgentsia, we initially went down a road of a muti-faceted approach for solving security concerns 1-3 (I am running out of sequences to use here, lol):

I. A new flag for .info.yml files, to allow declaring API dependencies. For example:

api_dependencies:
    - entity:node
    - entity:comment
    - nonentitysomething

Only the routes for APIs which are explicitly listed as dependencies by something would be turned on in this case. This helps address security point #1. For the Drupal Admin UI case, we'd need a hook or similar to turn all entities on, but that's by far not the standard use case.

II. An "opt-in" Entity annotation such as api_consumable = TRUE/FALSE, which would default to FALSE unless otherwise specified. TRUE means you've explicitly intended your entity type to be exposed as an API, meaning you as the entity type maintainer/developer are confident about its access control handler and validation constraints. If a module or theme declares an api_dependency on an entity type that does not have api_consumable = TRUE, then there are warnings and confirm forms to click through. This helps address security point #2, where contrib modules that were originally developed in the mid-2000s, with NO idea that API-First would ever be a thing, are not suddenly exposed on 1000s of sites.

III. For security point #3, a UI as extensive as what JSON:API Extras offers is not a good solution; this is much more helpful in the "I'm a developer and understand exactly how this works and want to tweak it" case (which is what @e0ipso uses it for: tweaking). What you want is something dead-ass simple because you might have MINUTES to respond in the event something really bad happened. For this, we proposed a simple config page with a 3-way toggle: "Full API access / Read-only API access / Turn off API access." This allows SAs to go out recommending mitigation steps without necessarily giving away the answer key, like "uncheck PATCH under File" or whatever. (There was agreement of @e0ipso and @Wim Leers, so if others are amenable to it, we can probably just do this.)

Taken together, measures I, II and III have the advantage of cleanly addressing use case A (users don't unknowingly expose tons of data they had no idea about), use case B (a module developer has knowledge of what data it needs to expose, and that data automatically gets exposed at the right time), and use case C ("there's a hook for that").

They do not, however, adequately address use case D. @e0ipso in particular feels very strongly about this initial experience. And they have data to back this up, in the form of user validation (listening to user feedback and adjusting to meet real-world needs and improve DX); adoption/usage (nearly 10K installs, getting close to REST’s number of installs); and numerous personal developer accounts cited throughout the issue. We had a whole freaking initiative around the out of the box experience for Drupal for site builders, and it feels like we probably need something similar here for headless developers. There's some pre-existing thinking at #2849726: Add an API-First installation profile to core.

But even beyond thinking about the initial experience, where this direction leads is a "disassociation" of the content model from the API, and that is purely antithetical to actual core values of "API-First." It requires going back to configure multiple times, such as when a reference field is added to a whitelisted entity type that links to an entity type that's not yet whitelisted. &^%&!@ None of our competitors in this space have this extra DX hurdle; as soon as you add content types and fields in those platforms, they're instantly available to the API. Consequently, Drupal would still be API-Second, not API-First.

We went back and forth for a few hours trying to determine how we could balance these, given a 2 week timeline before beta, and in the end, didn't come up successful. It's possible there's a pathway, but everyone is tired and stressed (and some of them are having babies ;)) so this is kinda the opposite of an optimal place to be making big decisions. :\

And so...

One "middle ground" idea floated by @e0ipso was committing this module to core in 8.7 as experimental, rather than stable. There's no data and no configuration, so no huge concern with any upgrade path. This would allow us to keep the existing (user-validated) behaviour, spend the next couple of weeks (+meeting at DrupalCon) really digging into the concerns and coming up with proposals and having time to actually think of the ramifications of them, line up resources to help address underlying entity access concerns and others that have nothing to do with this module apart from being exposed by it.

---

Sorry, that was a LOT. :\ Do folks have any thoughts on this direction?

Moving security concern #4 off to #3038716: Figure out how to solve the curious case of /jsonapi, so we can focus on the other 3.

I like #114 III and know that it would help mitigate problems long term (many decoupled sites are already ready-only in practice), and possible short term if a site could do without API writes until core was updated.

#114 II is good because it's a new explicit API we can point developers to, but if "api_consumable" defaults to TRUE for content entities, it provides no immediate protection for sites.

No strong feelings about #114 I - I do wonder how a decoupled application would verify that a site is properly configured, would it check to see if these API-dependent modules are enabled? I feel like the JS would need to be resilient to CRUD errors anyway, so users would know pretty quick that something was mis-configured.

The plan in #115 sounds good to me but I'm not a core maintainer and it sounds like more of a "that-realm" decision to make.

Thanks for all the hard work @webchick/all!

Thanks @webchick for writing this!

I'm specially impressed with your summary of my 97 minutes rambling into these beautiful words:

where this direction leads is a "disassociation" of the content model from the API, and that is purely antithetical to actual core values of "API-First."

👏

I'm increasingly excited about #114/III and curious about everyone else's thoughts. I already see that @samuel.mortenson's reaction was positive!

Yes, thanks @webchick and everyone else for your efforts to try to find a resolution to this thorny issue!

I don't really understand what #114.I hopes to solve. I thought the consumers of this API aren't likely to be Drupal modules at all. Say I've got my nice iOS app that speaks JSON:API to talk to my newspaper site -- where does that iOS app's .info.yml file live? ;) Who exactly is advertising their API dependencies like this and why? Am I expected to write and install a "custom module" solely to provide my ios_news_app.info.yml file to turn on the JSON:API endpoints I need? Isn't that vastly worse for the #113.D crowd? Furthermore, If Drupal Admin UI++ is going to "turn on everything", anyway, what does this really help? Seems like a lot of effort for little or no benefit. Maybe (probably) I misunderstand.

#114.II is exactly #98, right? Sounds like there's some general agreement that would be useful, but @samuel.mortenson already pointed out that would have been insufficient to protect or mitigate against many of the recent API-related sec holes. Is the difference that we don't do it at the ContentEntityType level, but do it separately for every content entity in core, so that contrib content entities don't get exposed by default until they update their annotations? Again, doesn't that further make life suck for the #113.D crowd?

#114.III sounds real good. ;) However, what's the difference between the "Turn off API access" radio choice vs. uninstalling jsonapi at /admin/modules/uninstall ? If I'm turning off API access entirely, wouldn't it be safer to completely uninstall the thing (especially since there's no config or content, anyway)? Or is the idea that if you select "No API", it uninstalls jsonapi (and rest?) for you? That seems complicated. E.g. when you turn it back on, do we remember which APIs you used to have on and restore those? Maybe instead of a 3rd choice, there should just be a link to /admin/modules/uninstall?jsonapi=1&rest=1 (and teach that form to default checkboxes based on the query args) and some help text saying: "To completely turn off API access, @link_to_uninstall" (more or less)?

I do see benefit to an "API read-only" mode vs. #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released. Back to my newspaper site, I might know that the only API I want to provide is strictly for read-only clients that are pulling down the latest news, so I'd never want to give the API write access to anything. But the editors still need to login to the backend and do their updates, so #2957061 wouldn't help me.

Thanks,
-Derek

For #114.3 I think we should extend this to more than just API but also to everything else - add shoot for something like #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released because a read only mode where the site is not read only makes the security messaging harder. Once implement that we could open a follow-up to discuss if we want to make the read-only mode more granular and have the ability to disable API write only.

@alexpott I think the negative of making it not API-specific is that, if it was API-specific, content editors could continue to work on content while keeping their read-only headless site running. In that way it isn't even necessarily a fail-safe, I think a lot of sites could run a read-only API regardless of security announcements. Also, non API-specific security vulnerabilities that bypass create/update/delete access are rare (I can't remember the last one).

+1 on #120 to make this non-API specific.

@samuel.mortenson remember this is to be used in case of panic:

A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released

I don't think we should NOT cover other cases and make this API specific so content editors can continue un-disturbed during a case of panic. Wouldn't security be more important during that critical time than allowing to carry-on with operations as usual?

@e0ipso Ideally, yes. :-)

Here's some more detail on #114 in the hopes of addressing #115, #117, and #119.

1. Introduce an api_consumable entity type annotation. Similar to #98, except defaulted to FALSE in all cases per #99 and #114.II. The idea is that only the maintainer of the module that defines the entity type knows if that entity type follows the required security best practices (e.g., around access control and validation constraints) needed to set it to TRUE.
2. Since core does (hopefully) follow security best practices for entity access control, validation, etc., we annotate almost all core entity types as api_consumable=TRUE, including most config entity types. Maybe there's a few we leave out, like the ones that are already internal=TRUE or other special cases that we might want to discuss further like Contact messages and Aggregator feed items.
3. All that api_consumable means is that the entity type is to the best of our knowledge sufficiently secured for API usage and that it's potentially legitimate to expose to HTTP APIs. It does not by itself expose the entity type to JSON:API without some additional site builder action, as described in the following points.
4. We introduce both an api_dependencies key in *.info.yml files (for both modules and themes) and a hook_api_dependencies() as described in #114.I. Perhaps we introduce a Drupal\Core\Http\ApiDependencyManager class/service with a getEntityTypeDependencies() method that could return all the entity types so identified (e.g., combine the info.yml and the hook results).
5. For extra DX, maybe we also add a EntityTypeInterface::isApiDependency() method that could just be a thin wrapper around ApiDependencyManager::getEntityTypeDependencies().
6. We change JSON:API from its current behavior of exposing all entity types whose isInternal() is false to exposing all entity types whose isApiConsumable() and isApiDependency() are both true. In other words, all entity types that as far as we know, are both secure enough and used by something.
7. TBD for what kinds of errors/warnings we want to provide in the cases where isApiDependency() is true but isApiConsumable() is false. Per above, at a minimum, we don't expose those to JSON:API, but TBD as to how we want to report those in hook_requirements() or wherever.

The above handles the cases where there exists a module or theme that can identify what its API consumers rely on. For example, #113 A-C. To support #113.D, I propose that we also add two new Experimental modules:

• api_all_content: Implements hook_api_dependencies() and returns all isApiConsumable() content entity types and all isApiConsumable() config entity types that are referenced by those content entity types (e.g., Vocabulary, because it's referenced by Term).
• api_all_config: Implements hook_api_dependencies() and returns all isApiConsumable() config entity types.

Unlike #115, this would allow JSON:API itself to be committed as stable. Only these two tiny modules would need to be Experimental. These Experimental modules would allow people to quickly get started with writing their JS apps without needing to first figure out how to create a module with an info.yml file. Because these are flagged as Experimental, people are suitably warned about "use at your own risk". For the people who eventually want to remove that warning, we can provide documentation on how to add the entity types their apps rely on to a custom module's or theme's .info.yml file, including how to create a custom module or theme if they're not already using one.

Additionally, contrib could provide a module that lets you add API dependencies via a configuration form (e.g., implements hook_api_dependencies() as a wrapper around configuration that it manages). But per #102, we leave such configuration out of core, for now.

Does this help clarify? And does it meet all the goals of security and zero-config (semi-cheating in that enabling an experimental module is a type of config) and the different use cases we care about?

@effulgentsia I don't see anything about the read only mode in your comment - is there a reason for that?

Oh, sorry, yes in addition to #124, I also do like #114.III's suggestion of a 3-way "Full API access / Read-only API access / Turn off API access" toggle in the UI somewhere: perhaps in a new "API Access" page under Configuration -> Web Services? And then we have a route filter that obeys that choice, for all non-HTML requests (it can't be any earlier than a route filter, because we're not done negotiating the format until RequestFormatRouteFilter runs).

I think a format-agnostic read-only mode #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released is a good idea as well. But even then, the API access toggle would still need at least the other two choices (On/Off), since per #113.3, there's the possibility of an SA where non-HTML GET requests are vulnerable (without HTML GET requests being similarly vulnerable). And per the end of #119, we might want the "Read-only API access" option, even if we also do #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released.

That said, I don't have strong feelings on how we want to prioritize these modes in terms of what we do first, and where in the UI to put them. As long as some combination of those non-entity-type-specific modes, along with the non-method-specific declarations in #124, combine to give us something we feel good about in terms of security hardening.

Thanks everyone for digging into possible solutions.

Regarding the idea of committing JSON:API as experimental: Generally security or data integrity concerns are blockers even to commit a module as alpha. The idea from #124 of committing a stable JSON:API that is hardened by default and then an (alpha) experimental module to enable our desired behavior is interesting, but I worry about the UX. (I think it should be at most one additional module.) Also concerned about the DX as the proposal includes a lot of complexity in new APIs.

Just as a reminder, alpha-stability experimental modules are not included in tagged releases. To be in a tagged release, a module needs to comply with the beta stability requirements. (We had to stop relying on the "use at your own risk" for experimental modules, because no matter how we warned site owners, people's sites still broke and it resulted in Drupal 8 being perceived as unstable.)

What about a hardened-by-default, beta core module, and for this release cycle JSON:API in contrib provides the preferred API-first UX/DX while we work more on the core APIs for security hardening? JSON:API's development has been very successful in contrib thus far. I think it's up to module maintainers whether we want to maintain the base functionality in core as experimental or in contrib for 8.7's alpha and possibly another minor.

Phew, tricky...

Okay, so sounds like experimental is not a way to buy us more time to sort out the DX, so if this is to go in to 8.7 it needs to address security concerns.

Would a combination of #98 (turning this on by default only for content entities, thus drastically reducing the overall attack surface), combined with #114.D (a UI toggle for switching APIs between full / read-only / off in case of mitigation emergency) possibly be a way forward? This is the only combination I can see here (though it is late, so I might've missed something) that balances the security concerns, without severely hampering the UX/DX at the same time, and which seems achievable in ~2 weeks.

So read-only mode per #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released seems useful generally and also for this case.

I don't think JSON:API needs a UI toggle for off, because as dww points out in #119 we already have a toggle for JSON:API being off - uninstalling the module.

So for a hypothetical security issue that only affects POST, toggle read-only mode. This would then be a single checkbox somewhere. If it's a general read-only mode it could go in the same place as the maintenance page? Or I guess it could even be 'read only' module.

For a hypothetical security issue that affects GET - we can tell people to uninstall JSON:API as mitigation.

I have not yet given up into finding a compromise on this, but I am not willing to put JSON:API in core "at all cost". That'd be very bad for our API-First aspirations.

What about a hardened-by-default, beta core module, and for this release cycle JSON:API in contrib provides the preferred API-first UX/DX while we work more on the core APIs for security hardening?

Sadly I think that an approach like this will be the last nail on the coffin for any API-First aspirations in Drupal 8. My personal opinion is that the API-First landscape (which the same JSON:API maintainers coordinate) is much healthier with JSON:API in contrib than JSON:API in core under those circumstances.

If Drupal 8 is not ready to be API-First, because many modules do not implement security best practices and core is not ready to enforce them, then that should be the conclusion of this issue. In that case the resolution of this issue is that JSON:API cannot be included into core. We may need to work on how to transmit that message in a positive way to the public, when we have to explain why JSON:API will not be included into core. This will make https://events.drupal.org/seattle2019/sessions/why-will-json-api-go-core "fun".

Next steps for the API-First initiative may be to provide enough feedback and information to the appropriate core sub-system maintainers to help make Drupal ready for an API-First move.

Would a combination of #98 (turning this on by default only for content entities, thus drastically reducing the overall attack surface), combined with #114.D (a UI toggle for switching APIs between full / read-only / off in case of mitigation emergency) possibly be a way forward? This is the only combination I can see here (though it is late, so I might've missed something) that balances the security concerns, without severely hampering the UX/DX at the same time, and which seems achievable in ~2 weeks.

This would work for me:

1. Enable by default all resource types (combination of entity type + bundle) for content entities and all the related config entities. Note that JSON:API today only allows read-only on configuration entities.
2. A UI toggle for switching APIs between full / read-only / off in case of mitigation emergency.

I think that a read-only mode and the eventual recommendation to turn off JSON:API should be enough of a security mitigation for the (hypothetical security announcement, for those sites that can't apply a patch). Is there any security concern not covered by this approach? @catch is in line with your comment in #129?

What Wim is about to propose below would also work for me.

I'm hearing a lot of +1s for a read-only mode that doesn't apply just to HTTP APIs, but to everything. So I'm fine with doing #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released.

However … making the entire site read-only is not what addresses the security team's concern/desire for a minimal HTTP API surface by default: it'd be making "too much" read-only.

Proposal 1: GET-only by default

As far as I can tell the security team wants a minimal HTTP API surface by default. Would the security team be okay with shipping GET-only out of the box, where you have to change from read-only mode to read-and-write mode? Because @samuel.mortenson is right in #117 when he says many decoupled sites are already ready-only in practice — that is a very common use case, and right now probably the most common use case.

It would still mean that Drupal isn't actually API-First, but at least it'll be API-Read-First. That's still a big step forward. Then as we mature the Entity/Field API further in future core minors (wrt validation & access control), we could enable all HTTP methods and become truly API-First.

Security team, would that be a reasonable compromise?

Proposal 2: GET-only by default, except for entity types with api_consumable=true

I believe we have sufficient confidence in Node, Comment and Taxonomy validation constraints and access control logic to trust those being exposed for writes by default. Can we allow writes on those by default? (This is purely additive to proposal 1.)
(This is an aspect from #98's proposal.)

@e0ipso yes for me that would be enough mitigation and minimal extra code.

Wim's proposal in #131 also works for me.

As far as #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released or #113.III... I'm totally fine with whichever one gets gets the job done from the security POV.

Because it was asked, the difference between #113.III and 'uninstall JSON:API module' would be a) this would be a level "above" JSON:API and could be made generic to all API-providing modules" (would've helped with the bug that also affected REST and RESTWS and and and...) and b) while JSON:API itself does not expose any data/configuration, and so uninstalling it is not destructive, this is not necessarily a safe assumption for all API-providing modules. So this toggle would effectively allow for a "disable" vs. "uninstall" in this case.

#131 also sounds fine to me, from a DX/UX POV. (Slight preference to #2, at least for core, but the risk is code generators / copy/pasters just pull that flag in without really thinking about it, and it doesn't provide the extra security that we hope.)

Going to take another crack at updating the issue summary, this time drastically cutting it down to cover the overview in #113 and the solutions proposed vs. the ones we've ruled out.

Ok, done. I encourage others who've been involved in this longer than me to further flesh out the "Summary of positions" part if you feel anything major has been missed. I tried to make it an actual summary vs. a running log of everything that has ever happened, unlike the last version. :)

Oops, I had misunderstood the proposed resolution, so updated that bit.

I'm sure this is not perfect, but it's at least updated enough to get rid of that tag. I've asked Wim to check it over, too.

Further refinement, based on some feedback from @Wim Leers and @xjm.

Re #130 and #131, thanks for those great ideas! How about this as a way to combine them?

We do all of the following:

1. Enable GET by default for all content entity types that are !isInternal() and their related config entity types that are !isInternal().
2. Have some mechanism by which config entity types that are not referenced by any content entity types can also be exposed. Maybe just a simple hook? I think writing an app that reads Drupal's configuration is sufficiently advanced that it's acceptable to require a site that wants to support such an app to have to add a module with a hook implementation that identifies the desired config entity types to expose.
3. Have a UI toggle for switching APIs (just APIs, not HTML) between full / read-only / off. Defaults to read-only.
4. Introduce a api_writable entity type annotation. Default it to FALSE, so contrib/custom entity types have to explicitly opt-in, at which point it's their responsibility to ensure proper access control, validation constraints, etc. We set it to TRUE for the core entity types that we feel sufficiently confident about.
5. For sites that explicitly toggle their API setting to "full" in #3 above, we add POST/PATCH/DELETE for all entity types that are annotated as api_writable=TRUE.

So, I wrote #139, because I didn't understand that #131.1 included two parts:

• Enable GET for all (non-internal) entity types (both content and config, same as current JSON:API behavior) by default.
• Have some toggle in the core UI to enable POST/PATCH/DELETE for all (non-internal) content entity types (same as current JSON:API behavior, except currently it does it without this toggle).

If there's agreement on doing both of those, then I'm +1.

#139 proposes to add additional API surface reductions, in case the current behavior of enabling GET access to all (non-internal) config entity types, or the current behavior of enabling POST/PATCH/DELETE for all (non-internal) content entity types (if you require that access for at least one of them), is still considered too risky.

I asked the security team about #131 proposal 1 (GET-only by default, but a configuration change can enable the current behavior with write access), and the nine team members who responded were all in favor of that proposal. (Didn't hear yet from @greggles or @samuel.mortenson who've given ongoing feedback here on the issue.)

The proposal hardens Drupal against most more serious attacks by default, and can also be used to switch an existing site back into read-only mode in case of emergency. There's still a decent risk of information disclosure, but such vulnerabilities are generally less serious. (While SA-CORE-2019-003 was exploitable over GET for core REST, that is definitely the exception rather than the rule.)

Marking RTBC since the JSON:API maintainers, the release managers, and at least a substantial portion of the security team can live with this compromise. (Not marking "Fixed" just yet so that others have a bit more time to respond, but note that unless we hear otherwise we'll go ahead and start implementing this proposal soon, probably first thing tomorrow, EU-time).

Note that the scope of the proposed resolution is significantly smaller than #2957061: A read only mode so that the security team could potentially recommend a course of action for sites that cannot be updated at the very moment a security fix is released, so we will probably want a new implementation issue for the "GET-only by default" proposal. The implementation issue can discuss implementation details, specifics of the UI, how to warn users of the implications of enabling write access, etc. It should also receive usability review.

After a lot of back-and-forth, the issue + patch that implement #141: #3039568: Add a read-only mode to JSON:API.

Ensuring everyone is credited 🙂 Thanks everyone! 🙏

@xjm sent me the nicknames of the Security Team members who responded to her poll, crediting them too :)

d.o does weird things apparently. Trying again with the people who weren't actually credited.

@Wim Leers added #3039687: Add an API read-only mode to Drupal core as a follow-up issue to explore in Drupal 8.8.

I added #3040025: [meta] Decide on and implement additional security hardenings for JSON:API and other HTTP APIs to which I'll also add some child issues with things like #139, and whatever else I find in re-reading this whole thread. Everyone's welcome to add child issues to there too. See you all there in the coming months!

#3040025: [meta] Decide on and implement additional security hardenings for JSON:API and other HTTP APIs now has 10 child issues. Can anyone here think of any other security hardening ideas discussed in this issue that aren't reflected in one of those?

Alright, so: #3039568: Add a read-only mode to JSON:API has been committed to adopt the MVP security hardening proposal.

The next steps are:

• Identifying additional hardenings we should implement (#3040025: [meta] Decide on and implement additional security hardenings for JSON:API and other HTTP APIs and children)
• Reviewing the JSON:API security considerations handbook page (tracking issue in #3039959: Documentation review of the "Security Considerations" page).

I read over the issue again and tried to ensure that most suggestions were captured in the followup issues. Since the MVP has implemented and we have issues filed for next steps, I think we can mark this discussion fixed!

Thanks everyone for your abundant contributions to this discussion!