Skip to content

Commit 65faa45

Browse files
JarbasAlclaude
andcommitted
fix: reject mismatched skill_id between payload and bus context (OVOS-INTENT-4 §3.2)
handle_register_template, handle_register_entity, handle_deregister_intent, handle_deregister_entity and handle_deregister_skill trusted message.data's skill_id unconditionally, letting any producer claim ownership of another skill's intents/entities by forging the payload field. message.context is attached by the bus client itself and cannot be forged by a message producer, so it is the authoritative attribution per OVOS-INTENT-4 §3.2. Add module-level _skill_id_from_context(message, handler), mirroring ovos_adapt/opm.py:73, and use it in all five handlers: when context carries a skill_id that disagrees with the payload, reject the call (return None, LOG.warning naming both values and the topic) instead of acting on either side. When context carries no skill_id (legacy, pre-spec producers), today's payload-trusting behaviour is unchanged. The legacy handle_detach_intent, which the bus-client twins from ovos.intent.deregister while carrying the original context, gets the same prefix check against its already-namespaced intent_name. Fail-before: both new regression tests (mismatched register.template indexing nothing, ovos.skill.deregister with data.skill_id=victim / context.skill_id=attacker leaving victim's intents matchable) failed against the unfixed source and pass after. Full suite: 90 passed, 2 skipped (pre-existing), 0 failed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent e063f0c commit 65faa45

2 files changed

Lines changed: 89 additions & 6 deletions

File tree

padacioso/opm.py

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -280,10 +280,27 @@ def __detach_intent(self, intent_name, lang=None):
280280
def handle_detach_intent(self, message):
281281
"""Messagebus handler for detaching padacioso intent.
282282
283+
The bus-client twins ``ovos.intent.deregister`` into this legacy
284+
topic carrying the producer's original context, so the same
285+
OVOS-INTENT-4 §3.2 identity check applies: ``intent_name`` is
286+
already namespaced ``<skill_id>:<name>``, so its prefix is compared
287+
against ``context["skill_id"]`` and a mismatch is rejected.
288+
283289
Args:
284290
message (Message): message triggering action
285291
"""
286-
self.__detach_intent(message.data.get('intent_name'))
292+
intent_name = message.data.get('intent_name')
293+
context_skill_id = message.context.get("skill_id") if message.context else None
294+
if context_skill_id is not None and intent_name:
295+
prefix_skill_id = intent_name.split(':', 1)[0]
296+
if prefix_skill_id != context_skill_id:
297+
LOG.warning(f"[handle_detach_intent] rejecting mismatched "
298+
f"skill_id on {message.msg_type}: "
299+
f"intent_name={intent_name!r} "
300+
f"(skill_id={prefix_skill_id!r}) != "
301+
f"message.context['skill_id']={context_skill_id!r}")
302+
return
303+
self.__detach_intent(intent_name)
287304

288305
def __detach_entity(self, name, lang):
289306
""" Remove an entity.
@@ -450,7 +467,7 @@ def handle_register_template(self, message: Message):
450467
"""
451468
topic = SpecMessage.INTENT_REGISTER_TEMPLATE.value
452469
data = message.data
453-
skill_id = data.get("skill_id")
470+
skill_id = _skill_id_from_context(message, "handle_register_template")
454471
intent_name = data.get("intent_name")
455472
samples = data.get("samples")
456473
if not samples: # §6.3 malformed
@@ -494,7 +511,7 @@ def handle_register_entity(self, message: Message):
494511
"""OVOS-INTENT-4 §7 — register an entity value-set hint."""
495512
topic = SpecMessage.ENTITY_REGISTER.value
496513
data = message.data
497-
skill_id = data.get("skill_id")
514+
skill_id = _skill_id_from_context(message, "handle_register_entity")
498515
entity_name = data.get("entity_name")
499516
samples = data.get("samples")
500517
if not samples: # §7.2 malformed
@@ -535,7 +552,7 @@ def _intent_langs(self, message: Message) -> List[str]:
535552

536553
def handle_deregister_intent(self, message: Message):
537554
"""OVOS-INTENT-4 §8.2 — remove one intent (all langs if lang omitted)."""
538-
skill_id = message.data.get("skill_id")
555+
skill_id = _skill_id_from_context(message, "handle_deregister_intent")
539556
intent_name = message.data.get("intent_name")
540557
name = self._internal_name(skill_id, intent_name)
541558
self.__detach_intent(name)
@@ -544,7 +561,7 @@ def handle_deregister_intent(self, message: Message):
544561

545562
def handle_deregister_entity(self, message: Message):
546563
"""OVOS-INTENT-4 §8.3 — remove one entity (all langs if lang omitted)."""
547-
skill_id = message.data.get("skill_id")
564+
skill_id = _skill_id_from_context(message, "handle_deregister_entity")
548565
entity_name = message.data.get("entity_name")
549566
name = self._internal_name(skill_id, entity_name)
550567
for lang in self._intent_langs(message):
@@ -554,7 +571,7 @@ def handle_deregister_entity(self, message: Message):
554571

