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

HTTP Redirection at Handshake #812

Closed
FlorianBELLAZOUZ opened this issue Aug 22, 2016 · 15 comments
Closed

HTTP Redirection at Handshake #812

FlorianBELLAZOUZ opened this issue Aug 22, 2016 · 15 comments

Comments

@FlorianBELLAZOUZ
Copy link

Hi there !

Like it say in the RFC 6455

   1.  If the status code received from the server is not 101, the
       client handles the response per HTTP [RFC2616] procedures.  In
       particular, the client might perform authentication if it
       receives a 401 status code; the server might redirect the client
       using a 3xx status code (but clients are not required to follow
       them), etc.  Otherwise, proceed as follows.

But when i try a redirect status 302.
The websocket throw an Error 'unexpected server response (302)'

// serv.js
var Net = require('net')
var Wss = require('ws').Server

var server = new Net.Server()

server.on('connection',socket=>{
  socket.write('HTTP/1.1 302 Found\nLocation: ws://localhost:8080\n\n')
  socket.end()
})

server.listen(80)

var wss = new Wss({port:8080})
// ws.js
var Ws = require('ws')

var ws = new Ws('ws://localhost') // throw Error: unexpected server response (302)

I misunderstand something or this is a divergence of the RFC 6455 ?

@dwang2
Copy link

dwang2 commented Nov 7, 2016

+1
Facing the same issue where "follow-redirect" isn't supported.

@lpinca
Copy link
Member

lpinca commented Jan 25, 2017

Yes, the client does not follow redirects, but if I read the spec correctly "clients are not required to follow them".

@danields761
Copy link

Hello!
@lpinca, is there any way to follow them?

@lpinca
Copy link
Member

lpinca commented Mar 24, 2017

No, at the moment there is no way unless you monkey patch the code.

@blandinw
Copy link

Not sure how you'll prioritize this, so here's our use case.
301 redirects are often used when server infra changes, etc.
In our case, we absolutely have to redirect, but users of this library will break when we do so.

Please upvote the original post if you need this feature

@lpinca
Copy link
Member

lpinca commented Mar 29, 2017

@blandinw a possible workaround is to use a proxy which follows redirects.
A PR to fix the issue is welcome.

@jklepatch
Copy link

jklepatch commented Sep 22, 2017

I started a fix, in lib/WebSocket.js.

During handshake, if server answer by 301 or 302 status code, initAsClient() is called recursively with updated address. It also store the state isRedirecting in the options object to avoid redirect loop if new handshake attempt result in another redirection.

I tried the testing code provided on top of this thread, and it seems the socket is created properly. If this approach receive a good feedback, should we also check that the location header provided by server is well formed before trying another handshake? if yes, is there a spec about what is a well formed url for a websocket?

 function initAsClient (address, protocols, options) {
  options = Object.assign({
    protocolVersion: protocolVersions[1],
    protocol: protocols.join(','),
    perMessageDeflate: true,
    handshakeTimeout: null,
    localAddress: null,
    headers: null,
    family: null,
    origin: null,
    agent: null,
    host: null,
    isRedirection: false,

line 654:

  this._req.on('response', (res) => {
    if(!options.isRedirection && ((res.statusCode == 301) || (res.statusCode == 302))){
      options.isRedirection = true;
      initAsClient.call(this, res.headers.location, protocols, options);
      return;
    }
    if (!this.emit('unexpected-response', this._req, res)) {
      this._req.abort();
      this.emit('error', new Error(`unexpected server response (${res.statusCode})`));
      this.finalize(true);
    }
  });

@luislobo
Copy link

+1 it should handle redirections, it's a normal use case.

@huan
Copy link
Contributor

huan commented May 5, 2018

+1 it should handle redirections, it's a normal use case.

@jklepatch How about yoru fix, did you finish it?

@blandinw
Copy link

blandinw commented Sep 7, 2018

@flavioespinoza I'm not sure what you're trying to achieve, but I doubt you'll succeed with personal attacks. People working on this project do so on their spare time and let you benefit from their efforts for free.

I understand (and share to some extent) your frustration, but maybe you can channel this energy into a clean pull request to fix this once and for all?

In any case, let's try to be civil online, especially on GitHub :-)

@flavioespinoza
Copy link

@bradisbell Good advice. I will. It will take me a few days as I am knee deep in deployment, but I'll fix it and submit a PR.

However, I suspect (as with this thread and this bug) it will be ignored. But, we shall see.

@lpinca
Copy link
Member

lpinca commented Sep 7, 2018

If you implement it well I'm more than happy to merge it when I can. As others already said, no one is getting paid to work on this project.

FYI the WebSocket implementation in most browsers does not follow redirects.

@piranna
Copy link

piranna commented Nov 4, 2018

Any update on this?

@kazuhitoyokoi
Copy link

I'm also interested in the redirects.

@lpinca
Copy link
Member

lpinca commented Jan 19, 2019

Please take a look at #1490.

lpinca added a commit that referenced this issue Feb 16, 2019
lpinca added a commit that referenced this issue Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests