fix handle non-numeric manifest icon keys in get_extension_icon to prevent ValueError crash #131#132
fix handle non-numeric manifest icon keys in get_extension_icon to prevent ValueError crash #131#132Th-Shivam wants to merge 3 commits into
Conversation
…lueError crash When a Chrome extension's manifest.json uses non-numeric icon size keys (e.g. "48x48" instead of "48"), the call to max(..., key=lambda x: int(x)) would raise ValueError. Although caught by the outer except block, this caused icon retrieval to silently fail and return a placeholder image. Fix: filter manifest icon keys to only those that are digit-only strings before calling max(), and raise a clear ValueError when none are found so the existing except handler produces a descriptive warning log. Agent-Logs-Url: https://github.com/Th-Shivam/ExtensionShield/sessions/318c38db-0228-407d-b8a3-98a363e523e3 Co-authored-by: Th-Shivam <169958957+Th-Shivam@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Th-Shivam/ExtensionShield/sessions/318c38db-0228-407d-b8a3-98a363e523e3 Co-authored-by: Th-Shivam <169958957+Th-Shivam@users.noreply.github.com>
fix: handle non-numeric manifest icon keys in get_extension_icon to prevent ValueError crash
There was a problem hiding this comment.
Pull request overview
Fixes issue #131 by making manifest icon key handling in get_extension_icon resilient to non-numeric keys to avoid ValueError during icon selection.
Changes:
- Filters manifest
iconskeys to numeric-only values before computing the “largest” icon size. - Adds an explicit error when no numeric icon size keys are found (currently caught by the surrounding
try/except).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # isinstance guard handles edge cases where manifest was parsed outside of json.load | ||
| numeric_keys = [k for k in manifest_icons.keys() if isinstance(k, str) and k.isdigit()] | ||
| if not numeric_keys: | ||
| raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}") | ||
| largest_size = max(numeric_keys, key=lambda x: int(x)) | ||
| icon_rel_path = manifest_icons[largest_size] |
There was a problem hiding this comment.
numeric_keys currently filters to isinstance(k, str) and k.isdigit(), but the preceding comment says it should also handle cases where the manifest was parsed outside json.load. If manifest_icons contains integer keys (e.g., {128: "..."}), this code will treat them as non-numeric and skip valid icons. Consider accepting int keys as well (and normalizing to the actual key type used for indexing) so numeric sizes aren’t dropped in that edge case.
| # isinstance guard handles edge cases where manifest was parsed outside of json.load | |
| numeric_keys = [k for k in manifest_icons.keys() if isinstance(k, str) and k.isdigit()] | |
| if not numeric_keys: | |
| raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}") | |
| largest_size = max(numeric_keys, key=lambda x: int(x)) | |
| icon_rel_path = manifest_icons[largest_size] | |
| # Accept int keys as well for edge cases where manifest was parsed outside of json.load | |
| numeric_icon_candidates = [] | |
| for k in manifest_icons.keys(): | |
| if isinstance(k, str) and k.isdigit(): | |
| numeric_icon_candidates.append((int(k), k)) | |
| elif isinstance(k, int) and not isinstance(k, bool): | |
| numeric_icon_candidates.append((k, k)) | |
| if not numeric_icon_candidates: | |
| raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}") | |
| _, largest_size_key = max(numeric_icon_candidates, key=lambda item: item[0]) | |
| icon_rel_path = manifest_icons[largest_size_key] |
| # Get the largest icon - only consider numeric keys (e.g. "128", "64") | ||
| # Some extensions use non-numeric keys like "48x48" which would cause ValueError | ||
| # isinstance guard handles edge cases where manifest was parsed outside of json.load | ||
| numeric_keys = [k for k in manifest_icons.keys() if isinstance(k, str) and k.isdigit()] | ||
| if not numeric_keys: | ||
| raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}") | ||
| largest_size = max(numeric_keys, key=lambda x: int(x)) |
There was a problem hiding this comment.
If an extension’s icons keys are only non-numeric strings (e.g. "48x48"), this block raises a ValueError and the icon selection from the manifest is skipped entirely. That prevents the crash but can still lead to a placeholder icon even when a valid icon path is present in the manifest. Consider extracting a numeric size from keys like "48x48" (e.g., split on "x" / regex for leading digits) so the endpoint can still serve the manifest-declared icon.
| numeric_keys = [k for k in manifest_icons.keys() if isinstance(k, str) and k.isdigit()] | ||
| if not numeric_keys: | ||
| raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}") | ||
| largest_size = max(numeric_keys, key=lambda x: int(x)) |
There was a problem hiding this comment.
Raising ValueError here is used for control flow and is immediately caught by the broad except Exception, which will log a warning ("Failed to read manifest for icons") even though the manifest was read successfully. Consider replacing the raise with a non-exceptional fallback (e.g., log at debug/info and continue) to avoid noisy warnings for manifests that simply don’t use numeric size keys.
|
@sapnilbiswas can you please review this pr and merge it as per your earliest convenience . |
sapnilbiswas
left a comment
sapnilbiswas
left a comment
There was a problem hiding this comment.
The PR only keeps str keys in main.py (line 4041) where k.isdigit(). If a manifest only has non-numeric keys, like "48x48," the code raises ValueError at main.py (line 4042) because numeric_keys is empty. The broad handler at main.py (line 4051) then eats that exception, so the endpoint still skips the manifest icon and goes back to the default behavior.
Please check the mentioned points
| # Get the largest icon - only consider numeric keys (e.g. "128", "64") | ||
| # Some extensions use non-numeric keys like "48x48" which would cause ValueError | ||
| # isinstance guard handles edge cases where manifest was parsed outside of json.load | ||
| numeric_keys = [k for k in manifest_icons.keys() if isinstance(k, str) and k.isdigit()] |
There was a problem hiding this comment.
The PR keeps only str keys where k.isdigit(). If a manifest has only non-numeric keys like "48x48", numeric_keys is empty and the code raises ValueError at main.py .
|
@Th-Shivam the pr needs some refinement please go through the changes required and ask for a re-review |
Fixes #131