\Drupal\rest\Plugin\views\row\DataFieldRow::render should take into account the 'exclude' flag

First stab at a fix. Setting to needs review to kick the testbot into gear, but needs work.

Adding a hidden field to one of the views used in \Drupal\rest\Tests\Views\StyleSerializerTest sort of illustrates the problem but would require a lot of refactoring of the existing test to take the hidden field into account because of the way those tests are build up.

So I'll take a look at adding a new view for this with a hidden field and running some tests on that.

Test and fix.

+++ b/core/modules/rest/src/Plugin/views/row/DataFieldRow.php
@@ -88,6 +88,10 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        // Don't show the field if it has been excluded.
+        if (!empty($field['exclude'])) {
+          continue;
+        }

@@ -138,6 +142,10 @@ public function render($row) {
+      // Don't render anything if this field is excluded.
+      if (!empty($field->options['exclude'])) {
+        continue;
+      }

There are two if's and only one test - are we missing test coverage here? I think we're only testing the second if.

Good point, added test for the row options.

Thinking about this bug fix some more - we're at the unfortunate point where doing this could potentially break people's sites - something could be relying on the excluded field not being excluded. Therefore I think we should fix this in 8.1.x and not do in a patch release and also have an upgrade path. The upgrade path should find all rest views with excluded fields and makes them not excluded and inform the user.

That makes sense to me - preserve the existing behaviour on existing sites, fix it for anything new.

@dawehner the difference is that the view is not broken and it is perfectly possible to use the output in a mobile app or whatever.

You could also argue that this bug might be displaying sensitive data that a site builder is expecting to be hidden, and it's a possible security risk to not fix it.

For what it's worth, I'm with @dawehner on this, it's a bug and

relying on the excluded field not being excluded

doesn't sound like the sort of thing we'd need to consider as valid configuration.

Sorry, I think I agree with Daniel here. Saying people are relying on broken behaviour could be said about almost any bug out there.

I'm not saying that we shouldn't fix the bug I'm saying that given we are changing the output of something that is meant for creating public APIs. We should do so in the most conservative way possible. All that means is changing existing REST views to not exclude fields in an update and tell the user we've done that.

Saying people are relying on broken behaviour could be said about almost any bug out there.

There's a big difference between broken behaviour that affects output vs. one that doesn't. In this particular case, how does the person interacting with the Views output know that it's broken?

The idea with moving this to 8.1.x is that we're fine with breaking reliance on broken behaviour in minor releases, but we try not to do that in patch releases.

I agree with #17 and #18. We can commit this to 8.1.x as soon as we have the upgrade path RTBC. Thanks!

+1 for fixing in 8.0.x

Trust the subsystem maintainers, please.

When a site builder goes into the Views UI and chooses to exclude a field from display, that is a very purposeful choice.
Nowhere do we default to excluding. They would have to explicitly click that, and have clear expectations for the result.

Not fixing this in 8.0.x is irresponsible.

Further discussion between the D8 Views maintainers and the original Views author:

11:18 merlinofchaos: Oh interesting. Just reading the description and not getting down into the details, I can see why you might NOT exclude them in this case.
11:18 dawehner: merlinofchaos: from a BC point of view?
11:19 merlinofchaos: No, from a REST is everything point of view.
11:19 dawehner: merlinofchaos: or from a REST point of view?
11:19 merlinofchaos: hasn't given it enough thought.
11:19 damiankloip: Just seems like if you applied that thinking to this bug, you could do that for literally any bug. Someone could be relying on broken behaviour somewhere
11:19 dawehner: merlinofchaos: well, then you would not use a field based view I would argue
11:19 timplunkett: if you want everything, use the entity style plugin
11:19 damiankloip: indeed
11:19 timplunkett: if you want the field based approach, exclude should allow you to hide stuff
11:19 dawehner: while you are all here: what do you think of https://www.drupal.org/node/2290127 especially in the context of BC issues
11:19 Druplicon: https://www.drupal.org/node/2290127 => pre_render not consistently passed result by reference #2290127: pre_render not consistently passed result by reference => 8 comments, 1 IRC mention
11:20 merlinofchaos: Is REST its own style plugin or is it a takeover of an existing style?
11:20 damiankloip: it's own thing
11:20 merlinofchaos: Ok, in that case, exclude should work.
11:20 damiankloip: its
11:20 merlinofchaos: And the argument that there is no BC to worry about with this, it's a clear bug if the checkbox doesn't work.
11:21 merlinofchaos: ...makes sense tome.
11:21 timplunkett: merlinofchaos: can i quote you on issue? :)
11:21 damiankloip: dawehner this is worrying for the outcome of the other issue to fix the raw values
11:22 dawehner: merlinofchaos: If you find this checkbox, you had a vision in mind
11:22 merlinofchaos: timplunkett: Yes, please feel free.
11:23 merlinofchaos: Really, I have trouble with the argument that someone checked the exclude box accidentally and someone is now using a value that was accidentally excluded. Is that really going to break people's code?
11:23 damiankloip: merlinofchaos++
11:23 merlinofchaos: At the cost of people not having the control over their output that we SAY they have.
11:24 timplunkett: merlinofchaos++
11:24 dawehner: merlinofchaos: for me its all about security and trusting the site builders opinion
11:24 dawehner: merlinofchaos++
11:25 merlinofchaos: Yeah, there's definitely a security implication there, it would be very easy to have data that isn't intended.