555572
def handle_deregister_skill(self, message: Message):
556573
"""OVOS-INTENT-4 §8.4 — remove everything owned by a skill_id."""
557-
skill_id = message.data.get("skill_id")
574+
skill_id = _skill_id_from_context(message, "handle_deregister_skill")
558575
if not skill_id:
559576
return
560577
prefix = skill_id + ":"
@@ -731,6 +748,38 @@ def shutdown(self):
731748
self.bus.remove(SpecMessage.INTENT_DISABLE.value, self.handle_disable_intent)
732749

733750

751+
def _skill_id_from_context(message: Message, handler: str) -> Optional[str]:
752+
"""OVOS-INTENT-4 §3.2 identity check for the registration/deregistration
753+
handlers.
754+
755+
``message.context["skill_id"]`` is the authoritative attribution of the
756+
producing component. A payload ``skill_id`` that disagrees with it is a
757+
forged or misrouted claim, so the registration is REJECTED (``None`` is
758+
returned, and neither value is used) rather than trusting either side.
759+
Producers that never set ``context["skill_id"]`` (legacy, pre-spec) keep
760+
today's behaviour: the payload value is trusted as before.
761+
762+
Args:
763+
message: the incoming bus message
764+
handler: name of the calling handler, for the warning message
765+
766+
Returns:
767+
The resolved skill id, or ``None`` if the payload and context
768+
disagree.
769+
"""
770+
context_skill_id = message.context.get("skill_id") if message.context else None
771+
payload_skill_id = message.data.get("skill_id")
772+
if context_skill_id is None:
773+
return payload_skill_id
774+
if payload_skill_id and payload_skill_id != context_skill_id:
775+
LOG.warning(f"[{handler}] rejecting mismatched skill_id on "
776+
f"{message.msg_type}: message.data['skill_id']="
777+
f"{payload_skill_id!r} != "
778+
f"message.context['skill_id']={context_skill_id!r}")
779+
return None
780+
return context_skill_id
781+
782+
734783
def _dealias_intent_name(name: Optional[str]) -> Optional[str]:
735784
"""Fold the legacy ``<skill_id>:<file>.intent`` id onto the OVOS-INTENT-4
736785
canonical ``<skill_id>:<file>`` id.

test/test_pipeline.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,40 @@ def test_legacy_still_works(self):
217217
intent = svc.calc_intent("hello there", "en-US")
218218
self.assertEqual(intent.name, "greet.skill:hello")
219219

220+
def test_mismatched_register_template_indexes_nothing(self):
221+
# OVOS-INTENT-4 §3.2 — a payload claiming a different skill_id than
222+
# the bus context is a forged/misrouted attribution and is rejected
223+
svc = self.get_service()
224+
msg = Message(self.SpecMessage.INTENT_REGISTER_TEMPLATE.value, {
225+
"skill_id": "victim.skill",
226+
"intent_name": "play_music",
227+
"lang": "en-US",
228+
"samples": ["play {query}"],
229+
}, {"skill_id": "attacker.skill"})
230+
svc.handle_register_template(msg)
231+
self.assertNotIn("victim.skill:play_music",
232+
svc.containers["en-US"].intent_samples)
233+
self.assertNotIn("attacker.skill:play_music",
234+
svc.containers["en-US"].intent_samples)
235+
236+
def test_mismatched_deregister_skill_leaves_victim_intents(self):
237+
# OVOS-INTENT-4 §3.2 — ovos.skill.deregister with data.skill_id=victim
238+
# but context.skill_id=attacker must not remove victim's intents
239+
svc = self.get_service()
240+
svc.handle_register_template(Message(
241+
self.SpecMessage.INTENT_REGISTER_TEMPLATE.value, {
242+
"skill_id": "victim.skill", "intent_name": "play_music",
243+
"lang": "en-US", "samples": ["play {query}"],
244+
}))
245+
svc.handle_deregister_skill(Message(
246+
self.SpecMessage.SKILL_DEREGISTER.value,
247+
{"skill_id": "victim.skill"},
248+
{"skill_id": "attacker.skill"}))
249+
self.assertIn("victim.skill:play_music",
250+
svc.containers["en-US"].intent_samples)
251+
intent = svc.calc_intent("play jazz", "en-US")
252+
self.assertEqual(intent.name, "victim.skill:play_music")
253+
220254

221255
class ContextGatingTest(unittest.TestCase):
222256
"""OVOS-CONTEXT-1 §6/§6.1 requires_context / excludes_context gating."""

0 commit comments

Comments
 (0)