Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSocket Topic URL Parser is Incorrect #6619

Closed
tschmidt64 opened this issue Mar 27, 2020 · 1 comment · Fixed by #6630
Closed

WebSocket Topic URL Parser is Incorrect #6619

tschmidt64 opened this issue Mar 27, 2020 · 1 comment · Fixed by #6630
Labels
type/bug The PR fixed a bug or issue reported a bug

Comments

@tschmidt64
Copy link

tschmidt64 commented Mar 27, 2020

Describe the bug
The code in the web socket interface does not parse topic names correctly. I will limit my example to a consumer but similar issues apply to readers and producers.
From the code (line numbers are for reference, they do not match the actual code)

   1  String uri = request.getRequestURI();
   2  List<String> parts = Splitter.on("/").splitToList(uri);
   3
   4  // V1 Format must be like :
   5  // /ws/producer/persistent/my-property/my-cluster/my-ns/my-topic
   6  // or
   7  // /ws/consumer/persistent/my-property/my-cluster/my-ns/my-topic/my-subscription
   8  // or
   9  // /ws/reader/persistent/my-property/my-cluster/my-ns/my-topic
  10
  11  // V2 Format must be like :
  12  // /ws/v2/producer/persistent/my-property/my-ns/my-topic
  13  // or
  14  // /ws/v2/consumer/persistent/my-property/my-ns/my-topic/my-subscription
  15  // or
  16  // /ws/v2/reader/persistent/my-property/my-ns/my-topic
  17
  18  checkArgument(parts.size() >= 8, "Invalid topic name format");
  19  checkArgument(parts.get(1).equals("ws"));
  20
  21  final boolean isV2Format = parts.get(2).equals("v2");
  22  final int domainIndex = isV2Format ? 4 : 3;
  23  checkArgument(parts.get(domainIndex).equals("persistent") ||
  24          parts.get(domainIndex).equals("non-persistent"));
  25
  26
  27  final String domain = parts.get(domainIndex);
  28  final NamespaceName namespace = isV2Format ? NamespaceName.get(parts.get(5), parts.get(6)) :
  29          NamespaceName.get( parts.get(4), parts.get(5), parts.get(6));
  30  final String name = parts.get(7);
  31
  32  return TopicName.get(domain, namespace, name);
  33

To Reproduce
As an breaking example, take the v2 web socket topic URL:

/ws/v2/consumer/persistent/my-tenant/my-ns/some/topic/with/slashes/my-sub

In the code above, the following assignments would be made

  27  final String domain = parts.get(4)

which translates to

  27  final String domain = "persistent"  // this seems good

And then:

 28  final NamespaceName namespace = NamespaceName.get(parts.get(5), parts.get(6))

translates to:

 28  final NamespaceName namespace = NamespaceName.get("my-tenant", "my-ns") // this seems good

But then,

  30  final String name = parts.get(7);

translates to

  30  final String name = "some"; // THIS IS ONLY THE FIRST PART OF THE TOPIC

Expected behavior
The topic name should have been parsed as "some/topic/with/slashes".
The code assumes that the topic will not have any slashes in it. But this assumption is incorrect. The documentation explicitly states that "topic names are freeform". The Java code even tests for this case.

Screenshots
N/a

Desktop (please complete the following information):
N/a

Additional context
N/a

@tschmidt64 tschmidt64 added the type/bug The PR fixed a bug or issue reported a bug label Mar 27, 2020
@tschmidt64 tschmidt64 changed the title Topic URL parser is incorrect WebSocket topic URL parser is incorrect Mar 27, 2020
@tschmidt64 tschmidt64 changed the title WebSocket topic URL parser is incorrect WebSocket Topic URL Parser is Incorrect Mar 27, 2020
@sijie
Copy link
Member

sijie commented Mar 30, 2020

The topic name should have been parsed as "some/topic/with/slashes"

I don't think we support "/" in the topic name. "/" should be prohibited.

jiazhai pushed a commit that referenced this issue Apr 2, 2020
fix #6619

* delete useless code

* fix #6619

* add license
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this issue Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants