Limit generated PWA icon size#13385
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the /pwa_icon/{size} route by enforcing bounds on requested icon resize dimensions, preventing oversized (and non-positive) resize requests from reaching the image open/resize path, and adds a regression test for oversized requests.
Changes:
- Introduces a maximum allowed PWA icon resize size (
MAX_PWA_ICON_SIZE = 512) and rejects invalid sizes with HTTP 400. - Ensures invalid sizes are rejected before opening/resizing the favicon image.
- Adds a regression test covering an oversized
/pwa_icon/{size}request.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
gradio/routes.py |
Adds max-size validation for /pwa_icon/{size} via a new constant and a 400 response on invalid sizes. |
test/test_routes.py |
Adds a regression test asserting oversized icon resize requests return HTTP 400. |
Comments suppressed due to low confidence (1)
gradio/routes.py:2031
PIL.Image.open(favicon_path)is used without an explicit close/context manager. Under repeated requests this can keep file descriptors open longer than necessary (PIL keeps the underlying file handle until the image is closed/GC’d). Consider opening the image via a context manager (or ensuring the image/file is closed) after resizing/saving.
detail=f"Icon size must be between 1 and {MAX_PWA_ICON_SIZE}.",
)
import PIL.Image
img = PIL.Image.open(favicon_path)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .as_posix(), # type: ignore | ||
| ) | ||
| VERSION = get_package_version() | ||
| MAX_PWA_ICON_SIZE = 512 | ||
| XSS_SAFE_MIMETYPES = { |
There was a problem hiding this comment.
Updated to use MAX_PWA_ICON_SIZE in the manifest entry as well, so the advertised 512x512 icon and the route validation share one source of truth.
| app = routes.App.create_app(io) | ||
| client = TestClient(app) | ||
|
|
||
| response = client.get("/pwa_icon/513") |
There was a problem hiding this comment.
Updated the test to request routes.MAX_PWA_ICON_SIZE + 1 instead of hard-coding 513.
| def test_pwa_icon_rejects_oversized_resize(self): | ||
| io = Interface(lambda x: x + x, "text", "text") | ||
| io.favicon_path = "test/test_files/bus.png" | ||
| app = routes.App.create_app(io) | ||
| client = TestClient(app) | ||
|
|
||
| response = client.get("/pwa_icon/513") | ||
|
|
||
| assert response.status_code == 400 | ||
|
|
There was a problem hiding this comment.
Parametrized the route test to cover 0, -1, routes.MAX_PWA_ICON_SIZE, and routes.MAX_PWA_ICON_SIZE + 1. The focused test now passes with 4 cases.
NguyenCong2k
force-pushed
the
fix-pwa-icon-size-limit
branch
from
214f3f8 to
bc6ef2d
Compare
| { | ||
| "src": app.url_path_for("pwa_icon", size=512), | ||
| "sizes": "512x512", | ||
| "src": app.url_path_for("pwa_icon", size=MAX_PWA_ICON_SIZE), |
There was a problem hiding this comment.
We should use the size specified by the user no?
Summary
/pwa_icon/{size}requestsTest
Note
Low Risk
Low risk: small change to a single route that rejects invalid/oversized icon resize requests and adds test coverage; only affects clients requesting out-of-range sizes.
Overview
Adds a
MAX_PWA_ICON_SIZEconstant (512) and uses it to cap/pwa_icon/{size}resize requests, returning400for non-positive or oversized sizes before any image processing.Updates
manifest.jsongeneration to reference the same max size, and adds a parametrized regression test ensuring out-of-range sizes are rejected while the max allowed size succeeds.Reviewed by Cursor Bugbot for commit bc6ef2d. Bugbot is set up for automated code reviews on this repo. Configure here.