Project

General

Profile

Actions

Support #15610

closed

Loops & Logic plugin

Added by Marilyn Weber about 2 years ago. Updated almost 2 years ago.

Status:
Rejected
Priority name:
Normal
Assignee:
Category name:
WordPress Plugins
Target version:
Start date:
2022-03-12
Due date:
% Done:

0%

Estimated time:
Deployment actions:

Description

Via ZD:

I am the webmaster for the Queens College English department website, which we've recently moved to the Commons here: https://qcenglish.commons.gc.cuny.edu/

I was wondering if it would be possible to install the Loops & Logic plugin: https://wordpress.org/plugins/tangible-loops-and-logic/

Since the Commons already has installed Advanced Custom Fields, it would be great to be able to display some of those custom fields as loops. What I have in mind is creating an automatically-updated table containing faculty profile information--displaying custom fields I've added, such as title, email address, teaching interests, etc.

Would this be possible to add within the next week or so, perhaps? Please let me know if you need any more information for me.

Best,

Cliff Mak
Assistant Professor
Department of English
Queens College, CUNY


Files

Scripts.png (26.5 KB) Scripts.png Raymond Hoh, 2022-03-14 07:34 PM
Actions #1

Updated by Boone Gorges about 2 years ago

  • Category name set to WordPress Plugins
  • Assignee set to Raymond Hoh
  • Target version set to 1.19.6

The plugin won't work as-is on the Commons. Its syntax requires authors to use pseudo-HTML tags, like <Loop>. But in our Multisite configuration, non-super-admins are limited in the markup that they are permitted in their posts. As such, when you try to use the plugin's tags, they're stripped by WordPress before being saved to the database.

The plugin looks useful and the concept is very clever, so I think it might be worth a small amount of time to figure out whether we can work around the limitation. Ray, I was hoping you could spend a few minutes to look it over. I'm thinking that we may be able to add the tags defined at https://plugins.trac.wordpress.org/browser/tangible-loops-and-logic/tags/2.3.9/vendor/tangible/template/tags/index.php to the list of tags allowed by kses. The tricky bit here is likely to be that we need to define attribute for each of these tags, and I'm not sure whether it'd be easy to come up with a clear list of these attributes.

Before working too much on this, I'd be curious to hear Ray's thoughts about the security implications of this kind of plugin. The "logic" parts - loops etc - seem like they're probably fairly innocuous, but arbitrary script elements seem particularly problematic to me.

