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

Node.js process crushes if a user has no permissions to run the 'monitor' command #1498

Closed
artyom-podymov opened this issue Jan 31, 2022 · 6 comments · Fixed by #1555
Closed

Comments

@artyom-podymov
Copy link

  1. If create a user that has no permissions to run the monitor command
$ redis-cli 
localhost:6379> ACL SETUSER alice on nopass ~* &* +@all -monitor
OK
  1. Connect to Redis and call monitor function
const Redis = require("ioredis");

var redis = new Redis({host: 'localhost', port: 6379, username: 'alice'});

redis.on("connect", () => {
  console.log('Connected to DB \n');
  redis.monitor((err, monitor) => {
    console.log('Error', err)
    if (!err) {
      monitor.on('monitor', (...args) => console.log(JSON.stringify(args)))
    }
  });
});
  1. As a result in the callback function we don't see any error and Node.js process just crushes (the same behavior for async function implementation)
Connected to DB 

Error null                                                                                    
node_modules\redis-parser\lib\parser.js:179    
    return new ReplyError(string)                                                             
           ^                                                                                  
                                                                                              
ReplyError: NOPERM this user has no permissions to run the 'monitor' command or its subcommand
    at parseError (node_modules\redis-parser\lib\parser.js:179:12)
    at parseType (node_modules\redis-parser\lib\parser.js:302:14) {
  command: { name: 'monitor', args: [] }
}

Is it possible to handle such cases and properly throw errors without stopping the Node.js process?

@udochify
Copy link

udochify commented Feb 4, 2022

Attach an event handler to the node.js process like this

process.on("uncaughtException", (error) => {
     console.error("Uncaught Exception Description: ", error);
     // process.exit(1); // uncomment this line if you want your application to close gracefully.
});

Just include it anywhere in the global scope of your file (i.e. not inside any function or class).
If you leave the last line commented, your node.js process won't crash. But it is really not advisable for production.

@artyom-podymov
Copy link
Author

Attach an event handler to the node.js process like this

process.on("uncaughtException", (error) => {
     console.error("Uncaught Exception Description: ", error);
     // process.exit(1); // uncomment this line if you want your application to close gracefully.
});

Just include it anywhere in the global scope of your file (i.e. not inside any function or class). If you leave the last line commented, your node.js process won't crash. But it is really not advisable for production.

@udochify Thanks. But this approach looks like a workaround and I think such cases should be handled on the ioredis side.

@udochify
Copy link

udochify commented Feb 4, 2022

ok. Have you tried putting the part where you call monitor in a try catch block, like this?

const Redis = require("ioredis"); 

var redis = new Redis({host: 'localhost', port: 6379, username: 'alice'}); 

redis.on("connect", () => { 
    console.log('Connected to DB \n'); 
    try {
        redis.monitor((err, monitor) => { 
            console.log('Error', err) // You only logged error here (and if the call to monitor completes). What if the call to monitor never starts as in your case
            if (!err) { 
                monitor.on('monitor', (...args) => console.log(JSON.stringify(args))) 
            } 
        });
    } catch (error) { // you caught and handled the error here
        console.error("Redis Client Monitor Exception ", error);
    }
});

Usually, catching errors (and not just logging them to the console) handles them and allows execution to continue.

Same thing goes when you use async-await syntax

async function foo () {
    await redis.monitor(...).catch((error) => console.error(error));
}

@artyom-podymov
Copy link
Author

@udochify Yes, I tried) it doesn't help, the error does not fall into the catch block

@artyom-podymov
Copy link
Author

@udochify By the way, did you manage to reproduce this issue? Or maybe it's just me having this problem?

@luin luin added the bug label Mar 28, 2022
luin added a commit that referenced this issue Mar 30, 2022
luin added a commit that referenced this issue Mar 30, 2022
luin added a commit that referenced this issue Mar 30, 2022
luin added a commit that referenced this issue Mar 30, 2022
luin added a commit that referenced this issue Mar 31, 2022
github-actions bot pushed a commit that referenced this issue Mar 31, 2022
## [5.0.3](v5.0.2...v5.0.3) (2022-03-31)

### Bug Fixes

* add named exports to keep compatible with @types/ioredis ([#1552](#1552)) ([a89a900](a89a900))
* Fix failover detector with sentinel and tls streams ([ac00a00](ac00a00))
* handle NOPERM error for monitor ([93b873d](93b873d)), closes [#1498](#1498)
* Hook up the keepAlive after a successful connect ([14f03a4](14f03a4)), closes [#1339](#1339)
@github-actions
Copy link

🎉 This issue has been resolved in version 5.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this issue Mar 1, 2024
## [5.0.3](redis/ioredis@v5.0.2...v5.0.3) (2022-03-31)

### Bug Fixes

* add named exports to keep compatible with @types/ioredis ([#1552](redis/ioredis#1552)) ([a89a900](redis/ioredis@a89a900))
* Fix failover detector with sentinel and tls streams ([ac00a00](redis/ioredis@ac00a00))
* handle NOPERM error for monitor ([93b873d](redis/ioredis@93b873d)), closes [#1498](redis/ioredis#1498)
* Hook up the keepAlive after a successful connect ([14f03a4](redis/ioredis@14f03a4)), closes [#1339](redis/ioredis#1339)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants