fix: normalize bundled catalog icon bounds

[vishu-bh]

Pull Request

🔗 Related Issue

Related to #6406

---

📝 Summary

Reduces excess transparent padding in bundled catalog icons while preserving source fidelity. Visible content is cropped, upscaled by at most 2×, and centered on a 128px transparent canvas.

• Adds idempotent --normalize-existing mode: local assets only, no remote fetches, skips missing assets.
• Makes --force and --normalize-existing mutually exclusive.
• Regenerates 83 affected bundled PNG assets using final normalization rules.
• Pins validated icon-fetch addresses while preserving TLS SNI and HTTP Host; redirect targets are revalidated and environment proxies are disabled.
• Adds regression coverage for trimming, capped resizing, empty assets, offline operation, idempotence, mode exclusivity, and DNS-rebinding protection.

---

📏 Reviewability

• This PR has one clear purpose
• The linked issue is not labeled triage
• Unrelated bugs or improvements are tracked in separate issues/PRs
• Tests are included with the code they validate

---

🏷️ Type of Change

• Bug fix
• Feature / Enhancement
• Documentation
• Refactor
• Chore (deps, CI, tooling)
• Other (describe below)

---

🧪 Verification

Check	Command	Status
Ruff	.venv/bin/ruff check scripts/fetch_catalog_icons.py tests/unit/scripts/test_fetch_catalog_icons.py	✅ Passed
Black	.venv/bin/black -l 200 --check scripts/fetch_catalog_icons.py tests/unit/scripts/test_fetch_catalog_icons.py	✅ Passed
Isort	.venv/bin/isort --check-only --diff scripts/fetch_catalog_icons.py tests/unit/scripts/test_fetch_catalog_icons.py	✅ Passed
Unit tests	.venv/bin/pytest tests/unit/scripts/test_fetch_catalog_icons.py -q	✅ 22 passed
Idempotence	Run python scripts/fetch_catalog_icons.py --normalize-existing twice	✅ Second run produced no diff

---

✅ Checklist

• Code formatted (make black isort pre-commit equivalents run on changed Python files)
• Tests added/updated for changed behavior
• CLI help documents capped local normalization behavior
• No secrets or credentials committed

---

📓 Notes (optional)

--normalize-existing only processes assets already present under mcpgateway/static/catalog-icons. Missing catalog assets remain untouched and no network calls are made. Empty transparent assets are reported as unresolved rather than treated as normalized.

[gandhipratik203]

Resize no longer bounded by source resolution (functionally impacting). thumbnail() never upscaled; resize() always fills 128 in one dimension. 36 of 77 assets are upscaled 4x+ (supabase, ferryhopper at 8x), giving blocky logos and a 372 KB to 1.17 MB bundle growth. Consider capping the upscale at ~2x.

Related: --normalize-existing works from the already-downsampled PNG, so lost detail cannot be recovered. A --force refetch on the worst entries would likely pick up a 180px apple-touch-icon.

Unguarded exceptions in the normalize branch (functionally impacting, small). scripts/fetch_catalog_icons.py:327-343 calls _has_normalized_icon_bounds() and _image_to_png() above the try/except that records misses, so one corrupt PNG aborts the run instead of printing a MISS.

[msureshkumar88]

Thanks for this — verified the trim/crop/upscale-cap logic decodes cleanly and the before/after byte diffs genuinely reduce padding (mean fill 41% → 69% across the 83 regenerated icons). Nice regression coverage on the offline/idempotence behavior too.

Blocking

Related Issue links to an unrelated issue. The PR references "Related to #4731", but #4731 is about adding Disconnect/Test actions to the React catalog UI (and a gateway_id field on CatalogServer) — nothing there concerns icon rendering or asset generation. Could you point this at the actual originating context (the #5966 → #6397 → #6406 icon-generation chain, or #5770 which is the issue that will actually render logo_url) so the issue graph doesn't end up misleading anyone tracing #4731?

Non-blocking (optional, no need to address before merge)

