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

feat: add support for rediss protocol in url #1282

Merged
merged 2 commits into from Jan 9, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Expand Up @@ -216,7 +216,7 @@ using unix sockets if possible to increase throughput.
| host | 127.0.0.1 | IP address of the Redis server |
| port | 6379 | Port of the Redis server |
| path | null | The UNIX socket string of the Redis server |
| url | null | The URL of the Redis server. Format: `[redis:]//[[user][:password@]][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]]` (More info avaliable at [IANA](http://www.iana.org/assignments/uri-schemes/prov/redis)). |
| url | null | The URL of the Redis server. Format: `[redis:][rediss:]//[[user][:password@]][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]]` (More info avaliable at [IANA](http://www.iana.org/assignments/uri-schemes/prov/redis)). |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please change this to the following?

[redis[s]:]//[[user][:password@]][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]]

Right now the syntax would be wrong as it would actually "allow" redis:rediss://....

| parser | javascript | __Deprecated__ Use either the built-in JS parser [`javascript`]() or the native [`hiredis`]() parser. __Note__ `node_redis` < 2.6 uses hiredis as default if installed. This changed in v.2.6.0. |
| string_numbers | null | Set to `true`, `node_redis` will return Redis number values as Strings instead of javascript Numbers. Useful if you need to handle big numbers (above `Number.MAX_SAFE_INTEGER === 2^53`). Hiredis is incapable of this behavior, so setting this option to `true` will result in the built-in javascript parser being used no matter the value of the `parser` option. |
| return_buffers | false | If set to `true`, then all replies will be sent to callbacks as Buffers instead of Strings. |
Expand Down
6 changes: 5 additions & 1 deletion lib/createClient.js
Expand Up @@ -32,7 +32,11 @@ module.exports = function createClient (port_arg, host_arg, options) {
options.password = parsed.auth.split(':')[1];
}
if (parsed.protocol && parsed.protocol !== 'redis:') {
console.warn('node_redis: WARNING: You passed "' + parsed.protocol.substring(0, parsed.protocol.length - 1) + '" as protocol instead of the "redis" protocol!');
if (parsed.protocol === 'rediss:') {
options.tls = options.tls || {};
} else {
console.warn('node_redis: WARNING: You passed "' + parsed.protocol.substring(0, parsed.protocol.length - 1) + '" as protocol instead of the "redis" protocol!');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please change this to the following:

if (parsed.protocol) {
  if (parsed.protocol === 'rediss:') {
    options.tls = ...
  } else if (parsed.protocol !== 'redis:') {
    console.warn('...')
  }
}

}
if (parsed.pathname && parsed.pathname !== '/') {
options.db = parsed.pathname.substr(1);
Expand Down
23 changes: 23 additions & 0 deletions test/tls.spec.js
Expand Up @@ -108,6 +108,29 @@ describe('TLS connection tests', function () {
client.get('foo', helper.isString('bar', done));
});

describe('using rediss as url protocol', function (done) {
var tls = require('tls')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the require to the top of the file.

var tlsConnect = tls.connect
beforeEach(function () {
tls.connect = function (options) {
options = utils.clone(options)
options.ca = tls_options.ca;
return tlsConnect.call(tls, options);
}
})
afterEach(function () {
tls.connect = tlsConnect;
})
it('connect with tls when rediss is used as the protocol', function (done) {
if (skip) this.skip();
client = redis.createClient('rediss://localhost:' + tls_port);
// verify connection is using TCP, not UNIX socket
assert(client.stream.encrypted);
client.set('foo', 'bar');
client.get('foo', helper.isString('bar', done));
});
})

it('fails to connect because the cert is not correct', function (done) {
if (skip) this.skip();
var faulty_cert = utils.clone(tls_options);
Expand Down