Remove SafeMarkup::set() from RestExport and replace it with ViewsRenderPipelineSafeString

@alexpott: so you said

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -311,7 +312,7 @@ public function execute() {
     $build['#markup'] = $this->renderer->executeInRenderContext(new RenderContext(), function() {
-      return $this->view->style_plugin->render();
+      return ViewsRenderPipelineSafeString::create($this->view->style_plugin->render());
     });

So I tried to understand how this works. But executeInRenderContext() documents that it returns whatever was passed in, and neither the style plugin renderer neither the ViewsRenderPipelineSafeString seem to explain what they would return/how.

So its hard to ensure based on my review how this is going to be safe :) I mean the class name is promising and all, but:

StylePluginBase::render() says

  /**
   * Render the display in this style.
   */
  public function render() {
}

so that will return something... Then ViewsRenderPipelineSafeString wraps it.

Then executeInRenderContext() returns "The callable's return value.". So it will just be a string after all, and that's it. How does that ensure anything about safeness?

So I tried to understand how this works. But executeInRenderContext() documents that it returns whatever was passed in, and neither the style plugin renderer neither the ViewsRenderPipelineSafeString seem to explain what they would return/how.

You give RendererInterface::executeInRenderContext() a callable. The return value of that callable is returned:

   * @return mixed
   *   The callable's return value.

EDIT: I saw too late that you already figured this out, in the bottom of your comment — oops.

StylePluginBase::render() says […]

Yeah, the docs aren't the best, but it always returns a string (a plain string, not a SafeStringInterface string).

So, to then get to the key part of your question: why can we mark this string returned by Views' style plugin rendering as safe?. The simple part of the answer is: "because REST export only renders to non-HTML formats, so we don't want additional filtering or escaping to happen just because Views happens to use part of the regular rendering pipeline — which is specifically designed for HTML — otherwise we'd end up sending HTML-escaped JSON, HAL, or whatever other format".
BUT! I think you're actually right that the changes in this patch are wrong. Because the removed code

-    elseif ($this->view->getRequest()->getFormat($this->view->element['#content_type']) !== 'html') {

was only marking the markup as safe for non-HTML formats. This means that in HEAD, in case of a non-HTML format, #markup gets the default safety strategy treatment: admin XSS tag filtering. But because this patch now effectively is always marking the rendered markup as safe… that means that filtering no longer happens.

This is a very complex intersection of different problems:

• Views having its own render pipeline that is called from within the regular render pipeline, and Views itself calling the regular render pipeline again for some things
• This particular Views style plugin not outputting HTML, but anything but HTML
• Yet there still apparently somehow being a case where RestExport could output HTML?!

There are only two possible explanations AFAICT:

1. Views is missing test coverage where RestExport is being used to output HTML.
2. Views is pretending like any style plugin could output HTML, even though that can never happen for RestExport, which means the quoted if-statement is actually wrong.

The patch is simpler than the interdiff :)

Follow-up filed: #2554819: RestExport detects if the content type is HTML, but it can never *be* HTML.

Yay, now I understand it better too :) Looks good.

Hmm, so this issue is now not doing what it says in the title. So we should at least re-scope it. Also I'd like to make sure @alexpott is on board with the current patch.

There are only two possible explanations AFAICT:

Views is missing test coverage where RestExport is being used to output HTML.
Views is pretending like any style plugin could output HTML, even though that can never happen for RestExport, which means the quoted if-statement is actually wrong.

The last time I looked into this (admittedly, several months ago) I believe I found that the rest data is output as HTML when it is used in the View preview, which is why it's necessary to support HTML.

Sorry, I misread the updated diff. Disregard #9. Still would like @alexpott's feedback though.

Edit: This was regarding:

There are only two possible explanations AFAICT:

Views is missing test coverage where RestExport is being used to output HTML.
Views is pretending like any style plugin could output HTML, even though that can never happen for RestExport, which means the quoted if-statement is actually wrong.

This was originally added in #2273925: Ensure #markup is XSS escaped in Renderer::doRender(). In #163 of that issue, @larowlan proposed returning a Response object directly inline there, but @dawehner pointed out there would be a conflict with #2477157: rest_export Views display plugin does not set necessary cache metadata.

@effulgentsia added the conditional and the long comment in #2273925-215: Ensure #markup is XSS escaped in Renderer::doRender().

I'd also like @dawehner to take a look here.

Edit: crossposted with @alexpott.

#13: this does not change the existing behavior at all. It's identical.

I agree this is all very confusing and weird. But fixing that is a different problem. For that, I opened #2554819: RestExport detects if the content type is HTML, but it can never *be* HTML in #7.

Views is pretending like any style plugin could output HTML, even though that can never happen for RestExport, which means the quoted if-statement is actually wrong.

So yeah HTML is just another representation of output.

I'm wondering we should set the entire output as ViewsRenderPipelineSafe, not just the one inside the !== 'html' case? I mean even for html, its semantically that we know its safe, either it
got safe already via the normal render workflow, check plained for the preview.

its semantically that we know its safe

How do we know this? StylePluginBase::render() returns a plain string, and style plugins can do any sort of rendering they want (even pure string concatenation).

Alright, sounds to me like that would need more discussion. So let's go ahead with this patch as it is, and then sort the rest of this out in #2554819: RestExport detects if the content type is HTML, but it can never *be* HTML.

Thanks for the reviews and the manual testing!

How do we know this? StylePluginBase::render() returns a plain string, and style plugins can do any sort of rendering they want (even pure string concatenation).

It can be a pure string, or a render array.

Exactly, so how do we know that a pure string is safe markup?

@Wim Leers SafeMarkup::isSafe($string) still works at the moment. Does that answer the question?

@joelpittet: Maybe :) I don't think Views style plugins are actually marking the output as safe, but I could very well be wrong. E.g. the \Drupal\rest\Plugin\views\style\Serializer::render() Views style plugin just returns:

return $this->serializer->serialize($rows, $content_type);

So I don't see how that ends up in SafeMarkup's list of safe strings.

So I don't see how that ends up in SafeMarkup's list of safe strings.

This assumption is wrong. Its not end up there, but rather its marked as safe by creating the new object in this issue, see
$string instanceOf SafeStringInterface in \Drupal\Component\Utility\SafeMarkup::isSafe