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

Params need double URL-safe-encoding for use with WebSocketConnector #40981

Closed
wabrit opened this issue Jun 5, 2024 · 1 comment · Fixed by #40992
Closed

Params need double URL-safe-encoding for use with WebSocketConnector #40981

wabrit opened this issue Jun 5, 2024 · 1 comment · Fixed by #40992
Assignees
Labels
area/websockets kind/bug Something isn't working
Milestone

Comments

@wabrit
Copy link

wabrit commented Jun 5, 2024

Describe the bug

I've implemented a ws server endpoint using the quarkus-websockets-next extension and have written a test using WebSocketConnector and a suitable client endpoint class.

One param within my server URL requires safe encoding so I do something like the following:

final WebSocketClientConnection connection = connector
        .baseUri(uri)
        .pathParam("param", URLEncoder.encode(paramValue))
        .connectAndAwait();

However that results in a URISyntaxException being thrown at the start of Http1xClientConnection.toWebSocket() as by that time the safe encoding has been removed.

I got round this by double-encoding the value, but I don't think that should be necessary; I think the issue is in the WebSocketConnectorImpl.connect() method:

       URI serverEndpointUri;
        try {
====>>> At this point serverEndpoint contains the safe-encoded value <<<==========
            serverEndpointUri = new URI(serverEndpoint.toString());
        } catch (URISyntaxException e) {
            throw new WebSocketClientException(e);
        }

       WebSocketConnectOptions connectOptions = new WebSocketConnectOptions()
                .setSsl(serverEndpointUri.getScheme().equals("https"))
                .setHost(serverEndpointUri.getHost())
                .setPort(serverEndpointUri.getPort());
        StringBuilder uri = new StringBuilder();
        if (serverEndpointUri.getPath() != null) {
====>>> Calling serverEndpointUri.getPath removes the safe-encoding <<<==========
            uri.append(serverEndpointUri.getPath());
        }

Expected behavior

It should only be necessary to safe-encode the param value at most once (preferably not all all)

Actual behavior

It is necessary to double-safe-encode the param value for the test to pass

How to Reproduce?

No response

Output of uname -a or ver

MINGW64_NT-10.0-19045 LAP1453 3.3.6-341.x86_64 2022-09-05 20:28 UTC x86_64 Msys

Output of java -version

openjdk version "17.0.5" 2022-10-18 LTS OpenJDK Runtime Environment Zulu17.38+21-CA (build 17.0.5+8-LTS) OpenJDK 64-Bit Server VM Zulu17.38+21-CA (build 17.0.5+8-LTS, mixed mode, sharing)

Quarkus version or git rev

3.11.0

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)

Additional information

No response

@wabrit wabrit added the kind/bug Something isn't working label Jun 5, 2024
@mkouba mkouba self-assigned this Jun 5, 2024
@mkouba
Copy link
Contributor

mkouba commented Jun 5, 2024

I think that we should also encode the param value by default.

mkouba added a commit to mkouba/quarkus that referenced this issue Jun 5, 2024
@mkouba mkouba added this to the 3.12 - main milestone Jun 5, 2024
@mkouba mkouba moved this from Todo to In Progress in WG - WebSocket Next Jun 6, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in WG - WebSocket Next Jun 6, 2024
@gsmet gsmet modified the milestones: 3.12 - main, 3.11.2 Jun 10, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jun 10, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/websockets kind/bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

3 participants