diff --git a/api/src/services/common_grants/url_utils.py b/api/src/services/common_grants/url_utils.py index c825c79460..0863d262eb 100644 --- a/api/src/services/common_grants/url_utils.py +++ b/api/src/services/common_grants/url_utils.py @@ -13,16 +13,17 @@ from marshmallow import ValidationError as MarshmallowValidationError from marshmallow import fields as marshmallow_fields -from pydantic import BaseModel, Field, HttpUrl, ValidationError +from pydantic import BaseModel, HttpUrl, ValidationError class _UrlValidator(BaseModel): - """Pydantic strict HttpUrl validator. + """Pydantic HttpUrl validator. Mirrors the HttpUrl field in OpportunityBase and other CommonGrants models. + Not strict: strict drops URLs (e.g. raw ``{}`` in a query) the pipeline serves fine. """ - url: HttpUrl = Field(strict=True) + url: HttpUrl _marshmallow_url_field = marshmallow_fields.URL() diff --git a/api/tests/src/services/common_grants/test_transformation.py b/api/tests/src/services/common_grants/test_transformation.py index 17e5921c40..cdf627a153 100644 --- a/api/tests/src/services/common_grants/test_transformation.py +++ b/api/tests/src/services/common_grants/test_transformation.py @@ -1,7 +1,7 @@ """Tests for the transformation utility.""" from datetime import date, datetime, timezone -from urllib.parse import urlparse +from http import HTTPStatus from uuid import uuid4 from common_grants_sdk.schemas.pydantic import ( @@ -12,15 +12,21 @@ MoneyRange, MoneyRangeFilter, OppFilters, + OpportunitiesSearchResponse, OppSortBy, OppSorting, OppStatusOptions, PaginatedBodyParams, + PaginatedResultsInfo, RangeOperator, + SortedResultsInfo, StringArrayFilter, ) from freezegun import freeze_time +from src.api.common_grants.schemas.marshmallow.schemas import ( + OpportunitiesSearchResponse as OpportunitiesSearchResponseSchema, +) from src.api.common_grants.schemas.marshmallow.schemas import OpportunityCustomFields from src.api.common_grants.schemas.pydantic.custom_fields import ( AgencyField, @@ -47,28 +53,42 @@ ) -def _legacy_validate_url(value: str | None) -> str | None: - """ - Validate a URL string. - - Args: - value: The string to validate - - Returns: - A valid URL string or None - """ - # Parse the string - parsed = urlparse(value) - - # Check for scheme and netloc (i.e. it's a complete url) - if parsed.scheme and parsed.netloc: - return value +def _opp_data_with_info_url(url: str, description: str) -> dict: + """Minimal search-result payload whose summary.additional_info_url is `url`.""" + return { + "opportunity_id": uuid4(), + "opportunity_title": "Test Opportunity", + "opportunity_status": OpportunityStatus.POSTED, + "created_at": datetime(2024, 1, 1, 12, 0, 0), + "updated_at": datetime(2024, 1, 2, 12, 0, 0), + "summary": { + "summary_description": "Test description", + "post_date": date(2024, 1, 1), + "close_date": date(2024, 12, 31), + "additional_info_url": url, + "additional_info_url_description": description, + "created_at": datetime(2024, 1, 1, 12, 0, 0), + "updated_at": datetime(2024, 1, 2, 12, 0, 0), + }, + } - # Check for netloc only (i.e. it's a domain name) - if not parsed.scheme and parsed.netloc: - return f"https://{value}" - return None +def _load_through_response_schema(cg_opportunity) -> dict: + """Run one CG opportunity down the route's response path: pydantic response model + -> model_dump -> marshmallow load. The load is what 500s on a divergent URL.""" + response = OpportunitiesSearchResponse( + status=HTTPStatus.OK, + message="ok", + items=[cg_opportunity], + pagination_info=PaginatedResultsInfo(page=1, page_size=1, totalItems=1, totalPages=1), + sort_info=SortedResultsInfo( + sort_by=OppSortBy.LAST_MODIFIED_AT.value, + sort_order="desc", + errors=[], + ), + filter_info=build_filter_info(None), + ) + return OpportunitiesSearchResponseSchema().load(response.model_dump(by_alias=True, mode="json")) DEFAULT_MOCK_OPP_FIELDS = { @@ -698,25 +718,23 @@ def test_transform_search_result_to_cg_with_invalid_data(self): # Should return None for invalid data assert result is None - def test_validate_url_with_nasa_url_bug(self): - """Test that validate_url() properly rejects URLs that Pydantic HttpUrl rejects. - - This test reproduces a bug where validate_url() lets through URLs - that Pydantic's HttpUrl validation rejects, causing transformations to fail. - - The NASA URL has curly braces or other characters that urlparse accepts but - Pydantic's strict HttpUrl validation rejects. + def test_validate_url_nasa_url_round_trips_through_response_pipeline(self): + """A URL that survives validate_url() must survive the whole response path. + Fails if a future SDK re-tightens URL validation, which would otherwise drop + the field (or 500) in prod instead of here. """ - # This URL has characters that urlparse accepts but Pydantic HttpUrl rejects - # Based on error: "non-URL code point" - likely curly braces or other invalid chars nasa_url = "https://nspires.nasaprs.com/external/solicitations/summary!init.do?solId={D8604BE7-CAB6-C1C0-B668-423042C43AA6}&path=&method=init" - # Old implementation used urlparse, new implementation uses Pydantic HttpUrl - old_result = _legacy_validate_url(nasa_url) - new_result = validate_url(nasa_url) + assert validate_url(nasa_url) == nasa_url - assert old_result is not None, "_legacy_validate_url() should accept NASA URL" - assert new_result is None, "validate_url() should reject NASA URL" + cg_opportunity = transform_search_result_to_cg( + _opp_data_with_info_url(nasa_url, "NSPIRES solicitation") + ) + assert cg_opportunity is not None + assert str(cg_opportunity.source) == nasa_url + + validated = _load_through_response_schema(cg_opportunity) + assert validated["items"][0]["source"] == nasa_url def test_validate_url_rejects_urls_that_marshmallow_url_field_rejects(self): """validate_url() must also reject URLs that pass Pydantic's HttpUrl but fail @@ -754,55 +772,15 @@ def test_search_response_marshmallow_load_does_not_500_on_problematic_url(self): items[0].customFields.additionalInfo.value.url. After the fix the record loads cleanly with additionalInfo absent (because validate_url returned None). """ - from http import HTTPStatus - - from common_grants_sdk.schemas.pydantic import ( - OpportunitiesSearchResponse, - PaginatedResultsInfo, - SortedResultsInfo, - ) - - from src.api.common_grants.schemas.marshmallow.schemas import ( - OpportunitiesSearchResponse as OpportunitiesSearchResponseSchema, - ) - problematic_url = "https://www.grants.gov,https://other.example" - opp_data = { - "opportunity_id": uuid4(), - "opportunity_title": "Test Opportunity", - "opportunity_status": OpportunityStatus.POSTED, - "created_at": datetime(2024, 1, 1, 12, 0, 0), - "updated_at": datetime(2024, 1, 2, 12, 0, 0), - "summary": { - "summary_description": "Test description", - "post_date": date(2024, 1, 1), - "close_date": date(2024, 12, 31), - "additional_info_url": problematic_url, - "additional_info_url_description": "Multiple links", - "created_at": datetime(2024, 1, 1, 12, 0, 0), - "updated_at": datetime(2024, 1, 2, 12, 0, 0), - }, - } - - cg_opportunity = transform_search_result_to_cg(opp_data) - assert cg_opportunity is not None, "transform itself must not drop the record" - response = OpportunitiesSearchResponse( - status=HTTPStatus.OK, - message="ok", - items=[cg_opportunity], - pagination_info=PaginatedResultsInfo(page=1, page_size=1, totalItems=1, totalPages=1), - sort_info=SortedResultsInfo( - sort_by=OppSortBy.LAST_MODIFIED_AT.value, - sort_order="desc", - errors=[], - ), - filter_info=build_filter_info(None), + cg_opportunity = transform_search_result_to_cg( + _opp_data_with_info_url(problematic_url, "Multiple links") ) - response_json = response.model_dump(by_alias=True, mode="json") + assert cg_opportunity is not None, "transform itself must not drop the record" - # This is the line that raises marshmallow.ValidationError in prod. - validated = OpportunitiesSearchResponseSchema().load(response_json) + # The load inside is the line that raises marshmallow.ValidationError in prod. + validated = _load_through_response_schema(cg_opportunity) assert len(validated["items"]) == 1 # Once validate_url filters the bad URL, additionalInfo is absent; that's the @@ -896,43 +874,6 @@ def test_dropped_url_log_includes_field_path_and_opportunity_id(self, caplog): assert getattr(r, "opportunity_id", None) == opp_id assert r.levelname == "WARNING" - def test_transform_search_result_to_cg_with_nasa_url_bug(self): - """Test that transformation works correctly with NASA URL that Pydantic rejects. - - This test ensures that when validate_url() properly rejects invalid URLs, - the transformation still succeeds (with source=None) rather than failing. - """ - # This URL has characters that urlparse accepts but Pydantic HttpUrl rejects - nasa_url = "https://nspires.nasaprs.com/external/solicitations/summary!init.do?solId={D8604BE7-CAB6-C1C0-B668-423042C43AA6}&path=&method=init" - assert ( - _legacy_validate_url(nasa_url) is not None - ), "_legacy_validate_url() should accept NASA URL" - - opp_data = { - "opportunity_id": uuid4(), - "opportunity_title": "Test Opportunity", - "opportunity_status": OpportunityStatus.POSTED, - "created_at": datetime(2024, 1, 1, 12, 0, 0), - "updated_at": datetime(2024, 1, 2, 12, 0, 0), - "summary": { - "summary_description": "Test description", - "post_date": date(2024, 1, 1), - "close_date": date(2024, 12, 31), - "estimated_total_program_funding": 1000000, - "award_ceiling": 500000, - "award_floor": 10000, - "additional_info_url": nasa_url, - }, - } - - # The transformation should succeed without raising a Pydantic validation error - result = transform_search_result_to_cg(opp_data) - - # After fix: validate_url() should reject the URL, so source should be None - # but transformation should still succeed - assert result is not None, "Transformation should succeed even with invalid URL" - assert result.source is None, "Invalid URL should result in source=None" - def test_build_money_range_filter(self): """Test building money range filters.""" # Test with min and max amounts @@ -1087,8 +1028,9 @@ def test_url_validation_error_logging(self, caplog): # Set up logging to capture info level logs caplog.set_level(logging.INFO) - # Test with an invalid URL that should trigger the logging - invalid_url = "https://example.com/path/{invalid-chars}" + # Comma-separated URL: pydantic accepts it, marshmallow rejects it, so the + # dual-check drops it and logs. + invalid_url = "https://example.com,https://other.example" result = validate_url(invalid_url) @@ -1162,8 +1104,7 @@ def test_transformation_with_invalid_url_logs_but_succeeds(self, caplog): # a data-suppression event, not a routine validation observation. caplog.set_level(logging.WARNING) - # Create opportunity data with an invalid URL - invalid_url = "https://example.com/path/{invalid}" + invalid_url = "https://example.com,https://other.example" opp_data = { "opportunity_id": uuid4(), "opportunity_title": "Test Opportunity", diff --git a/api/tests/src/services/common_grants/test_url_utils.py b/api/tests/src/services/common_grants/test_url_utils.py index 7a9d179d76..0e98d84c87 100644 --- a/api/tests/src/services/common_grants/test_url_utils.py +++ b/api/tests/src/services/common_grants/test_url_utils.py @@ -3,7 +3,7 @@ import pytest from marshmallow import ValidationError as MarshmallowValidationError from marshmallow import fields as marshmallow_fields -from pydantic import BaseModel, Field, HttpUrl +from pydantic import BaseModel, HttpUrl from src.services.common_grants.url_utils import validate_url_compatible @@ -11,7 +11,7 @@ class _PydanticHttpUrl(BaseModel): """Minimal pydantic HttpUrl validator used to demonstrate the divergence.""" - url: HttpUrl = Field(strict=True) + url: HttpUrl class TestValidateUrlCompatible: @@ -36,6 +36,16 @@ def test_rejects_garbage(self): assert validate_url_compatible("not-a-url") is None assert validate_url_compatible("sam.gov") is None # missing scheme + def test_accepts_unencoded_braces_in_query(self): + """Raw {} in a query is RFC-invalid but the whole pipeline accepts it.""" + url = "https://nspires.nasaprs.com/x/summary.do?solId={9455D565-3411-0574}&method=init" + assert validate_url_compatible(url) == url + + def test_rejects_non_http_schemes(self): + # HttpUrl restricts scheme to http/https regardless of strictness. + assert validate_url_compatible("javascript:alert(1)") is None + assert validate_url_compatible("ftp://example.gov/file") is None + @pytest.mark.parametrize( "bad_url", [