Skip to content

qualify_topic_name does not validate absolute topic/service/action names #264

Description

@YuanYuYuan

Summary

qualify_topic_name validates name components only on the relative and ~private branches. Absolute names — anything starting with / — are returned unchecked, so invalid components reach Zenoh unvalidated.

This affects every entity type, since publishers, subscribers, services and actions all route through this function.

Reproduction

node.create_client("/bad name", example_interfaces.AddTwoInts)   # accepted

No error is raised at construction, and the client is created.

Where

crates/hiroz/src/topic_name.rs, in qualify_topic_name:

let qualified = if topic.starts_with('/') {
    // Absolute topic - use as-is, but remove trailing slash if present
    let topic = topic.strip_suffix('/').unwrap_or(topic);
    if topic.is_empty() || topic == "/" { /* ... */ }
    topic.to_string()          // <-- no component validation
} else if topic.starts_with('~') {
    // ... validates each component via is_valid_topic_component
} else {
    // relative: also validates each component
}

is_valid_topic_component already encodes the right rule (must start with a letter or underscore, then alphanumerics/underscores). It is simply never applied to the absolute branch.

Related: the same function skips empty components rather than rejecting them (if !part.is_empty() && !is_valid_topic_component(part)), so //a//b passes ROS validation and fails later inside Zenoh's key-expression parser with an error that cites a cargo registry path and never names the offending topic.

Impact

Invalid names fail late and obscurely — at key-expression parsing or, worse, silently as a name that can never match a peer — instead of at construction with a clear error.

Why this wasn't fixed in #192

#192 (hiroz-py rclpy alignment) added a binding-level check for empty components and trailing slashes, because those produce the opaque Zenoh error. Extending validation to absolute names changes a function on the hot path of every entity in the project and could reject names existing users depend on, so it warrants its own change with its own review rather than riding along in a bindings PR.

Suggested fix

Apply is_valid_topic_component to the absolute branch, and reject empty components rather than skipping them. Both are breaking for anyone currently relying on lenient names, so this likely wants a deprecation note in the changelog.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions