Hide password when used with jsonrpc #103
Conversation
Blank out the password. Fixes: OCA#70 To test I modified odoorpc/tests/__init__.py and added: import logging logging.basicConfig() logger = logging.getLogger('odoorpc') logger.setLevel(logging.DEBUG)
The logic here is a bit expensive on CPU, and only need it if debug logging is enabled.
|
Okay, so this has been approved two days ago. How does it get merged and a fixed release cut? |
|
Can a @OCA/core-maintainers please review this PR? |
| # The password is the 3rd element of the args array. | ||
| if 'args' in data['params']: | ||
| if 2 in data['params']['args']: | ||
| log_data['params']['args'][2] = "**********" |
There was a problem hiding this comment.
I think, reading this line args is a list, not a dict, can you confirm?
>>> test = ["a", "b", "c"]
>>> 2 in test
False
>>> test[2]
'c'It could be nice to add a unit test of this use case ?
As this is a proxy, I guess there are other method going over this method that could be legitimate logged. I would suggest something likes
| # The password is the 3rd element of the args array. | |
| if 'args' in data['params']: | |
| if 2 in data['params']['args']: | |
| log_data['params']['args'][2] = "**********" | |
| # The password is the 3rd element of the args array. | |
| if 'args' in data['params'] and data['params'].get("method") == "login": | |
| if len(data['params']['args']) >= 3: | |
| log_data['params']['args'][2] = "**********" |
you may check common service as well I suppose ?!
There was a problem hiding this comment.
I agree, method accepting passwords have to be checked 👍
There was a problem hiding this comment.
Details: the password is sent on every call sadly on /jsonrpc endpoint, both for the initial login (from common service) and then through execute (from object service).
So I would hide the password if the endpoint URL is /jsonrpc for common and object service. And regarding the db service, we could keep things simple: hide all the args params (which contain super-admin password, database name etc).
There is also the report service but it's not working since Odoo 11.0, not sure we should try to handle it.
There was a problem hiding this comment.
This sounds very sensible. Personally I only ever use the web interface of Odoo and have no knowledge of the APIs other than tracking this issue internally at work once we discovered it.
I am totally fine with this PR being replaced by a more comprehensive fix.
|
@sebalix ping |
| for log purpose. | ||
| """ | ||
| log_data = data | ||
| log_data = copy.deepcopy(data) |
There was a problem hiding this comment.
The idea to defer the deep copy later in the code was to avoid it if nothing needed to be hidden.
If we are sending a huge payload (like a DB dump through db.restore, or product images), copying it in memory with deepcopy will consume twice the memory.
Your issue has to be address anyway, we cannot keep passwords in clear in logs.
There was a problem hiding this comment.
That said, maybe for debug purpose, it's OK to go this way
There was a problem hiding this comment.
Yeah, this is why I added the guard to only call this function if debug mode is enabled.
There was a problem hiding this comment.
I moved the deepcopy as I was originally removing the password and then the RPC call was failing. :)
5 participants
If debugging is enabled, the password used to authenticate jsonrpc calls is logged.
This PR also fixes deepcopy being called on every jsonrpc call.
Fixes #70.