WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#22354 closed defect (bug) (fixed)

Gravatar Shouldn't Use images/blank.gif

Reported by: miqrogroove Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4.2
Component: General Keywords: has-patch
Focuses: Cc:

Description

There is a very subtle flow-of-control error in pluggable.php:

	elseif ( 'blank' == $default )
		$default = includes_url('images/blank.gif');

This makes sense when $email has no value, because it will cause the local blank.gif file to be displayed. However...

When $email is not empty, this function is causing the gravatar.com server to make a proxy request to the blank.gif file and display the local file through a remote request. This is not the intended behavior. When the $email value is present, the Gravatar request should simply contain "&d=blank" to get a blank image.

Attachments (4)

miqro-blank-gravatars.diff (1.3 KB) - added by miqrogroove 2 years ago.
22354.simple.patch (604 bytes) - added by Viper007Bond 2 years ago.
Simple fix
22354.fixes-plus-formatting.patch (4.1 KB) - added by Viper007Bond 2 years ago.
The fix plus a fix to allow https:// default image URLs as well as bringing the whole function up to modern coding standards
22354.patch (674 bytes) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 @miqrogroove2 years ago

  • Keywords has-patch added

comment:3 @Viper007Bond2 years ago

I think you should just do this:

elseif ( empty($email) && 'blank' == $default )

It's cleaner and more consistent with the existing code.

@Viper007Bond2 years ago

Simple fix

comment:4 follow-up: @miqrogroove2 years ago

Yes... somewhat related. However, this bug patch is ready to commit and shouldn't wait on new features.

Viper007Bond: Your code is not logically equivalent to mine. Please explain why you would make that additional change?

@Viper007Bond2 years ago

The fix plus a fix to allow https:// default image URLs as well as bringing the whole function up to modern coding standards

comment:5 in reply to: ↑ 4 @Viper007Bond2 years ago

Replying to miqrogroove:

Viper007Bond: Your code is not logically equivalent to mine. Please explain why you would make that additional change?

Good catch. My patches should be disregarded. Silly mistake on my part.

The whole logic on that block is messy. I'm tempted to say we replace it with a cleaner switch and two ifs.

Last edited 2 years ago by Viper007Bond (previous) (diff)

comment:6 @miqrogroove2 years ago

I'm still in favor of the original patch if we can get it into beta. I wouldn't attempt a re-write for 3.5 at this point.

comment:7 @nacin2 years ago

  • Milestone changed from Awaiting Review to 3.5

This is particularly timely because of http://blog.gravatar.com/2012/10/09/default-image-changes/, which was a change to the Gravatar service about a month ago that could affect the usage of blank.gif. Because of that, moving to 3.5.

@SergeyBiryukov2 years ago

comment:8 @SergeyBiryukov2 years ago

miqro-blank-gravatars.diff makes sense to me.

22354.patch is a simplified version without reformatting the block.

comment:9 @miqrogroove2 years ago

Thanks nacin. Do you want a more involved patch for this? In addition to the concerns from above, I just noticed on http://en.gravatar.com/site/implement/images/ that query strings are disallowed on default images now.

Sergey, yours is fine too.

comment:10 @miqrogroove2 years ago

Testing shows query strings are still allowed within the d= variable. Disregard that.

comment:11 @nacin2 years ago

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

In 22566:

Pass 'blank' to Gravatar rather than sending blank.gif for Gravatar to proxy. props miqrogroove, fixes #22354.

comment:12 @miqrogroove2 years ago

Cool. Thanks everyone!

Note: See TracTickets for help on using tickets.