Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 2.16 KB | claudiu.cristea |
#23 | 2555973-23.patch | 6.01 KB | claudiu.cristea |
#5 | 2555973-5-test-only.patch | 3.69 KB | claudiu.cristea |
Comments
Comment #2
Mark LaCroix CreditAttribution: Mark LaCroix commentedComment #3
Mark LaCroix CreditAttribution: Mark LaCroix commentedComment #4
Mark LaCroix CreditAttribution: Mark LaCroix commentedComment #5
claudiu.cristea@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.
Comment #6
claudiu.cristeaComment #7
claudiu.cristeaComment #10
claudiu.cristeaHm. The failure from #9 seems something randomly. That test passes locally.
Comment #12
Mark LaCroix CreditAttribution: Mark LaCroix commentedThanks 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?)
Comment #13
claudiu.cristea@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.
Comment #14
claudiu.cristeaThis seems to me also Critical while the user cannot access anymore the content edit form.
Added also the Beta Evaluation.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedI 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.
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:
This wrapping is not correct.
Comment #16
claudiu.cristea@pjonckiere, thank you for review. I checked twice and that wrapping is correct. You cannot break the line just before the comma :)
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedOh 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.
Comment #18
stefan.r CreditAttribution: stefan.r commentedManually 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.
Comment #19
claudiu.cristea@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.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAdded #18 to the IS.
Comment #21
stefan.r CreditAttribution: stefan.r commentedThe "...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."
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.
This comment could also be updated to mention disabled text formats.
Comment #22
stefan.r CreditAttribution: stefan.r commentedComment #23
claudiu.cristea@stefan.r, thank you. Docs fixed.
Comment #24
Mark LaCroix CreditAttribution: Mark LaCroix commentedThanks 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). :-)
Comment #25
dawehnerI'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
Comment #26
claudiu.cristea@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):where #default_value is set to NULL.
@stefan.r if you're OK with docs update, please re-RTBC.
Comment #27
stefan.r CreditAttribution: stefan.r commentedI 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.
How do we capitalize/punctuate these kinds of lists elsewhere?
Comment #30
webchickI'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:
This is testing format disabling, not deletion. Fixed on subsequent commit.
Committed and pushed to 8.0.x. Thanks!