Follow redirects when downloading AI vision images #107
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/issue-105-follow-media-proxy-redirects"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
Posts whose attachments are served through Mastodon's
media_proxyURLs never received a vision verdict: the URL answers302 Found, and httpx2'sraise_for_status()raises on 3xx responses, so the image download failed and the post stayedAI: pendingfor manual review (affected: posts 42, 43, 44, 46).Fix
classify_postscreates the image-download client withfollow_redirects=True.download_first_imagenow guesses the MIME type from the post-redirect final URL (response.url), so proxied PNG/WebP/GIF attachments are labelled correctly instead of always falling back toimage/jpeg.Also on this branch
uv.lock/pylock.tomlin sync). The newertyflagsasyncio.iscoroutinefunctionas deprecated, so this PR also swaps bothzhongli/circuit_breaker.pycall sites toinspect.iscoroutinefunction(behaviour-equivalent on Python 3.12+; existing sync/async circuit-breaker tests cover it).alembic/env.pycallingfileConfigwith its defaultdisable_existing_loggers=True, which setdisabled=Trueon existing loggers and silently suppressed their records — intermittently breakingtest_main/test_startupcapture tests depending on worker order, and dropping the "Database migrations applied" log line in production. Fixed withdisable_existing_loggers=Falseplus hermetic restore oflogger.disabledin the capture helpers.Testing
classify_postsviaMockTransport; fails (red) without the fix, reproducing the productionHTTPStatusError..pngfinal URL yieldsimage/png.fenliu-tryke-3.14run 8× consecutively clean after the #110 fix (previously flaked ~1-in-3).Closes #105
Closes #110
WuMing
Found 1 issue(s). See inline comments below.
@ -75,3 +75,3 @@now = datetime.now(UTC)headers = {"User-Agent": "fenliu-ai/0.1"}async with httpx2.AsyncClient(timeout=180, headers=headers) as http:async with httpx2.AsyncClient(timeout=180, headers=headers, follow_redirects=True) as http:security [HIGH]
Enabling follow_redirects=True on an HTTP client used to fetch post media URLs allows an attacker-controlled media_proxy URL to redirect the server-side request to internal addresses (e.g. cloud metadata 169.254.169.254) or other internal services, a Server-Side Request Forgery (A10). Validate/allow-list redirect targets (scheme, host, port) and block private/link-local IPs before following, or use a hardened outbound proxy.
packages/fenliu/src/fenliu/services/ai_pipeline.py🔴 Declining for this PR, tracked properly in #108. The SSRF primitive predates this change:
download_first_imagealready fetched attacker-chosenpreview_url/urlvalues directly, so the initial URL needs the same scheme/DNS/private-range validation as the redirect hops — a redirect-only guard here would be a partial fix with false assurance. #105's scope was to restore intended behaviour (media_proxy posts were stuck unclassified); the full hardening (initial URL plus every redirect hop, IPv4/IPv6 private and link-local ranges) is designed in #108.WuMing
Found 2 issue(s). See inline comments below.
@ -10,3 +10,4 @@---Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file — and reads the image type from the final URL, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always assumed JPEG.docs [LOW]
Grammar: 'instead of always assumed JPEG' is incomplete. Change to 'instead of always being assumed to be JPEG' or 'instead of always assuming JPEG' to make the meaning clear.
@ -285,3 +285,3 @@def decorator(func: Callable) -> Callable:if asyncio.iscoroutinefunction(func):if inspect.iscoroutinefunction(func):tests [LOW]
No test file appears in the diff for this zhongli source change. Replacing asyncio.iscoroutinefunction with inspect.iscoroutinefunction (also at line 324) changes how coroutine functions are detected by the circuit-breaker decorators; add or update tests covering sync and async callables so this behavior is verified.
WuMing
Found 2 issue(s). See inline comments below.
@ -10,3 +10,4 @@---Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file — and reads the image type from the final URL, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always assumed JPEG.docs [LOW]
Clarity: the final phrase 'instead of always assumed JPEG' is elliptical and awkward. Consider 'instead of always being assumed to be JPEG' or 'instead of always falling back to image/jpeg'.
@ -6,3 +6,3 @@import asyncioimport functoolsimport inspecttests [LOW]
circuit_breaker.py is modified to switch from asyncio.iscoroutinefunction to inspect.iscoroutinefunction in two places, but no zhongli test file appears in this diff. Add or update tests that exercise the async/sync wrapper detection paths, or explicitly document this as an equivalent no-behavior-change dependency/API swap.
WuMing
Found 1 issue(s). See inline comments below.
@ -138,0 +166,4 @@with (patch("fenliu.services.ai_pipeline.text_classifier", return_value=text),patch("fenliu.services.ai_pipeline.vision_classifier", return_value=vision),patch("fenliu.services.ai_pipeline.httpx2", new=SimpleNamespace(AsyncClient=TransportInjectedClient)),code [LOW]
Patching the entire fenliu.services.ai_pipeline.httpx2 module with SimpleNamespace only provides AsyncClient. This will hide or break any other httpx2 attributes used by ai_pipeline (such as HTTPError or Response), and it makes the test dependent on the current implementation. Prefer patching only httpx2.AsyncClient or using a small namespace that preserves all existing module attributes.
WuMing
Found 1 issue(s). See inline comments below.
@ -5,3 +5,3 @@fenliu 3.0.0-- Fixed: AI vision now follows media-proxy redirects, so proxy-served attachments get classified instead of stalling as pending.docs [LOW]
Terminology is inconsistent with line 13: line 13 uses Mastodon's
media_proxyidentifier, while this line uses 'media-proxy'. Usemedia_proxyfor the code/feature name, or use 'media proxy' as a common noun.WuMing waves 201–204 — responses
Release-Notes.md✅ Fixed in commit
5af9684: now reads 'instead of always falling back to image/jpeg'.zhongli/circuit_breaker.py✅ Covered by existing tests, no new test needed:
tests/test_circuit_breaker.pyalready exercises both wrapper-selection paths — sync (circuit_breaker_decorator_simple, asynccircuit_protected_callsuccess/failure/circuit-open,circuit_breaker_decoratorasync success). All pass with the swapped predicate; the swap is behaviour-equivalent on Python 3.12+, as documented in the commit message. The duplicate finding in wave 202 (9cc0de6f…) is answered by this reply.fenliu/tests/test_ai_pipeline.py✅ Fixed in commit
5af9684: the test now patches onlyhttpx2.AsyncClientdirectly; the rest of the module stays intact.Release-Notes.md✅ Fixed in commit
5af9684: the bullet now usesmedia_proxy.WuMing
Found 2 issue(s). See inline comments below.
@ -10,3 +10,4 @@---Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file — and reads the image type from the final URL, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always falling back to image/jpeg.docs [LOW]
The MIME literal
image/jpegshould be wrapped in backticks for consistency with the inline-code style used formedia_proxyearlier in the same release note.docs [LOW]
The em dash in "real file — and reads" separates two parallel actions awkwardly. Consider wording it as "real file and reads the image type" or splitting into two sentences for clearer prose.
Release-Notes.md✅ Both fixed in commit
f25f2b3:image/jpegis now inline code and the sentence reads 'follows the redirect to the real file and reads the image type from the final URL'.WuMing
Found 1 issue(s). See inline comments below.
@ -10,3 +10,4 @@---Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file and reads the image type from the final URL, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always falling back to `image/jpeg`.docs [LOW]
The wording "reads the image type from the final URL" overstates the implementation; the fix guesses/derives the MIME type from the final URL's file extension and falls back to image/jpeg when no extension is recognized. For accuracy, consider wording like "infers the image type from the final URL's file extension".
Release-Notes.md✅ Fixed in commit
30fa687: the note now says it 'infers the image type from the final URL's file extension'.WuMing
Found 1 issue(s). See inline comments below.
@ -5,3 +5,3 @@fenliu 3.0.0-- Fixed: AI vision now follows `media_proxy` redirects, so proxy-served attachments get classified instead of stalling as pending.docs [LOW]
Minor clarity/subject mismatch: “proxy-served attachments” cannot “stall as pending”; it is the post or its review status that remains pending. Consider rephrasing to something like “so posts with proxy-served attachments get classified instead of remaining pending.”
Release-Notes.md✅ Fixed in commit
ea42a15: the bullet now reads 'so posts with proxy-served attachments get classified instead of remaining pending'.