Skip to:
Content

Opened 5 years ago

Closed 3 years ago

#979 closed defect (bug) (fixed)

SSL Support for bp_core_get_avatar()

Reported by: r-a-y Owned by: Jason_JM
Milestone: 1.5 Priority: minor
Severity: Version:
Component: Core Keywords: ssl, gravatar, avatar, https, security
Cc: Jason_JM

Description

More SSL goodness! Wanted to secure the BP settings pages (eg. example.com/members/username/settings), so I had to tackle getting the avatars secure.

In bp_core_avatars.php, for the bp_core_get_avatar() function, change this line:

$gravatar = 'http://www.gravatar.com/avatar/' . md5( $ud->user_email ) . '?d=' . $default_grav . '&s=';

to:

if ($_SERVER['HTTPS'] == 'on')
	$gravatar = 'https://secure.gravatar.com/avatar/' . md5( $ud->user_email ) . '?d=' . $default_grav . '&s=';
else
	$gravatar = 'http://www.gravatar.com/avatar/' . md5( $ud->user_email ) . '?d=' . $default_grav . '&s=';

---

You could probably save a few lines by using PHP conditional shorthand, but just thought I'd jot down the easiest-to-read version.

Okay, so that handles gravatars; but for locally-uploaded avatars, the $url uses $bp->root_domain, so we need to modify the bp_core_get_root_domain() function in bp-core.php:

Change:

return apply_filters( 'bp_core_get_root_domain', get_blog_option( BP_ROOT_BLOG, 'siteurl' ) );

to:

if ($_SERVER['HTTPS'] == 'on')
	return apply_filters( 'bp_core_get_root_domain', str_replace('http://', 'https://', get_blog_option(BP_ROOT_BLOG, 'siteurl')) );
else
	return apply_filters( 'bp_core_get_root_domain', get_blog_option( BP_ROOT_BLOG, 'siteurl' ) );

There might be a cleaner way to switch to SSL for the bp_core_get_root_domain() function, but I've just provided one version.

Bonus for modifying the bp_core_get_root_domain() function is it switches mostly everything over to HTTPS as well (form actions, etc.)

Change History (21)

comment:1 r-a-y5 years ago

  • Keywords https added

Just wanted to say that this is for partially-enabled SSL BP sites, so if SSL is enabled site-wide, the existing bp_core_get_root_domain() function would probably already return as HTTPS via WPMU's get_blog_option() function.

There's no harm in the proposed patch; there's just an added conditional. But, like I said, there might be a better way to make the SSL switch...

comment:2 Jason_JM5 years ago

  • Cc Jason_JM added
  • Keywords security added
  • Owner set to Jason_JM
  • Status changed from new to accepted
  • Type changed from enhancement to defect

'is_ssl()' will do.

If the page enacted on is using the https protocol, 'is_ssl()' will return true.

I'll send a patch Monday.

This really is a defect in my eyes, but not for long thanks to R-A-Y!

comment:3 apeatling5 years ago

I'll wait on a patch before doing anything to this. The current posted method looks cumbersome.

comment:4 apeatling5 years ago

  • Priority changed from major to minor

comment:5 Jason_JM5 years ago

Working on it now.

comment:6 Jason_JM5 years ago

This one will have to be deferred as I would like to use these I just submitted to
WPMU:

http://trac.mu.wordpress.org/attachment/ticket/1111/

The function can also be used like so (and will have to):

add_filter('bp_loggedin_user_domain', 'filterSSL');

comment:7 Jason_JM5 years ago

Am I crazy?

comment:8 r-a-y5 years ago

You're not crazy, Jason! The best way would be to modify MU's functions before BuddyPress does any checks (like you have done).

My version, like Andy said, is cumbersome, but it gets the job done until the official patches are provided!

comment:9 jason_jm5 years ago

  • Milestone changed from 1.1 to 1.2

While the ticket is submitted for mu I wouldn't want it to hold up 1.1.
Marked for 1.2.

Thx r-a-y, for the vote of sanity. :-)

comment:10 Jason_JM5 years ago

Have no idea when this is rolling into Mu...or if it will be declined, dunno...

comment:11 Jason_Jm5 years ago

As a bridge until WP/WPMu Merge is complete and some more info comes out of that domain, I'm going to resurrect and fork AdminSSL.

comment:12 r-a-y5 years ago

That's great news, Jason!

I'll be a tester ;)

comment:13 jason_jm4 years ago

Still working on AdminSSL and several other things. It's now a priority however!

comment:14 r-a-y4 years ago

Jason, your MU patch got added:
http://trac.mu.wordpress.org/changeset/1979

Congrats!

comment:15 apeatling4 years ago

Are we in a position to fix this now?

comment:16 r-a-y4 years ago

bp_core_get_avatar() would still need a conditional for the secure gravatar subdomain, but Jason's MU patch, in theory, can be applied to bp_core_get_root domain().

add_filter('bp_core_get_root_domain', 'filter_SSL');

Unfortunately I can't test this at the moment.

comment:17 apeatling4 years ago

  • Milestone changed from 1.2 to 1.3

comment:18 r-a-y4 years ago

FYI, this is what I'm using currently in BP 1.1.3, and should work in 1.2 as well:

function bp_ssl_avatar($url) {
  if(is_ssl())
  	$url = str_replace('http:','https:',$url);
  return $url;
}
add_filter( 'bp_core_avatar_folder_url', 'bp_ssl_avatar');

function bp_ssl_gravatar($url) {
  if(is_ssl())
  	$url = str_replace('http://www','https://secure',$url);
  return $url;
}
add_filter( 'bp_gravatar_url', 'bp_ssl_gravatar');

With the new filters in BP 1.1+, I didn't need to apply SSL to bp_core_get_root_domain like mentioned above.

I just needed SSL support for avatars.

These are not worthy to be in core as they're essentially filter overrides, but will help for those looking for a solution.

comment:19 cnorris234 years ago

  • Component set to Core

The first part of this ticket, fixing bp_core_fetch_avatar() (formally named bp_core_get_avatar()) has been fixed ([2876]). At the moment, I'm unsure as to whether bp_core_get_root_domain() properly obeys SSL settings as suggested by @r-a-y.

comment:20 boonebgorges3 years ago

Anyone have a setup where they can easily test what cnorris23 says?

comment:21 DJPaul3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

#1948 (r3343) added https support to bp_core_get_root_domain().

Note: See TracTickets for help on using tickets.