Problem/Motivation

The editor XSS filter editor_filter_xss() uses a NULL instead of a valid \Drupal\filter\FilterFormatInterface and pops-up a fatal error when:

  • A user with access to more than one format, each one having editor, creates a node with body and sets the format (let's say f1).
  • The format f1 is deleted (disabled)
  • User tries to edit the node.

He will receive:

Fatal error: Call to a member function id() on null in core/modules/editor/editor.module on line 274

Proposed resolution

Check the value of $format for NULL and make editor_filter_xss() return FALSE in that case because, anyway, the format being disabled cannot display the text with editor and in this case we don't need XSS filtering.

When a node uses a disabled format, the format dropdown will be empty on node edit and the content editor will be forced to choose a new format.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug it breaks the content edit form.
Issue priority Critical because the user is locked out of the content edit form.
Prioritized changes The main goal of this issue is fixing a bug.
Disruption No disruption.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mark LaCroix created an issue. See original summary.

Mark LaCroix’s picture

Issue summary: View changes
Mark LaCroix’s picture

Issue summary: View changes
Mark LaCroix’s picture

Issue summary: View changes
claudiu.cristea’s picture

Title: Disabling text formats causing "unexpected error" when attempting to edit content. » fatal error when editing content after disabling format with editor
Issue summary: View changes
Status: Active » Needs review
FileSize
3.69 KB
5.07 KB

@Mark LaCroix, good catch. Let's see if this patch is fixing the issue or, maybe, we have also another bug here.

I uploaded also a "test only" patch to reveal the bug.

claudiu.cristea’s picture

Title: fatal error when editing content after disabling format with editor » Fatal error when editing content after disabling format with editor
claudiu.cristea’s picture

Component: filter.module » editor.module
Issue tags: +Needs beta evaluation

The last submitted patch, 5: 2555973-5-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2555973-5.patch, failed testing.

claudiu.cristea’s picture

Hm. The failure from #9 seems something randomly. That test passes locally.

Status: Needs work » Needs review

claudiu.cristea queued 5: 2555973-5.patch for re-testing.

Mark LaCroix’s picture

Thanks for deducing and describing the actual error!

I applied the patch to my test installation, and it allowed me to edit a node after the format it used was disabled.

However, after I disabled the format (before I edited the node) the field content was still rendered and visible on the page.

Also, I still can't access /admin/config/content/formats until I manually remove the disabled format from the database (is this a separate issue?)

claudiu.cristea’s picture

@Mark LaCroix, the other one seems critical (the display of the value in node view). I opened a new issue: #2556617: [regression] Disabled format still filters and displays the value. Thank you very much.

I cannot reproduce the one with accessing /admin/config/content/formats.

claudiu.cristea’s picture

Priority: Major » Critical
Issue summary: View changes

This seems to me also Critical while the user cannot access anymore the content edit form.

Added also the Beta Evaluation.

Anonymous’s picture

Version: 8.0.0-beta14 » 8.0.x-dev
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs beta evaluation

I managed to reproduce this on HEAD.

Added the "prioritized" part of the beta evaluation. I left the priority critical, since I currently don't seem to be able to recover the text using just the UI. The text is still in the database, but I can't re-enable the custom format. It's not a data loss per se, but at least it's very annoying and non-technical users wont be able to recover the text. Once #2502637: Disabled text formats can't be seen in the GUI gets in there will be a work-around which probably makes this major instead of critical.

Also, I still can't access /admin/config/content/formats until I manually remove the disabled format from the database (is this a separate issue?)

That seems like another issue, but I cannot reproduce it locally.

As for the patch, I manually tested it and it seems to work. Combined with #2502637: Disabled text formats can't be seen in the GUI and #2556617: [regression] Disabled format still filters and displays the value this looks like a decent solution.

So I think this patch is good, and the test seems solid to me. I just found one nit:

+++ b/core/modules/editor/editor.module
@@ -254,8 +254,9 @@ function editor_load($format_id) {
+ *   The text format whose text editor will be used. If that format was
+ *   disabled, NULL will be sent.

This wrapping is not correct.

claudiu.cristea’s picture

Status: Needs work » Needs review

This wrapping is not correct.

@pjonckiere, thank you for review. I checked twice and that wrapping is correct. You cannot break the line just before the comma :)

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Oh yes, you are totally right.

Manual testing revealed that this works as expected, and we have decent coverage to prevent regressions. I think this is good to go.

stefan.r’s picture

Manually tested this with patch and the new behavior makes sense -- we display a plain text field instead and force the user to choose a new format.

claudiu.cristea’s picture

@stefan.r, @pjonckiere, there is also a related Critical here #2556617: [regression] Disabled format still filters and displays the value. That raises also a security problem.

Anonymous’s picture

Issue summary: View changes

Added #18 to the IS.

stefan.r’s picture

Status: Reviewed & tested by the community » Needs review
  1. --- a/core/modules/editor/editor.module
    +++ b/core/modules/editor/editor.module
    

    The "...if that format was disabled, NULL will be sent" part sounds a bit confusing. How about
    "...or NULL if the previously defined text format is now disabled."

  2. +++ b/core/modules/editor/editor.module
    @@ -270,8 +271,8 @@ function editor_load($format_id) {
       // If no text editor is associated with this text format, then we don't need
       // text editor XSS filtering either.
    

    This comment now also applies to our case so either it would need updating or maybe our own return FALSE in case of a disabled text format.

  3.  *   FALSE when no XSS filtering needs to be applied (either because no text
     *   editor is associated with the text format, or because the text editor is
     *   safe from XSS attacks, or because the text format does not use any XSS
     *   protection filters), otherwise the XSS filtered string.
    

    This comment could also be updated to mention disabled text formats.

stefan.r’s picture

Status: Needs review » Needs work
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
2.16 KB

@stefan.r, thank you. Docs fixed.

Mark LaCroix’s picture

I cannot reproduce the one with accessing /admin/config/content/formats.

That seems like another issue, but I cannot reproduce it locally.

Thanks for testing that for me. I'll check my logs and open a separate support request for this one. Sadly, my hosting provider doesn't support PHP 5.5 yet, so I can't confirm if it shows up on a properly configured server (time to upgrade to a VPS, I guess). :-)

dawehner’s picture

+++ b/core/modules/editor/editor.module
@@ -254,8 +254,9 @@ function editor_load($format_id) {
+ * @param \Drupal\filter\FilterFormatInterface|null $format
+ *   The text format whose text editor will be used or NULL if the previously
+ *   defined text format is now disabled.

@@ -263,18 +264,21 @@ function editor_load($format_id) {
+function editor_filter_xss($html, FilterFormatInterface $format = NULL, FilterFormatInterface $original_format = NULL) {
+  $editor = $format ? editor_load($format->id()) : NULL;

I'm a bit confused: The calls basically load the format: $format = FilterFormat::load($element['format']['format']['#value']);

How can it happen that a disabled format returns NULL then? I'm curious whether this is more of a problem with the caller code

claudiu.cristea’s picture

@dawehner, It's true, $format is processed upstream in \Drupal\editor\Element with $format = FilterFormat::load($element['format']['format']['#value']) but in this particular case $element['format']['format']['#value'] == NULL. That's because that value is set in \Drupal\filter\Element\TextFormat here (~215):

    if ((!$format_exists || !$format_allowed) && $user_is_admin) {
      $element['format']['format']['#required'] = TRUE;
      $element['format']['format']['#default_value'] = NULL;
      ...
    elseif (!$user_has_access || !$format_exists) {

where #default_value is set to NULL.

@stefan.r if you're OK with docs update, please re-RTBC.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

I think the docs look OK, the way we implemented the validation here is not very pretty but it looks like fixing this in the caller code isn't easily possible.

+++ b/core/modules/editor/editor.module
@@ -263,18 +264,21 @@ function editor_load($format_id) {
+ *   - No text editor is associated with the text format,
+ *   - The previously defined text format is now disabled,
+ *   - The text editor is safe from XSS,
+ *   - The text format does not use any XSS protection filters.

How do we capitalize/punctuate these kinds of lists elsewhere?

  • webchick committed b1a7e56 on
    Issue #2555973 by claudiu.cristea, Mark LaCroix, stefan.r, pjonckiere,...

  • webchick committed 95008e3 on 8.0.x
    Issue #2555973 follow-up: Fix incorrect comment
    
webchick’s picture

Status: Reviewed & tested by the community » Fixed

I'm not sure, and can't be arsed to check right now. :) If anything, it can be a minor follow-up to clean that up.

This patch looks great. Nice and simple.

Only thing that stood out (not before pushing, unfortunately) is that this comment is incorrect:

+  /**
+   * Tests format deletion.
+   */
+  public function testDisableFormatWithEditor() {

This is testing format disabling, not deletion. Fixed on subsequent commit.

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.