Skip to content

tests/e2e/test_entity_smuggling_e2e.py asserts conditionally and has been passing on an empty loop #620

Closed

Description

@ffroliva

Found during the council review of #618, independently by two reviewers.

The defect

tests/e2e/test_entity_smuggling_e2e.py wraps both of its assertion blocks in conditionals — if bodies: (:75) and if modified: (:83). The test passes identically whether the guard fires or never fires.

Worse, the gate can probably never be true. bodies is populated from the ui_automation.batch_request_body event, emitted by a page.on("request") listener (ui_automation.py:2554). The codebase already records, at ui_automation.py:2881, that:

page-level network capture is dead in this cohort — requests are Web-Worker-delegated, so 0 entries are captured

So the list is plausibly always empty, and the assertion body has never executed.

Why this matters

This is the same defect that let #615 survive for months, one directory over.

In #615 the unit test asserted that a route pattern string had been registered, then invoked the handler by hand — it stayed green while the guard never fired once in production. This e2e has the identical shape: it looks like it verifies the entity-smuggling guard end to end, and it verifies nothing. It was present throughout #615 and did not catch it.

A test that cannot fail is worse than no test, because it stops anyone from writing the one that would.

Fix

Make the empty case an explicit failure rather than a silent pass:

  • if bodies is empty, pytest.fail(...) naming what was expected — do not skip past it
  • same for modified
  • if the page-level observer genuinely cannot see these requests, the test needs a different evidence source. The guard logs ui_automation.batch_request_modified when it strips an entity; asserting on that structlog event is the honest signal, since it is emitted by the handler itself rather than by an observer that cannot see the traffic.

Scope note

This is live-gated (tests/e2e/), so it cannot be validated from a normal branch — which is presumably how it drifted in the first place. Deliberately not changed as a drive-by in #618; it needs a run that can actually exercise it.

Related: #615 (the guard that never ran), #619 (direct-wire routes are not routable at all).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions