From 3bfedc0d83d82897e48c4ea37d3b032151276cfc Mon Sep 17 00:00:00 2001 From: HC-ONLINE Date: Fri, 16 Oct 2026 12:59:48 -0500 Subject: [PATCH] fix(scrape): improve error handling and response propagation for scraping failures --- docs/API.md | 22 ++ src/ciberwebscan/api/routes/scrape.py | 4 + src/ciberwebscan/cli/commands/scrape.py | 3 + src/ciberwebscan/services/scrape_service.py | 41 +++- tests/unit/api/routes/test_scrape_routes.py | 86 +++++++- tests/unit/cli/test_commands.py | 85 ++++++- tests/unit/services/test_quick_service.py | 53 ++++- tests/unit/services/test_scrape_service.py | 232 ++++++++++++++++++++ 8 files changed, 516 insertions(+), 10 deletions(-) diff --git a/docs/API.md b/docs/API.md index 49e96dc..9b26c02 100644 --- a/docs/API.md +++ b/docs/API.md @@ -252,6 +252,21 @@ Scrape a single URL with flexible extraction options. } ``` +**Error behavior:** + +Real scraping failures (HTTP 4xx/5xx responses, connection errors, +robots.txt blocks, browser timeouts) are returned as +`500 Internal Server Error` — never as HTTP 200 with +`success: true`. The `detail` message carries the original failure +reason (for example `HTTP 404`). + +| Scenario | Status | `detail` | +| ------------------------------------------- | ------ | --------------------------------- | +| Scraping failure (404, connection, robots) | 500 | Original reason (e.g. `HTTP 404`) | +| SSRF-protected target (metadata/private IP) | 500 | SSRF protection reason | +| Malformed request body | 422 | FastAPI validation error | +| Missing or invalid API key | 401 | Authentication error | + #### POST /api/scrape/batch Scrape multiple URLs in batch. @@ -296,6 +311,13 @@ Scrape multiple URLs in batch. } ``` +**Batch error behavior:** + +- Mixed batches (some URLs succeed, some fail) return HTTP 200 with + `success: true`; failed URLs are listed individually in `failed_urls`. +- If **every** URL fails, the endpoint returns HTTP 500 and `detail` + summarizes the per-URL errors. + ### Attack Simulation #### POST /api/attack diff --git a/src/ciberwebscan/api/routes/scrape.py b/src/ciberwebscan/api/routes/scrape.py index f042bd8..5b2823b 100644 --- a/src/ciberwebscan/api/routes/scrape.py +++ b/src/ciberwebscan/api/routes/scrape.py @@ -95,6 +95,8 @@ def scrape_url( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid request: {e}", ) from e + except HTTPException: + raise except Exception as e: logger.error(f"Error scraping URL {request.url}: {e}", exc_info=True) raise HTTPException( @@ -193,6 +195,8 @@ def scrape_batch( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Invalid request: {e}", ) from e + except HTTPException: + raise except Exception as e: logger.error(f"Error in batch scraping: {e}", exc_info=True) raise HTTPException( diff --git a/src/ciberwebscan/cli/commands/scrape.py b/src/ciberwebscan/cli/commands/scrape.py index 542099b..199891c 100644 --- a/src/ciberwebscan/cli/commands/scrape.py +++ b/src/ciberwebscan/cli/commands/scrape.py @@ -17,6 +17,7 @@ print_error, print_header, print_info, + print_warning, ) from ciberwebscan.cli.validators import ( ValidationError, @@ -386,6 +387,8 @@ def scrape_batch( print_info(f" [{i}] {r.url} - {r.status_code}") if len(result.data) > 5: print_info(f" ... and {len(result.data) - 5} more") + for warning in result.warnings: + print_warning(warning) exit_code = format_service_result(result, json_output=False) sys.exit(exit_code) diff --git a/src/ciberwebscan/services/scrape_service.py b/src/ciberwebscan/services/scrape_service.py index 456b30a..21da8e3 100644 --- a/src/ciberwebscan/services/scrape_service.py +++ b/src/ciberwebscan/services/scrape_service.py @@ -345,15 +345,19 @@ def scrape_multiple( ) single_result = self.scrape(opts) - if single_result.success and single_result.data: - if isinstance(single_result.data, ScrapeResult): - results.append(single_result.data) - else: - errors.append(f"{url}: {single_result.error}") - result.warnings.append(f"Failed to scrape {url}") + if not single_result.success: + reason = single_result.error or "Scraping failed" + errors.append(f"{url}: {reason}") + result.warnings.append(f"Failed to scrape {url}: {reason}") + elif isinstance(single_result.data, ScrapeResult): + results.append(single_result.data) result.data = results - result.success = len(results) > 0 + if results: + result.success = True + elif errors: + result.error = "; ".join(errors) + result.error_code = "SCRAPE_ERROR" # Export all results if requested if options.export and results: @@ -395,6 +399,15 @@ def _scrape_static(self, url: str, options: ScrapeOptions) -> ScrapeResult: cookies=options.cookies or None, ).scrape(url, config) + if not core_result.success: + details: dict[str, Any] = {"url": url} + if core_result.status_code is not None: + details["status_code"] = core_result.status_code + raise ExecutionError( + core_result.error or "Static scraping failed", + details=details, + ) + # Map core ScrapeResult to export ScrapeResult from ciberwebscan.export.models import ( FormInfo, @@ -442,6 +455,8 @@ def _scrape_static(self, url: str, options: ScrapeOptions) -> ScrapeResult: elapsed_ms=(core_result.elapsed_time or 0.0) * 1000, ) + except ExecutionError: + raise except Exception as e: raise ExecutionError(f"Static scraping failed: {e}") from e @@ -465,6 +480,16 @@ def _scrape_dynamic(self, url: str, options: ScrapeOptions) -> ScrapeResult: # Run the async scrape method safely from sync context core_result = run_async(self.dynamic_scraper.scrape(url, config)) + if not getattr(core_result, "success", True): + details: dict[str, Any] = {"url": url} + status_code = getattr(core_result, "status_code", None) + if status_code is not None: + details["status_code"] = status_code + raise ExecutionError( + getattr(core_result, "error", None) or "Dynamic scraping failed", + details=details, + ) + # dynamic scraper returns DynamicScrapeResult-like dataclass from ciberwebscan.export.models import ( FormInfo, @@ -513,6 +538,8 @@ def _scrape_dynamic(self, url: str, options: ScrapeOptions) -> ScrapeResult: elapsed_ms=(getattr(core_result, "elapsed_time", 0.0) or 0.0) * 1000, ) + except ExecutionError: + raise except Exception as e: raise ExecutionError(f"Dynamic scraping failed: {e}") from e diff --git a/tests/unit/api/routes/test_scrape_routes.py b/tests/unit/api/routes/test_scrape_routes.py index 3f379a8..b287826 100644 --- a/tests/unit/api/routes/test_scrape_routes.py +++ b/tests/unit/api/routes/test_scrape_routes.py @@ -4,7 +4,7 @@ from __future__ import annotations -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, Mock, patch import pytest from fastapi.testclient import TestClient @@ -160,3 +160,87 @@ def test_post_scrape_batch_service_failure_returns_500(self, client: TestClient) def test_post_scrape_batch_empty_urls_returns_422(self, client: TestClient): response = client.post("/api/scrape/batch", json={"urls": []}) assert response.status_code == 422 + + +def _mock_http_response( + status: int, text: str = "ok" +) -> Mock: + """Build a minimal HTTP response stub for the real StaticScraper.""" + response = Mock() + response.status_code = status + response.headers = {"content-type": "text/html"} + response.text = text + response.cookies.items.return_value = {} + return response + + +class TestScrapeFailurePropagation: + """Real scrape failures must surface as HTTP 500, never HTTP 200. + + These tests run the real ``ScrapeService`` (only the HTTP client is + mocked) to regression-test BUG-005 end to end through the API layer. + """ + + def test_real_scrape_failure_returns_500_with_clean_detail( + self, client: TestClient + ): + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response( + 404, "Not Found" + ) + response = client.post( + "/api/scrape", + json={ + "url": "https://example.com/missing", + "check_robots": False, + }, + ) + + assert response.status_code == 500 + detail = response.json()["detail"] + assert "404" in detail + assert "Scraping failed: 500" not in detail + + def test_real_batch_mixed_returns_partial_success(self, client: TestClient): + ok_response = _mock_http_response(200) + missing_response = _mock_http_response(404, "Not Found") + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.side_effect = lambda url, **kwargs: ( + ok_response if url.endswith("/ok") else missing_response + ) + response = client.post( + "/api/scrape/batch", + json={ + "urls": [ + "https://example.com/ok", + "https://example.com/missing", + ] + }, + ) + + assert response.status_code == 200 + body = response.json() + assert body["success"] is True + assert body["data"]["total_success"] == 1 + assert body["data"]["total_failed"] == 1 + assert body["data"]["failed_urls"][0]["url"] == "https://example.com/missing" + + def test_real_batch_all_failures_returns_500(self, client: TestClient): + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response( + 404, "Not Found" + ) + response = client.post( + "/api/scrape/batch", + json={ + "urls": [ + "https://example.com/a", + "https://example.com/b", + ] + }, + ) + + assert response.status_code == 500 + detail = response.json()["detail"] + assert "404" in detail + assert "Batch scraping failed: 500" not in detail diff --git a/tests/unit/cli/test_commands.py b/tests/unit/cli/test_commands.py index d6b2c4d..b52f53c 100644 --- a/tests/unit/cli/test_commands.py +++ b/tests/unit/cli/test_commands.py @@ -3,7 +3,7 @@ from __future__ import annotations import re -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, Mock, patch import pytest from typer.testing import CliRunner @@ -20,6 +20,23 @@ def _strip_ansi(text: str) -> str: return re.sub(r"\x1b\[[0-9;]*m", "", text) +def _cli_output(result) -> str: + """Combined stdout/stderr from a CliRunner result (click >= 8.2).""" + return f"{result.output}{result.stderr}" + + +def _mock_http_response( + status: int, text: str = "ok" +) -> Mock: + """Build a minimal HTTP response stub for the real StaticScraper.""" + response = Mock() + response.status_code = status + response.headers = {"content-type": "text/html"} + response.text = text + response.cookies.items.return_value = {} + return response + + class TestAppBasic: """Tests for basic CLI app functionality.""" @@ -92,6 +109,72 @@ def test_scrape_batch_help(self): result = runner.invoke(app, ["scrape", "batch", "--help"]) assert result.exit_code == 0 + def test_scrape_url_http_404_exits_nonzero(self): + """A real HTTP 404 scrape must exit with a non-zero code.""" + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response( + 404, "Not Found" + ) + result = runner.invoke( + app, + [ + "scrape", + "url", + "https://example.com/missing", + "--no-check-robots", + "-q", + ], + ) + + assert result.exit_code == 1 + assert "404" in _strip_ansi(_cli_output(result)) + + def test_scrape_batch_all_failures_exits_nonzero(self): + """A fully failed batch must exit with a non-zero code.""" + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response( + 404, "Not Found" + ) + result = runner.invoke( + app, + [ + "scrape", + "batch", + "https://example.com/a", + "https://example.com/b", + "--no-check-robots", + "-q", + ], + ) + + assert result.exit_code == 1 + assert "404" in _strip_ansi(_cli_output(result)) + + def test_scrape_batch_partial_failure_shows_warnings(self): + """Partial batches surface failed URLs instead of looking all-good.""" + ok_response = _mock_http_response(200) + missing_response = _mock_http_response(404, "Not Found") + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.side_effect = lambda url, **kwargs: ( + ok_response if url.endswith("/ok") else missing_response + ) + result = runner.invoke( + app, + [ + "scrape", + "batch", + "https://example.com/ok", + "https://example.com/missing", + "--no-check-robots", + ], + ) + + assert result.exit_code == 0 + output = _strip_ansi(_cli_output(result)) + assert "1 successful" in output + assert "Failed to scrape" in output + assert "404" in output + class TestAnalyzeCommand: """Tests for analyze command.""" diff --git a/tests/unit/services/test_quick_service.py b/tests/unit/services/test_quick_service.py index 28f91fe..20bdefa 100644 --- a/tests/unit/services/test_quick_service.py +++ b/tests/unit/services/test_quick_service.py @@ -9,7 +9,7 @@ import pytest -from ciberwebscan.export.models import AnalysisReport +from ciberwebscan.export.models import AnalysisReport, ExportMeta from ciberwebscan.services.quick_service import PRESETS, QuickOptions, QuickService pytestmark = pytest.mark.unit @@ -359,3 +359,54 @@ def test_no_export_when_no_output( assert result.success is True assert result.exported is False + + +# ============================================================================= +# Scrape Failure Handling Tests (BUG-005) +# ============================================================================= + + +class TestRunScrapeFailureHandling: + """Failed scrapes must not be incorporated into the quick report.""" + + def test_scrape_failure_sets_warning_not_report( + self, + quick_service: QuickService, + caplog: pytest.LogCaptureFixture, + ): + """A real scrape failure warns with its reason and skips report.scrape.""" + import logging + + from ciberwebscan.services.scrape_service import ScrapeService + + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + response = MagicMock() + response.status_code = 404 + response.headers = {"content-type": "text/html"} + response.text = "Not Found" + response.cookies.items.return_value = {} + mock_http_cls.return_value.get.return_value = response + + service = ScrapeService() + quick_service._scrape_service = service + report = AnalysisReport( + meta=ExportMeta(target_url="https://example.com/missing") + ) + + with caplog.at_level( + logging.WARNING, + logger="ciberwebscan.services.quick_service", + ): + quick_service._run_scrape( + "https://example.com/missing", + QuickOptions(url="https://example.com/missing"), + {"scrape": {"dynamic": False}}, + report, + ) + service.close() + + assert report.scrape is None + log_messages = [record.getMessage() for record in caplog.records] + assert any("Scrape failed" in message for message in log_messages) + assert any("HTTP 404" in message for message in log_messages) + assert not any("Scrape failed: None" in message for message in log_messages) diff --git a/tests/unit/services/test_scrape_service.py b/tests/unit/services/test_scrape_service.py index 69f070e..fa91dbd 100644 --- a/tests/unit/services/test_scrape_service.py +++ b/tests/unit/services/test_scrape_service.py @@ -9,6 +9,7 @@ import pytest +from ciberwebscan.core.scraping.dynamic import DynamicScrapeResult from ciberwebscan.export.models import ScrapeResult from ciberwebscan.services.base import ServiceResult from ciberwebscan.services.scrape_service import ( @@ -55,6 +56,18 @@ def mock_scrape_result() -> ScrapeResult: ) +def _mock_http_response( + status: int, text: str = "ok" +) -> Mock: + """Build a minimal HTTP response stub for StaticScraper.""" + response = Mock() + response.status_code = status + response.headers = {"content-type": "text/html"} + response.text = text + response.cookies.items.return_value = {} + return response + + # ============================================================================= # ScrapeOptions Tests # ============================================================================= @@ -474,3 +487,222 @@ def test_close_cleans_resources(self, scrape_service: ScrapeService): scrape_service.close() assert scrape_service._dynamic_scraper is None + + +# ============================================================================= +# Failure Propagation Tests (BUG-005) +# ============================================================================= + + +class TestScrapeFailurePropagation: + """Core scraping failures must surface as service failures. + + These tests cross the real core/service boundary: the real + ``StaticScraper`` runs against a mocked ``HTTPClient`` so that + ``success=False`` results produced by the engine actually reach + ``ScrapeService`` (regression for BUG-005). + """ + + def test_static_http_200_still_succeeds(self): + """HTTP 200 keeps producing a successful service result.""" + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response(200) + service = ScrapeService() + result = service.scrape( + ScrapeOptions(url="https://example.com", check_robots=False) + ) + service.close() + + assert result.success is True + assert result.error is None + assert isinstance(result.data, ScrapeResult) + assert result.data.status_code == 200 + + def test_static_http_404_propagates_failure(self): + """Engine failure for HTTP 404 must not become a service success.""" + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response( + 404, "Not Found" + ) + service = ScrapeService() + result = service.scrape( + ScrapeOptions(url="https://example.com/missing", check_robots=False) + ) + service.close() + + assert result.success is False + assert "404" in (result.error or "") + assert result.error_code == "EXECUTION_ERROR" + assert result.data is None + + def test_static_connection_error_propagates_failure(self): + """Connection errors surface as failures with the original message.""" + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.side_effect = ConnectionError( + "Connection refused" + ) + service = ScrapeService() + result = service.scrape( + ScrapeOptions(url="https://example.com/down", check_robots=False) + ) + service.close() + + assert result.success is False + assert "Connection refused" in (result.error or "") + assert result.error_code == "EXECUTION_ERROR" + + def test_static_robots_block_propagates_reason(self): + """robots.txt blocking keeps its reason in the service error.""" + with ( + patch("ciberwebscan.core.client.HTTPClient"), + patch( + "ciberwebscan.core.scraping.static.check_robots_txt", + return_value=(False, "Blocked by robots.txt"), + ), + ): + service = ScrapeService() + result = service.scrape( + ScrapeOptions(url="https://example.com/private", check_robots=True) + ) + service.close() + + assert result.success is False + assert "robots" in (result.error or "").lower() + assert result.error_code == "EXECUTION_ERROR" + + def test_ssrf_blocked_url_keeps_validation_error(self): + """SSRF protection still classifies as VALIDATION_ERROR.""" + service = ScrapeService() + result = service.scrape( + ScrapeOptions(url="http://169.254.169.254/latest/meta-data/") + ) + service.close() + + assert result.success is False + assert result.error_code == "VALIDATION_ERROR" + assert "ssrf" in (result.error or "").lower() + + def test_static_failure_is_not_exported(self): + """Failed scrapes never produce export artifacts.""" + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response(500) + service = ScrapeService() + result = service.scrape( + ScrapeOptions( + url="https://example.com/error", + check_robots=False, + export="out.json", + ) + ) + service.close() + + assert result.success is False + assert not result.exported + assert result.export_path is None + + def test_dynamic_failure_does_not_fabricate_http_200(self): + """Dynamic engine failures propagate without fake success metadata.""" + failed = DynamicScrapeResult( + url="https://example.com/spa", + success=False, + error="Timeout waiting for page", + ) + with ( + patch( + "ciberwebscan.services.scrape_service.is_playwright_available", + return_value=True, + ), + patch( + "ciberwebscan.services.scrape_service.run_async", + return_value=failed, + ), + ): + service = ScrapeService() + service._dynamic_scraper = Mock() + service._dynamic_scraper.scrape = Mock(return_value=failed) + result = service.scrape( + ScrapeOptions(url="https://example.com/spa", dynamic=True) + ) + service.close() + + assert result.success is False + assert "Timeout waiting for page" in (result.error or "") + assert result.error_code == "EXECUTION_ERROR" + assert result.data is None + + def test_dynamic_success_still_maps_result(self): + """Dynamic engine successes keep mapping to a valid ScrapeResult.""" + ok = DynamicScrapeResult( + url="https://example.com/spa", + success=True, + html="Spaready", + title="Spa", + ) + with ( + patch( + "ciberwebscan.services.scrape_service.is_playwright_available", + return_value=True, + ), + patch( + "ciberwebscan.services.scrape_service.run_async", + return_value=ok, + ), + ): + service = ScrapeService() + service._dynamic_scraper = Mock() + service._dynamic_scraper.scrape = Mock(return_value=ok) + result = service.scrape( + ScrapeOptions(url="https://example.com/spa", dynamic=True) + ) + service.close() + + assert result.success is True + assert isinstance(result.data, ScrapeResult) + assert result.data.title == "Spa" + assert result.data.status_code == 200 + + def test_batch_mixed_counts_and_warns_failures(self): + """Mixed batches keep successes, count failures, warn with reason.""" + ok_response = _mock_http_response(200) + missing_response = _mock_http_response(404, "Not Found") + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.side_effect = lambda url, **kwargs: ( + ok_response if url.endswith("/ok") else missing_response + ) + service = ScrapeService() + result = service.scrape_multiple( + [ + "https://example.com/ok", + "https://example.com/missing", + ], + ScrapeOptions(url="https://example.com", check_robots=False), + ) + service.close() + + assert result.success is True + assert len(result.data) == 1 + assert result.data[0].url == "https://example.com/ok" + assert len(result.warnings) == 1 + assert "404" in result.warnings[0] + + def test_batch_all_failures_is_informative_failure(self): + """A fully failed batch is a failure with an informative error.""" + with patch("ciberwebscan.core.client.HTTPClient") as mock_http_cls: + mock_http_cls.return_value.get.return_value = _mock_http_response( + 404, "Not Found" + ) + service = ScrapeService() + result = service.scrape_multiple( + [ + "https://example.com/a", + "https://example.com/b", + ], + ScrapeOptions(url="https://example.com", check_robots=False), + ) + service.close() + + assert result.success is False + assert result.data == [] + assert result.error_code == "SCRAPE_ERROR" + assert "404" in (result.error or "") + assert len(result.warnings) == 2