python-matrix-bot-api icon indicating copy to clipboard operation
python-matrix-bot-api copied to clipboard

Implemented super simple modular bot.

Open vranki opened this issue 6 years ago • 3 comments

No tested thoroughly but ready for review.

vranki avatar Jan 18 '19 15:01 vranki

Hi, thanks for the contribution.

Could you please explain your rationale for dynamically loading modules? While it's a cool trick, I don't really see the real world use case. Also, it seems like a security minefield (even with the regex sanitization).

shawnanastasio avatar Jan 18 '19 18:01 shawnanastasio

The idea behind the modules is that developer does not need to change code of the bot to implement new features. The modules don't need to care about internals of the bot, except implementing the required API.

Do you have any concrete attack vectors I have missed? The regex should remove all special characters including / and .. so the bot shouldn't be able to load modules from anywhere except the modules directory.

Ps. I'm planning to add lifecycle management so that the modules are loaded on startup instead of on command. This allows modules to run in background, for example receiving events from other sources.

vranki avatar Jan 21 '19 08:01 vranki

The idea behind the modules is that developer does not need to change code of the bot to implement new features.

If I'm understanding this correctly, the goal is to be able to use some reference bot code from this repository without having to make a separate copy for any added commands. If that's the case then I'm not convinced that it's even worth it. After all, the API is simple enough where new bots only have to make 3 calls to start using it (construct, add_handler, start_polling). I don't think any value is provided to users by being able to use the reference code in this way.

Do you have any concrete attack vectors I have missed?

Frankly, I'm not qualified to do a security analysis on this usage of python's importlib.import_module but regardless it seems that passing user input as a code import path is a risk that can be entirely avoided. Perhaps a better approach would be to scan the directory for all existing modules and form a whitelist which the user input can be checked against.

Overall, I'm not opposed to accepting something like this, but I'm still not 100% sure what the use-case here is. Also, I'd like to see more documentation on how to use this (for example, it's not currently explained that the module name must match the name of the command it wishes to implement).

shawnanastasio avatar Jan 22 '19 03:01 shawnanastasio