• The description says "the 77 bundled PNG assets" / "15 passed" — actual counts are 83 regenerated PNGs and 17 tests.
• "fills the shared 128px canvas consistently" is a bit strong — post-fix fill still ranges 23%–100% across icons (most are capped by MAX_UPSCALE_FACTOR=2.0, not by canvas size). Might be worth softening the wording.
• --normalize-existing's idempotence relies on the contextforge_normalized PNG marker; if that chunk is ever stripped (external optimizer, hand-added icon) a subsequent run will reprocess and regrow the icon over ~2 runs before converging. No current exposure (all 105 bundled assets are covered today) and nothing in this repo's toolchain strips PNG chunks, but worth a note for future contributors, maybe tracked alongside #6406.

[vishu-bh]

Thanks for this — verified the trim/crop/upscale-cap logic decodes cleanly and the before/after byte diffs genuinely reduce padding (mean fill 41% → 69% across the 83 regenerated icons). Nice regression coverage on the offline/idempotence behavior too.

Blocking

Related Issue links to an unrelated issue. The PR references "Related to #4731", but #4731 is about adding Disconnect/Test actions to the React catalog UI (and a gateway_id field on CatalogServer) — nothing there concerns icon rendering or asset generation. Could you point this at the actual originating context (the #5966 → #6397 → #6406 icon-generation chain, or #5770 which is the issue that will actually render logo_url) so the issue graph doesn't end up misleading anyone tracing #4731?

Non-blocking (optional, no need to address before merge)

