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

Remove session on 4xx response from WebSocket handshake #25608

Closed
wants to merge 1 commit into from

Conversation

yfei-z
Copy link

@yfei-z yfei-z commented Aug 18, 2020

The sockjs session will be removed after websocket handshake request failed, therefore the next downgrade session could be created.

@pivotal-issuemaster
Copy link

@yfei-z Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@yfei-z Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 18, 2020
@rstoyanchev
Copy link
Contributor

@yfei-z downgrading from WebSocket to HTTP transports should work. Are you saying that it doesn't or that it doesn't in some specific scenario? Do you have a sample that demonstrates the issue?

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) labels Aug 20, 2020
@yfei-z
Copy link
Author

yfei-z commented Aug 21, 2020

If the handshake failed because request missing some required header for example, the session still there wait for specified delay to be removed, in the meantime client downgrade request arrived, and the invalid session will be found by the same session ID.

Here is the java client code to simulate the handshake failing and downgrade to xhr streaming:

SockJsClient sockJsClient = new SockJsClient(new ArrayList<Transport>() {{
    this.add(new WebSocketTransport(new JettyWebSocketClient(new WebSocketClient() {
        @Override
        public Future<Session> connect(Object websocket, URI toUri, ClientUpgradeRequest request) throws IOException {
            return super.connect(websocket, toUri, request, new UpgradeListener() { // hacking web socket failing
                @Override
                public void onHandshakeRequest(UpgradeRequest request) {
                    Map<String, List<String>> headers = request.getHeaders();
                    headers.put("Upgrade", Collections.emptyList());
                    request.setHeaders(headers);
                }

                @Override
                public void onHandshakeResponse(UpgradeResponse response) {
                }
            });
        }
    })));

    this.add(new RestTemplateXhrTransport());
}});

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 21, 2020
@rstoyanchev
Copy link
Contributor

Okay thanks for clarifying. I think we'll need to solve it differently since we can't change the contract. Probably check a) keep a boolean flag whether the session was just created (i.e. trying to connect) and b) whether the response status is in the 4xx range and if so remove the session. Would you like to update the PR accordingly?

@rstoyanchev rstoyanchev added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 21, 2020
@rstoyanchev rstoyanchev self-assigned this Aug 21, 2020
@rstoyanchev rstoyanchev added this to the 5.2.9 milestone Aug 21, 2020
@rstoyanchev rstoyanchev changed the title Maintain sockjs session while websocket handshake failed Remove session on 4xx response from WebSocket handshake Aug 21, 2020
@yfei-z
Copy link
Author

yfei-z commented Aug 24, 2020

Please review

@rstoyanchev
Copy link
Contributor

Yes that looks better, thanks.

@yfei-z
Copy link
Author

yfei-z commented Aug 25, 2020

I'm not sure if it's the best way to obtain the status code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants