Repository navigation
Conversation
`flexible_bool`/`flexible_bool_opt` decided truthiness of a JSON number with `n.as_i64() != Some(0)`. `serde_json::Number::as_i64()` returns `None` for any non-integer number (e.g. a JSON float `0.0` or `1.0`), so `None != Some(0)` evaluated to `true` — a JSON `0.0` silently deserialized to `true`, the opposite of what it represents. MISP's `Attribute.to_ids` (and other flags routed through these helpers) is validated server-side with CakePHP's `boolean` rule (app/Model/MispAttribute.php:427-431 @ 843b67445060e4da71ebf75cc4a9646c301f749d), so a compliant MISP instance always emits 0/1/true/false, never a float, for these fields. This fix is therefore defensive robustness for malformed/proxied payloads rather than a wire-protocol correction, but the inversion is real for any input that does hit this path. Fixed by deciding truthiness from `Number::as_f64()` (always `Some` for a standard, non-arbitrary-precision `serde_json::Number`) compared against `0.0`, with the previous `true` fallback preserved for the [theoretical, feature-gated] case where `as_f64()` returns `None`. Added tests covering `0.0`/`1.0`/`2.5` for both `flexible_bool` and `flexible_bool_opt`, which fail against the old `as_i64()` check and pass with the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JoSButbVaWEtGMXwTx73BZ
elhoim
marked this pull request as draft
September 3, 2026 05:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BLUF —
flexible_boolturned a JSON0.0intotrue; truthiness now goes throughas_f64().flexible_boolandflexible_bool_optinsrc/models/serde_helpers.rs(:132, :169) testedn.as_i64() != Some(0), butas_i64()returnsNonefor any non-integer number, so a JSON float0.0deserialized astrue.Number::as_f64() != 0.0, keeping fallback-to-true only whereas_f64()itself returnsNone. Adds tests for0.0,1.0and2.5.to_ids,publishedanddeletedcan no longer be inverted by malformed or proxied JSON. A compliant MISP emits only0/1, so this is hardening.ubuntu-22.04runner with no php8.3mod_php, plus a publish-worker race intest_event_crud_lifecycle), fixed in PR fix: [ci] Run integration tests on ubuntu-24.04 to match php8.3 #53 and unrelated to this PR.Finding
n.as_i64() != Some(0) returns true when as_i64() is None
A JSON float 0.0 deserializes to true - a silent inversion rather than an error. Affects :132 and :169.
Change
flexible_bool/flexible_bool_opt in src/models/serde_helpers.rs used
n.as_i64() != Some(0)to decide truthiness of a JSON number. serde_json::Number::as_i64() returns None for any non-integer number, so a JSON float like 0.0 producedNone != Some(0)= true, silently inverting the value. Fixed both the :132 (flexible_bool) and :169 (flexible_bool_opt) sites to decide truthiness viaNumber::as_f64() != 0.0instead, preserving the previous fallback-to-true only for the theoretical case where as_f64() itself returns None (arbitrary-precision feature). Added unit tests for 0.0/1.0/2.5 on both helpers.Upstream evidence
MISP validates Attribute.to_ids (one of several fields routed through these helpers, alongside published/deleted/distribution flags in event.rs, object.rs, correlation.rs, noticelist.rs, event_report.rs) with CakePHP's
booleanrule: MISP core app/Model/MispAttribute.php:427-431 ("'to_ids' => array('boolean' => array('rule' => array('boolean'), 'required' => false)))") @ 843b67445060e4da71ebf75cc4a9646c301f749d. A compliant MISP instance therefore only ever emits 0/1/true/false for these fields, never a float — so this bug is not a wire-protocol mismatch with any specific controller/PyMISP call, it's defensive robustness against malformed or third-party-proxied JSON that happens to send a float. The finding itself is accurate as a pure Rust/serde logic defect (Number::as_i64() returning None for non-integers, inverting the boolean via the != comparison); no additional affected call sites beyond :132/:169 — string_or_i64/string_or_i64_opt already error explicitly on non-integer numbers rather than silently inverting, so they needed no change.Verified against MISP core
843b6744and PyMISPe7debb4f.Tests
deserialize_flexible_bool_float_numbersdeserialize_flexible_bool_opt_float_numbersCaveats
Per task constraints I did not run cargo build/test/clippy (serialized centrally); the two new tests are asserted correct by construction/manual trace (as_i64() returns None for 0.0f64 -> old code's
!= Some(0)is true; as_f64() returns Some(0.0) for both JSON int 0 and float 0.0 in a standard, non-arbitrary-precision serde_json::Number -> new code returns false), not by an actualcargo testrun in this environment. This is a defensive-robustness fix, not a protocol correction — no PyMISP or MISP controller behavior needed to change, and no existing wiremock/integration test asserted the wrong (buggy) 0.0 behavior, so there was no pre-existing wrong test to correct, only new coverage to add.Verified locally:
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features --lib,--test integration_tests --no-run, andcargo check --examples, each in an isolated target directory.🤖 Generated with Claude Code