Skip to content

Commit cb2815f

Browse files
ZingzyZingzy
committed
fix(metadata): read to the end of the head, not a fixed prefix
The fetch stopped at 512KB on the assumption that meta tags sit early in the document. Youtube puts roughly 700KB of inline JSON in its head before them, so the checker read past the favicon, stopped short of every other tag, and reported a page with nine missing tags rather than one it could not finish reading. The read now ends at </head>, with the byte cap kept as a backstop and raised to a megabyte. Stopping at the marker makes the common case cheaper than before, since most heads close long before either limit: github at 32KB against 456KB of document, nytimes at 190KB against 1.1MB. Verified against the real pages. Youtube returns 14 og and 18 twitter tags where it previously returned none.
1 parent 12271cf commit cb2815f

3 files changed

Lines changed: 94 additions & 8 deletions

File tree

‎infrastructure/safe_fetch.py‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,21 @@ def _bracket(ip: str) -> str:
120120

121121

122122
async def _read_body(
123-
resp: httpx.Response, max_bytes: int, truncate_over_cap: bool
123+
resp: httpx.Response,
124+
max_bytes: int,
125+
truncate_over_cap: bool,
126+
stop_after: bytes | None = None,
124127
) -> bytearray:
125128
buf = bytearray()
129+
searched = 0
126130
async for chunk in resp.aiter_bytes():
127131
buf += chunk
132+
if stop_after is not None:
133+
# Resume one marker-width back: it can straddle two chunks.
134+
found = buf.find(stop_after, max(0, searched - len(stop_after)))
135+
if found != -1:
136+
return buf[: found + len(stop_after)]
137+
searched = len(buf)
128138
if len(buf) > max_bytes:
129139
if truncate_over_cap:
130140
return buf[:max_bytes]
@@ -141,16 +151,20 @@ async def fetch_public(
141151
max_bytes: int = 1_048_576,
142152
max_redirects: int = 3,
143153
truncate_over_cap: bool = False,
154+
stop_after: bytes | None = None,
144155
user_agent: str = DEFAULT_USER_AGENT,
145156
) -> FetchedBody:
146157
"""Fetch *url* with SSRF guards. ``accept_content`` are content-type
147158
prefixes (e.g. ``("image/",)``); ``reject_content`` are substrings that
148159
fail even when a prefix matched (e.g. ``("svg",)``).
149160
150161
``truncate_over_cap=True`` returns the first ``max_bytes`` instead of
151-
failing when the body exceeds the cap — right for HTML meta parsing
152-
(tags live in <head>; github.com's homepage alone is >512KB), wrong
153-
for images (a truncated image is not a valid image)."""
162+
failing when the body exceeds the cap, which is right for HTML meta
163+
parsing and wrong for images (a truncated image is not a valid image).
164+
165+
``stop_after`` ends the read at a marker, so an HTML caller pays for
166+
the head rather than the cap. Heads are not reliably small: youtube
167+
puts ~700KB of inline JSON before its meta tags."""
154168
# httpx timeouts are per-operation and reset each chunk, so the body
155169
# read below is bounded by an explicit wall-clock ceiling instead.
156170
hop_deadline = timeout * 3
@@ -214,7 +228,7 @@ async def fetch_public(
214228
# wall-clock ceiling a slow-drip server can't evade.
215229
try:
216230
buf = await asyncio.wait_for(
217-
_read_body(resp, max_bytes, truncate_over_cap),
231+
_read_body(resp, max_bytes, truncate_over_cap, stop_after),
218232
timeout=hop_deadline,
219233
)
220234
except (asyncio.TimeoutError, TimeoutError) as exc:

‎routes/api_v1/metadata.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,10 @@
3939

4040
router = APIRouter(tags=["Metadata"])
4141

42-
_FETCH_MAX_BYTES = 524_288 # tags must be in the head; 512KB is generous
42+
# Heads are not reliably small (youtube: ~700KB before its meta tags),
43+
# so the read stops at </head> and this is only the backstop.
44+
_FETCH_MAX_BYTES = 1_048_576
45+
_HEAD_END = b"</head>"
4346
_FETCH_TIMEOUT = 5.0
4447

4548
_metadata_limit, _metadata_key = dynamic_limit(
@@ -112,8 +115,8 @@ async def get_metadata(
112115
accept_content=("text/html", "application/xhtml"),
113116
timeout=_FETCH_TIMEOUT,
114117
max_bytes=_FETCH_MAX_BYTES,
115-
# Big pages are fine — tags live in <head>, parse the prefix.
116118
truncate_over_cap=True,
119+
stop_after=_HEAD_END,
117120
user_agent=settings.meta_tags.fetch_user_agent,
118121
)
119122
except FetchTransientError as exc:

‎tests/unit/infrastructure/test_safe_fetch.py‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@
22

33
from __future__ import annotations
44

5-
from unittest.mock import AsyncMock, patch
5+
from unittest.mock import AsyncMock, MagicMock, patch
66

77
import pytest
88

99
from infrastructure.safe_fetch import (
1010
FetchHardError,
1111
_is_public,
12+
_read_body,
1213
fetch_public_image,
1314
resolve_public_ip,
1415
)
@@ -129,3 +130,71 @@ async def _fake_send(self, request, **kwargs):
129130
):
130131
await fetch_public_image("https://example.com/a.png")
131132
assert captured["accept_encoding"] == "identity"
133+
134+
135+
class TestReadBodyStopMarker:
136+
"""``stop_after`` ends the read at </head> so an HTML caller pays for the
137+
head, not the cap. Heads are not reliably small: youtube puts ~700KB of
138+
inline JSON before its meta tags, which a 512KB cap silently cut off."""
139+
140+
@staticmethod
141+
def _resp(chunks):
142+
resp = MagicMock()
143+
144+
async def aiter_bytes():
145+
for chunk in chunks:
146+
yield chunk
147+
148+
resp.aiter_bytes = aiter_bytes
149+
return resp
150+
151+
@pytest.mark.asyncio
152+
async def test_read_stops_at_the_marker(self):
153+
body = await _read_body(
154+
self._resp([b"<head><title>x</title></head>", b"<body>" + b"z" * 5000]),
155+
max_bytes=1_000_000,
156+
truncate_over_cap=True,
157+
stop_after=b"</head>",
158+
)
159+
assert bytes(body) == b"<head><title>x</title></head>"
160+
161+
@pytest.mark.asyncio
162+
async def test_marker_split_across_chunks_is_still_found(self):
163+
body = await _read_body(
164+
self._resp([b"<head>a</he", b"ad><body>ignored"]),
165+
max_bytes=1_000_000,
166+
truncate_over_cap=True,
167+
stop_after=b"</head>",
168+
)
169+
assert bytes(body) == b"<head>a</head>"
170+
171+
@pytest.mark.asyncio
172+
async def test_tags_past_the_old_512kb_cap_survive(self):
173+
"""The youtube shape: a huge head, then the tags."""
174+
head = b"<head>" + b"j" * 700_000 + b'<meta property="og:title"></head>'
175+
body = await _read_body(
176+
self._resp([head, b"<body>" + b"z" * 600_000]),
177+
max_bytes=1_048_576,
178+
truncate_over_cap=True,
179+
stop_after=b"</head>",
180+
)
181+
assert b"og:title" in bytes(body)
182+
183+
@pytest.mark.asyncio
184+
async def test_a_missing_marker_still_honours_the_cap(self):
185+
body = await _read_body(
186+
self._resp([b"x" * 900, b"y" * 900]),
187+
max_bytes=1_000,
188+
truncate_over_cap=True,
189+
stop_after=b"</head>",
190+
)
191+
assert len(body) == 1_000
192+
193+
@pytest.mark.asyncio
194+
async def test_without_a_marker_the_whole_body_is_read(self):
195+
body = await _read_body(
196+
self._resp([b"<head></head>", b"<body>tail"]),
197+
max_bytes=1_000,
198+
truncate_over_cap=True,
199+
)
200+
assert bytes(body) == b"<head></head><body>tail"

0 commit comments

Comments
 (0)