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

Add formatting for SockJS close GoAway frame to prevent infinite loop for xhr-polling and xhr-streaming transport #28000

Closed
wants to merge 1 commit into from

Conversation

zbykovskyi
Copy link

@zbykovskyi zbykovskyi commented Feb 3, 2022

This pull request is regarding the issue of infinite loop in SockJS library for xhr-polling and xhr-streaming transport.

When Spring sends unformatted frame SockJsFrame.closeFrameGoAway() without ending \n like c[1000, "Go Away!"] then SockJS library is unable to parse it correctly and to recognise it as native close frame. Instead of that SockJS treats it as just plain text.

xhr.js file has next method to parse inbound text message:

XhrReceiver.prototype._chunkHandler = function(status, text) {
  debug('_chunkHandler', status);
  if (status !== 200 || !text) {
    return;
  }

  for (var idx = -1; ; this.bufferPosition += idx + 1) {
    var buf = text.slice(this.bufferPosition);
    idx = buf.indexOf('\n');
    if (idx === -1) {
      break;
    }
    var msg = buf.slice(0, idx);
    if (msg) {
      debug('message', msg);
      this.emit('message', msg);
    }
  }
};

That method expects \n at the end of the SockJS text frame.

But in case of "Connection already closed (but not removed yet) for sessionId" Spring Framework sends unformatted SockJsFrame.closeFrameGoAway() without ending \n. That leads to infinite loop at SockJS library because it is unable to parse that frame.

This fix is about to add formatting for SockJsFrame.closeFrameGoAway() before sending to the client.

Some reported issues:
sockjs/sockjs-client#418
sockjs/sockjs-client#308
https://stackoverflow.com/questions/37030416/sockjs-infinite-xhr-streaming-calls-after-reconnect/70957300#70957300

… for xhr-polling and xhr-streaming transport
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 3, 2022
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Feb 3, 2022
@sbrannen sbrannen added this to the Triage Queue milestone Feb 3, 2022
@angusoid
Copy link

angusoid commented Feb 6, 2022

This one caused me some headaches and made my userbase a DDoS army.

Good job for figuring this one out!

@rstoyanchev rstoyanchev self-assigned this Feb 8, 2022
@rstoyanchev rstoyanchev added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 8, 2022
@rstoyanchev rstoyanchev modified the milestones: Triage Queue, 5.3.16 Feb 8, 2022
@rstoyanchev
Copy link
Contributor

Thanks for figuring out the root cause!

@ankitpec72
Copy link

Thank you for figuring this out. @zbykovskyi I am struggling with the same issues since last 3 Months. When can we expect the new release?

Cheers!!

@zbykovskyi
Copy link
Author

Thank you for figuring this out. @zbykovskyi I am struggling with the same issues since last 3 Months. When can we expect the new release?

Cheers!!

You can put files with fix locally to be the first in your classpath to override original files for now until official bugfix will be released

Cheers!

@radonaldson54
Copy link

@zbykovskyi Can you please give an example of the classpath for this and the file to include first? We are trying to fix here and would really appreciate the help :-) Thanks!

@ankitpec72
Copy link

Thank you for figuring this out. @zbykovskyi I am struggling with the same issues since last 3 Months. When can we expect the new release?
Cheers!!

You can put files with fix locally to be the first in your classpath to override original files for now until official bugfix will be released

Cheers!

You are our savior. Thanks a lot :)

@zbykovskyi
Copy link
Author

zbykovskyi commented Feb 9, 2022

@zbykovskyi Can you please give an example of the classpath for this and the file to include first? We are trying to fix here and would really appreciate the help :-) Thanks!

Copy the next original Spring files and put them in your project`s codebase in the same package:

  • org.springframework.web.socket.sockjs.transport.session.AbstractHttpSockJsSession.java
  • org.springframework.web.socket.sockjs.transport.handler.AbstractHttpSendingTransportHandler.java

Then apply patch from current pull request and compile your project.
These fixed files will be before original files in the classpath. After official release just remove these files.

rstoyanchev pushed a commit that referenced this pull request Feb 14, 2022
Prevents infinite loop for xhr-polling and xhr-streaming transports.

See gh-28000
@hatvqd
Copy link

hatvqd commented Mar 21, 2024

anyone met this bug again?
I just implemented base on Spring Boot 2.6.6, sockjs 1.6.1 and still got that bug sometime.
I try to simulate on my local case by case or in product by my account case by case but can not

Screenshot from 2024-03-21 16-35-49

@zbykovskyi
Copy link
Author

anyone met this bug again? I just implemented base on Spring Boot 2.6.6, sockjs 1.6.1 and still got that bug sometime. I try to simulate on my local case by case or in product by my account case by case but can not

Screenshot from 2024-03-21 16-35-49

Could you add more details on your issue? What is the body of http POST requests and how to reproduce that behaviour?

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.

8 participants