[Maffooch]

Description

GET /api/v2/findings/?engagement=<id> was not a filter. A Finding has no engagement field and ApiFindingFilter declared no filter by that name (only test__engagement), so django-filter dropped the parameter without an error and the list answered with every finding the caller could see. A request shaped ?engagement=<id>&limit=200 therefore paged through the whole visible set instead of one engagement. ?product= and ?prod_type= had the same problem.

This declares engagement, product and prod_type on ApiFindingFilter as short names for test__engagement, test__engagement__product and test__engagement__product__prod_type: the same NumberInFilter on the same path, so each takes a comma-separated list of ids, ORs within the parameter, ANDs with every other filter, and rejects a non-integer with the same 400 as the long form. The filters narrow the queryset the view has already authorized, so nobody sees more than before. A query parameter that is not a filter is still ignored, as before.

The English Findings pages (OS and Pro) now mention the three parameters, and the 3.5.x upgrade page describes the behavior change for clients that already send them.

Test results

New unittests/test_api_finding_relation_filters.py (9 tests): the three names are declared on the long-form fields, each narrows to the right findings, each matches its long form, comma-separated lists, combination with severity and with each other, non-integer values are invalid, and an API list with limit/offset reports the scoped count and pages while an unknown parameter is still ignored, and a Reader on one product who asks for another product's engagement, product or product type gets an empty page with count 0. Without the filter change 11 assertions fail; with it, all 9 tests pass (python manage.py test unittests.test_api_finding_relation_filters). ruff check (0.16.10) is clean on the changed files.

Documentation

docs/content/asset_modelling/engagements_tests/OS__findings.md and PRO__findings.md (English), and docs/content/releases/os_upgrading/3.5.md.

[Maffooch]

Adversarial review: full tier

Verdict: changes requested, no blockers. The filter change is correct and does not widen what a caller can see. Two this-pr findings remain: how the behavior change is announced, and a missing non-superuser test.

I posted no approval and did not merge.

CI (read, not re-run)

All 49 checks pass, the unit tests included, so the new unittests/test_api_finding_relation_filters.py runs green in CI.

What I verified

• Root cause. Before this change, ApiFindingFilter declares test__engagement, test__engagement__product and test__engagement__product__prod_type, but no engagement, product or prod_type. A Finding has no fields by those names, and django-filter drops undeclared parameters without an error, so ?engagement=<id> was ignored and the list returned every finding the caller could see. The new declarations use the same NumberInFilter, path and lookup as the long forms.
• Authorization does not widen, and existence does not leak. NumberInFilter runs no queryset or choice validation, so an id outside the caller's scope and an id that does not exist both return 200 with an empty page, and nothing tells them apart. The filter narrows the queryset the view has already authorized.
• Repeated parameters (?engagement=1&engagement=2): BaseCSVWidget reads QueryDict.get, so the last value wins, which is the same as the long form. Comma lists OR within the parameter. A blank value applies no filter. A non-numeric value is a 400, which the PR's test covers. These all behave like the long form.
• Combinations. The aliases AND with every other filter, and with their own long form, so engagement=1&test__engagement=2 returns nothing. count and next follow the filtered set, which the PR's limit/offset test covers.
• Field order. I ran django-filter 26.2's FilterSet metaclass in a throwaway container. The aliases appear in the form right after test__engagement__product__prod_type, and a subclass that redeclares them keeps that position.
• Performance. The filter is a plain join, dojo_test.engagement_id IN (...), then dojo_finding.test_id. On a synthetic schema of 2M findings, 100k tests and 20k engagements (a scratch database I created and then dropped), a count for one engagement took 2.5 ms and a count for one product took 16 ms. Both used indexes, with no sequential scan.

Findings

• R1 suggestion this-pr docs/content/releases/os_upgrading/3.5.md: the behavior change is real. A client that sent ?engagement=, ?product= or ?prod_type= used to page through every finding it could see. It now gets a scoped list, with a different count and different pages. For example, a script that "exported findings for engagement X" and quietly wrote out every finding now writes out only X's findings. The only announcement is one sentence on the two Findings docs pages describing the new behavior, and nothing says it changed. The 3.5.x upgrade page, where an operator looks, still says "There are no special instructions". Add a short note there saying that these three parameters on /api/v2/findings/ were ignored before and are now applied, so a client that sent them should expect fewer results.
• R2 nit this-pr unittests/test_api_finding_relation_filters.py:30: every test runs as a superuser (is_superuser=True). Add one case with a non-superuser who is a member of only product A. Asking for product B's engagement or product type should return an empty page with count 0, and asking for their own should still return it. That pins "does not widen", which the description promises.

Not covered

I did not drive the endpoint against a running instance or a production-sized database. The performance figures come from a synthetic schema that has only the join columns.

🤖 Generated with Claude Code

[Maffooch]

Review findings: resolution

• R1 fixed in e49d5b5: docs/content/releases/os_upgrading/3.5.md has a new section, "Findings API: engagement, product and prod_type filters now apply". It says the three parameters on /api/v2/findings/ were ignored before and are applied now. A client that sends them should expect a smaller count and different pages, and a non-numeric value now gets a 400. Clients that use the test__engagement* parameters see no change.
• R2 fixed in e49d5b5: new test test_api_does_not_widen_for_a_member_of_one_product. The user is a Reader on product A only. Asking for product B's engagement, product or product type returns 200 with count 0 and no results. Asking for product A's engagement A2 still returns exactly A2's findings. python manage.py test unittests.test_api_finding_relation_filters: 9 tests, OK. The branch also merges the current dev (09aeb59).