Skip to content

Commit 69521df

Browse files
FlyM1ssclaude
andcommitted
perf(security): skip non-text subresources + fetch via per-page cache in guards
Route guards now abort image/media/font requests (never needed for inner_text) and fetch the rest through the per-page _PinnedSessionCache (DNS + keep-alive) instead of building a fresh session per subresource. Per-hop re-validation and IP-pinning unchanged. Route-guard tests migrated to the cache seam; skip tests added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f43877e commit 69521df

2 files changed

Lines changed: 99 additions & 48 deletions

File tree

Main/backend/datascraper/ssrf_guard.py

Lines changed: 52 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,17 @@ def _should_proxy(route) -> bool:
409409
return urlparse(request.url).scheme in _ALLOWED_SCHEMES
410410

411411

412+
def _should_skip_resource(route) -> bool:
413+
"""True for in-browser subresource types we never need for text extraction
414+
(image/media/font by default — see ``_SKIP_RESOURCE_TYPES``). Aborting them
415+
cuts DNS+TLS work and egress with no effect on ``page.inner_text``; aborting
416+
more requests is also strictly less egress, so there is no SSRF downside."""
417+
try:
418+
return route.request.resource_type in _SKIP_RESOURCE_TYPES
419+
except Exception:
420+
return False
421+
422+
412423
def _forward_headers(route) -> dict:
413424
"""The browser request's headers to replay through ``safe_get``, minus the
414425
framing/encoding headers requests must control (see
@@ -426,20 +437,20 @@ def _forward_headers(route) -> dict:
426437
}
427438

428439

429-
def _proxied_response_kwargs(request_url: str, headers: dict):
430-
"""Fetch an intercepted in-browser GET through the IP-pinned, byte-capped
431-
``safe_get`` and return the kwargs to fulfill the Playwright route with, or
432-
``None`` to fail closed (abort). Blocking — the async guard dispatches it via
433-
``asyncio.to_thread``. Both route guards share this one fail-closed policy
434-
(which exceptions abort, what gets logged, which headers fulfill) so the sync
435-
and async paths cannot drift. ``headers`` are the browser's own request
436-
headers (UA/Accept/...) so the IP-pinned fetch stays indistinguishable to the
437-
origin."""
440+
def _proxied_response_kwargs(cache: "_PinnedSessionCache", request_url: str, headers: dict):
441+
"""Fetch an intercepted in-browser GET through the per-page IP-pinned,
442+
byte-capped ``cache`` and return the kwargs to fulfill the Playwright route
443+
with, or ``None`` to fail closed (abort). Blocking — the async guard
444+
dispatches it via ``asyncio.to_thread``. Both route guards share this one
445+
fail-closed policy (which exceptions abort, what gets logged, which headers
446+
fulfill) so the sync and async paths cannot drift. ``headers`` are the
447+
browser's own request headers (UA/Accept/...) so the IP-pinned fetch stays
448+
indistinguishable to the origin."""
438449
try:
439-
# safe_get validates, pins to the resolved IP, follows redirects
440-
# re-validating each, and byte-caps the body — raising UnsafeURLError on
441-
# any violation.
442-
response = safe_get(request_url, headers=headers)
450+
# cache.fetch validates, pins to the resolved IP (reusing a per-host
451+
# keep-alive session), follows redirects re-validating each, and byte-caps
452+
# the body — raising UnsafeURLError on any violation.
453+
response = cache.fetch(request_url, headers=headers)
443454
except UnsafeURLError as exc:
444455
logger.warning(
445456
"[ssrf_guard] aborting in-browser request to %s: %s",
@@ -464,24 +475,30 @@ def _proxied_response_kwargs(request_url: str, headers: dict):
464475
async def install_route_guard(page) -> None:
465476
"""Register an async Playwright route handler on ALL URLs that fetches each
466477
intercepted in-browser request (top-level navigation OR subresource) through
467-
the IP-pinned, byte-capped ``safe_get`` and fulfills Chromium with the
468-
buffered response. For these HTTP(S) requests Chromium therefore never opens
469-
its own socket and cannot be redirected to a private address by a
470-
DNS-rebinding answer.
471-
472-
MUST be called BEFORE the first ``page.goto`` in every Playwright entrypoint
473-
so EVERY navigation/subresource is pinned, not just the seed URL. Non-GET and
474-
non-http(s) requests fail closed (aborted). WebSocket/WebRTC are not routed
475-
through ``page.route`` and are out of scope (see module docstring)."""
478+
a per-page IP-pinned, byte-capped, keep-alive cache and fulfills Chromium with
479+
the buffered response. For these HTTP(S) requests Chromium therefore never
480+
opens its own socket and cannot be redirected to a private address by a
481+
DNS-rebinding answer. image/media/font requests are aborted outright (they
482+
never feed text extraction); non-GET and non-http(s) requests fail closed.
483+
484+
MUST be called BEFORE the first ``page.goto`` so EVERY navigation/subresource
485+
is pinned, not just the seed URL. Installed centrally by the PlaywrightBrowser
486+
factory. WebSocket/WebRTC are not routed through ``page.route`` and are out of
487+
scope (see module docstring)."""
488+
cache = _PinnedSessionCache()
489+
page.on("close", lambda *_: cache.close())
476490

