Enable PDF scraping through the Docker API - #2150
Conversation
|
Thanks @SohamKukreti — reviewed alongside #2130 and verified both locally. Gating on the scraping strategy is the right trigger. Merging #2130 first. This branch doesn't contain it, so it needs a rebase. Two things for that pass: 1. The test conflict you flagged is real. Stacked on current 2. The SSRF guard misses the Minor, not blocking: the stream handler's |
…trategy (#2127) Headless Chromium can't render PDFs inline, so pair the client's PDF scraping strategy with a per-request PDFCrawlerStrategy crawler instead of a pooled browser, as the library documents.
Follow redirects manually in PDFContentScrapingStrategy so the Docker server's validate_url_destination vets the download URL and every hop before fetch, blocking redirects to internal addresses.
…rs (#2127) Vet PDF strategies sent via crawler_configs, close PDF crawlers on stream error paths, and fix the stream test to spy on AsyncWebCrawler instead of the pool.
6f98a6c to
2341507
Compare
|
@ntohidi I have rebased my commits and made all the changes you mentioned. |
ntohidi
left a comment
There was a problem hiding this comment.
Verified 2341507 locally — all three addressed.
Rebase is in (branch contains #2130) and test_stream_handler_preserves_requested_scraping_strategy passes now; spying on AsyncWebCrawler instead of the pool is the right fix, since a PDF crawl never goes through the pool. 8/8 across both PDF test files.
On the SSRF gap I checked the behaviour rather than the diff: sending a PDF strategy via crawler_configs and inspecting what reaches arun_many, url_validator is None on 4138ada and validate_url_destination on 2341507. Closed.
_dispose_crawler is nicer than what I asked for — one helper across the finally and all three except blocks, and I confirmed a PDF crawler gets close() while a pooled one is released and never closed.
One follow-up, not blocking: nothing in the suite covers the per-URL crawler_configs path, so that guard is unprotected against future refactors. I'll send a regression test for it separately.
Merging. Thanks!
handle_crawl_request builds the per-URL config list on a separate branch from the top-level config, so the url_validator wiring has to be repeated there. unclecode#2150 fixed that, but nothing exercised it — the existing pairing tests only go through the top-level path, leaving the guard free to be dropped by a future refactor without a red test. Verified by mutation: removing the guard from api.py fails this test and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
unclecode#2150 switched the PDF download to allow_redirects=False and followed hops by hand so url_validator can vet each one before it is fetched. Each hop used a bare requests.get(), which starts with an empty cookie jar, so a host that sets a cookie and then redirects never gets its own cookie back and answers 403. That is the normal shape for gated and CDN-signed PDFs, and it worked before unclecode#2150 because allow_redirects=True carried cookies implicitly. Use one requests.Session for the chain. The SSRF guarantee is unchanged: hops are still validated before the fetch and still not auto-followed. Verified by mutation: reverting session.get to requests.get fails the new test and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Depends on #2130 - needs pypdf in the image and the client's
scraping_strategyhonored on/crawl/stream.Fixes the remaining blocker for PDF scraping through the Docker API (#2127). Even with PR: #2130, every PDF crawl still failed at browser navigation with
Page.goto: Download is starting, because the server always used the pooled Playwright crawler and headless Chromium can't render PDFs inline.When a request's
scraping_strategyisPDFContentScrapingStrategy, both crawl handlers now build a per-requestAsyncWebCrawler(crawler_strategy=PDFCrawlerStrategy())- the exact pairing the library documents - instead of borrowing a pooled browser.PDFCrawlerStrategystarts no browser (it returns a placeholder and the scraping strategy downloads/parses the PDF itself), so a per-request instance is cheap and is closed after use.This is gated on the scraping strategy, mirroring the library, rather than sniffing the URL or Content-Type.
Also included
400instead ofAttributeError: 'PDFCrawlerStrategy' object has no attribute 'set_hook'→500.JSONResponsecan serialize/crawlresults (gated to PDF requests, so normal crawls pay no cost).PDFContentScrapingStrategydownloads the PDF itself viarequests, which previously followed redirects blindly. It now follows redirects manually (cap 10) and, when the server passes itsvalidate_url_destinationin as aurl_validator, vets the download URL and every redirect hop before it is fetched — closing a redirect-to-internal-address (e.g. cloud metadata) SSRF vector. Theurl_validatorparam defaults toNone, so library users are unaffected.List of files changed and why
deploy/docker/api.py— route PDF-strategy requests toPDFCrawlerStrategy, guard hooks, close the per-request crawler, sanitize datetimes, wire the SSRF validator.crawl4ai/processors/pdf/__init__.py— optionalurl_validatorhook and manual per-hop redirect handling inPDFContentScrapingStrategy._get_pdf_path;MAX_PDF_DOWNLOAD_REDIRECTSconstant.tests/test_docker_pdf_crawler_pairing.py— handler-level routing/hooks/pool tests and library-level redirect/validator tests.How Has This Been Tested?
Image built from
develop+ #2130 + this branch:/crawland/crawl/stream.169.254.169.254is vetted and rejected before fetch (verified with the realvalidate_url_destination+ the redirect loop); a direct internal seed is still400at the seed layer.LXMLWebScrapingStrategy, multi-URL, streaming, screenshot,/md, and hooks-on-HTML all behave as before; repeated PDF crawls leave no lingering crawlers in the pool.tests/test_docker_pdf_crawler_pairing.py(6 tests) pass; each fix verified to fail with its change reverted.293 passed(one pre-existing unrelatedtransformers-missing failure).Note for after #2130 merges
Its
test_stream_handler_preserves_requested_scraping_strategymockscrawler_pool.get_crawler, which PDF requests no longer call (they build aPDFCrawlerStrategycrawler directly). The test uses a PDF strategy, so after this merge the mock is never hit and itsarun_manyassertion fails onawait_args is None. The fix is small — spy onapi.AsyncWebCrawler(or otherwise intercept the PDF-branch crawler) instead of mocking the pool. Happy to include the adjustment on rebase.Checklist: