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

WebsockTransport mishandles websocket extensions #873

Open
zenbones opened this issue Aug 20, 2019 · 0 comments

Comments

@zenbones
Copy link

commented Aug 20, 2019

The problematic code is this...

    @Override
    public List<Extension> getNegotiatedExtensions(List<Extension> installed, List<Extension> requested) {
        List<Extension> negotiated = new ArrayList<>();
        for (Extension extension : requested) {
            String name = extension.getName();
            boolean option = getOption(ENABLE_EXTENSION_PREFIX_OPTION + name, true);
            if (option) {
                negotiated.add(extension);
            }
        }
        return negotiated;
    }

The problems are...

  1. The transport assumes that if the extensions is requested, and not explicitly turned off in the options, that it's negotiated, whether it's actually installed or not. If the option is true, we should still be checking whether there's an installed extension which at least has the same name as the requested extension.
  2. The transport returns a list of requested extensions, and not a list of installed extensions. The requested extensions are just wrappers around the requested extension names and parameters. They have no intelligence or business logic and can't do anything but parrot back the requested names and parameters. The installed extensions contain the business logic and can return not only their names, but a set of parameters representing the implementations of the extension types that they are actually capable of supporting. As a consequence, the client, also CometD code, rightly rejects all attempts at Per Message Deflate extensions because the requested parameter 'client_max_window_bits' is just parroted back with no value. Whereas, if the negotiated extension which was returned was the installed extension, it could include a parameter set which made sense, with the proper values (if any) included.

I would suggest this code...

@override
public List getNegotiatedExtensions(List installed, List requested) {

HashSet<Extension> negotiated = new HashSet<>();

for (Extension requestedExtension : requested) {
  String name = requestedExtension.getName();
  boolean option = getOption(ENABLE_EXTENSION_PREFIX_OPTION + name, true);
  if (option) {
    for (Extension installedExtension: installed) {
      if (installedExtension.getName().equals(name)) {
        negotiated.add(installedExtension);   
        break;
      }
    }
  }
}

return new LinkedList<>(negotiated);

}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.