[I] [BUG] TopicUtil.isValidTopicFilter accepts "$share/g/" — a shared subscription with an empty filter that can never match [bifromq]

["ImDanXie (via GitHub)" <gi...@apache.org>]

ImDanXie opened a new issue, #288:
URL: https://github.com/apache/bifromq/issues/288

   ### Environment
   
   - BiFroMQ `4.0.0`
   - **Verified against `main` @ `ba2c34e0`** — still present on the latest `main` at the time of filing
   - Module: `bifromq-util`
   - File: `bifromq-util/src/main/java/org/apache/bifromq/util/TopicUtil.java:74-161`
   
   ### Summary
   
   `isValidTopicFilter` validates the share name and then relies on the main scan loop to validate the *real* topic filter. When the filter part is empty (`"$share/g/"`), the loop body never executes and the method returns `true`.
   
   ### Root cause
   
   ```java
   if (topicFilter.startsWith(PREFIX_ORDERED_SHARE) || topicFilter.startsWith(PREFIX_UNORDERED_SHARE)) {
       // validate share name
       for (i = topicFilter.indexOf(DELIMITER_CHAR) + 1; i < topicFilter.length(); i++) { ... }
       if (topicLevelLength == 0) {
           return false;                       // [MQTT-4.8.2-1]
       }
       if (i == topicFilter.length()) {
           return false;                       // [MQTT-4.8.2-2]
       }
       topicLevelLength = 0;
       // skip one separator to real topicFilter start pos
       i++;                                    // :112
   }
   int startIdx = i;
   int level = 1;
   for (; i < topicFilter.length(); i++) {     // :116  body never runs when i == length
       ...
   }
   ...
   return topicLevelLength <= maxLevelLength;  // :161  0 <= maxLevelLength -> true
   ```
   
   The two guards at `:102` and `:106` only constrain the **share name**. Nothing checks that the filter part is non-empty. For `"$share/g/"`, `i` is advanced to `topicFilter.length()`, so the loop at `:116` is skipped entirely and the method falls through to the final `return`.
   
   ### Verification
   
   Trace for `"$share/g/"` (length 9):
   
   | Step | Value |
   |---|---|
   | `i = indexOf('/') + 1` | `7` |
   | scan share name, `break` on `'/'` at index 8 | `topicLevelLength = 1` → passes `:102` |
   | `i (8) == length (9)`? | no → passes `:106` |
   | `i++` → | `9` |
   | `for (; i < 9; i++)` | body not executed |
   | final return | `topicLevelLength (0) <= maxLevelLength` → **`true`** |
   
   Probing the compiled classes confirms `isValidTopicFilter("$share/g/", ...) == true` while `"$share/g"`, `"$share//t"`, `"$share/"` and `"$share/+/t"` are all correctly rejected. `"$oshare/g/"` behaves the same.
   
   The neighbouring cases having been rejected correctly is what makes this a plain validation gap rather than an intentional relaxation.
   
   ### Impact
   
   `"$share/g/"` passes every gate in `MQTTSessionHandler.checkAndSubscribe` (`isWellFormed` → true, `isValidTopicFilter` → true, `isWildcardTopicFilter` → false), so:
   
   - `SUBACK` is returned with success,
   - a topic-filter slot and a shared-subscription route are consumed,
   - `MqttSharedSubNumGauge` is incremented,
   
   while the effective filter is the empty string. `TopicUtil.from("$share/g/")` yields `filterLevel=[<empty level>]`, and the only topic that expands to an empty level is `""` — which is itself invalid (`TopicUtil.isValidTopic("") == false`). **No publishable topic can ever match, so the subscriber never receives a message.**
   
   ### Suggested fix
   
   Require the filter part to be non-empty immediately after skipping the separator:
   
   ```java
       topicLevelLength = 0;
       // skip one separator to real topicFilter start pos
       i++;
       if (i >= topicFilter.length()) {
           return false;      // "$share/g/" has an empty topic filter
       }
   }
   ```
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@bifromq.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [BUG] TopicUtil.isValidTopicFilter accepts "$share/g/" — a shared subscription with an empty filter that can never match [bifromq]

["popduke (via GitHub) via dev" <de...@bifromq.apache.org>]

popduke closed issue #288: [BUG] TopicUtil.isValidTopicFilter accepts "$share/g/" — a shared subscription with an empty filter that can never match
URL: https://github.com/apache/bifromq/issues/288


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@bifromq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org