Disable ssl for websocket #782
Conversation
| @@ -117,6 +117,7 @@ class RecognizeStream extends Duplex { | |||
| * @param {string} [options.base_model_version] - The version of the specified base model that is to be used with recognition request or, for the **Create a session** method, with the new session. | |||
| * Multiple versions of a base model can exist when a model is updated for internal improvements. The parameter is intended primarily for use with custom models that have been upgraded for a new base model. | |||
| * The default value depends on whether the parameter is used with or without a custom model. For more information, see [Base model version](https://console.bluemix.net/docs/services/speech-to-text/input.html#version). | |||
| * @param {Boolean} [options.rejectUnauthorized] - If true, disable SSL verification for the WebSocket connection | |||
There was a problem hiding this comment.
is rejectUnauthorized the name we use in the other SDKs?
There was a problem hiding this comment.
That's the name of the parameter for the request objects, so it's used internally but it's not user-facing. The user passes in a parameter called disable_ssl, and rejectUnauthorized just becomes the opposite of whatever Boolean value is passed in.
Java is using disableSslVerification. Not sure about the other SDKs. I can change the parameter name to disable_ssl_verification to be consistent if you think I should.
lib/recognize-stream.ts
Outdated
| const socket = (this.socket = new w3cWebSocket( | ||
| url, | ||
| null, | ||
| null, | ||
| options.headers, | ||
| null | ||
| null, | ||
| options.rejectUnauthorized ? { tlsOptions: { rejectUnauthorized: false }} : null |
There was a problem hiding this comment.
Looks like the ternary expressions need to be swapped. Right now we're setting rejectUnauthorized to false if it's true.
There was a problem hiding this comment.
Oh shoot I did something silly. I'm not supposed to be setting rejectUnauthorized at all. I'll fix that now.
Codecov Report
@@ Coverage Diff @@
## master #782 +/- ##
=========================================
+ Coverage 83.2% 83.2% +<.01%
=========================================
Files 36 36
Lines 4560 4561 +1
Branches 578 578
=========================================
+ Hits 3794 3795 +1
Misses 364 364
Partials 402 402
Continue to review full report at Codecov.
|
|
🎉 This PR is included in version 3.11.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The SSL disabling option I added for the last release did not cover the WebSocket connection (thanks @lpatino10 for the catch).
This PR resolves this by taking the user option, passing it to the
RecognizeStreamclass, and using it to configure the connection. It does this by creating atlsOptionsparameter, which gets passed into Node's nativehttplibrary according to thewebsocketdocs.@lpatino10 confirmed this fix works with ICP.
cc @germanattanasio