Project

General

Profile

Bug #8813

`cac_activity_secondary_avatar_enlarge()` can generate invalid avatar URLs

Added by Boone Gorges over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority name:
Normal
Assignee:
Category name:
My Commons
Target version:
Start date:
2017-10-16
Due date:
% Done:

0%

Estimated time:

Description

cac_activity_secondary_avatar_enlarge() swaps '-bpthumb' with '-bpfull' to get larger avatars. In some cases, this technique is not reliable, because the -bpfull.jpg and -bpthumb.jpg avatars can be generated with different UUIDs. See https://buddypress.trac.wordpress.org/browser/tags/2.9.1/src/bp-core/classes/class-bp-attachment-avatar.php?marks=287#L278. uniqid() will generate a timestamp based on microtime(), which could tick between generation of bpfull and bpthumb if things are slow enough. See https://secure.php.net/manual/en/function.uniqid.php

We might consider an enhancement in BP that would use a single UUID prefix for both avatar types, but this wouldn't do anything for legacy avatars.

I think the only solution is to do a separate bp_core_fetch_avatar() lookup to get the bpfull avatar. I know this is not ideal from a performance perspective, but I think it's necessary.

I found this while working on #8639, but I'm pretty sure it's an independent issue - a spot-check of a few filenames suggests that it's not uncommon to see differently named bpfull and bpthumb for actual users, and I seem to recall seeing these 404s in the past.

It'd be great to get this into the 1.12 release, but please feel free to push off if you think it needs more research.


Related issues

Related to CUNY Academic Commons - Feature #8639: Add photo upload to registration pageResolved2017-08-31

History

#1 Updated by Boone Gorges over 2 years ago

  • Related to Feature #8639: Add photo upload to registration page added

#2 Updated by Raymond Hoh over 2 years ago

  • Category name set to My Commons
  • Status changed from New to In Progress

My Commons predates the uniqid() change, which is why this problem hasn't really surfaced until now.

The uniqid() change was made for security reasons, based on the changeset message, so I don't think we can really make any upstream changes here.

I'll do what is recommended and that is to do another lookup for the full avatar, but only if the avatar does not match a Gravatar one, which will help minimize the lookup somewhat.

#3 Updated by Boone Gorges over 2 years ago

My memory is less than perfect :-D

The crux of the security fix was not using user-facing info to generate filenames, not that the two images share the same hash. So I think it's still possible to make an upstream change. But it'd only mitgate the issue for future avatars.

Thanks for looking into this!

#4 Updated by Raymond Hoh over 2 years ago

  • Status changed from In Progress to Resolved

Should be fixed in https://github.com/cuny-academic-commons/cac/commit/4e534c560fac89d591fd59af6cc6c9b2dbd29fa6.

This is available for testing on the development server.

#5 Updated by Boone Gorges over 2 years ago

Thank you so much!

Also available in: Atom PDF