OF-3314, OF-3315 & OF-3316: Minor FaviconServlet fixes

[guusdk]

The FaviconServlet is used to display an icon on the admin console, when displaying server-to-server connections. The changes in this PR harden the implementation. Please refer to individual commit messages for details.

The one notable change is that this replaces the caches named "Favicon Hits" and "Favicon Misses" with one new cache: "Favicon".

[akrherz]

@coderabbitai please review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 3394 --repo igniterealtime/Openfire --json title,body,files,additions,deletions

echo "=== PR DIFF ==="
gh pr diff 3394 --repo igniterealtime/Openfire

Length of output: 15409

---

🏁 Script executed:

#!/bin/bash
# Find and read CacheableOptional class
fd -i "CacheableOptional.java" --exec cat {}

echo "=== Check cache name consistency ==="
# The code uses "Favicon Cache" but CacheFactory registers "Favicon"
rg -n "Favicon" xmppserver/src/main/java/org/jivesoftware/util/FaviconServlet.java
rg -n "Favicon" xmppserver/src/main/java/org/jivesoftware/util/cache/CacheFactory.java

echo "=== Check existing uses of `@Nonnull` ==="
rg -n "`@Nonnull`" xmppserver/src/main/java/org/jivesoftware/util/FaviconServlet.java

echo "=== Check existing import for `@Nonnull` ==="
rg -n "import.*Nonnull" xmppserver/src/main/java/org/jivesoftware/util/FaviconServlet.java

echo "=== Check for other cache XML/config files ==="
fd -e xml -e properties --exec grep -l "favicon" {} 2>/dev/null

echo "=== Check ofcache.xml or similar ==="
fd "ofcache" --exec cat {}

echo "=== Check for 'Favicon Hits' or 'Favicon Misses' references elsewhere ==="
rg -rn "Favicon Hits\|Favicon Misses\|faviconHits\|faviconMisses\|Favicon Cache\|faviconCache" --type java

echo "=== Check redirect handling - does disableRedirectHandling affect anything else ==="
rg -n "disableRedirectHandling\|LaxRedirectStrategy" --type java

echo "=== Check GraphicsUtils.isImage ==="
fd "GraphicsUtils.java" --exec cat {}

Length of output: 8018

---

🏁 Script executed:

#!/bin/bash
# Check if there are any Hazelcast/clustering cache config files that reference old cache names
fd -e xml --exec grep -l -i "favicon" {} 2>/dev/null
rg -rn "favicon" --type xml

echo "=== Check the full current FaviconServlet for context ==="
cat xmppserver/src/main/java/org/jivesoftware/util/FaviconServlet.java

Length of output: 11790

---

---

@akrherz Here is my review of PR #3394.

---

Overall Assessment

The PR is a solid hardening of FaviconServlet. The SSRF mitigation through manual redirect following, the cleaner single-cache design, null-host guarding, and the Cache-Control header are all good improvements. I found one bug and a few minor observations.

---

🐛 Bug: Cache name mismatch

FaviconServlet.java line 103:

faviconCache = CacheFactory.createCache("Favicon Cache");

CacheFactory.java line 108:

cacheNames.put("Favicon", "favicon");

The servlet requests a cache named "Favicon Cache" but CacheFactory only maps "Favicon" → "favicon". Because the names don't match, the size limit of 1_048_576L configured for "favicon" will not be applied to the cache actually created. Depending on CacheFactory's fallback behaviour, the cache may grow unbounded or use some default sizing.

Fix: Either change the createCache call:

faviconCache = CacheFactory.createCache("Favicon");

or update CacheFactory to register "Favicon Cache".

---

Minor observations

1. byte[] serializability — CacheableOptional<byte[]> is fine; all Java arrays are Serializable, so this satisfies the T extends Serializable bound.

2. Redirect loop with isConnectedHost check — The decision to reject redirects to hosts that are not an active S2S peer is conservative but intentional for SSRF prevention. Worth a note that legitimate favicon CDN redirects (e.g., to cdn.example.com) will always be blocked, but this is an acceptable trade-off for a security-hardened path.

3. Cache size halved — The old implementation had two caches each at 1 MB (total 2 MB effective budget). The new single cache is 1 MB. Since it now holds CacheableOptional<byte[]> wrapping actual image bytes or absent markers, this is roughly equivalent to the old hits cache alone. If many favicons are cached, eviction may occur sooner. Consider whether 1 MB is still adequate or if it should be bumped slightly (e.g., 2_097_152L).

4. graphicsUtils.isImage on 200 response only — The image validation is correctly applied only on HTTP 200, so redirect Location header values are never passed to ImageIO. ✅

5. redirectsRemaining-- > 0 allows 5 iterations — When starting at 5, the post-decrement check allows redirectsRemaining = 5, 4, 3, 2, 1 → 5 total requests per candidate URL. This is correct and safe.

---

The only thing that needs fixing before merge is the cache name mismatch (#1 above). Everything else is an improvement over the prior implementation.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

FaviconServlet is refactored to replace two separate hitsCache/missesCache fields with a single faviconCache storing CacheableOptional<byte[]>. CacheFactory is updated to register one "Favicon" cache entry in place of the previous "Favicon Hits"/"Favicon Misses" pair. In doGet, hostname validation and disconnected-host checks are reorganized using a new isConnectedHost() helper, with explicit 404 responses when no default favicon bytes exist. The LaxRedirectStrategy is removed; getImage() is rewritten to manually follow redirects within a bounded loop, validating redirect targets for http/https scheme and connected-host membership before proceeding.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The pull request description accurately describes the changes: hardening the FaviconServlet implementation and consolidating the favicon caches from two separate caches to one unified cache.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@xmppserver/src/main/java/org/jivesoftware/util/FaviconServlet.java`:
- Around line 102-103: The cache name passed to CacheFactory.createCache() in
the FaviconServlet initialization does not match the registered cache name in
CacheFactory.cacheNames, preventing the configured 1 MB size limit
(cache.favicon.size = 1_048_576) from being applied. Change the cache name
parameter in the CacheFactory.createCache() call to match the exact registered
cache name in CacheFactory.cacheNames for the Favicon cache, ensuring the
configuration property is correctly resolved instead of falling back to the
default 256 KB limit.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 833d7923-0333-4846-9552-081b7d32c7a8

📥 Commits

Reviewing files that changed from the base of the PR and between 46b02b7 and 5b6f362.

📒 Files selected for processing (2)

• xmppserver/src/main/java/org/jivesoftware/util/FaviconServlet.java
• xmppserver/src/main/java/org/jivesoftware/util/cache/CacheFactory.java

[Fishbowler]

Approved, with 1 small question

[Fishbowler]

Should this check be this strict? If we attempt to get the favicon of "xmpp.example.org", we wouldn't allow a redirect to example.org?