This is also a similar ilk as #2574077: REST Export display cannot show any raw output for fields using the Field field handler. Which I think we should fix asap too.

Here is my reasoning for saying that we should remove the exclude option from existing REST views.

REST is for creating public APIs. Changing site's public APIs from underneath them should be avoided. If we do change the behaviour we should communicate this in a release note / change record or even update.php. However, given that REST is used to create public APIs that are consumed by applications that the site owner and developers might not even know about this information is highly unlikely to reach the correct ears. Therefore I proposed changing the views and informing the site owner / site developers as part of update.php. This gives the site the most options - they can decide they don't care about public consumers and exclude the field, or they can canvas their users, or they can just decide it is not an issue. Maybe the best solution would be inform the user prior to update and let them choose how they want they specific site to handle this as the answer is probably going to depend on what is actually disclosed and how public the REST is.

So for me both #26 and the information disclosure potential are about 50/50.

I think it's very unlikely that there'll be actual information disclosure (usually when exclude is used, it's because it's being used in a rewrite or something and gets rendered one way or the other anyway).

But it's also very unlikely that that particular field is going to be used directly by a web service consumer - at least while the number of 8.x sites is very low. The more sites start providing REST APIs the more chance one gets impacted (although still low).

So we are balancing two unlikely scenarios against each other, depending on which type of breakage you prioritise, both are valid.

An update with a choice in general seems like a good approach for this, although I wouldn't hold this issue up on that - it could happen in 8.1.x either way with an update.php and a release notes mention.

We should not make assumptions over users will, vs. some freaking arbitrary academic situation.

I don't think it's an arbitrary academic situation - if you're providing REST views, and the excluded fields are included, it's not academic that someone might end up relying on those fields - the consumer of the REST API has no way to know what you intended (unless you have documentation for every individual field somewhere).

Overall I'm coming 'round to treating this as a straight bug fix though, and if there is breakage then we just say we erred on the side of caution against information disclosure.

Well I think it's unlikely, in the same way that I think an actual information disclosure issue (as opposed to just extra unwanted data which you could probably find elsewhere on a site) is unlikely - which is why for me I'd be OK with the patch as is for 8.0.x with a release notes mention. We're not changing Drupal's API, we're potentially changing the API that a site provides, but that also would have happened with the raw output issue.

Straight reroll.

And want to thank everybody for their detailed arguments, really helped me see what the pros and cons were here.

Committed 1673fa6 and pushed to 8.0.x and 8.1.x. Thanks!

It is hard to know who to give review credit on this issue so I'm giving to everyone as I know that everyone spent some time discussing this issue.

diff --git a/core/modules/rest/src/Tests/Views/StyleSerializerTest.php b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
index c26292a..b590504 100644
--- a/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -471,7 +471,7 @@ public function testFieldRawOutput() {
       $this->assertIdentical($values['name'], $view->result[$index]->views_test_data_name, 'Expected raw name value found.');
     }
 
-    // Test result with an excluded field
+    // Test result with an excluded field.
     $view->setDisplay('rest_export_1');
     $view->displayHandlers->get('rest_export_1')->overrideOption('fields', [
       'name' => [

Added missing fullstop on commit.