From 9d03ea46535c054e2f797fd342acd751f93185a5 Mon Sep 17 00:00:00 2001 From: Alastair Porter Date: Fri, 10 Jul 2026 12:24:22 +0200 Subject: [PATCH 1/2] Validate search query values before sending to solr We found that scrapers/attackers construct search urls incorrectly (they double-escape spaces) so we end up with filters like `samplerate:22050+tag:"x"` which parse as `samplerate:22050+tag` and causes solr to raise an error. Validate filter strings and values before sending the result to solr. If it looks invalid then directly return an error page instead of trying the search. Check for errors on search, tag, map bytearray, and clustering views --- geotags/tests.py | 7 + geotags/views.py | 16 ++- search/tests/test_views.py | 15 +++ search/views.py | 8 +- utils/search/filter_validation.py | 89 +++++++++++++ utils/search/search_query_processor.py | 27 +++- utils/tests/test_filter_validation.py | 177 +++++++++++++++++++++++++ 7 files changed, 324 insertions(+), 15 deletions(-) create mode 100644 utils/search/filter_validation.py create mode 100644 utils/tests/test_filter_validation.py diff --git a/geotags/tests.py b/geotags/tests.py index 375c6af24..159f7135f 100644 --- a/geotags/tests.py +++ b/geotags/tests.py @@ -110,3 +110,10 @@ def test_browse_geotags_for_query(self): resp = self.client.get(reverse("geotags-query") + "?q=barcelona") check_values = {"query_description": '"barcelona"'} self.check_context(resp.context, check_values) + + def test_geotags_for_query_barray_invalid_filter_returns_empty(self): + # A corrupted/invalid filter sets sqp.errors, which must short-circuit before + # Solr; the endpoint returns an empty bytearray rather than crashing. + resp = self.client.get(reverse("geotags-for-query-barray") + "?f=samplerate%3Aabc") + self.assertEqual(resp.status_code, 200) + self.assertEqual(len(resp.content), 0) diff --git a/geotags/views.py b/geotags/views.py index 2131f29f6..e2374eba1 100644 --- a/geotags/views.py +++ b/geotags/views.py @@ -169,12 +169,16 @@ def geotags_for_query_barray(request): else: # Otherwise, perform a search query to get the results sqp = SearchQueryProcessor(request) - query_params = sqp.as_query_params() - if "facets" in query_params: - # No need to compute facets for bytearray query - del query_params["facets"] - results, _ = perform_search_engine_query(query_params) - results_docs = results.docs + if sqp.errors: + # invalid filter: don't search on solr + results_docs = [] + else: + query_params = sqp.as_query_params() + if "facets" in query_params: + # No need to compute facets for bytearray query + del query_params["facets"] + results, _ = perform_search_engine_query(query_params) + results_docs = results.docs if results_docs is None: results_docs = [] diff --git a/search/tests/test_views.py b/search/tests/test_views.py index 3bad506dd..a125fc140 100644 --- a/search/tests/test_views.py +++ b/search/tests/test_views.py @@ -301,6 +301,21 @@ def test_failed_search_result_clustering_view(self, get_clusters_for_query, get_ self.assertTemplateUsed(resp, "search/clustering_results.html") self.assertEqual(resp.context["clusters_data"], None) + def test_clusters_section_invalid_filter_returns_no_clusters(self): + # A corrupted/invalid filter sets sqp.errors, which must short-circuit + # and not send a request to solr and return an empty page. + resp = self.client.get(reverse("clusters-section") + "?f=samplerate%3Aabc") + self.assertEqual(resp.status_code, 200) + self.assertTemplateUsed(resp, "search/clustering_results.html") + self.assertEqual(resp.context["clusters_data"], None) + + def test_clustered_graph_invalid_filter_returns_error(self): + # A corrupted/invalid filter sets sqp.errors, which must short-circuit + # and not send a request to solr and return an empty response. + resp = self.client.get(reverse("clustered-graph-json") + "?f=samplerate%3Aabc") + self.assertEqual(resp.status_code, 200) + self.assertIn(b"error", resp.content) + @pytest.mark.django_db class TestSearchDeepPagination: diff --git a/search/views.py b/search/views.py index 0911b7c38..717579ccf 100644 --- a/search/views.py +++ b/search/views.py @@ -328,7 +328,7 @@ def _get_clusters_data_helper(sqp): def clusters_section(request): sqp = search_query_processor.SearchQueryProcessor(request) - clusters_data = _get_clusters_data_helper(sqp) + clusters_data = None if sqp.errors else _get_clusters_data_helper(sqp) if clusters_data is None: return render(request, "search/clustering_results.html", {"clusters_data": None}) return render(request, "search/clustering_results.html", {"sqp": sqp, "clusters_data": clusters_data}) @@ -339,9 +339,13 @@ def clustered_graph(request): # TODO: this view is currently not used in the new UI, but we could add a modal in the # clustering section to show results in a graph. sqp = search_query_processor.SearchQueryProcessor(request) + if sqp.errors: + return JsonResponse(json.dumps({"error": True}), safe=False) + results = get_clusters_for_query(sqp) if results is None: - JsonResponse(json.dumps({"error": True}), safe=False) + return JsonResponse(json.dumps({"error": True}), safe=False) + graph = get_clustering_data_for_graph_display(sqp, results["graph"]) return JsonResponse(json.dumps(graph), safe=False) diff --git a/utils/search/filter_validation.py b/utils/search/filter_validation.py new file mode 100644 index 000000000..c00e48e08 --- /dev/null +++ b/utils/search/filter_validation.py @@ -0,0 +1,89 @@ +import luqum.tree +from luqum.parser import parser + +FIELD_TYPES_MAP = { + "samplerate": int, + "bitrate": int, + "bitdepth": int, + "channels": int, + "duration": float, +} + +COMPLEX_EXPR_TYPES = ( + luqum.tree.Range, + luqum.tree.OpenRange, + luqum.tree.From, + luqum.tree.To, + luqum.tree.FieldGroup, + luqum.tree.Boost, +) + + +def parse_filter(filter_string): + """Parse a Lucene-style filter string into its list of top-level nodes. + Empty string -> []. Raises luqum.exceptions.ParseError on malformed input. + """ + if not filter_string: + return [] + tree = parser.parse(filter_string) + return [tree] if type(tree) == luqum.tree.SearchField else tree.children + + +def validate_filter_types(nodes) -> str | None: + """Validate that in the given top-level filter nodes, the values of fields + with a known type (FIELD_TYPES_MAP) can be cast to that type. Return a + human-readable error string on first failure, or None on success. + """ + for node in nodes: + if type(node) != luqum.tree.SearchField: + continue + if node.name not in FIELD_TYPES_MAP: + continue + expr = node.expr + if isinstance(expr, COMPLEX_EXPR_TYPES): + # Ranges/inequalities/groups/boosts etc. e.g. samplerate:[a TO b] + # validate directly in solr + continue + if isinstance(expr, luqum.tree.Phrase): + # String in "quotes" + value = expr.value[1:-1] + else: + value = str(expr) + if value == "*": + # `field:*` is a valid solr query, continue + continue + expected_type = FIELD_TYPES_MAP[node.name] + # This does let through some values that are valid in python but not solr + # (e.g. int(1_000)), but we accept these few cases because they are rare + try: + expected_type(value) + except (ValueError, TypeError): + return f"Filter parsing error: '{node.name}' value must be {expected_type.__name__}, got {value!r}" + return None + + +def find_dropped_filters(nodes) -> list[str]: + """Return a list of human-readable representations of filters that + SearchQueryProcessor would silently drop. + + Hand-made URLs sometimes use `%2B` instead of a space between filters, which + decodes as a `+` instead of a space. Lucene reads that `+` as a MUST prefix, + so the following filter parses as a top-level Plus-wrapped node: + + sent: f=tag%3A%22tap%22%2Busername%3A%22ascap%22 + decoded: tag:"tap"+username:"ascap" + expected: tag:"tap" username:"ascap" + parsed: [SearchField('tag', '"tap"'), Plus(SearchField('username', '"ascap"'))] + expected [SearchField('tag', '"tap"'), SearchField('username', '"ascap"')] + + SearchQueryProcessor currently drops the second part of the search query, so + this function returns ['username:"ascap"']. + + Empty list means nothing was removed. + """ + dropped = [] + for node in nodes: + if isinstance(node, luqum.tree.Plus) and isinstance(node.a, luqum.tree.SearchField): + # str() of a luqum node can keep trailing whitespace from the source string + dropped.append(str(node.a).strip()) + return dropped diff --git a/utils/search/search_query_processor.py b/utils/search/search_query_processor.py index a61da4d63..270a70cd9 100644 --- a/utils/search/search_query_processor.py +++ b/utils/search/search_query_processor.py @@ -22,12 +22,12 @@ import json import urllib.parse +import luqum.exceptions import luqum.tree from django.conf import settings from django.core.exceptions import ValidationError from django.urls import reverse from django.utils.http import urlencode -from luqum.parser import parser from luqum.pretty import prettify from sounds.models import Sound @@ -36,6 +36,7 @@ from utils.clustering_utilities import get_clusters_for_query, get_ids_in_cluster from utils.encryption import create_hash from utils.search import SearchEngineException +from utils.search.filter_validation import parse_filter, validate_filter_types from utils.search.search_sounds import allow_beta_search_features from .search_query_processor_options import ( @@ -66,6 +67,9 @@ class SearchQueryProcessor: """The SearchQueryProcessor class is used to parse and process search query information from a request object and compute a number of useful items for displaying search information in templates, constructing search URLs, and preparing search options to be passed to the backend search engine. + + You must check `self.errors` after constructing this object. If it's not empty then it's not guaranteed + to be completely initialised. """ request = None @@ -337,14 +341,10 @@ def __init__(self, request, facets=None): self.options[option_name] = option # Get filter and parse it. Make sure it is iterable (even if it only has one element) - self.f = urllib.parse.unquote(request.GET.get("f", "")).strip().lstrip() + self.f = urllib.parse.unquote(request.GET.get("f", "")).strip() if self.f: try: - f_parsed = parser.parse(self.f) - if type(f_parsed) == luqum.tree.SearchField: - self.f_parsed = [f_parsed] - else: - self.f_parsed = f_parsed.children + self.f_parsed = parse_filter(self.f) except luqum.exceptions.ParseError as e: self.errors = f"Filter parsing error: {e}" self.f_parsed = [] @@ -364,6 +364,14 @@ def __init__(self, request, facets=None): self.errors = f"Filter parsing error: invalid tag value '{tag_value}'" return + # Validate that values on filter fields are the correct type. Catches issues where incorrectly formatted + # queries cause a solr exception (e.g. `samplerate:22050+tag:"x"` parses as "22050+tag" instead of 22050) + if not self.errors: + type_error = validate_filter_types(self.f_parsed) + if type_error is not None: + self.errors = type_error + return + # Remove duplicate filters if any nodes_in_filter = [] f_parsed_no_duplicates = [] @@ -686,7 +694,12 @@ def as_query_params(self, exclude_facet_filters=False): Returns: dict: Dictionary with the query parameters to be used by the SearchEngine.search_sounds method. + + Raises: + SearchEngineException: if self.errors is set (see class docstring). """ + if self.errors: + raise SearchEngineException(f"Cannot build query params, filter has errors: {self.errors}") # Filter field weights by "search in" options field_weights = self.get_option_value_to_apply("field_weights") diff --git a/utils/tests/test_filter_validation.py b/utils/tests/test_filter_validation.py new file mode 100644 index 000000000..6762028eb --- /dev/null +++ b/utils/tests/test_filter_validation.py @@ -0,0 +1,177 @@ +import urllib.parse + +import pytest +from django.contrib.auth.models import AnonymousUser +from django.test import RequestFactory + +from utils.search import SearchEngineException +from utils.search.filter_validation import find_dropped_filters, parse_filter, validate_filter_types +from utils.search.search_query_processor import SearchQueryProcessor + + +def _decode_f_from_url(path: str) -> str: + """Same as in SearchQueryProcessor.__init__""" + request = RequestFactory().get(path) + return urllib.parse.unquote(request.GET.get("f", "")).strip() + + +def _build_sqp(url): + request = RequestFactory().get(url) + request.user = AnonymousUser() + return SearchQueryProcessor(request) + + +TYPE_CORRUPTION_URLS = [ + pytest.param( + # filter decodes to bitdepth:16+bitrate:1379, which parses + # to bitdepth value '16+bitrate:1379', and is rejected by solr + "/browse/tags/?f=bitdepth%3A16%2Bbitrate%3A1379", + "bitdepth", + id="bitdepth+bitrate", + ), + pytest.param( + # filter decodes to samplerate:22050+tag:"simmons", which parses + # to samplerate value '22050+tag:"simmons"', and is rejected by solr + "/search/?f=samplerate%3A22050%2Btag%3A%22simmons%22", + "samplerate", + id="synthetic_original-sentry-shape", + ), +] + + +@pytest.mark.parametrize("url_path, field", TYPE_CORRUPTION_URLS) +def test_url_type_corruption_detected(url_path, field): + # badly encoded + makes the value of a field invalid + nodes = parse_filter(_decode_f_from_url(url_path)) + verdict = validate_filter_types(nodes) + assert verdict is not None and field in verdict + assert find_dropped_filters(nodes) == [] + + +DROPPED_FILTER_URLS = [ + pytest.param( + # Decodes to tag:"tap"+username:"ascap" - tag:tap and username:ascap + # with a + before it - turns into 'MUST username:ascap' rather than a space + # Our query parser ignores `Plus` fields, so it is reported as a dropped field + "/browse/tags/?f=tag%3A%22tap%22%2Busername%3A%22ascap%22", + ['username:"ascap"'], + [("tag", '"tap"')], + id="tag+username", + ), + pytest.param( + # Decodes to tag:"lounge"+samplerate:8000. Same as above, samplerate is validated + # as a valid int, but is still dropped because it's a `Plus` field + "/browse/tags/?f=tag%3A%22lounge%22%2Bsamplerate%3A8000", + ["samplerate:8000"], + [("tag", '"lounge"')], + id="tag+samplerate-unquoted", + ), + pytest.param( + # Multiple %2B decodes to +, all filters after the first one are dropped + "/browse/tags/?f=tag%3A%22electronic%22%2Btag%3A%22synth%22%2Btag%3A%22beat%22", + ['tag:"synth"', 'tag:"beat"'], + [("tag", '"electronic"')], + id="tag+tag+tag", + ), + pytest.param( + # Mixed encoding: samplerate:"192000" is quoted but is still a valid int + # after removing "" so is kept + "/browse/tags/?f=tag%3A%22Blood%22%2Btag%3A%22heart-beat%22+samplerate%3A%22192000%22", + ['tag:"heart-beat"'], + [("tag", '"Blood"'), ("samplerate", '"192000"')], + id="quoted-samplerate-not-flagged", + ), +] + + +@pytest.mark.parametrize("url_path, dropped, kept", DROPPED_FILTER_URLS) +def test_url_dropped_filters_reported(url_path, dropped, kept): + # These URLs all have valid types for filter values, but SearchQueryProcessor drops + # Plus-wrapped filters without telling the user. The remaining filters + # are used for the search query. + nodes = parse_filter(_decode_f_from_url(url_path)) + assert validate_filter_types(nodes) is None + assert find_dropped_filters(nodes) == dropped + assert _build_sqp(url_path).non_option_filters == kept + + +def test_url_legitimate_plus_not_flagged(): + # literal `+` between filters (decodes to space) and valid `+` inside a + # Phrase ("Sampling+" is a real CC license): both accepted. + nodes = parse_filter(_decode_f_from_url("/search/?f=tag%3A%22ducks%22+license%3A%22Sampling%2B%22")) + assert validate_filter_types(nodes) is None + assert find_dropped_filters(nodes) == [] + + +def test_sqp_rejects_invalid_filter(): + # Check that SearchQueryProcessor sets self.errors if a field is invalid + sqp = _build_sqp("/search/?f=samplerate%3Aabc") + assert "samplerate" in sqp.errors + + +def test_sqp_with_errors_refuses_to_build_query_params(): + # a SQP with errors must never produce query params for the search engine. + sqp = _build_sqp("/search/?f=samplerate%3Aabc") + assert sqp.errors + with pytest.raises(SearchEngineException): + sqp.as_query_params() + + +SQP_ACCEPT_CASES = [ + pytest.param("/search/?f=samplerate%3A44100", id="bare-typed-int"), + pytest.param("/search/?f=tag%3A%22reverb%22", id="bare-tag-phrase"), + pytest.param("/search/?f=duration%3A%5B0+TO+10%5D", id="range-expr"), + pytest.param("/search/?f=tag%3A%22ducks%22+license%3A%22Sampling%2B%22", id="legit-plus-in-phrase"), + pytest.param( + "/browse/tags/?f=tag%3A%22Blood%22%2Btag%3A%22heart-beat%22+samplerate%3A%22192000%22", + id="quoted-samplerate-not-false-positive", + ), + pytest.param("/search/?f=", id="empty-f"), + pytest.param("/search/?q=drum", id="no-f-param"), +] + + +@pytest.mark.parametrize("url", SQP_ACCEPT_CASES) +def test_sqp_accepts_valid(url): + sqp = _build_sqp(url) + assert not sqp.errors, f"Expected no errors for {url!r}, got {sqp.errors!r}" + + +VALID_FILTERS = [ + pytest.param("samplerate:44100", id="bare-int-typed"), + pytest.param('bitdepth:16 tag:"reverb" channels:1', id="multi-filter-all-valid"), + pytest.param("duration:[0 TO 10]", id="range-expr-skipped"), + pytest.param("samplerate:[* TO 48000]", id="open-range-expr-skipped"), + pytest.param("samplerate:>=44100", id="from-expr-skipped"), + pytest.param("samplerate:(44100 OR 48000)", id="fieldgroup-expr-skipped"), + pytest.param("samplerate:44100^2", id="boost-expr-skipped"), + pytest.param('tag:"reverb"', id="non-typed-field-ignored"), + pytest.param('tagfacet:"can"', id="legacy-tagfacet-ignored"), + pytest.param('license:"Sampling+"', id="plus-inside-phrase-tolerated"), + pytest.param('samplerate:"192000"', id="quoted-int-on-pint-field"), + pytest.param("samplerate:*", id="wildcard-exists-query-tolerated"), + pytest.param("", id="empty-string"), +] + + +@pytest.mark.parametrize("filter_string", VALID_FILTERS) +def test_valid_filters_accepted(filter_string): + nodes = parse_filter(filter_string) + assert validate_filter_types(nodes) is None + assert find_dropped_filters(nodes) == [] + + +INVALID_FILTERS = [ + pytest.param("samplerate:abc", "samplerate", id="non-numeric-samplerate"), + pytest.param("channels:stereo", "channels", id="non-numeric-channels"), + pytest.param("bitdepth:twentyfour", "bitdepth", id="non-numeric-bitdepth"), + pytest.param("bitdepth:16+bitrate:1379", "bitdepth", id="type-corruption-bare-string"), + pytest.param('samplerate:22050+tag:"simmons"', "samplerate", id="original-sentry-bare-string"), +] + + +@pytest.mark.parametrize("filter_string, must_mention", INVALID_FILTERS) +def test_invalid_filters_rejected(filter_string, must_mention): + verdict = validate_filter_types(parse_filter(filter_string)) + assert verdict is not None, f"Expected error for {filter_string!r}" + assert must_mention in verdict, f"Expected {must_mention!r} in verdict, got {verdict!r}" From c8ef2a5ea041ca43a06f6ba7889a8ec7d5386018 Mon Sep 17 00:00:00 2001 From: Alastair Porter Date: Fri, 10 Jul 2026 12:24:53 +0200 Subject: [PATCH 2/2] Fix clustered_graph KeyError on clustering timeout get_clusters_for_query returns the {"clusters": None} sentinel when the clustering task fails or times out, so don't try and load results["graph"] unconditionally --- search/tests/test_views.py | 8 ++++++++ search/views.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/search/tests/test_views.py b/search/tests/test_views.py index a125fc140..4cebf6c03 100644 --- a/search/tests/test_views.py +++ b/search/tests/test_views.py @@ -316,6 +316,14 @@ def test_clustered_graph_invalid_filter_returns_error(self): self.assertEqual(resp.status_code, 200) self.assertIn(b"error", resp.content) + @mock.patch("search.views.get_clusters_for_query") + def test_clustered_graph_clustering_timeout_returns_error(self, get_clusters_for_query): + # On clustering timeout/failure get_clusters_for_query returns {"clusters": None} + get_clusters_for_query.return_value = {"clusters": None} + resp = self.client.get(reverse("clustered-graph-json")) + self.assertEqual(resp.status_code, 200) + self.assertIn(b"error", resp.content) + @pytest.mark.django_db class TestSearchDeepPagination: diff --git a/search/views.py b/search/views.py index 717579ccf..ba3465a75 100644 --- a/search/views.py +++ b/search/views.py @@ -343,7 +343,7 @@ def clustered_graph(request): return JsonResponse(json.dumps({"error": True}), safe=False) results = get_clusters_for_query(sqp) - if results is None: + if results is None or results.get("clusters") is None: return JsonResponse(json.dumps({"error": True}), safe=False) graph = get_clustering_data_for_graph_display(sqp, results["graph"])