If you agree that it's worthwhile, could you spend a small amount of time to see whether it's feasible to make Commons interoperability with an hour or two of work? If it seems like you're going to hit snags, I think we'll have to say no. (I've put into the 1.19.6 milestone but this can definitely be bumped if necessary.)

Actions #2

Updated by Raymond Hoh about 2 years ago

I did some basic testing and the plugin wouldn't require any major modifications on our end for the most part.

As long as users input their templates directly into the "Tangible > Templates" admin page and uses either the provided [template id=XXX] shortcode or the "Tangible Templates" block in their post, they should be good to go. I haven't tested the plugin's integration with ACF, but I assume that will also work.

The KSES problem only occurs if the Tangible syntax is used directly into the post's Code Editor. Using the template is easier to manage for the user, so we should be okay here.

but arbitrary script elements seem particularly problematic to me.

This is the big problem. I entered a <script src="XXX"></script> line into a Tangible Template and when rendered on the page, the JS loaded in the footer. It's possible to unhook the Tangible footer routine: https://plugins.trac.wordpress.org/browser/tangible-loops-and-logic/tags/2.3.9/vendor/tangible/template/actions/index.php#L29 on the 'tangible_templates_ready' hook and run our own without rendering the scripts call:

/**
 * Do not allow scripts to load for Tangible Loops and Logic.
 */
add_action( 'tangible_templates_ready', function( $html ) {
    // Remove default footer hooks.
    remove_action( 'wp_footer', $html->footer_action, 99 );
    remove_action( 'admin_footer', $html->footer_action, 99 );

    // Redo footer actions, but only allow styles.
    $html->footer_action = function() use ($html) {
        $html->load_all_enqueued_styles();
        $html->set_action_done( 'foot' );
    };

    // Add back footer hooks.
    add_action( 'wp_footer', $html->footer_action, 99 );
    add_action( 'admin_footer', $html->footer_action, 99 );
} );

I've added the fix for this here: https://github.com/cuny-academic-commons/cac/commit/f7929d245f3ae2606e881eba508f76a1d0c6ce11

One other thing is we will want to remove the "Tangible > Scripts" menu item and the "Script" tab from the Template and Layout admin pages to prevent confusion since the JS will no longer render on the frontend. See attached screenshot: https://redmine.gc.cuny.edu/attachments/21456/Scripts.png. I've just done this here: https://github.com/cuny-academic-commons/cac/commit/9ba9179aff4d74726b8ffc3b676437075ef9b5d8

I also tried to write some JS in the "Style" tab and Tangible prevented the JS from rendering on the frontend, so we should be okay with allowing the plugin to run on the Commons.

Actions #3

Updated by Matt Gold about 2 years ago

Just wondering -- are these changes specific to the CAC or would it be useful to share them with the plugin author as a kind of safe-for-multisite version of the plugin?

Actions #4

Updated by Marilyn Weber about 2 years ago

Sorry - should I let the user know anything at this point?

Actions #5

Updated by Boone Gorges about 2 years ago

  • Status changed from New to Staged for Production Release

Awesome, Ray! Thank you for your great work! I had been looking only at the editor, and didn't notice that there was a different way to create templates.

Just wondering -- are these changes specific to the CAC or would it be useful to share them with the plugin author as a kind of safe-for-multisite version of the plugin?

I didn't see a GitHub repo for the plugin, but maybe I missed it. In any case, these improvements wouldn't be applicable to the public plugin, but Ray could probably make some suggestions that would address the same underlying issues - such as a toggle that disables script support, or improved allowedtags filtering so that you don't need unfiltered_html to use the plugin in Multisite.

Sorry - should I let the user know anything at this point?

The plugin will be available for use after the 1.19.6 release, on Tuesday March 22.

Actions #7

Updated by Raymond Hoh about 2 years ago

Awesome, Ray! Thank you for your great work! I had been looking only at the editor, and didn't notice that there was a different way to create templates.

I spoke too soon. My initial tests when creating the Tangible templates were with a super administrator account instead of a regular site admin. As a regular site admin, KSES steps in and strips Tangible's custom elements as Boone mentioned above.

Attempting to use the 'wp_kses_allowed_html' filter to add in Tangible's custom HTML elements will not work because Tangible capitalizes their HTML elements and KSES (as well as force_balance_tags()) does all element checks against strtolower(). I've filed a ticket on WP Trac here: https://core.trac.wordpress.org/ticket/55407.

This isn't great, but I've added some code to bypass KSES only for the 'tangible_template' and 'tangible_layout' post types: https://github.com/cuny-academic-commons/cac/commit/42aa542d5de317df557ae27d06f01581f73ffdbd. Coupled with the javascript mods I did above, this should be okay. However, it is still possible to write inline JS to an attribute like onmouseover="alert('hi')". Because of this, we might want to limit activation of the plugin only to the gcenglish site. What do you think, Boone?

I've also written an email to Tangible about KSES on multisite, so perhaps they might address this in a future update of the plugin.

Actions #8

Updated by Boone Gorges about 2 years ago

  • Status changed from Staged for Production Release to Hold

Crud.

This isn't great, but I've added some code to bypass KSES only for the 'tangible_template' and 'tangible_layout' post types: https://github.com/cuny-academic-commons/cac/commit/42aa542d5de317df557ae27d06f01581f73ffdbd. Coupled with the javascript mods I did above, this should be okay. However, it is still possible to write inline JS to an attribute like onmouseover="alert('hi')". Because of this, we might want to limit activation of the plugin only to the gcenglish site. What do you think, Boone?

Just to be clear about this patch, it looks like you are bypassing kses, but you had to polyfill force_balance_tags() in a way that avoids problems with upper-case tags. Is this correct?

Removing KSES and allowing JS, even just for this one site and these post types, feels pretty bad to me. There's the direct security risks of an editor knowingly or unknowingly using bad scripts. And there's the long-term maintenance burden - if Tangible changes the way the plugin works, we may have to revisit the fix.

I'm leaning toward a reverse of course, and telling the requester that we can't install the plugin for security reasons. Matt, if you're reading this, can you weigh in? I hate to be in the situation where we've told a user that we can accommodate a special request and then have to rescind it, but this may be one of those cases.

Actions #9

Updated by Marilyn Weber about 2 years ago

Boone/ Matt/ Ray -

We don't have to walk anything back. I wrote that this was still being considered but is complicated.

Actions #10

Updated by Boone Gorges about 2 years ago

Thanks, Marilyn. This helps :)

