Skip to content

fix: [client] Send org_id and give publish_timestamp priority in search_sightings - #43

Draft
elhoim wants to merge 1 commit into
MISP:mainfrom
elhoim:fix/sightings-search
Draft

elhoim wants to merge 1 commit into
MISP:mainfrom
elhoim:fix/sightings-search

Conversation

@elhoim

@elhoim elhoim commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

BLUF — search_sightings sends org_id and lets publish_timestamp win over the deprecated last.

  • Problem — search_sightings in src/client.rs emitted the org filter as org, which Sighting::restSearch never reads, a silent no-op; publish_timestamp and last both wrote the key last, so passing both discarded one by write order.
  • Fix — Emit org_id, and collapse the two via publish_timestamp.or(last) so the canonical value wins in the single last key MISP actually understands; added a wiremock test asserting the request body.
  • Effect — Org-scoped sighting searches actually filter, and the precedence between the two time parameters is defined rather than accidental.
  • CI — Red from a repo-level CI bug (ubuntu-22.04 has no php8.3 mod_php; test_event_crud_lifecycle raced the publish worker), fixed in unmerged PR fix: [ci] Run integration tests on ubuntu-24.04 to match php8.3 #53; not caused by this PR.

Finding

search_sightings sends org filter as org, and publish_timestamp collides with last
MISP Sighting model knows org_id (PyMISP query['org_id']); the org filter is a silent no-op. Separately at :2661 publish_timestamp and last both write key 'last', so passing both discards publish_timestamp.

Change

search_sightings sent the org filter as "org" (a silent no-op) instead of "org_id", and used undocumented last-write-wins precedence between publish_timestamp and last, both mapping to the wire key "last". Fixed by emitting "org_id" and by collapsing publish_timestamp/last via publish_timestamp.or(last), so the canonical publish_timestamp explicitly wins over the deprecated last synonym when both are supplied. Added a wiremock test (there was no existing one for this method) asserting the request body via body_partial_json.

Upstream evidence

MISP core app/Model/Sighting.php:1096-1106 (restSearch reads only $filters['from']/['to'] or a single $filters['last'], defaulting to '30d'; no 'publish_timestamp' key exists) and Sighting.php:1112-1119 (org filter block keys off $filters['org_id'] only, never 'org'). PyMISP pymisp/api.py:3298-3299 confirms query['last'] = publish_timestamp and query['org_id'] = org; the docstring calls last a "deprecated synonym for publish_timestamp" and the function body never reads the last parameter at all, i.e. publish_timestamp is the canonical/wired value.

Verified against MISP core 843b6744 and PyMISP e7debb4f.

Tests

  • search_sightings_sends_org_id_and_prefers_publish_timestamp (src/client.rs, in the existing #[cfg(test)] mod tests block)

Note on the original finding

Partially accurate. The org-filter claim is correct and confirmed (org must be org_id). The publish_timestamp/last collision is real (silent last-write-wins), but the finding's suggested remedy ("give publish_timestamp its own key") is wrong per upstream: Sighting.php's restSearch has no publish_timestamp key at all -- it only understands a single 'last' key (or a from/to pair). Emitting a separate 'publish_timestamp' field would be silently ignored by the real server, reproducing the same class of bug the review is meant to catch. Instead I kept one 'last' key with explicit precedence (publish_timestamp wins over the deprecated last synonym), matching PyMISP's own wiring where only publish_timestamp is ever read into the query.

Caveats

client_blocking.rs's search_sightings is a thin wrapper over the async client and needed no change. examples/sightings.rs call site is unaffected (same signature). tests/integration_tests.rs has a test_sighting_operations test but it requires a live MISP instance and cannot be run in this environment; the wiremock unit test added to src/client.rs is the enforceable regression check for this fix.


Verified locally: cargo fmt --check, cargo clippy --all-targets --all-features -- -D warnings, cargo test --all-features --lib, --test integration_tests --no-run, and cargo check --examples, each in an isolated target directory.

🤖 Generated with Claude Code

…ch_sightings

Sighting::restSearch (MISP core app/Model/Sighting.php) only ever reads
$filters['org_id'] for the organisation filter (Sighting.php:1112-1119) and
a single $filters['last'] key for the relative/absolute time filter
(Sighting.php:1096-1106, defaulting to '30d' when unset). It has no
'org' key and no separate 'publish_timestamp' key at all.

PyMISP's search_sightings (pymisp/api.py:3298-3299) confirms this wiring:
query['last'] = publish_timestamp and query['org_id'] = org. Its own `last`
parameter is a documented "deprecated synonym for publish_timestamp" and is
never read when building the query, i.e. publish_timestamp is canonical.

RustMISP's search_sightings previously sent 'org' (a silent no-op against
Sighting.php) and let whichever of publish_timestamp/last was passed last
silently clobber the 'last' key. Fixed by:
- emitting 'org_id' instead of 'org'
- collapsing publish_timestamp/last into a single 'last' write via
  `publish_timestamp.or(last)`, so publish_timestamp (canonical) wins over
  the deprecated `last` synonym when both are supplied, instead of an
  undocumented last-call-wins order

The finding's suggested fix ("give publish_timestamp its own key") is not
correct: Sighting.php has no such key, so a separate 'publish_timestamp'
field would be silently ignored server-side. Kept a single 'last' key with
explicit precedence instead.

Corrected the wiremock coverage in src/client.rs (there was none for
search_sightings before) to assert the request body carries 'org_id' and
that publish_timestamp wins over `last` via body_partial_json.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JoSButbVaWEtGMXwTx73BZ
@elhoim
elhoim marked this pull request as draft September 3, 2026 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant