Skip to:

Opened 7 years ago

Closed 6 years ago

#7656 closed enhancement (fixed)

Update `bp_new_group_invite_friend_list` for new $args to support full list markup

Reported by: hnla's profile hnla Owned by: hnla's profile hnla
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch commit


bp_new_group_invite_friend_list() generates a list of friends wrapped in li markup however it does not account for the required parent markup leaving that to the template and therefore additional checks to ensure hardcoded markup not displayed if list function empty.

Patch adds two args to handle before & after args string & a simple class arg to output classes on the parent element.

The args are deliberately defaulted to empty strings to ensure we don't upset existing use, they would need to be explicitly passed for any generated markup.

This relates to a recent change to Nouveau create group steps where we needed to update the template and used this function before realising that it only rendered li elements without a 'ul' construct.

Attachments (3)

7656-01.patch (1.7 KB) - added by hnla 7 years ago.
7656-02.patch (1.6 KB) - added by hnla 7 years ago.
Update $args, add allowed array to use to filter through wp_kses, remove class arg.
7656-03.patch (1.4 KB) - added by hnla 6 years ago.
Final version: Simplified approach, let user pass markup in, no kses.

Download all attachments as: .zip

Change History (13)

7 years ago

#1 @DJPaul
7 years ago

1) Let's use sanitize_html_class() on the supplied classes.
2) I am wondering if and how we could use KSES to whitelist the HTML attribute we're building here. Or if we need to. Any similar code in WP core we could compare this against would be interesting.

Apart from that, idea is sound.

#2 @hnla
7 years ago

  1. Will patch to.
  1. looked at kses but not sure it's appropriate here? In only accepting a plain string isn't kses and it's element array overkill, I'm not sure how we use it in this specific context ( we do utilise wp_kses elsewhere in BP core iirc though).

Am in favour though of preventing daft strings being passed in and rendered e.g 'dib', 'uk' so maybe a simpler approach, create our own array of allowed strings and then do a basic in_array check whilst retaining the esc_html... maybe:

$allowed_elements = array('ul', 'div', 'p');
if ( ! empty( $r['after_list'] ) && in_array( $r['after_list'], $allowed_elements ) ) {

#3 @DJPaul
7 years ago

  1. Great!
  1. I don't think that's necessary. As I said, I can't remember if WordPress does a similar construction, but in its widgets, it has before_titleetc which is handled like so:
$output .= $args['before_title'] . $title . $args['after_title'];

How about having this patch's before_list and after_list treated the same - not be escaped and just accept whatever it's given? (when you call the function, you'd pass it e.g. <h3> rather than h3.) It'd give flexibility in case someone needs to do some crazy WordPress-style hacks. :)

If we did that, we'd need to remote the parent_classes attribute and rely on people passing the entire element e.g. <h3 class="yolo"> which is what the widgets code does.

(The widgets) isn't the best code in the world, by any means, but just wanted a contrasting option for your consideration. You are more familiar than I about consistency with (and if) how we do this sort of thing for other BuddyPress functions -- if we do, consistency is more important to me.

#4 @hnla
7 years ago

not be escaped and just accept whatever it's given?

rely on people passing the entire element e.g. <h3 class="yolo"> which is what the widgets code does

We certainly can do, however does this not present 'opportunities' do we not worry about what string may be passed through, if we have no particular security concerns I would be happy to just let the dev pass in what they need i.e '<ul class="invite-friends-list">'

I'll think on more about this and other BP core examples.

#5 @DJPaul
7 years ago

Any developer can pass any value to any function. We all know that. That's why we're so careful with sanitising user-supplied data, because it could be anything.

If there's no way to inject a value into a unit of code at runtime (be that a search form value, or the result of an API request, or data from an RSS feed, etc), then it's safe -- at least from this very specific perspective.

We don't need to harden BuddyPress against developers making poor choices with how they write their code (i.e. making up their own HTML elements, because that's harmless - it just wouldn't work).

Last edited 7 years ago by DJPaul (previous) (diff)

#6 @hnla
7 years ago


However taking into account we can pass strings of type '<div class="">' in to various arg functions unfettered in core and also use wp_kses liberally in places I've re-factored patch along the lines you suggested:

Allow args in style of array( 'before' = > '<ul class="">')
Added a simple allowed array and filtered the before/after args on wp_kses.

We manage things somewhat, in this context there are limited options, so ensure what is passed in confirms to that narrow set.

Equally I can appreciate just leaving it wide open.

Last edited 7 years ago by hnla (previous) (diff)

7 years ago

Update $args, add allowed array to use to filter through wp_kses, remove class arg.

#7 @DJPaul
6 years ago

The indentation for $allowed is incorrect, but that aside, I don't see much point to KSES this now we've explored this more; user-supplied data will not be directly passed to this function. Bigger chance that it annoys a developer trying to some crazy customisation, especially as $allowed element is hardcoded.

I know I suggested looking at KSES originally and you weren't sure :) but please remember this is as much code review as it is a collaborative technical discussion, and that I can talk rubbish with the best of them. :)

I think the couple of new arguments and the change to $retval is all we need!

#8 @hnla
6 years ago

Agreed, will re-factor to remove the KSES, and make it more KISS!

The indentation for $allowed is incorrect

Yep be better as spaces :)

6 years ago

Final version: Simplified approach, let user pass markup in, no kses.

#9 @DJPaul
6 years ago

  • Keywords commit added

Do it!

#10 @hnla
6 years ago

  • Owner set to hnla
  • Resolution set to fixed
  • Status changed from new to closed

In 11828:

Pass additional $args to 'bp_get_new_group_invite_friend_list()`

This function renders markup for the looped item defaulting to 'li' but leaves it to the template to provide the parent element.

Commit provides two new args 'before' & 'after' to allow the function to return the complete element markup if desired, simplifies templates having to render markup and checks for empty function return.

Props DJPaul.

Fixes #7656

Note: See TracTickets for help on using tickets.