From dd2868d76539f792472119a99177d63214c092a2 Mon Sep 17 00:00:00 2001 From: Bryan Thompson Date: Mon, 27 Jul 2026 09:43:32 -0700 Subject: [PATCH 1/2] Relax CG URL pre-validation from strict to lax pydantic HttpUrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dual-validator in url_utils.py pre-screened URLs with strict-mode HttpUrl, inherited from when SDK <=0.5.x ran model_validate() with model_config strict=True and would crash on RFC-invalid characters. The SDK dropped model-level strict in 0.6.x, so the strict pre-check now rejects URLs (e.g. raw {} in query strings, common in agency NSPIRES links) that every validator in the actual response pipeline accepts — dropping the source field and logging a WARNING for URLs that would serve fine. Lax mode matches the SDK model exactly; the marshmallow leg of the dual-check still drops the genuinely batch-breaking shapes from #9904 (comma-separated URLs, TLD-less hosts). A round-trip test guards against a future SDK version re-tightening URL validation. --- api/src/services/common_grants/url_utils.py | 9 +- .../common_grants/test_transformation.py | 131 ++++++++++-------- .../services/common_grants/test_url_utils.py | 16 ++- 3 files changed, 92 insertions(+), 64 deletions(-) diff --git a/api/src/services/common_grants/url_utils.py b/api/src/services/common_grants/url_utils.py index c825c79460..3df1bb8f57 100644 --- a/api/src/services/common_grants/url_utils.py +++ b/api/src/services/common_grants/url_utils.py @@ -13,16 +13,19 @@ 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. + Deliberately not strict: the SDK models dropped model-level strict mode in + 0.6.x, so strict here would drop URLs (e.g. unencoded ``{}`` in a query + string) that the response pipeline accepts and 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..8379f5d0c5 100644 --- a/api/tests/src/services/common_grants/test_transformation.py +++ b/api/tests/src/services/common_grants/test_transformation.py @@ -1,7 +1,6 @@ """Tests for the transformation utility.""" from datetime import date, datetime, timezone -from urllib.parse import urlparse from uuid import uuid4 from common_grants_sdk.schemas.pydantic import ( @@ -46,31 +45,6 @@ validate_url, ) - -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 - - # Check for netloc only (i.e. it's a domain name) - if not parsed.scheme and parsed.netloc: - return f"https://{value}" - - return None - - DEFAULT_MOCK_OPP_FIELDS = { "legacy_opportunity_id": 67890, "opportunity_number": "2024-010", @@ -698,25 +672,73 @@ 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. + def test_validate_url_nasa_url_round_trips_through_response_pipeline(self): + """Guard: URLs that survive validate_url() must round-trip through the full + response pipeline — pydantic OpportunityBase construction AND the marshmallow + response load. The NASA URL (raw {} in the query string) is RFC-invalid but + accepted by both, so it must be served, not dropped. + + History: validate_url() once pre-screened with strict-mode HttpUrl because + SDK <=0.5.x ran model_validate() with model_config strict=True, which rejected + this URL and crashed the transformation. The SDK dropped model-level strict + in 0.6.x, so validate_url() is lax again. If a future SDK version re-tightens + URL validation, this test fails in CI instead of the URL 500ing (or being + silently dropped) in prod. + """ + from http import HTTPStatus - This test reproduces a bug where validate_url() lets through URLs - that Pydantic's HttpUrl validation rejects, causing transformations to fail. + from common_grants_sdk.schemas.pydantic import ( + OpportunitiesSearchResponse, + PaginatedResultsInfo, + SortedResultsInfo, + ) + + from src.api.common_grants.schemas.marshmallow.schemas import ( + OpportunitiesSearchResponse as OpportunitiesSearchResponseSchema, + ) - The NASA URL has curly braces or other characters that urlparse accepts but - Pydantic's strict HttpUrl validation rejects. - """ - # 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) + result = validate_url(nasa_url) + assert result == nasa_url, "validate_url() must accept the NASA URL unchanged" - assert old_result is not None, "_legacy_validate_url() should accept NASA URL" - assert new_result is None, "validate_url() should reject 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), + "additional_info_url": nasa_url, + "additional_info_url_description": "NSPIRES solicitation", + "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 + assert str(cg_opportunity.source) == nasa_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), + ) + response_json = response.model_dump(by_alias=True, mode="json") + + validated = OpportunitiesSearchResponseSchema().load(response_json) + 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 @@ -896,17 +918,10 @@ 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 + def test_transform_search_result_to_cg_with_nasa_url(self): + """RFC-invalid but pipeline-safe URLs (raw {} in query) must survive the + transformation into `source` rather than being dropped.""" 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(), @@ -925,13 +940,10 @@ def test_transform_search_result_to_cg_with_nasa_url_bug(self): }, } - # 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" + assert result is not None + assert str(result.source) == nasa_url def test_build_money_range_filter(self): """Test building money range filters.""" @@ -1087,8 +1099,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 +1175,8 @@ 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}" + # Create opportunity data with an invalid URL (dual-check drops comma-URLs) + 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..7a747ae448 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,18 @@ 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): + """Agency URLs with raw {} in query strings (e.g. NASA NSPIRES solId GUIDs) + are RFC-invalid but accepted by every validator in the response pipeline; + they must be served, not dropped.""" + 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", [ From e1c6b7f8e6e2c8f4a1bc9eea6a1c0117b337c2d2 Mon Sep 17 00:00:00 2001 From: Bryan Thompson Date: Mon, 27 Jul 2026 10:41:10 -0700 Subject: [PATCH 2/2] Trim comment bloat and dedupe response-pipeline test scaffolding Cuts history exposition from the validator docstring and the round-trip test, removes a transform-only NASA test that the round-trip test fully subsumes, and extracts the shared opp-data and response-load scaffolding used by the accept and reject halves of the dual-validator contract. --- api/src/services/common_grants/url_utils.py | 4 +- .../common_grants/test_transformation.py | 190 ++++++------------ .../services/common_grants/test_url_utils.py | 4 +- 3 files changed, 61 insertions(+), 137 deletions(-) diff --git a/api/src/services/common_grants/url_utils.py b/api/src/services/common_grants/url_utils.py index 3df1bb8f57..0863d262eb 100644 --- a/api/src/services/common_grants/url_utils.py +++ b/api/src/services/common_grants/url_utils.py @@ -20,9 +20,7 @@ class _UrlValidator(BaseModel): """Pydantic HttpUrl validator. Mirrors the HttpUrl field in OpportunityBase and other CommonGrants models. - Deliberately not strict: the SDK models dropped model-level strict mode in - 0.6.x, so strict here would drop URLs (e.g. unencoded ``{}`` in a query - string) that the response pipeline accepts and serves fine. + Not strict: strict drops URLs (e.g. raw ``{}`` in a query) the pipeline serves fine. """ url: HttpUrl diff --git a/api/tests/src/services/common_grants/test_transformation.py b/api/tests/src/services/common_grants/test_transformation.py index 8379f5d0c5..cdf627a153 100644 --- a/api/tests/src/services/common_grants/test_transformation.py +++ b/api/tests/src/services/common_grants/test_transformation.py @@ -1,6 +1,7 @@ """Tests for the transformation utility.""" from datetime import date, datetime, timezone +from http import HTTPStatus from uuid import uuid4 from common_grants_sdk.schemas.pydantic import ( @@ -11,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, @@ -45,6 +52,45 @@ validate_url, ) + +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), + }, + } + + +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 = { "legacy_opportunity_id": 67890, "opportunity_number": "2024-010", @@ -673,71 +719,21 @@ def test_transform_search_result_to_cg_with_invalid_data(self): assert result is None def test_validate_url_nasa_url_round_trips_through_response_pipeline(self): - """Guard: URLs that survive validate_url() must round-trip through the full - response pipeline — pydantic OpportunityBase construction AND the marshmallow - response load. The NASA URL (raw {} in the query string) is RFC-invalid but - accepted by both, so it must be served, not dropped. - - History: validate_url() once pre-screened with strict-mode HttpUrl because - SDK <=0.5.x ran model_validate() with model_config strict=True, which rejected - this URL and crashed the transformation. The SDK dropped model-level strict - in 0.6.x, so validate_url() is lax again. If a future SDK version re-tightens - URL validation, this test fails in CI instead of the URL 500ing (or being - silently dropped) in prod. + """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. """ - 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, - ) - nasa_url = "https://nspires.nasaprs.com/external/solicitations/summary!init.do?solId={D8604BE7-CAB6-C1C0-B668-423042C43AA6}&path=&method=init" - result = validate_url(nasa_url) - assert result == nasa_url, "validate_url() must accept the NASA URL unchanged" + assert validate_url(nasa_url) == 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), - "additional_info_url": nasa_url, - "additional_info_url_description": "NSPIRES solicitation", - "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) + 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 - 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), - ) - response_json = response.model_dump(by_alias=True, mode="json") - - validated = OpportunitiesSearchResponseSchema().load(response_json) + 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): @@ -776,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 @@ -918,33 +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(self): - """RFC-invalid but pipeline-safe URLs (raw {} in query) must survive the - transformation into `source` rather than being dropped.""" - nasa_url = "https://nspires.nasaprs.com/external/solicitations/summary!init.do?solId={D8604BE7-CAB6-C1C0-B668-423042C43AA6}&path=&method=init" - - 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, - }, - } - - result = transform_search_result_to_cg(opp_data) - - assert result is not None - assert str(result.source) == nasa_url - def test_build_money_range_filter(self): """Test building money range filters.""" # Test with min and max amounts @@ -1175,7 +1104,6 @@ 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 (dual-check drops comma-URLs) invalid_url = "https://example.com,https://other.example" opp_data = { "opportunity_id": uuid4(), 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 7a747ae448..0e98d84c87 100644 --- a/api/tests/src/services/common_grants/test_url_utils.py +++ b/api/tests/src/services/common_grants/test_url_utils.py @@ -37,9 +37,7 @@ def test_rejects_garbage(self): assert validate_url_compatible("sam.gov") is None # missing scheme def test_accepts_unencoded_braces_in_query(self): - """Agency URLs with raw {} in query strings (e.g. NASA NSPIRES solId GUIDs) - are RFC-invalid but accepted by every validator in the response pipeline; - they must be served, not dropped.""" + """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