• The description says "the 77 bundled PNG assets" / "15 passed" — actual counts are 83 regenerated PNGs and 17 tests.
• "fills the shared 128px canvas consistently" is a bit strong — post-fix fill still ranges 23%–100% across icons (most are capped by MAX_UPSCALE_FACTOR=2.0, not by canvas size). Might be worth softening the wording.
• --normalize-existing's idempotence relies on the contextforge_normalized PNG marker; if that chunk is ever stripped (external optimizer, hand-added icon) a subsequent run will reprocess and regrow the icon over ~2 runs before converging. No current exposure (all 105 bundled assets are covered today) and nothing in this repo's toolchain strips PNG chunks, but worth a note for future contributors, maybe tracked alongside [CHORE]: Follow-ups from catalog icon generation review (#6397) #6406.

for blocking one, this is issue found in testing of 4731. Hence added in relates to. But if it is misleading I have removed it for now

[gandhipratik203]

LGTM!

[Lang-Akshay]

Blocking Changes

#	Area	File	Line	Blocking reason	Required change
1	Security	scripts/fetch_catalog_icons.py	127, 151–153	High / CWE-918: hostname DNS is resolved for validation and then resolved again by httpx for the connection. A catalog or override hostname can return a public address during validation and a private address during the request (DNS-rebinding TOCTOU), allowing the maintainer process to reach an internal service.	Pin the validated address through the connection (while preserving TLS SNI/HTTP Host), or use an address-pinning resolver/transport; revalidate every redirect and ensure proxy/environment settings cannot bypass the policy.

[Lang-Akshay]

LGTM ✅

[gandhipratik203]

LGTM!

[msureshkumar88]

Thanks for tackling this — the trim/crop/idempotence approach is solid, and the --normalize-existing offline-only mode is a nice addition. I verified the logic locally (crop → capped rescale → composite, marker-based idempotence) and the 20 unit tests all pass, CI is green. Two things need another pass before merge, both verified empirically rather than just read from the diff:

Blocking

1. Shipped PNGs aren't reproducible from the PR's own final code.

I regenerated all 105 icons from the pre-PR baseline using the generator as it stands at the tip of this branch (--normalize-existing from scratch). 96/105 come out pixel-identical to what's committed, but 9 don't — e.g.:

icon	regenerated from base w/ HEAD code	committed in PR
carbon-voice.png	extent 120 (KEEP, no rewrite)	extent 128
neon.png	extent 120 (KEEP)	extent 128
ean-search.png	extent 120 (KEEP)	extent 128
parallel-search.png	extent 64 (2x cap applied)	extent 96

These 9 were generated by the intermediate state after commit 1 (d1a575e5) before the upscale cap landed in commit 2 (085dd447), and never got regenerated against the final generator. So right now, a maintainer who runs make catalog-icons --normalize-existing on a clean checkout gets different bytes than what's in this PR. Given the whole point of the branch is "the generator and the bundled assets agree," this needs a final regen-and-recommit pass before merge (python scripts/fetch_catalog_icons.py --normalize-existing, no --dry-run, then diff/commit only what changes).

2. The PR's stated goal isn't what's actually delivered — needs disclosure or a design change.

"Normalizes bundled catalog icons so visible logo content fills the shared 128px transparent canvas consistently."

Measured all 105 icons before/after: MAX_UPSCALE_FACTOR = 2.0 (added in 085dd447, "cap catalog icon normalization") caps how far a cropped glyph can grow, and it's binding on the large majority of changed icons:

• 63 of 82 changed icons land at exactly 2x post-crop (cap-bound, not fill-bound)
• 64 of 105 icons are still below the script's own NORMALIZED_ICON_MIN_EXTENT = 120 threshold after this PR — i.e., below what the code itself considers "normalized"
• Fill distribution after the PR: 5 icons <40%, 30 at 40–60%, 29 at 60–80%, only 41 ≥94%

This isn't a bug in the cap itself — capping upscale to avoid blur on tiny source rasters is a reasonable call. But as written, the PR both claims and (via --strict/_has_normalized_icon_bounds) codifies "fills the canvas" as the success criterion, while the cap guarantees a large fraction of icons won't meet it. Two ways to close this, whichever fits the intent better:

• Reword the summary/CLI help to describe the actual capped behavior ("reduces excess padding up to 2x, preserving source fidelity") rather than "fills consistently," or
• If full fill really is the goal, the cap needs to come from improving source resolution (e.g. requiring a minimum candidate size, or trying apple-touch-icon sizes beyond the first hit) rather than raw upscale of whatever was fetched — post-hoc upscaling of a 16–32px favicon can't recover detail no matter the cap value.

Also flagging root-cause: _fetch_icon picks the first successful candidate with no minimum-resolution floor, so tiny favicons get baked in permanently regardless of this fix. Worth a follow-up if not folded in here.

Non-blocking (would still like to see addressed, doesn't need to hold up this PR)

• No issue linked (Related to is blank in the PR body). Given this fixes a real, verifiable visual bug plus a DNS-rebinding TOCTOU hardening (nice catch, matches #6406 item 2), it'd help future readers to link a tracking issue and call out the security fix explicitly in the summary — right now it's bundled into an "icon normalization" PR without being mentioned.
• --force/--normalize-existing mutual exclusivity is a listed feature but has no regression test (confirmed manually it works via argparse's mutually-exclusive group, just uncovered).
• docs/docs/manage/catalog.md still has no mention of --normalize-existing or the local-asset convention (carried over from #6406 item 5).
• _has_normalized_icon_bounds treats a fully-transparent asset as "normalized" (peek.png is blank — 0 opaque pixels — and reports KEEP, never flagged). Might be worth an explicit empty-canvas check.
• The contextforge_normalized PNG text chunk is the fast-path idempotence signal, but it doesn't survive a plain re-save (verified: stripping it and rerunning re-upscales an already-upscaled github.png from 64→128, compounding blur). No PNG optimizer runs in this repo's CI/pre-commit today, so this is latent, not active — but worth a comment noting the assumption.

Happy to take another look once the assets are regenerated against the final code — the mechanism itself checks out.

[msureshkumar88]

Re-reviewed after the follow-up commits (pin catalog icon fetch destinations, align catalog icon normalization assets, refresh secrets baseline). All previously requested changes are addressed:

• Issue link added (#6406) in the PR description.
• The DNS-rebinding/SSRF fix (ValidatedDestination, pinned-address connect, trust_env=False) is now called out explicitly in the summary instead of being an undisclosed bundled change.
• Summary language now accurately describes the capped 2x upscale behavior instead of implying full canvas fill.
• Shipped PNG assets are now byte-reproducible from the final generator code (verified via full regen + diff — 0 differences across all 105 icons).
• Added regression test for --force/--normalize-existing mutual exclusivity (test_icon_refresh_modes_are_mutually_exclusive).
• Fully transparent source images (e.g. peek.png) are now surfaced as unresolved (IconFetchError: Image has no visible pixels) rather than silently treated as normalized.
• docs/docs/manage/catalog.md updated with a "Bundled Icon Maintenance" section covering --normalize-existing.

Verified locally: pytest tests/unit/scripts/test_fetch_catalog_icons.py → 22 passed, and a full --normalize-existing regen against the shipped assets produces no diff.

Nice work tightening this up. Approving.

[Lang-Akshay]

LGTM