{body}
" if body else "", + ) + + +@pytest.fixture +def issues(db, project): + """The motivating case, plus neighbours that must not be swept up.""" + return { + "gateway": _issue( + project, + "Select the payment gateway on Level 3 capability and effective rate", + "Northwind is the incumbent gateway. Compare Northwind-Meridian Level 3 rates before the review.", + ), + "unrelated": _issue(project, "Ship the marketing site", "Nothing to do with payments."), + "decoy": _issue(project, "Rate limit the public API", "Throttling, not billing."), + } + + +def _search(client, slug, term, **params): + response = client.get( + GLOBAL_SEARCH.format(slug=slug), + {"search": term, "workspace_search": "true", "entities": "issue", **params}, + ) + assert response.status_code == status.HTTP_200_OK + return response.json()["results"]["issue"] + + +def _names(results): + return {row["name"] for row in results} + + +@pytest.mark.contract +class TestWorkItemSearchFindsBodies: + def test_a_word_only_in_the_body_is_found(self, session_client, workspace, issues): + """The case the old search could not reach by any phrasing.""" + results = _search(session_client, workspace.slug, "northwind") + assert _names(results) == {issues["gateway"].name} + + def test_search_is_case_insensitive_in_the_body(self, session_client, workspace, issues): + assert len(_search(session_client, workspace.slug, "NORTHWIND")) == 1 + + def test_body_matching_does_not_match_the_markup(self, session_client, workspace, issues): + """Bodies are searched as stripped text, so tag names are not matchable.""" + assert _search(session_client, workspace.slug, "") == [] + + def test_titles_still_match(self, session_client, workspace, issues): + results = _search(session_client, workspace.slug, "marketing") + assert _names(results) == {issues["unrelated"].name} + + +@pytest.mark.contract +class TestWorkItemSearchMatchesWords: + def test_words_need_not_be_adjacent(self, session_client, workspace, issues): + """ "payment" and "gateway" from the title, "review" from the body.""" + results = _search(session_client, workspace.slug, "payment gateway review") + assert _names(results) == {issues["gateway"].name} + + def test_word_order_does_not_matter(self, session_client, workspace, issues): + assert len(_search(session_client, workspace.slug, "gateway payment")) == 1 + + def test_every_word_must_match(self, session_client, workspace, issues): + """Tokens are AND-ed, so one unmatched word rules the record out.""" + assert _search(session_client, workspace.slug, "payment gateway alpaca") == [] + + def test_words_from_different_records_do_not_combine(self, session_client, workspace, issues): + """ "rate" is in the decoy's title, "payment" is not.""" + results = _search(session_client, workspace.slug, "rate payment") + assert _names(results) == {issues["gateway"].name} + + +@pytest.mark.contract +class TestSequenceIdLookup: + def test_a_bare_number_finds_the_work_item(self, session_client, workspace, issues): + target = issues["gateway"] + results = _search(session_client, workspace.slug, str(target.sequence_id)) + assert target.name in _names(results) + + def test_identifier_and_number_together_find_it(self, session_client, workspace, issues): + target = issues["gateway"] + results = _search(session_client, workspace.slug, f"PAY-{target.sequence_id}") + assert target.name in _names(results) + + def test_a_number_inside_a_phrase_still_matches_by_id(self, session_client, workspace, issues): + """Unchanged behaviour, pinned deliberately. + + A number anywhere in the query matches by sequence id, OR-ed onto the + whole predicate, so the work item carrying it comes back even though + neither word matches it. Noisy, but narrowing it would drop results + that match today and this change is a strict superset. + """ + decoy = issues["decoy"] + results = _search(session_client, workspace.slug, f"payment gateway {decoy.sequence_id}") + assert decoy.name in _names(results) + + +@pytest.mark.contract +class TestSearchDoesNotWidenVisibility: + """Searching more fields must not surface more records than the caller may see.""" + + def test_another_tenants_work_item_is_not_returned(self, session_client, workspace, issues): + uid = uuid4().hex[:8] + # username is unique and not auto-populated, so it has to be set here + other_user = User.objects.create(email=f"other-{uid}@plane.so", username=f"other_{uid}") + other_ws = Workspace.objects.create(name="Other WS", owner=other_user, slug=f"other-{uid}") + WorkspaceMember.objects.create(workspace=other_ws, member=other_user, role=20) + other_project = Project.objects.create( + name="Other", identifier="OTH", workspace=other_ws, created_by=other_user + ) + _issue(other_project, "Their gateway work", "Northwind everywhere in this body too.") + + results = _search(session_client, workspace.slug, "northwind") + assert _names(results) == {issues["gateway"].name} + + def test_a_project_in_my_workspace_that_i_am_not_a_member_of_is_not_searched( + self, session_client, workspace, issues, create_user + ): + """Same workspace, different project, no membership row at all. + + Distinct from the cross-tenant case above, which the workspace filter + alone would catch — this one only passes because of the project + membership filter. + """ + uid = uuid4().hex[:8] + other_user = User.objects.create(email=f"peer-{uid}@plane.so", username=f"peer_{uid}") + WorkspaceMember.objects.create(workspace=workspace, member=other_user, role=20) + their_project = Project.objects.create( + name="Theirs", identifier=f"T{uid[:3].upper()}", workspace=workspace, created_by=other_user + ) + ProjectMember.objects.create(project=their_project, member=other_user, role=20, is_active=True) + _issue(their_project, "Their gateway work", "Northwind appears only in this body.") + + results = _search(session_client, workspace.slug, "northwind") + assert _names(results) == {issues["gateway"].name} + + def test_a_project_the_caller_left_is_not_searched(self, session_client, workspace, project, issues, create_user): + ProjectMember.objects.filter(project=project, member=create_user).update(is_active=False) + assert _search(session_client, workspace.slug, "northwind") == [] + + +@pytest.mark.contract +class TestProjectScopedIssueSearch: + """The other endpoint, which goes through plane.utils.issue_search.""" + + def test_body_search_applies_here_too(self, session_client, workspace, project, issues): + response = session_client.get( + PROJECT_ISSUE_SEARCH.format(slug=workspace.slug, project_id=project.id), + {"search": "northwind"}, + ) + assert response.status_code == status.HTTP_200_OK + assert _names(response.json()) == {issues["gateway"].name} + + def test_multi_word_applies_here_too(self, session_client, workspace, project, issues): + response = session_client.get( + PROJECT_ISSUE_SEARCH.format(slug=workspace.slug, project_id=project.id), + {"search": "payment gateway review"}, + ) + assert response.status_code == status.HTTP_200_OK + assert _names(response.json()) == {issues["gateway"].name} + + +@pytest.mark.contract +class TestPageSearch: + """Pages gained body search in the same change; cover the endpoint, not + just the field constant.""" + + def _page(self, workspace, project, owner, name, body): + page = Page.objects.create( + name=name, + workspace=workspace, + owned_by=owner, + access=0, + description_html=f"
{body}
", + ) + ProjectPage.objects.create(project=project, page=page, workspace=workspace) + return page + + def test_a_word_only_in_the_page_body_is_found(self, session_client, workspace, project, create_user): + page = self._page(workspace, project, create_user, "Vendor evaluation", "Northwind pricing notes.") + response = session_client.get( + GLOBAL_SEARCH.format(slug=workspace.slug), + {"search": "northwind", "workspace_search": "true", "entities": "page"}, + ) + assert response.status_code == status.HTTP_200_OK + assert _names(response.json()["results"]["page"]) == {page.name} + + def test_page_titles_still_match(self, session_client, workspace, project, create_user): + page = self._page(workspace, project, create_user, "Runbook", "Nothing relevant here.") + response = session_client.get( + GLOBAL_SEARCH.format(slug=workspace.slug), + {"search": "runbook", "workspace_search": "true", "entities": "page"}, + ) + assert response.status_code == status.HTTP_200_OK + assert page.name in _names(response.json()["results"]["page"]) diff --git a/apps/api/plane/tests/contract/app/test_workspace_cycles_modules_project_scope_app.py b/apps/api/plane/tests/contract/app/test_workspace_cycles_modules_project_scope_app.py index c18acc86406..2a212eb3640 100644 --- a/apps/api/plane/tests/contract/app/test_workspace_cycles_modules_project_scope_app.py +++ b/apps/api/plane/tests/contract/app/test_workspace_cycles_modules_project_scope_app.py @@ -42,9 +42,7 @@ def project(db, workspace, create_user): workspace=workspace, created_by=create_user, ) - ProjectMember.objects.create( - project=project, member=create_user, workspace=workspace, role=20 - ) + ProjectMember.objects.create(project=project, member=create_user, workspace=workspace, role=20) return project diff --git a/apps/api/plane/tests/contract/app/test_workspace_file_asset_project_scope_app.py b/apps/api/plane/tests/contract/app/test_workspace_file_asset_project_scope_app.py index 8845fbeda77..709cc4af141 100644 --- a/apps/api/plane/tests/contract/app/test_workspace_file_asset_project_scope_app.py +++ b/apps/api/plane/tests/contract/app/test_workspace_file_asset_project_scope_app.py @@ -43,9 +43,7 @@ def project(db, workspace, create_user): workspace=workspace, created_by=create_user, ) - ProjectMember.objects.create( - project=project, member=create_user, workspace=workspace, role=20 - ) + ProjectMember.objects.create(project=project, member=create_user, workspace=workspace, role=20) return project @@ -67,9 +65,7 @@ def outsider_user(db): @pytest.fixture def outsider_client(db, workspace, outsider_user): """Session client for a workspace member who is not in ``project``.""" - WorkspaceMember.objects.create( - workspace=workspace, member=outsider_user, role=15 - ) + WorkspaceMember.objects.create(workspace=workspace, member=outsider_user, role=15) client = APIClient() client.force_authenticate(user=outsider_user) return client @@ -115,17 +111,13 @@ class TestWorkspaceFileAssetProjectScope: """A workspace member who is not in the asset's project must be blocked.""" @pytest.mark.django_db - def test_get_project_asset_denied_for_non_project_member( - self, outsider_client, workspace, project_asset - ): + def test_get_project_asset_denied_for_non_project_member(self, outsider_client, workspace, project_asset): """GET on a project asset by a non-project-member must 403, not mint a presigned download URL.""" url = detail_url(workspace.slug, project_asset.id) with mock.patch(S3_STORAGE_PATH) as mock_storage: - mock_storage.return_value.generate_presigned_url.return_value = ( - "https://signed.example/download" - ) + mock_storage.return_value.generate_presigned_url.return_value = "https://signed.example/download" response = outsider_client.get(url) assert response.status_code == status.HTTP_403_FORBIDDEN, ( @@ -134,18 +126,14 @@ def test_get_project_asset_denied_for_non_project_member( mock_storage.return_value.generate_presigned_url.assert_not_called() @pytest.mark.django_db - def test_patch_project_asset_denied_for_non_project_member( - self, outsider_client, workspace, project_asset - ): + def test_patch_project_asset_denied_for_non_project_member(self, outsider_client, workspace, project_asset): """PATCH on a project asset by a non-project-member must 403 and leave the asset untouched.""" url = detail_url(workspace.slug, project_asset.id) project_asset.is_uploaded = False project_asset.save(update_fields=["is_uploaded"]) - response = outsider_client.patch( - url, {"attributes": {"name": "hacked.pdf"}}, format="json" - ) + response = outsider_client.patch(url, {"attributes": {"name": "hacked.pdf"}}, format="json") assert response.status_code == status.HTTP_403_FORBIDDEN, ( f"Got {response.status_code}: {getattr(response, 'data', None)!r}" @@ -155,9 +143,7 @@ def test_patch_project_asset_denied_for_non_project_member( assert project_asset.attributes.get("name") == "secret.pdf" @pytest.mark.django_db - def test_delete_project_asset_denied_for_non_project_member( - self, outsider_client, workspace, project_asset - ): + def test_delete_project_asset_denied_for_non_project_member(self, outsider_client, workspace, project_asset): """DELETE on a project asset by a non-project-member must 403 and must not soft-delete the asset.""" url = detail_url(workspace.slug, project_asset.id) @@ -171,17 +157,13 @@ def test_delete_project_asset_denied_for_non_project_member( assert project_asset.is_deleted is False @pytest.mark.django_db - def test_get_project_asset_allowed_for_project_member( - self, session_client, workspace, project_asset - ): + def test_get_project_asset_allowed_for_project_member(self, session_client, workspace, project_asset): """Positive control: an active project member can still download the asset, so the fix does not over-block legitimate callers.""" url = detail_url(workspace.slug, project_asset.id) with mock.patch(S3_STORAGE_PATH) as mock_storage: - mock_storage.return_value.generate_presigned_url.return_value = ( - "https://signed.example/download" - ) + mock_storage.return_value.generate_presigned_url.return_value = "https://signed.example/download" response = session_client.get(url) assert response.status_code == status.HTTP_302_FOUND, ( @@ -198,9 +180,7 @@ def test_get_workspace_level_asset_allowed_for_non_project_member( url = detail_url(workspace.slug, workspace_logo_asset.id) with mock.patch(S3_STORAGE_PATH) as mock_storage: - mock_storage.return_value.generate_presigned_url.return_value = ( - "https://signed.example/download" - ) + mock_storage.return_value.generate_presigned_url.return_value = "https://signed.example/download" response = outsider_client.get(url) assert response.status_code == status.HTTP_302_FOUND, ( diff --git a/apps/api/plane/tests/contract/app/test_workspace_user_preference_app.py b/apps/api/plane/tests/contract/app/test_workspace_user_preference_app.py index e2ef308b82f..534bc1c6550 100644 --- a/apps/api/plane/tests/contract/app/test_workspace_user_preference_app.py +++ b/apps/api/plane/tests/contract/app/test_workspace_user_preference_app.py @@ -48,9 +48,7 @@ def test_patch_only_updates_requesting_users_preference(self, session_client, cr WorkspaceUserPreference.objects.filter(pk=other_pref.pk).update(created_at=now + timedelta(minutes=1)) url = reverse("workspace-user-preference", kwargs={"slug": workspace.slug}) - response = session_client.patch( - url, [{"key": self.KEY, "is_pinned": True, "sort_order": 999}], format="json" - ) + response = session_client.patch(url, [{"key": self.KEY, "is_pinned": True, "sort_order": 999}], format="json") assert response.status_code == status.HTTP_200_OK @@ -72,9 +70,7 @@ def test_patch_updates_own_preference(self, session_client, create_user, workspa ) url = reverse("workspace-user-preference", kwargs={"slug": workspace.slug}) - response = session_client.patch( - url, [{"key": self.KEY, "is_pinned": True, "sort_order": 42}], format="json" - ) + response = session_client.patch(url, [{"key": self.KEY, "is_pinned": True, "sort_order": 42}], format="json") assert response.status_code == status.HTTP_200_OK diff --git a/apps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py b/apps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py index ea4adbc1c20..294cbdf4c67 100644 --- a/apps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py +++ b/apps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py @@ -70,14 +70,14 @@ class TestWebhookUrlValidation: "ip", [ "169.254.169.254", # AWS/GCP metadata (CVE-2026-30242 PoC) - "127.0.0.1", # loopback - "10.0.0.1", # private - "172.16.0.1", # private - "192.168.0.1", # private - "::1", # IPv6 loopback - "100.64.0.1", # CGNAT / RFC 6598 (GHSA-75fg) - "2002:7f00:1::", # 6to4 -> 127.0.0.1 (GHSA-75fg) - "224.0.0.1", # multicast (GHSA-75fg) + "127.0.0.1", # loopback + "10.0.0.1", # private + "172.16.0.1", # private + "192.168.0.1", # private + "::1", # IPv6 loopback + "100.64.0.1", # CGNAT / RFC 6598 (GHSA-75fg) + "2002:7f00:1::", # 6to4 -> 127.0.0.1 (GHSA-75fg) + "224.0.0.1", # multicast (GHSA-75fg) "::ffff:169.254.169.254", # IPv4-mapped metadata ], ) @@ -173,9 +173,7 @@ def test_webhook_does_not_follow_redirects(self, mock_resolve, mock_session_cls) mock_resolve.return_value = ["93.184.216.34"] session = mock_session_cls.return_value # The endpoint replies 302 -> internal; the webhook client must NOT follow. - session.request.return_value = _resp( - 302, headers={"Location": "http://169.254.169.254/latest/meta-data/"} - ) + session.request.return_value = _resp(302, headers={"Location": "http://169.254.169.254/latest/meta-data/"}) resp = pinned_fetch("POST", "https://hooks.example.com/x", json={}) @@ -195,17 +193,13 @@ class TestFaviconRedirect: @patch("plane.utils.url_security.requests.Session") @patch("plane.utils.url_security.resolve_and_validate") @patch("plane.bgtasks.work_item_link_task.socket.getaddrinfo") - def test_favicon_redirect_to_private_returns_default( - self, mock_pre_dns, mock_resolve, mock_session_cls - ): + def test_favicon_redirect_to_private_returns_default(self, mock_pre_dns, mock_resolve, mock_session_cls): # validate_url_ip pre-check (work_item_link_task.socket) sees a public IP. mock_pre_dns.return_value = [_addr("93.184.216.34")] # safe_get: hop0 public, hop1 (redirect target) blocked. mock_resolve.side_effect = [["93.184.216.34"], ValueError(_BLOCKED)] session = mock_session_cls.return_value - session.request.return_value = _resp( - 302, headers={"Location": "http://192.168.8.14:8081/"} - ) + session.request.return_value = _resp(302, headers={"Location": "http://192.168.8.14:8081/"}) soup = BeautifulSoup( '', @@ -227,11 +221,9 @@ class TestFaviconRebinding: @patch("plane.utils.url_security.requests.Session") @patch("plane.utils.url_security.resolve_and_validate") @patch("plane.bgtasks.work_item_link_task.socket.getaddrinfo") - def test_favicon_rebind_to_private_returns_default( - self, mock_pre_dns, mock_resolve, mock_session_cls - ): + def test_favicon_rebind_to_private_returns_default(self, mock_pre_dns, mock_resolve, mock_session_cls): mock_pre_dns.return_value = [_addr("93.184.216.34")] # pre-check: public - mock_resolve.side_effect = ValueError(_BLOCKED) # fetch-time: rebound -> blocked + mock_resolve.side_effect = ValueError(_BLOCKED) # fetch-time: rebound -> blocked session = mock_session_cls.return_value session.request.return_value = _resp(200) @@ -268,12 +260,8 @@ def test_avatar_redirect_to_internal_is_blocked(self, mock_resolve, mock_session # Public avatar URL that 302-redirects to the metadata service. mock_resolve.side_effect = [["93.184.216.34"], ValueError(_BLOCKED)] session = mock_session_cls.return_value - session.request.return_value = _resp( - 302, headers={"Location": "http://169.254.169.254/imds"} - ) - result = self._adapter().download_and_upload_avatar( - "https://evil.example.com/avatar", user=MagicMock() - ) + session.request.return_value = _resp(302, headers={"Location": "http://169.254.169.254/imds"}) + result = self._adapter().download_and_upload_avatar("https://evil.example.com/avatar", user=MagicMock()) assert result is None @patch("plane.authentication.adapter.base.pinned_fetch_following_redirects") @@ -281,9 +269,7 @@ def test_avatar_uses_ssrf_safe_client(self, mock_fetch): # Wiring guard: the avatar path must go through the pinned client, never # a raw requests.get (which would re-resolve + follow redirects freely). mock_fetch.side_effect = ValueError(_BLOCKED) - result = self._adapter().download_and_upload_avatar( - "https://cdn.example.com/a.png", user=MagicMock() - ) + result = self._adapter().download_and_upload_avatar("https://cdn.example.com/a.png", user=MagicMock()) assert result is None assert mock_fetch.call_args.args[0] == "GET" assert mock_fetch.call_args.args[1] == "https://cdn.example.com/a.png" diff --git a/apps/api/plane/tests/unit/bg_tasks/test_url_security.py b/apps/api/plane/tests/unit/bg_tasks/test_url_security.py index 3a1e8d3dba1..31b3a268499 100644 --- a/apps/api/plane/tests/unit/bg_tasks/test_url_security.py +++ b/apps/api/plane/tests/unit/bg_tasks/test_url_security.py @@ -186,9 +186,7 @@ def test_ipv6_validated_ip_is_bracketed(self, mock_resolve, mock_session_cls): @patch("plane.utils.url_security.resolve_and_validate") def test_blocked_target_raises_before_any_request(self, mock_resolve): - mock_resolve.side_effect = ValueError( - "Access to private/internal networks is not allowed" - ) + mock_resolve.side_effect = ValueError("Access to private/internal networks is not allowed") with pytest.raises(ValueError, match="private/internal"): pinned_fetch("POST", "https://attacker.com/hook") @@ -275,9 +273,7 @@ def test_blocks_redirect_to_private_ip(self, mock_resolve, mock_session_cls): ValueError("Access to private/internal networks is not allowed"), ] session = mock_session_cls.return_value - session.request.return_value = _resp( - 302, headers={"Location": "http://169.254.169.254/latest/meta-data/"} - ) + session.request.return_value = _resp(302, headers={"Location": "http://169.254.169.254/latest/meta-data/"}) with pytest.raises(ValueError, match="private/internal"): pinned_fetch_following_redirects("GET", "https://evil.com/r") @@ -286,13 +282,9 @@ def test_blocks_redirect_to_private_ip(self, mock_resolve, mock_session_cls): def test_too_many_redirects(self, mock_resolve, mock_session_cls): mock_resolve.return_value = ["93.184.216.34"] session = mock_session_cls.return_value - session.request.return_value = _resp( - 302, headers={"Location": "https://example.com/loop"} - ) + session.request.return_value = _resp(302, headers={"Location": "https://example.com/loop"}) with pytest.raises(requests.TooManyRedirects): - pinned_fetch_following_redirects( - "GET", "https://example.com/start", max_redirects=3 - ) + pinned_fetch_following_redirects("GET", "https://example.com/start", max_redirects=3) # --------------------------------------------------------------------------- diff --git a/apps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.py b/apps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.py index 2599126ff49..e29b66b7e1a 100644 --- a/apps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.py +++ b/apps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.py @@ -196,9 +196,7 @@ def test_blocks_redirect_to_private_ip(self, mock_resolve, mock_session_cls): ValueError("Access to private/internal networks is not allowed"), ] session = mock_session_cls.return_value - session.request.return_value = _make_response( - status_code=302, headers={"Location": "http://192.168.1.1:8080"} - ) + session.request.return_value = _make_response(status_code=302, headers={"Location": "http://192.168.1.1:8080"}) with pytest.raises(ValueError, match="private/internal"): safe_get("https://evil.com/redirect") @@ -208,9 +206,7 @@ def test_blocks_redirect_to_private_ip(self, mock_resolve, mock_session_cls): def test_raises_on_too_many_redirects(self, mock_resolve, mock_session_cls): mock_resolve.return_value = ["93.184.216.34"] session = mock_session_cls.return_value - session.request.return_value = _make_response( - status_code=302, headers={"Location": "https://example.com/loop"} - ) + session.request.return_value = _make_response(status_code=302, headers={"Location": "https://example.com/loop"}) with pytest.raises(requests.TooManyRedirects): safe_get("https://example.com/start") diff --git a/apps/api/plane/tests/unit/middleware/test_api_authentication.py b/apps/api/plane/tests/unit/middleware/test_api_authentication.py index 5f67537c228..9947437e276 100644 --- a/apps/api/plane/tests/unit/middleware/test_api_authentication.py +++ b/apps/api/plane/tests/unit/middleware/test_api_authentication.py @@ -23,9 +23,7 @@ class TestAPIKeyAuthentication: @pytest.mark.django_db def test_validate_api_token_authenticates_active_user(self, create_user): - token = APIToken.objects.create( - user=create_user, label="Active Token", token="active-user-token" - ) + token = APIToken.objects.create(user=create_user, label="Active Token", token="active-user-token") user, returned_token = APIKeyAuthentication().validate_api_token(token.token) @@ -34,9 +32,7 @@ def test_validate_api_token_authenticates_active_user(self, create_user): @pytest.mark.django_db def test_validate_api_token_rejects_deactivated_user(self, create_user): - token = APIToken.objects.create( - user=create_user, label="Stale Token", token="deactivated-user-token" - ) + token = APIToken.objects.create(user=create_user, label="Stale Token", token="deactivated-user-token") # Account is deactivated by an administrator after the token was issued. create_user.is_active = False diff --git a/apps/api/plane/tests/unit/middleware/test_logger.py b/apps/api/plane/tests/unit/middleware/test_logger.py index 5c13f53f6f7..31c4131530c 100644 --- a/apps/api/plane/tests/unit/middleware/test_logger.py +++ b/apps/api/plane/tests/unit/middleware/test_logger.py @@ -56,9 +56,7 @@ def _captured_log_data(self, middleware, request_factory): def test_token_identifier_is_hashed_not_plaintext(self, middleware, request_factory): log_data = self._captured_log_data(middleware, request_factory) - expected_hash = hmac.new( - settings.SECRET_KEY.encode(), self.API_KEY.encode(), hashlib.sha256 - ).hexdigest() + expected_hash = hmac.new(settings.SECRET_KEY.encode(), self.API_KEY.encode(), hashlib.sha256).hexdigest() assert log_data["token_identifier"] == expected_hash assert self.API_KEY not in log_data["token_identifier"] diff --git a/apps/api/plane/tests/unit/utils/test_order_by_sanitize.py b/apps/api/plane/tests/unit/utils/test_order_by_sanitize.py index 12fde9376bf..fa15c4c1c78 100644 --- a/apps/api/plane/tests/unit/utils/test_order_by_sanitize.py +++ b/apps/api/plane/tests/unit/utils/test_order_by_sanitize.py @@ -50,36 +50,24 @@ class TestProjectOrderBySanitization: def test_injection_payload_falls_back_to_default(self, payload): """Any non-allowlisted / relational value is replaced with the endpoint's safe default instead of reaching .order_by().""" - assert ( - sanitize_order_by(payload, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == self.DEFAULT - ) + assert sanitize_order_by(payload, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == self.DEFAULT @pytest.mark.parametrize( "value", ["created_at", "updated_at", "name", "network", "sort_order"], ) def test_legitimate_ascending_values_pass_through(self, value): - assert ( - sanitize_order_by(value, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == value - ) + assert sanitize_order_by(value, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == value @pytest.mark.parametrize( "value", ["-created_at", "-updated_at", "-name", "-network", "-sort_order"], ) def test_legitimate_descending_values_pass_through(self, value): - assert ( - sanitize_order_by(value, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == value - ) + assert sanitize_order_by(value, PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == value def test_empty_value_uses_default(self): - assert ( - sanitize_order_by("", PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == self.DEFAULT - ) + assert sanitize_order_by("", PROJECT_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == self.DEFAULT @pytest.mark.unit @@ -91,10 +79,7 @@ class TestIssueOrderBySanitization: @pytest.mark.parametrize("payload", INJECTION_PAYLOADS) def test_injection_payload_falls_back_to_default(self, payload): - assert ( - sanitize_order_by(payload, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == self.DEFAULT - ) + assert sanitize_order_by(payload, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == self.DEFAULT @pytest.mark.parametrize( "value", @@ -115,18 +100,9 @@ def test_injection_payload_falls_back_to_default(self, payload): def test_legitimate_values_pass_through(self, value): """Every value the endpoint's branch logic special-cases must survive sanitization, otherwise legitimate ordering would silently break.""" - assert ( - sanitize_order_by(value, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == value - ) + assert sanitize_order_by(value, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == value # Descending variant is equally valid. - assert ( - sanitize_order_by(f"-{value}", ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == f"-{value}" - ) + assert sanitize_order_by(f"-{value}", ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == f"-{value}" def test_default_is_preserved_for_missing_param(self): - assert ( - sanitize_order_by(None, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) - == self.DEFAULT - ) + assert sanitize_order_by(None, ISSUE_ORDER_BY_ALLOWLIST, default=self.DEFAULT) == self.DEFAULT diff --git a/apps/api/plane/tests/unit/utils/test_search.py b/apps/api/plane/tests/unit/utils/test_search.py new file mode 100644 index 00000000000..eda71138a2f --- /dev/null +++ b/apps/api/plane/tests/unit/utils/test_search.py @@ -0,0 +1,219 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +import pytest +from django.db.models import Q + +from plane.utils.search import ( + ISSUE_SEARCH_FIELDS, + ISSUE_SEQUENCE_FIELDS, + MAX_SEARCH_TOKENS, + PAGE_SEARCH_FIELDS, + build_search_query, +) + + +def _children(q): + """Flatten a Q tree into the set of leaf lookups it applies.""" + leaves = set() + for child in q.children: + if isinstance(child, Q): + leaves |= _children(child) + else: + leaves.add(child) + return leaves + + +@pytest.mark.unit +class TestBuildSearchQuery: + """Multi-word queries must match on words, not on one contiguous string.""" + + def test_empty_query_matches_nothing(self): + assert build_search_query("", fields=["name"]) == Q() + assert build_search_query(None, fields=["name"]) == Q() + assert build_search_query(" ", fields=["name"]) == Q() + + def test_single_token_ors_across_fields(self): + q = build_search_query("northwind", fields=["name", "description_stripped"]) + assert _children(q) == { + ("name__icontains", "northwind"), + ("description_stripped__icontains", "northwind"), + } + assert q.connector == Q.OR + + def test_tokens_are_anded_not_matched_as_a_phrase(self): + q = build_search_query("payment gateway", fields=["name"]) + # Each token contributes its own leaf; the phrase itself is never a leaf + assert _children(q) == { + ("name__icontains", "payment"), + ("name__icontains", "gateway"), + } + assert ("name__icontains", "payment gateway") not in _children(q) + assert q.connector == Q.AND + + def test_word_order_and_interleaving_are_irrelevant(self): + forward = build_search_query("payment gateway", fields=["name"]) + reversed_ = build_search_query("gateway payment", fields=["name"]) + assert _children(forward) == _children(reversed_) + + def test_repeated_whitespace_does_not_create_empty_tokens(self): + q = build_search_query(" payment gateway \n", fields=["name"]) + assert _children(q) == { + ("name__icontains", "payment"), + ("name__icontains", "gateway"), + } + + def test_numeric_token_also_matches_sequence_id(self): + q = build_search_query( + "22", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert ("sequence_id", "22") in _children(q) + + def test_sequence_match_is_ored_onto_the_whole_predicate(self): + """A bare issue number reaches the issue regardless of its title.""" + q = build_search_query( + "22", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert q.connector == Q.OR + assert ("sequence_id", "22") in _children(q) + + def test_identifier_and_number_in_one_token_still_matches_by_number(self): + q = build_search_query( + "PAY-22", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert ("sequence_id", "22") in _children(q) + + def test_sequence_lookup_still_applies_to_multi_word_queries(self): + """Unchanged from before this refactor, and deliberately so. + + A number anywhere in the query still matches by sequence id, OR-ed onto + the whole predicate, so a work item carrying that number is returned + even when the words do not match it. That is noisy — "level 3 rate" + returns every work item numbered 3 — but narrowing it would remove + results that match today, and this change is meant to be a strict + superset. Pinned here so the behaviour is a decision rather than an + accident. + """ + q = build_search_query( + "level 3 rate", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert ("sequence_id", "3") in _children(q) + assert q.connector == Q.OR + + def test_decimals_do_not_produce_sequence_matches(self): + q = build_search_query( + "3.5", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert ("sequence_id", "3") not in _children(q) + assert ("sequence_id", "5") not in _children(q) + + def test_leading_dot_decimals_do_not_produce_sequence_matches(self): + """ ".5" is a decimal, not work item number 5.""" + q = build_search_query( + ".5", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert ("sequence_id", "5") not in _children(q) + + def test_version_strings_do_not_produce_sequence_matches(self): + q = build_search_query( + "v1.4.0", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert not any(lookup == "sequence_id" for lookup, _ in _children(q)) + + def test_trailing_punctuation_still_matches_a_sequence_id(self): + """A trailing dot is sentence punctuation, not a decimal point.""" + q = build_search_query( + "22.", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert ("sequence_id", "22") in _children(q) + + def test_long_queries_are_not_mined_for_sequence_ids(self): + query = "the payment gateway on level 3 and its effective rate" + q = build_search_query( + query, + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + sequence_query_max_length=20, + ) + assert ("sequence_id", "3") not in _children(q) + + def test_no_sequence_fields_yields_no_sequence_leaves(self): + q = build_search_query("22", fields=["name"]) + assert _children(q) == {("name__icontains", "22")} + + +@pytest.mark.unit +class TestTokenBudget: + """One predicate per token per field, so the token count has to be bounded. + + The predicate this replaced was a single icontains over the whole query — + constant size no matter how long the query was. Tokenizing removes that + property, so a request could otherwise build arbitrarily large SQL. + """ + + def test_predicate_size_is_proportional_to_tokens(self): + fields = ["name", "description_stripped"] + q = build_search_query("alpha beta gamma", fields=fields) + assert len(_children(q)) == 3 * len(fields) + + def test_tokens_at_the_limit_are_all_used(self): + fields = ["name"] + tokens = [f"t{i}" for i in range(MAX_SEARCH_TOKENS)] + q = build_search_query(" ".join(tokens), fields=fields) + assert len(_children(q)) == MAX_SEARCH_TOKENS + + def test_tokens_beyond_the_limit_are_dropped(self): + fields = ["name"] + tokens = [f"t{i}" for i in range(MAX_SEARCH_TOKENS + 50)] + q = build_search_query(" ".join(tokens), fields=fields) + assert len(_children(q)) == MAX_SEARCH_TOKENS + + def test_a_pathological_query_stays_bounded(self): + """The case the bound exists for: thousands of tokens, many fields.""" + fields = ISSUE_SEARCH_FIELDS + q = build_search_query(" ".join(str(i) for i in range(5000)), fields=fields) + assert len(_children(q)) <= MAX_SEARCH_TOKENS * len(fields) + + +@pytest.mark.unit +class TestSearchableFields: + """Bodies are searchable, not just titles.""" + + def test_issues_search_their_description(self): + assert "description_stripped" in ISSUE_SEARCH_FIELDS + + def test_pages_search_their_description(self): + assert "description_stripped" in PAGE_SEARCH_FIELDS + + def test_a_word_only_in_the_body_is_matchable(self): + q = build_search_query( + "northwind", + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + ) + assert ("description_stripped__icontains", "northwind") in _children(q) + + def test_words_split_across_title_and_body_still_match(self): + """ "payment" and "gateway" from the title, "review" from the body.""" + q = build_search_query("payment gateway review", fields=ISSUE_SEARCH_FIELDS) + leaves = _children(q) + for token in ("payment", "gateway", "review"): + assert ("name__icontains", token) in leaves + assert ("description_stripped__icontains", token) in leaves diff --git a/apps/api/plane/utils/issue_search.py b/apps/api/plane/utils/issue_search.py index 7e5fab8fea3..54c36a2e7eb 100644 --- a/apps/api/plane/utils/issue_search.py +++ b/apps/api/plane/utils/issue_search.py @@ -2,23 +2,23 @@ # SPDX-License-Identifier: AGPL-3.0-only # See the LICENSE file for details. -# Python imports -import re - -# Django imports -from django.db.models import Q - # Module imports +from plane.utils.search import ( + ISSUE_SEARCH_FIELDS, + ISSUE_SEQUENCE_FIELDS, + build_search_query, +) + +# Queries longer than this are treated as prose and not mined for sequence ids +SEQUENCE_QUERY_MAX_LENGTH = 20 def search_issues(query, queryset): - fields = ["name", "sequence_id", "project__identifier"] - q = Q() - for field in fields: - if field == "sequence_id" and len(query) <= 20: - sequences = re.findall(r"\b\d+\b", query) - for sequence_id in sequences: - q |= Q(**{"sequence_id": sequence_id}) - else: - q |= Q(**{f"{field}__icontains": query}) - return queryset.filter(q).distinct() + return queryset.filter( + build_search_query( + query, + fields=ISSUE_SEARCH_FIELDS, + sequence_fields=ISSUE_SEQUENCE_FIELDS, + sequence_query_max_length=SEQUENCE_QUERY_MAX_LENGTH, + ) + ).distinct() diff --git a/apps/api/plane/utils/search.py b/apps/api/plane/utils/search.py new file mode 100644 index 00000000000..17ef894926d --- /dev/null +++ b/apps/api/plane/utils/search.py @@ -0,0 +1,102 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Python imports +import re + +# Django imports +from django.db.models import Q + +# Match whole integers only. The lookaround excludes the components of a +# decimal: a plain \b\d+\b treats the dot in "3.5" as a word boundary and +# yields both 3 and 5, so searching a version string surfaced unrelated issues +# by sequence id. Any digit preceded by a dot is part of a decimal too — ".5" +# must not yield 5. A trailing dot not followed by a digit ("issue 22.") is +# sentence punctuation, and 22 stays matchable. +SEQUENCE_PATTERN = re.compile(r"(?