Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.If you try to embed itself there is an error. That's fine. But not when you embed circularly.
It could be fixed by having a stack on NIDs that it has worked on and making sure that array_unique($stack) == $stack - i.e. that there are no duplicate NIDs in the stack and if so, stop embedding :)
I actually created this by mistake.
Problem is this module is ideal for embedding stuff like galleries, slideshows, etc. by clients. Therefore "try not to create circular embedding" is not a sufficient answer. We need to make sure that they won't break their site like this. Therefore I flagged the issue as critical. Because it can bring the whole site down by seemingly normal change to a node. The above suggestion would be sufficient, because it fails gracefully. It may display a message to logged in user, but most important thing is that it won't break the site.
How to reproduce:
- drush qd
- drush en node_embed -y
- setup node_embed in configuration
- node 1: article
- node 2: article
- node 3: page
- 3: embed NID 1 and NID 2
- 2: embed NID 3 and NID 1
- wild error appears
Fatal error: Maximum function nesting level of '100' reached, aborting! in /Users/tfejfar/sites/drupal/quick-drupal-20141209134744/drupal/modules/filter/filter.module on line 166
Call Stack
# Time Memory Function Location
1 0.0012 635936 {main}( ) ../index.php:0
2 0.2859 30434944 menu_execute_active_handler( ) ../index.php:21
3 0.2860 30435720 call_user_func_array:{/Users/tfejfar/sites/drupal/quick-drupal-20141209134744/drupal/includes/menu.inc:517} ( ) ../menu.inc:517
4 0.2860 30435968 node_page_default( ) ../menu.inc:517
5 0.3002 31274512 node_view_multiple( ) ../node.module:2709
6 0.3050 31540960 node_view( ) ../node.module:2670
7 0.3050 31540960 node_build_content( ) ../node.module:1335
8 0.3052 31541432 field_attach_view( ) ../node.module:1414
9 0.3052 31541952 _field_invoke_default( ) ../field.attach.inc:1228
10 0.3053 31542408 _field_invoke( ) ../field.attach.inc:385
11 0.3053 31545120 field_default_view( ) ../field.attach.inc:209
12 0.3054 31546288 text_field_formatter_view( ) ../field.default.inc:210
13 0.3054 31546936 _text_sanitize( ) ../text.module:278
14 0.3055 31546936 check_markup( ) ../text.module:319
15 0.3079 31594432 node_embed_filter_node_embed_process( ) ../filter.module:799
16 0.3079 31594664 preg_replace_callback ( ) ../node_embed.module:42
17 0.3079 31595488 _node_make_replacements( ) ../node_embed.module:42
18 0.3119 31604528 node_view( ) ../node_embed.module:85
...
90 0.3468 32368272 node_view( ) ../node_embed.module:85
91 0.3468 32368272 node_build_content( ) ../node.module:1335
92 0.3470 32368432 field_attach_view( ) ../node.module:1414
93 0.3470 32368952 _field_invoke_default( ) ../field.attach.inc:1228
94 0.3470 32369408 _field_invoke( ) ../field.attach.inc:385
95 0.3471 32371912 field_default_view( ) ../field.attach.inc:209
96 0.3472 32373080 text_field_formatter_view( ) ../field.default.inc:210
97 0.3472 32373728 _text_sanitize( ) ../text.module:264
98 0.3472 32373728 check_markup( ) ../text.module:319
99 0.3472 32373776 filter_format_load( ) ../filter.module:764| Comment | File | Size | Author |
|---|---|---|---|
| #31 | node_embed-endless-loop-2389893-31-d6.patch | 1.17 KB | osopolar |
| #16 | endless_loop_with-2389893-16.patch | 5.27 KB | jorisdejong |
| #15 | endless_loop_with-2389893-15.patch | 5.19 KB | tessa bakker |
Comments
Comment #1
tessa bakkerHere is a patch to fix a small part of the problem.
With this patch, the node edit form will validate every field with the node embed filter if it doesn't embed its own node.
A follow up is needed to check on replacement if the original caller is embedding itself.
Another way to fix this is to change from form validation to element validation, but this won't speed up the progress, because of the extra query load to get all the right filters.
Comment #2
mrharolda commented@Tessa, while your patch works, I guess it could be cleaner and faster.
I also find this line a bit of weird, and I should test it if it introduces any security issues because of the !:
form_set_error("{$field_name}][{$language}][{$key}][value", t('!label can not embed its own node.', array('!label' => $field['label'])));Comment #3
spadxiii commentedUnfortunately, this patch does not handle circular references across multiple nodes:
Comment #4
dziabodo commentedComment #5
dziabodo commentedHi,
I hope to have solved the task, please check.
Comment #6
dziabodo commentedSorry,
last patch i wrong :) because i send dsm :) this is good.
Comment #7
mrharolda commenteddziabodo, I guess you made the patch on the 1.6 codebase, not on the -dev version? Your patch reverts all fixes and updates since 1.6.
Please create a patch against -dev, and I'll look into your solution!
Comment #8
dziabodo commentedSorry :) This is my first patch. I hope this is good version.
Comment #9
tessa bakker@dziabodo, Thanks for creating a patch. Although the code of your patch isn't very strong it did inspire me to give it a try again :)
@SpadXIII, This patch does handle circular references across multiple nodes :)
@MrHaroldA, fixed the !label issue, if you see something else, please tell
Todo: The validation of the fields should use the same checks as in the filter callback or we could remove the validation and depend only on the callback.
Shortened the issue summary for better reading.
Comment #10
dziabodo commented@Tessa Bakker my patch resolve this problem, what is wrong ?
Comment #11
tessa bakker@dziabodo, patch #8 works only for the first body field with LANGUAGE_NONE. So this wouldn't work for a website with field translation and other fields of the type #textarea with more than one value.
Another problem would be, that if you have the following setup of nodes, the webserver will still response with an error.
* node 1 with [[nid:3]]
* node 2 with [[nid:1]]
* node 3 with [[nid:2]]
* node 4 with [[nid:3]]
There is a module that can help you start writing better code, Coder Review scans any module or theme for small coding issues.
If you have any other question about your patch or mine, feel free to ask.
Comment #12
tessa bakkerAfter talking to SpadXIII the patch needs more work:
* @label > %label
* No watchdog message
** Helper function for drupal_set_message or a inline style like drupal_set_message for users with 'update' access?!
** drupal_page_is_cacheable(FALSE)
* $placeholder > $output
* basic validation, can we exploit drupal_set_message for this, without risking a time-out?
Comment #13
spadxiii commentedWhen you use drupal_set_message, there is no need to call drupal_page_is_cacheable; already done there :)
For basic validation, I'd suggest keeping it really simple and for recursive checks, a maximum number of loops.
Comment #14
mrharolda commentedOther thought: what if upon validation, everythig checks out and the loop is created by updating the embedded node?
1. Edit node 1
2. Embed node 2.
3. Edit node 2.
4. Embed node 1.
This creates an infinite loop, but will pass all validations, right?
What if we'd shift the check to the render part? Would that make sense?
Comment #15
tessa bakkerHi everyone,
A new patch with a new approach.
No validation anymore, because it requires rendering of the current unsaved node, which makes it to complicated for now. Also a valid node save could be invalid a second later if an embed node is updated.
A nice add-on (or sub module) would be a stylish inline-error, but for now let's fix the problem first.
Here are some tests that I tried and were a success with the patch:
Comment #16
jorisdejong commented@Tessa Bakker : made a few small changes to your patch. The message types are now set to 'warning' and the endless loop message to 'error'. Also replaced the static $stack variable to use drupal_static().
Either way, your code is working smoothly as far as i've seen.
Comment #17
tessa bakker@jorisdejong: Nice change!
I think it is safe to say 'Reviewed & Tested'.
Comment #18
mrharolda commentedTnx!
Committed to -dev.
Comment #21
osopolarWouldn't that issue fix the security issue Node Embed - Less critical - Denial of Service - SA-CONTRIB-2016-034?
Comment #22
mrharolda commented@osopolar, if that indeed fixes this, I'll gladly roll out a new version!
Comment #23
deker0 commentedIt definitely sounds like it would resolve the security issue. Go ahead and roll out the new version with the patch, so that the module is no longer shown as "unsupported" and the recommendation being to uninstall it.
Comment #24
mrharolda commentedI've tagged 7.x-1.3, but I'm not allowed to create any new releases? Well, that's it then ... anybody who wants to maintain this further, go right ahead and apply for it with the security team.
Comment #25
deker0 commentedThanks MrHaroldA for patching the module and tagging it as 7.x-1.3. I've sent a message to the Security Team informing them that the updated module is ready for release and it resolves the issue.
Comment #26
annikaC commentedHey there, thanks for fixing the security issue - we can't see the latest release on the module page, is there some sort of process ongoing to get it there?
Comment #27
tessa bakkerHi annikaC,
There is some progress: https://www.drupal.org/node/2746007
Comment #28
David_Rothstein commentedNote that 7.x-1.2 is now retroactively tagged as a security release and listed as the recommended release on the project page.
So it's not necessary to release 7.x-1.3 for security reasons, although it could still be released for other reasons of course.
Comment #29
michaelfavia commentedI can confirm that this issue was resolved in the 1.2 release of this module. We're going to go through some of the other open issues in the queue and see if we can put together a new bug fix release to remove ambiguity around the SA and the old version warning.
Comment #31
osopolarADD Drupal 6 patch, just in case if somebody still needs that.
EDIT:
See also Drupal 6 Long Term Support issue: #2877504: [node_embed] Add D6 patch for DRUPAL-SA-CONTRIB-2016-034