Matt, I'd still like to hear your thoughts about whether we should commit our resources on this. I lean toward no given the complexity.

Actions #11

Updated by Raymond Hoh about 2 years ago

Just to be clear about this patch, it looks like you are bypassing kses, but you had to polyfill force_balance_tags() in a way that avoids problems with upper-case tags. Is this correct?

Yes, that's correct.

Removing KSES and allowing JS, even just for this one site and these post types, feels pretty bad to me.

To be clear, inline JS in HTML event attributes such as onclick and onmouseover would pass through. We could mitigate this by attempting to remove these event attributes ourselves, which I know is something we probably do not want to do.

If the user attempts to use the <script> tag, Tangible does some processing to move all <script> related calls to the footer; I've already removed this rendering in this commit, so this part is covered.

Actions #12

Updated by Matt Gold about 2 years ago

Hi All -- given my reading of the above, this sounds like a security risk, one we shouldn't take on. I agree with your leaning towards no on this, Boone.

I will say that even after all of this, I'm not completely clear on what the user is trying to do, and I'm wondering whether there might be alternate plugins out there that would help them solve this problem.

In terms of resource allocation, I will say that we are a bit tight until we hear that our budget situation is improved by Central (which very well might happen before the end of the year). so for now, I'd lean towards keeping things lean but suggest that we can possibly explore this in the future.

Actions #13

Updated by Boone Gorges about 2 years ago

Thanks, Matt. I agree that there might be another way to do at least some of what the user wants. The cool thing about the Loops plugin is that it gives very wide flexibility to show custom content in any context. Adding Scott as a watcher in case he has suggestions for existing tools. (For example, Gutenberg's 'Query Loop' block can do a lot.)

Marilyn, please let the user know that we've done an analysis of the tool. Due to technical incompatibilities between the plugin and the Commons's WordPress Multisite network, it's not possible to allow the tool at this time.

Ray, could you please pull the plugin and your mods from the codebase?

Actions #14

Updated by scott voth about 2 years ago

Perhaps the member could use the "Teams" plugin to accomplish this.

Actions #15

Updated by Raymond Hoh about 2 years ago

  • Status changed from Hold to Rejected
  • Target version changed from 1.19.6 to Not tracked

Ray, could you please pull the plugin and your mods from the codebase?

Done in https://github.com/cuny-academic-commons/cac/commit/a9c244cfdee1eb86bf02d3c8471b59ffc92a1bdf.

I was also emailing back-and-forth with one of the developers of Loops & Logic and he said that their team does not plan on implementing overrides for the default 'unfiltered_html' behavior. So this is a no-go as far as looking at adding Loops & Logic in the future.

Actions #16

Updated by Raymond Hoh about 2 years ago

As far as the intended use-case for the qcenglish site, there is an Advanced Custom Fields block plugin that allows for meta field display: https://acfblocks.com/blocks/acf-meta-display/.

This cannot be used in a loop context as mentioned in the ticket description though, but I thought I'd mention it.

Actions #17

Updated by Marilyn Weber almost 2 years ago

We did receive a gracious reply from the requester:

"Ah, well! Thank you so much for trying, though. I appreciate it!

Best,

Cliff

Cliff Mak
Assistant Professor"

Actions

Also available in: Atom PDF