fix(screenshots): validate cached screenshot image bytes on read and write - #42120
Conversation
…write Reject stale/empty/corrupt cached screenshot payloads instead of serving them: get_from_cache_key now treats a payload that claims a successful screenshot but has empty or non-image bytes as a cache miss, and compute_and_cache applies the same cheap header check before marking a result as cached-success, falling back to ERROR status otherwise. Co-Authored-By: Claude <noreply@anthropic.com>
dosubot
Bot
added
the
infra:caching
|
Thanks Elizabeth! LGTM on the approach, and thanks for the tests. One nit: since Will take another look once CI's green. |
There was a problem hiding this comment.
Pull request overview
This PR hardens screenshot/thumbnail caching by adding lightweight validation of cached screenshot bytes on both read and write paths in superset/utils/screenshots.py, preventing empty or non-image payloads from being served (or recorded as successful cache entries) indefinitely.
Changes:
- Add
validate_screenshot_image()(PNG/JPEG magic-byte + non-empty check) and apply it when reading cached payloads and when caching newly computed screenshots. - Treat invalid cached
UPDATEDpayloads as cache misses (None) and emit a warning log with the cache key and invalid reason. - Update and extend unit tests to use minimal valid PNG-header bytes for “success” cases, plus new coverage for invalid read/write payload handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
superset/utils/screenshots.py |
Adds shared image-bytes validation and enforces it during cache reads and writes to avoid serving/caching invalid images as successful. |
tests/unit_tests/utils/test_screenshot_cache_fix.py |
Updates fixtures to use valid PNG-header bytes and adds tests for read-side invalid cache rejection + write-side invalid payload handling and logging. |
tests/unit_tests/utils/screenshot_test.py |
Updates legacy “plain bytes” cache test data to pass new validation and updates compute/caching fixtures accordingly. |
| if invalid_reason: | ||
| logger.warning( | ||
| "Not caching screenshot result for %s: %s image payload", | ||
| cache_key, | ||
| invalid_reason, | ||
| ) |
|
The warning message can be updated to better reflect that an error payload is being cached, and the logic can be adjusted to avoid redundant warnings when the status is already set to ERROR. You can modify the else:
if invalid_reason and cache_payload.status != StatusValues.ERROR:
logger.warning(
"Caching error payload for %s: %s image payload",
cache_key,
invalid_reason,
)
if cache_payload.status != StatusValues.ERROR:
cache_payload.error()superset/utils/screenshots.py |
Answers review feedback: image and is redundant at runtime since validate_screenshot_image only returns None for truthy, valid bytes, but mypy can't infer that relationship across the function-call boundary, so the guard stays for type narrowing. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42120 +/- ##
==========================================
- Coverage 65.08% 64.90% -0.18%
==========================================
Files 2752 2785 +33
Lines 154475 156870 +2395
Branches 35404 35791 +387
==========================================
+ Hits 100544 101821 +1277
- Misses 52019 53071 +1052
- Partials 1912 1978 +66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #8be4c1Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SUMMARY
Dashboard/chart screenshot and thumbnail caching (
ScreenshotCachePayloadinsuperset/utils/screenshots.py) had two gaps:BaseScreenshot.get_from_cache_key()returned whatever was in the cache as long as a status ofUPDATEDwas recorded, even if the stored image wasNone/0-byte or otherwise not a real image. Since the cache key is digest-based, an unchanged dashboard/chart kept serving the same stale/blank entry indefinitely (e.g. a blank PDF download).ERRORstatus when the screenshot task produced a falsy (None/b"") result, but a non-empty, non-image payload (e.g. truncated/corrupt bytes) still passed theif image:check and got cached withUPDATEDstatus.This PR adds a shared, cheap validator (
validate_screenshot_image()— checks non-empty + PNG/JPEG magic-byte header, no full decode) used on both paths:get_from_cache_key()now rejects a cached payload that claims a successful screenshot (status == UPDATED) but fails validation, returningNone— the same value callers already treat as a cache miss — and logs aWARNINGwith the cache key and the reason (emptyvsundecodable). Because both the dashboard and chartscreenshot/thumbnail/cache_*endpoints already call this same shared classmethod, this closes the hole for both resource types without touchingcharts/api.pyordashboards/api.py.BaseScreenshot.compute_and_cache()now runs the same check on the freshly generated image before caching it as a success. If it fails, the payload is markedERROR(consistent with fix(screenshots): catch empty-bytes tiled result and set ERROR on falsy image #41097's approach) instead ofUPDATED, and aWARNINGwith the cache key and reason is logged.Scope and known limitation — what this does and does NOT protect against
This validates image bytes and headers only. It deliberately does not inspect image content:
Preventing those is capture-side responsibility, and is where the readiness/budget work lives (#42253/#42427 positive readiness checks, #42273 no-unguarded-fallback, #42118 tiled wait budget, #42624 report deadline): with those in place, a blank/spinner capture fails the capture instead of ever reaching the cache. This PR is the complementary cache-layer guarantee for the class of corruption those fixes can't address: empty, truncated, or non-image bytes can never be cached as success nor served from cache — a failure class observed in production (0-byte cached assets) that, with digest-based keys and no TTL eviction in some deployments, previously poisoned an entry indefinitely. A pixel-level blank-image detector would be the durable catch-all for the remaining gap, but that's a separate, heavier change with genuine false-positive risk (legitimately near-empty dashboards) and is intentionally out of scope here.
Also explicitly not changed:
superset/utils/screenshot_utils.py,superset/utils/webdriver.py) — untouched; owned by the readiness/budget PRs above.TESTING INSTRUCTIONS
Added unit tests in
tests/unit_tests/utils/test_screenshot_cache_fix.pyand updated fixtures intests/unit_tests/utils/screenshot_test.py(existing tests used non-image placeholder bytes likeb"image_data"as stand-ins for a "successful" screenshot; these now use a minimal valid PNG header so they still exercise the success path under the new validation):None) with aWARNINGlogged; a payload with non-image garbage bytes is likewise treated as a cache miss; a valid PNG-header payload is served normally; a non-UPDATED(e.g.PENDING) payload is returned as-is.compute_and_cache()with an empty or garbage-bytes screenshot result cachesERRORstatus (neverUPDATED) and logs aWARNINGincluding the cache key and reason.Run:
48 passed. Also ran the broader
tests/unit_tests/utils/ -k screenshotsuite (76 passed); 3 unrelated pre-existing failures inwebdriver_test.py/test_screenshot_utils.pyreproduce identically onmaster(playwright-version mismatch in the test environment, unrelated to this change and in files this PR does not touch).ruff checkpasses on the changed files;mypyreports no errors insuperset/utils/screenshots.py.ADDITIONAL INFORMATION
🤖 Generated with Claude Code