Bug #8813
closed`cac_activity_secondary_avatar_enlarge()` can generate invalid avatar URLs
0%
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
Updated by Boone Gorges over 6 years ago
- Related to Feature #8639: Add photo upload to registration page added
Updated by Raymond Hoh over 6 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.
Updated by Boone Gorges over 6 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!
Updated by Raymond Hoh over 6 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.