Skip to content

Hide password when used with jsonrpc #103

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.

Sign up for GitHub

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

Open
puck wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Hide password when used with jsonrpc #103

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f29c34a
Hide the password from jsonrpc debugging
Sep 30, 2025
713ded3
Only call get_json_log_data when DEBUG logging is enabled
Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions odoorpc/rpc/jsonrpclib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,16 @@ def get_json_log_data(data):
"""Returns a new `data` dictionary with hidden params
for log purpose.
"""
log_data = data
log_data = copy.deepcopy(data)
Copy link
Collaborator

@sebalix sebalix Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sebalix sebalix Oct 13, 2025

Choose a reason for hiding this comment

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

That said, maybe for debug purpose, it's OK to go this way

Copy link
Author

@puck puck Oct 13, 2025

Choose a reason for hiding this comment

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

Yeah, this is why I added the guard to only call this function if debug mode is enabled.

Copy link
Author

@puck puck Oct 13, 2025

Choose a reason for hiding this comment

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

I moved the deepcopy as I was originally removing the password and then the RPC call was failing. :)

for param in LOG_HIDDEN_JSON_PARAMS:
if param in data['params']:
if log_data is data:
log_data = copy.deepcopy(data)
log_data['params'][param] = "**********"

# 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] = "**********"
Comment on lines +57 to +60
Copy link

@petrus-v petrus-v Oct 13, 2025

Choose a reason for hiding this comment

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

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

Suggested change
# 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 ?!

Copy link
Collaborator

@sebalix sebalix Oct 13, 2025

Choose a reason for hiding this comment

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

I agree, method accepting passwords have to be checked 👍

Copy link
Collaborator

@sebalix sebalix Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@puck puck Oct 13, 2025

Choose a reason for hiding this comment

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

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.


return log_data


Expand Down Expand Up @@ -92,6 +96,7 @@ def __init__(
):
Proxy.__init__(self, host, port, timeout, ssl, opener)
self._deserialize = deserialize
self.debug = logger.isEnabledFor(logging.DEBUG)

def __call__(self, url, params=None):
if params is None:
Expand All @@ -105,8 +110,12 @@ def __call__(self, url, params=None):
if url.startswith('/'):
url = url[1:]
full_url = self._get_full_url(url)
log_data = get_json_log_data(data)
logger.debug(LOG_JSON_SEND_MSG, {'url': full_url, 'data': log_data})

log_data = None
if self.debug:
log_data = get_json_log_data(data)
logger.debug(LOG_JSON_SEND_MSG, {'url': full_url, 'data': log_data})

data_json = json.dumps(data)
request = Request(url=full_url, data=encode_data(data_json))
request.add_header('Content-Type', 'application/json')
Expand Down