Fix: profile share-card cache key omits fields that affect the render - #578
Open
Deez-Automations wants to merge 1 commit into
Open
Conversation
The public /share/profile/{username}/card.png endpoint cached rendered
PNG cards keyed on only 4 fields (username, total_points, badges_earned,
challenges_completed). Two users sharing identical stats -- common at
0/0/0 right after registration -- collided on the same cache file,
serving one user's card to the other. Same-user avatar/bio/featured-
badge changes were also invisible to the cache: the old card stayed
stale until stats next changed, with no way to invalidate it short of
an admin manually deleting the file.
Extended the cache key with every field that actually affects the
rendered image, not just the linked issue's own suggested set
(avatar_type/avatar_url/avatar_emoji/bio). Two gaps found and closed
beyond what the issue proposed:
- avatar_type == "gravatar" resolves its image from the user's email
via resolve_avatar_url(), not from avatar_url at all -- the issue's
own suggested fix would still let two gravatar-type profiles with
matching stats and no bio collide. Added user_email to the key.
- PUT /featured-badges lets a user re-curate which earned badges are
featured with zero change to the 4 count fields, leaving the card
permanently stale after a re-curation. Same class of gap covers
"latest badge" staleness if badge composition changes at an
unchanged total count. Added earned_badge_ids (sorted -- composition,
not order, is what's rendered) and featured_badge_ids (kept in the
user's chosen order, since that's what's actually displayed).
Also switched hashlib.md5 to sha256 per the issue's own optional-
hardening suggestion, and joined badge-ID lists with a separator that
can't appear in a developer-defined slug ID, closing even the
theoretical (not currently reachable) join-ambiguity case for free.
Extracted the key construction into a small, pure, directly-testable
function -- this file had zero test coverage before this change.
Resolves GenAI-Security-Project#508
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The public
/share/profile/{username}/card.pngendpoint caches rendered PNG cards to disk keyed on only 4 fields:username,total_points,badges_earned,challenges_completed. Two users sharing identical stats — common at 0/0/0 right after registration — collide on the identical cache file; whichever renders first is permanently served to the other. Same-user avatar/bio/featured-badge changes are also invisible to the cache: the old card stays stale indefinitely, with no way to invalidate it short of an admin manually deleting the file (there's no TTL/eviction anywhere for this cache directory).Beyond the linked issue's own suggested fix
The issue proposes adding
avatar_type/avatar_url/avatar_emoji/bioto the key. Two real gaps in that proposal, found before writing anything:The gravatar case is still collision-prone.
resolve_avatar_url()(finbot/apps/ctf/routes/profile.py) resolves the actual rendered image from the user's email whenavatar_type == "gravatar", not fromavatar_urlat all:Two profiles with
avatar_type="gravatar", matching stats, and no bio would still produce identical cache keys under the issue's own suggested fix. Addeduser_emailto the key.Featured-badge re-curation is invisible.
PUT /featured-badgeslets a user change which of their earned badges are featured, with zero change to the 4 count fields — a re-curation would leave the card permanently stale. The same gap covers "latest badge" staleness if badge composition changes at an unchanged total count (e.g. losing one badge and gaining a different one). Addedearned_badge_ids(sorted before joining — badges aren't shown in a user-chosen order outside the featured section, only composition matters there) andfeatured_badge_ids(kept in the user's chosen order, since that IS what's rendered).Comparison against another open fix for the same issue
Another contributor has an independent, already-open PR (#510) for the same issue. Worth comparing openly since both are unmerged: #510 implements the issue's own suggested fix directly (
avatar_type/avatar_url/avatar_emoji/bioadded to the key, MD5→SHA-256 — genuinely equivalent to this PR on that part). It doesn't cover the two gaps above though, so it inherits both: a gravatar user changing their linked email still collides under #510's key, and a featured-badge re-curation still serves a stale card. Not a criticism of #510's approach, both gaps only turn up if you traceresolve_avatar_url()and the/featured-badgesendpoint specifically rather than working from the issue's own field list — flagging so whoever merges has the full picture.Fix
Extracted
_profile_card_cache_key()— a small, pure, directly-testable function (this file had zero test coverage before this change) — and wired it into the route using values already in scope. Also switchedhashlib.md5tohashlib.sha256, per the issue's own optional-hardening suggestion (MD5 gets flagged by security scanners even in this non-adversarial cache-busting context). Joined badge-ID lists with\x1f(unit separator) rather than,, closing even the theoretical (not currently reachable, since badge IDs are developer-defined slugs, never user input) join-ambiguity case for free.Known, documented limitations (not fixed here, noted rather than silently ignored)
_fetch_avatar_b64fails (timeout, non-200, oversized image), the fallback-letter card is written to the cache and served for that key indefinitely, even after the fetch would have succeeded on retry. Pre-existing, not introduced by this diff.profile/userfields are threaded through correctly at the call site (e.g. an accidental future swap ofavatar_url/avatar_emojiwouldn't be caught). Full route testing here needs the image-rendering pipeline (Playwright viaget_renderer()), which has been unreliable in this dev environment this session — didn't want to build new, possibly-flaky test infra just for this fix. Noted as a gap, not silently skipped.Test plan
tests/unit/apps/test_profile_card_cache_key.py(13 tests): deterministic for identical inputs; different usernames differ; two users with matching stats but different avatar don't collide; avatar_url/bio/avatar_emoji changes each invalidate the key; gravatar users with different emails don't collide; featured-badge re-curation invalidates the key; featured-badge reorder (same set) invalidates the key; earned-badge composition change at the same count invalidates the key; earned-badge reorder (same composition) does not invalidate the key (confirms the sort-before-join design is intentional, not an oversight); None values handled without raising; returns a valid hex digestpytest tests/unit/apps/ tests/unit/ctf/ -q— 53 passed, 1 pre-existing unrelated skiporigin/main: clean on its own, and clean paired individually against every other currently-open PR from this batch (Fix: orchestrator confirms payment to vendor even when it fails #574, Fix: DNS-based SSRF bypass in guardrail webhook URL validation #575, Fix: guardrail payload corruption before signing + missing after_tool for complete_task #576, Fix: Dark Lab tool override silently drops parameters/inputSchema #577) — zero new conflicts introduced by this branch