477491
async def _handler(route):
492+
if _should_skip_resource(route):
493+
await route.abort()
494+
return
478495
if not _should_proxy(route):
479496
await route.abort()
480497
return
481-
# _proxied_response_kwargs is blocking (it calls safe_get); run it off the
482-
# event loop. route.request.* is read here on the loop before dispatch.
498+
# _proxied_response_kwargs is blocking (it calls cache.fetch); run it off
499+
# the event loop. route.request.* is read here on the loop before dispatch.
483500
kwargs = await asyncio.to_thread(
484-
_proxied_response_kwargs, route.request.url, _forward_headers(route)
501+
_proxied_response_kwargs, cache, route.request.url, _forward_headers(route)
485502
)
486503
if kwargs is None: # fetch was unsafe or failed — fail closed.
487504
await route.abort()
@@ -493,17 +510,23 @@ async def _handler(route):
493510

494511
def install_route_guard_sync(page) -> None:
495512
"""Synchronous twin of :func:`install_route_guard` for the sync Playwright
496-
fallback (``url_tools.scrape_with_playwright``). Same guarantee: every
497-
in-browser GET is fulfilled from the IP-pinned ``safe_get`` so Chromium never
498-
re-resolves DNS on its own socket; non-GET / non-http(s) fail closed.
513+
fallback (``url_tools.scrape_with_playwright``). Same guarantees: every
514+
in-browser GET is fulfilled from a per-page IP-pinned, keep-alive cache so
515+
Chromium never re-resolves DNS on its own socket; image/media/font are
516+
aborted outright; non-GET / non-http(s) fail closed.
499517
500518
MUST be called BEFORE the first ``page.goto``."""
519+
cache = _PinnedSessionCache()
520+
page.on("close", lambda *_: cache.close())
501521

502522
def _handler(route):
523+
if _should_skip_resource(route):
524+
route.abort()
525+
return
503526
if not _should_proxy(route):
504527
route.abort()
505528
return
506-
kwargs = _proxied_response_kwargs(route.request.url, _forward_headers(route))
529+
kwargs = _proxied_response_kwargs(cache, route.request.url, _forward_headers(route))
507530
if kwargs is None: # fetch was unsafe or failed — fail closed.
508531
route.abort()
509532
return

Main/backend/tests/test_ssrf_guard.py

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -189,10 +189,11 @@ async def fake_route(pattern, handler):
189189
"Host": "example.com",
190190
}
191191

192-
def _route(self, url, method="GET"):
192+
def _route(self, url, method="GET", resource_type="document"):
193193
route = MagicMock()
194194
route.request.url = url
195195
route.request.method = method
196+
route.request.resource_type = resource_type
196197
route.request.headers = dict(self._BROWSER_HEADERS)
197198
route.abort = AsyncMock()
198199
route.fulfill = AsyncMock()
@@ -209,8 +210,9 @@ async def run():
209210

210211
asyncio.run(run())
211212

212-
@patch("datascraper.ssrf_guard.safe_get")
213-
def test_route_guard_aborts_blocked_request(self, m_get):
213+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
214+
def test_route_guard_aborts_blocked_request(self, m_cache):
215+
m_get = m_cache.return_value.fetch
214216
m_get.side_effect = UnsafeURLError("blocked")
215217
route = self._route("http://evil.example.test/x")
216218
self._drive(route)
@@ -219,8 +221,9 @@ def test_route_guard_aborts_blocked_request(self, m_get):
219221
# Never delegate the fetch back to Chromium (would re-resolve DNS).
220222
route.continue_.assert_not_awaited()
221223

