Fix requests if SOAP service is not on port 80, do not crash on errors

[jvah]

Tests added for the crash.

[jvah]

@jsdevel I probably should, the problem is that the error only happens when the server gets its own hostname from the HTTP host headers, which is the way most SOAP servers work, but not how node-soap server works. So to write a test for that using the current node-soap as a server I should first add that functionality to the server... The crash is probably much easier to reproduce, should look into that.

I request this pull request is not closed until these issues are resolved... unless it's ok to merge.

[jsdevel]

makes sense

[jvah]

Added a test for the crash, it required to take the server object out of the single test scope but I hope that is ok.

It is bad practice btw to start the server in the first test, because the order of tests is not guaranteed. You should use before, after, before each and after each hooks for preparing and closing the server.

[jsdevel]

I think with mocha I've observed that tests are executed as listed. Still, better to avoid it.

[jvah]

@jsdevel That's because in Node.js uses v8 in which object keys are in the same order as they are added to the object. However this is not guaranteed in JavaScript specification itself, and I would therefore like to avoid it. Quite unlikely that v8 would change in this respect though, because it would break a lot of crappy code.

[jsdevel]

Nice. I wasn't aware of Object.keys!

[jvah]

Modified the comparison from port != null to just port. As mentioned currently the node-soap doesn't generate the <soap:address location="..."/> from HTTP Host-header as most servers, so it's damn difficult to write tests for the not on port 80 part. However HTTP spec is quite clear on the fact that port number should be included in the HTTP Host-header.

[jsdevel]

Should it return with ":80" then by default?

[jvah]

Not necessarily

14.23 Host

The Host request-header field specifies the Internet host and port number of the resource being requested, as
obtained from the original URI given by the user or referring resource (generally an HTTP URL, as described in
section 3.2.2). The Host field value MUST represent the naming authority of the origin server or gateway given
by the original URL. This allows the origin server or gateway to differentiate between internally-ambiguous URLs,
such as the root "/" URL of a server for multiple host names on a single IP address.

       Host = "Host" ":" host [ ":" port ] ; Section 3.2.2
A "host" without any trailing port information implies the default port for the service requested (e.g., "80" for an HTTP
URL). For example, a request on the origin server for <http://www.w3.org/pub/WWW/> would properly include:

       GET /pub/WWW/ HTTP/1.1
       Host: www.w3.org
A client MUST include a Host header field in all HTTP/1.1 request messages . If the requested URI does not include an
Internet host name for the service being requested, then the Host header field MUST be given with an empty value. An
HTTP/1.1 proxy MUST ensure that any request message it forwards does contain an appropriate Host header field that
identifies the service being requested by the proxy. All Internet-based HTTP/1.1 servers MUST respond with a 400 (Bad
Request) status code to any HTTP/1.1 request message which lacks a Host header field.

See sections 5.2 and 19.6.1.1 for other requirements relating to Host.

[jsdevel]

Can you write more of a unit test around lib/http.js if it's difficult to test using the server?

[jvah]

Actually it already defaults to :80 actually... see var port = parseInt(curl.port || (secure ? 443 : 80)); which means the port can never be null... Also should always provide a radix for parseInt in the form parseInt(curl.port || (secure ? 443 : 80), 10); because it is famous for its insane default of radix guessing.

I could create a fake server and check that the host header is correct, that would probably be the most sane test. Is that really necessary and should I separate this to two pull requests if I don't have time for that today?

[jvah]

@aheckmann could come back and see this pull request now...

[aheckmann]

this use of server is confusing me. may be better to wrap the server startup/shutdown with in before() and after() so its more clear we clean up appropriately.

[jvah]

Worth to note however, that using before and after doesn't change the fact that you need an external variable referencing to the server instance, in order to be able to close it.

On 23.1.2014, at 17.06, Aaron Heckmann [email protected] wrote:

In test/server-test.js:

@@ -113,6 +115,18 @@ module.exports = {
done();
});
});

•    },
  

•    'should return a valid error if the server stops responding': function(done) {
  

•        soap.createClient('http://localhost:15099/stockquote?wsdl', function(err, client) {
  

•            assert.ok(!err);
  

•            server.close(function() {
  

  this use of server is confusing me. may be better to wrap the server startup/shutdown with in before() and after() so its more clear we clean up appropriately.

�
Reply to this email directly or view it on GitHub.

[aheckmann]

Also needs a rebase.

[jvah]

Rebase is done, the server startup/shutdown should be fixed separately IMHO, but I can try to find time to fix that as well.

[jvah]

@aheckmann Tests are now using beforeEach and afterEach, they still require a reference to the server variable, but nice thing is that the server is started separately for each test. So if a test closes the server it will be restarted for the next test.

[jvah]

Crashes on me when I revert the lib/client.js, are you sure? I'm getting tired of this pull request...

  15 passing (178ms)
  1 failing

  1) SOAP Server should return a valid error if the server stops responding:
     Uncaught TypeError: Cannot read property 'headers' of undefined
      at /Users/jvah/Source/node-soap/lib/client.js:148:40
      at Request._callback (/Users/jvah/Source/node-soap/lib/http.js:49:7)
      at self.callback (/Users/jvah/Source/node-soap/node_modules/request/request.js:122:22)
      at Request.EventEmitter.emit (events.js:95:17)
      at ClientRequest.self.clientErrorHandler (/Users/jvah/Source/node-soap/node_modules/request/request.js:231:10)
      at ClientRequest.EventEmitter.emit (events.js:95:17)
      at Socket.socketErrorListener (http.js:1547:9)
      at Socket.EventEmitter.emit (events.js:95:17)
      at net.js:441:14
      at process._tickCallback (node.js:415:13)

[aheckmann]

ah my mistake. merged.