fix: harden MediaEntry against invalid numeric fields and dict2entry error type - #429
fix: harden MediaEntry against invalid numeric fields and dict2entry error type#429JarbasAl wants to merge 1 commit into
Conversation
…error type
mpris_metadata used Variant('d', length) for mpris:length, but MPRIS2
requires signature 'x' (int64 microseconds); a non-finite length also
crashed at Variant construction. update() set attributes via raw setattr
with no validation, letting a non-numeric length/position/match_confidence
poison later consumers such as Playlist.length's sum(). dict2entry raised
AttributeError on non-dict input instead of a consistent ValueError.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reporting for duty! The automated checks have completed. 🎖️I've aggregated the results of the automated checks for this PR below. 🔍 LintThe automated checks have finished their work. 🏁 ❌ ruff: issues found — see job log 🏷️ Release PreviewChecking for any potential release blockers. 🚧 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)Locking the doors and checking the windows... 🔒 ✅ No known vulnerabilities found (47 packages scanned). 📋 Repo HealthEnsuring the repo isn't allergic to new features. 🤧 ✅ All required files present. Latest Version: ✅ ⚖️ License CheckDouble-checking the fine print for any surprises. 🔍 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 📊 CoverageQuantifying the robustness of your changes. 🏋️ ✅ 85.4% total coverage Files below 80% coverage (5 files)
Full report: download the 🔨 Build TestsEnsuring the gears are properly lubricated. 💧 ✅ All versions pass
Your digital assistant in the world of OVOS 🤖 |
1 participant
MediaEntry.mpris_metadatabuilt thempris:lengthfield asVariant('d', self.length). MPRIS2 definesmpris:lengthas an int64 in microseconds, signaturex, not a double. Any consumer speaking real MPRIS2 (a desktop shell,playerctl, GNOME's media controls) would read that field with the wrong type, and iflengthever held a non-finite value (NaN, inf, or something non-numeric)dbus_nextraisesSignatureBodyMismatchErroratVariantconstruction, crashing the property outright. The fix uses signaturexwith an explicit int cast, and omits the key entirely when the length is missing or not a finite number rather than raising.That length field could get poisoned in the first place because
MediaEntry.update()copied every key from an incoming dict straight onto the dataclass withsetattr, with no type checking. A malformed dict (or a plugin returninglength: Noneor a string) would silently overwrite a good numeric value, and that bad value then flows into things likePlaylist.length, which sumse.length for e in self.entries— one poisoned entry makes the whole playlist duration wrong or throws downstream.update()now validateslength,position, andmatch_confidenceagainst a simple finite-number check (rejecting bools, NaN, and inf) and keeps the previous value with a debug log when the incoming value doesn't qualify, instead of accepting whatever came in.dict2entry()is meant to be a boundary function — the thing callers use to turn untrusted dict input into aMediaEntry,PluginStream, orPlaylist— but it only validated dicts that were missing the right keys; it never checked whether the input was a dict at all. PassingNone, an int, a string, or a list raised a bareAttributeErrorfrom the first.get()call instead of theValueErrorthe function otherwise promises. Callers that catchValueErroras their validation signal would see an unhandledAttributeErrorinstead. The fix adds an explicitisinstance(track, dict)check up front so every rejection path raisesValueErrorconsistently; the existing behavior of raising (not swallowing) on bad input is unchanged.All three defects were verified against current
origin/devsource before fixing: theVariant('d', ...)line, the rawsetattrloop inupdate(), and the missing type guard indict2entry()were all present as described. Fail-before evidence: the new tests intest/unittests/test_ocp_extra.pywere run against the unmodified source (via a reverted patch) and 15 of the 16 new cases failed — the MPRIS tests failed on the wrong signature or an unguarded crash, theupdate()tests failed because bad values overwrote good ones, and thedict2entrytests failed withAttributeErrorinstead ofValueError. After the fix, the same 16 tests pass, and the full existing suite (977 passed, 1 skipped) shows no regressions.ovos-media, the OCP-native playback daemon, already works around all three issues on its own side — it never fed non-numeric lengths through this path and doesn't rely on
mpris:length's signature matching the MPRIS2 spec — so this only affects other consumers ofovos_utils.ocpthat hit these same code paths with less careful input.