222-
@patch("datascraper.ssrf_guard.safe_get")
223-
def test_route_guard_fulfills_public_request_from_pinned_fetch(self, m_get):
224+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
225+
def test_route_guard_fulfills_public_request_from_pinned_fetch(self, m_cache):
226+
m_get = m_cache.return_value.fetch
224227
resp = _FakeResp(
225228
status_code=200,
226229
headers={"Content-Type": "text/html", "Content-Encoding": "gzip"},
@@ -243,8 +246,9 @@ def test_route_guard_fulfills_public_request_from_pinned_fetch(self, m_get):
243246
m_get.assert_called_once()
244247
self.assertEqual(m_get.call_args.args[0], "http://example.com/x")
245248

246-
@patch("datascraper.ssrf_guard.safe_get")
247-
def test_route_guard_forwards_browser_headers_to_pinned_fetch(self, m_get):
249+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
250+
def test_route_guard_forwards_browser_headers_to_pinned_fetch(self, m_cache):
251+
m_get = m_cache.return_value.fetch
248252
# The pinned fetch must present the browser's own User-Agent (the
249253
# context deliberately sets a Chrome UA to avoid bot-gating); SSRF
250254
# safety comes from IP-pinning, not from hiding the UA. Framing/encoding
@@ -264,15 +268,26 @@ def test_route_guard_forwards_browser_headers_to_pinned_fetch(self, m_get):
264268
self.assertNotIn("host", lowered)
265269
self.assertNotIn("accept-encoding", lowered)
266270

267-
@patch("datascraper.ssrf_guard.safe_get")
268-
def test_route_guard_aborts_non_get(self, m_get):
269-
# safe_get is GET-only; non-GET in-browser requests fail closed.
271+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
272+
def test_route_guard_aborts_non_get(self, m_cache):
273+
m_get = m_cache.return_value.fetch
274+
# cache.fetch is GET-only; non-GET in-browser requests fail closed.
270275
route = self._route("http://example.com/api", method="POST")
271276
self._drive(route)
272277
route.abort.assert_awaited_once()
273278
route.fulfill.assert_not_awaited()
274279
m_get.assert_not_called()
275280

281+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
282+
def test_route_guard_aborts_skipped_resource(self, m_cache):
283+
# image/media/font are aborted before any fetch — they don't feed
284+
# inner_text, so we never spend DNS+TLS or egress on them.
285+
route = self._route("http://example.com/logo.png", resource_type="image")
286+
self._drive(route)
287+
route.abort.assert_awaited_once()
288+
route.fulfill.assert_not_awaited()
289+
m_cache.return_value.fetch.assert_not_called()
290+
276291

277292
class SyncRouteGuardTests(SimpleTestCase):
278293
"""install_route_guard_sync mirrors the async guard for the sync
@@ -287,10 +302,11 @@ def route(pattern, handler):
287302
page.route = route
288303
return page
289304

290-
def _route(self, url, method="GET"):
305+
def _route(self, url, method="GET", resource_type="document"):
291306
route = MagicMock()
292307
route.request.url = url
293308
route.request.method = method
309+
route.request.resource_type = resource_type
294310
route.request.headers = {
295311
"User-Agent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64) Chrome/120.0.0.0",
296312
"Accept-Encoding": "gzip, deflate, br",
@@ -304,16 +320,18 @@ def _drive(self, route):
304320
ssrf_guard.install_route_guard_sync(page)
305321
captured["handler"](route)
306322

307-
@patch("datascraper.ssrf_guard.safe_get")
308-
def test_sync_guard_aborts_blocked_request(self, m_get):
323+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
324+
def test_sync_guard_aborts_blocked_request(self, m_cache):
325+
m_get = m_cache.return_value.fetch
309326
m_get.side_effect = UnsafeURLError("blocked")
310327
route = self._route("http://evil.example.test/x")
311328
self._drive(route)
312329
route.abort.assert_called_once()
313330
route.fulfill.assert_not_called()
314331

315-
@patch("datascraper.ssrf_guard.safe_get")
316-
def test_sync_guard_fulfills_public_request(self, m_get):
332+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
333+
def test_sync_guard_fulfills_public_request(self, m_cache):
334+
m_get = m_cache.return_value.fetch
317335
resp = _FakeResp(status_code=200, headers={"Content-Type": "text/html"})
318336
resp._content = b"hi"
319337
m_get.return_value = resp
@@ -323,8 +341,9 @@ def test_sync_guard_fulfills_public_request(self, m_get):
323341
self.assertEqual(route.fulfill.call_args.kwargs["body"], b"hi")
324342
route.abort.assert_not_called()
325343

326-
@patch("datascraper.ssrf_guard.safe_get")
327-
def test_sync_guard_forwards_browser_user_agent(self, m_get):
344+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
345+
def test_sync_guard_forwards_browser_user_agent(self, m_cache):
346+
m_get = m_cache.return_value.fetch
328347
resp = _FakeResp(status_code=200, headers={"Content-Type": "text/html"})
329348
resp._content = b"hi"
330349
m_get.return_value = resp
@@ -337,13 +356,22 @@ def test_sync_guard_forwards_browser_user_agent(self, m_get):
337356
self.assertNotIn("host", lowered)
338357
self.assertNotIn("accept-encoding", lowered)
339358

340-
@patch("datascraper.ssrf_guard.safe_get")
341-
def test_sync_guard_aborts_non_get(self, m_get):
359+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
360+
def test_sync_guard_aborts_non_get(self, m_cache):
361+
m_get = m_cache.return_value.fetch
342362
route = self._route("http://example.com/api", method="POST")
343363
self._drive(route)
344364
route.abort.assert_called_once()
345365
m_get.assert_not_called()
346366

367+
@patch("datascraper.ssrf_guard._PinnedSessionCache")
368+
def test_sync_guard_aborts_skipped_resource(self, m_cache):
369+
route = self._route("http://example.com/font.woff2", resource_type="font")
370+
self._drive(route)
371+
route.abort.assert_called_once()
372+
route.fulfill.assert_not_called()
373+
m_cache.return_value.fetch.assert_not_called()
374+
347375

348376
class PinnedSessionCacheTests(SimpleTestCase):
349377
"""The per-page cache reuses validated DNS + keep-alive sessions per host,

0 commit comments

Comments
 (0)