Rica carv missing avatar check reopens pull #5241

[rica-carv]

Motivation and Context

External avatar files, when missing, give this output:

Description

Added code to revert to default avatar image, when external file is missing, output like this:

How Has This Been Tested?

Tested in e107 v2.3.3 clean install,
Output tested in admin area and in the front end (pics above)

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist

• My code adheres to the e107 code style standard.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests pass.

[e107help]

@rica-carv The problem you're solving is real: a user whose remote avatar URL has gone dead currently renders a broken image icon, and falling back to the generic avatar is the right outcome. It's the detection method I can't sign off on.

@fopen() on a user-supplied URL

user_image is user-controlled. Core documents it as such (user_handler.php#L826: "may be external URL or file on server"), the field strips only "'() (#L107), and XUP social login writes $profile->photoURL straight in.

Today the visitor's browser fetches that URL. With this change the server fetches it, on every render of that avatar. Three consequences:

1. Server-side request forgery. Any registered user can point user_image at http://127.0.0.1:8080/, a cloud metadata endpoint, or an internal host, and make the web server issue the request. That's a new attack surface where there wasn't one.
2. Blocking network I/O per avatar. No timeout is set, so a slow or blackholed host stalls page render for the PHP default default_socket_timeout (60s). A forum listing with 20 remote avatars serialises 20 round trips.
3. Handle leak. The return of @fopen() is never fclose()d, so each call leaks a stream resource for the request's lifetime.

It also silently depends on allow_url_fopen, which is off on plenty of shared hosts. Where it's off, every remote avatar falls back to the generic one, which is a worse bug than the one being fixed.

The out-of-scope line

-$genericImg = $tp->thumbUrl($genericFile, 'w=' . $width . '&h=' . $height, true, $full);
+$genericImg = $tp->thumbUrl($genericFile, 'w=' . $width . '&h=' . $height, false, $full);

That third argument is $raw (e_parse_class.php#L2745). With true the path is run through createConstants($url, 'mix') so the emitted URL carries {e_IMAGE} rather than the literal directory. Flipping it changes the generated URL of every generic avatar on the site, which busts every cached copy of it and has nothing to do with checking a missing avatar. If there's a reason for it, it wants its own PR and its own justification.

What I'd do instead

Validation belongs at write time, not render time. Check the URL once when the user saves their settings (or when XUP imports it), and store the outcome. Rendering a page is the wrong moment to discover a third-party host is down.

If it has to be render-time, the only version I'd be comfortable with is client-side: emit the remote URL as normal plus an onerror handler that swaps in the generic avatar. Zero server-side fetches, zero SSRF, and it fails over instantly in the browser rather than after a socket timeout.

Smaller things

• The new block is indented with spaces; the surrounding file is tabs.
• strpos($image, '://') !== false already accepts javascript: style values only if they contain ://, so the check itself is fine, but any new fetch path should whitelist http/https explicitly rather than trusting that.

I'd rather not merge this shape. If you want to take it either way, say which and I'll draft the write-time validation or the onerror fallback with you.