admin/Odoo-Modules
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(fusion_helpdesk): close IDOR + reliability gaps from code review

Browse Source 
- P0 IDOR: escape LIKE wildcards in build_scope_domain + normalise emails (email_normalize) on both write and scope sides, so a self-set email of '%' can no longer match every ticket in a deployment (+2 regression tests).
- Convert XML-RPC ProtocolError (central 502/503/429) to _RemoteError instead of a raw 500.
- _resolve_author: log + post as service account on failure instead of silently impersonating the ticket customer.
- _mark_seen now best-effort (runs after the remote post) so a bookkeeping failure can't report a successful reply as failed (no duplicate replies).
- Attachment loop catches network errors + reports failed count (no duplicate-ticket trap); dialog surfaces failures.
- console.error in JS catches for diagnosability.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsinghpal
2026-05-27 03:53:37 -04:00
parent 6ce49573ef
commit fb345be375
5 changed files with 112 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

95
fusion_helpdesk/controllers/main.py
View File

@@ -22,12 +22,14 @@ from urllib.parse import urljoin, urlparse
from odoo import _, http
from odoo.exceptions import UserError
from odoo.http import request
from odoo.tools import email_normalize
from odoo.addons.fusion_helpdesk.utils import (
build_ticket_vals,
build_scope_domain,
is_public_message,
compute_unread_count,
escape_like,
)
_logger = logging.getLogger(__name__)
@@ -86,7 +88,10 @@ class FusionHelpdeskController(http.Controller):
# the scoped "My Tickets" inbox. reply_email is the (editable) value the
# user confirmed in the dialog; fall back to their Odoo email/login.
user = request.env.user
reporter_email = (reply_email or user.email or user.login or '').strip()
# Normalise the confirmed email (and fall back to the user's own).
# Normalising rejects garbage / wildcard-bearing values so the stored
# partner_email — which is also the inbox scope key — stays clean.
reporter_email = _norm_email(reply_email, user.email, user.login)
ticket_vals = build_ticket_vals(
kind=kind, subject=subject, body_html='\n'.join(body_parts),
team_id=cfg['team_id'], client_label=cfg['client_label'],
@@ -143,7 +148,12 @@ class FusionHelpdeskController(http.Controller):
return _network_error_response(cfg['url'], e)
# ---- Push attachments -----------------------------------------
# The ticket already exists; an attachment failure must NOT bubble up
# as a 500 (the user would think the whole submission failed and file
# a duplicate). Catch network errors too, count failures, and report
# them back so the dialog can tell the user which files didn't make it.
attached = 0
failed = 0
for att in attachments or []:
data_b64 = (att or {}).get('data_b64')
name = (att or {}).get('name') or 'attachment.bin'
@@ -162,10 +172,12 @@ class FusionHelpdeskController(http.Controller):
}],
)
attached += 1
except xmlrpc.client.Fault as e:
except (xmlrpc.client.Fault, xmlrpc.client.ProtocolError,
socket.timeout, OSError, ssl.SSLError) as e:
failed += 1
_logger.warning(
'fusion_helpdesk: attachment "%s" upload failed: %s',
name, e.faultString,
name, e,
)
ticket_url = urljoin(
@@ -173,9 +185,9 @@ class FusionHelpdeskController(http.Controller):
'odoo/helpdesk/%s' % ticket_id,
)
_logger.info(
'fusion_helpdesk: created remote ticket #%s (%s attachments) '
'fusion_helpdesk: created remote ticket #%s (%s attached, %s failed) '
'on %s for user %s',
ticket_id, attached, cfg['url'],
ticket_id, attached, failed, cfg['url'],
request.env.user.login,
)
return {
@@ -183,6 +195,7 @@ class FusionHelpdeskController(http.Controller):
'ticket_id': ticket_id,
'ticket_url': ticket_url,
'attached': attached,
'failed': failed,
}
# ------------------------------------------------------------------
@@ -374,7 +387,8 @@ class FusionHelpdeskController(http.Controller):
cfg = self._read_config()
return {
'cfg': cfg,
'email': (user.email or user.login or '').strip(),
# Normalised so a self-set wildcard email ('%') can't widen scope.
'email': _norm_email(user.email, user.login),
'label': cfg['client_label'],
'is_admin': user.has_group('fusion_helpdesk.group_reporter_admin'),
'name': user.name,
@@ -384,11 +398,25 @@ class FusionHelpdeskController(http.Controller):
return all([cfg['url'], cfg['db'], cfg['login'], cfg['password']])
def _rpc(self, cfg, model, method, args, kw=None):
"""Authenticate + execute_kw against the central Odoo as the bot."""
"""Authenticate + execute_kw against the central Odoo as the bot.
A ProtocolError on the execute_kw leg (e.g. a 502/503/429 from the
central reverse proxy) is NOT an OSError subclass, so we convert it to
a _RemoteError here — otherwise it would escape every endpoint's
except-tuple and surface as a raw 500 (mislabelled "Network error")."""
uid, proxy = self._authenticate(cfg)
return proxy.execute_kw(
cfg['db'], uid, cfg['password'], model, method, args, kw or {},
)
try:
return proxy.execute_kw(
cfg['db'], uid, cfg['password'], model, method, args, kw or {},
)
except xmlrpc.client.ProtocolError as e:
_logger.warning('fusion_helpdesk: HTTP %s on %s.%s: %s',
e.errcode, model, method, e.errmsg)
raise _RemoteError(
'remote_http_error',
_('The central Helpdesk returned HTTP %(code)s. Please try '
'again in a moment.') % {'code': e.errcode},
)
def _internal_subtype_map(self, cfg, subtype_ids):
"""{subtype_id: internal_bool} so internal notes can be hidden."""
@@ -465,22 +493,45 @@ class FusionHelpdeskController(http.Controller):
return out
def _resolve_author(self, cfg, ident, ticket):
"""Find-or-create the replier's partner on central so their reply is
correctly attributed. Falls back to the ticket's customer if partner
creation isn't permitted for the bot."""
"""Find-or-create the replier's OWN partner on central so their reply
is correctly attributed.
`ident['email']` is already normalised (no wildcards); we escape it for
the =ilike search as belt-and-suspenders. On any failure we log and
return False — message_post then attributes the reply to the service
account, which is honest. We deliberately do NOT fall back to the
ticket's customer: for an admin replying to a colleague's ticket that
would silently impersonate the customer."""
email = ident['email']
ticket_partner = ticket['partner_id'][0] if ticket.get('partner_id') else False
if not email:
return ticket_partner
return False
try:
pids = self._rpc(cfg, 'res.partner', 'search',
[[('email', '=ilike', email)]], {'limit': 1})
[[('email', '=ilike', escape_like(email))]], {'limit': 1})
if pids:
return pids[0]
return self._rpc(cfg, 'res.partner', 'create',
[{'name': ident['name'], 'email': email}])
except (xmlrpc.client.Fault, _RemoteError):
return ticket_partner
except (xmlrpc.client.Fault, _RemoteError) as e:
_logger.warning(
'fusion_helpdesk: could not resolve reply author for %s on '
'ticket %s (%s); posting as the service account.',
email, ticket.get('id'), e)
return False
def _mark_ticket_seen(self, ticket_id, messages):
"""Best-effort read-tracking. Runs AFTER the remote read/post, so it
must never raise — otherwise a local DB hiccup here would turn an
already-successful reply into a reported failure, and the user would
resubmit (posting a duplicate). Bookkeeping only; log and swallow."""
if not messages:
return
try:
request.env['fusion.helpdesk.ticket.seen']._mark_seen(
ticket_id, max(m['id'] for m in messages))
except Exception: # noqa: BLE001 — non-critical bookkeeping
_logger.exception(
'fusion_helpdesk: mark-seen failed for ticket %s', ticket_id)
def _remote_failure(self, cfg, err):
"""Map a mid-RPC failure to the dialog's response shape."""
@@ -559,9 +610,7 @@ class FusionHelpdeskController(http.Controller):
messages = self._public_messages(cfg, ticket_id)
except (_RemoteError, xmlrpc.client.Fault, OSError, ssl.SSLError) as e:
return self._remote_failure(cfg, e)
if messages:
request.env['fusion.helpdesk.ticket.seen']._mark_seen(
ticket_id, max(m['id'] for m in messages))
self._mark_ticket_seen(ticket_id, messages)
t = found[0]
# Magic link: the customer's own access-token URL on central, so they
# can open the full ticket (incl. attachments) in the portal if needed.
@@ -611,9 +660,7 @@ class FusionHelpdeskController(http.Controller):
messages = self._public_messages(cfg, ticket_id)
except (_RemoteError, xmlrpc.client.Fault, OSError, ssl.SSLError) as e:
return self._remote_failure(cfg, e)
if messages:
request.env['fusion.helpdesk.ticket.seen']._mark_seen(
ticket_id, max(m['id'] for m in messages))
self._mark_ticket_seen(ticket_id, messages)
return {'ok': True, 'messages': messages}
@http.route('/fusion_helpdesk/unread_count',

6
fusion_helpdesk/static/src/js/fusion_helpdesk_dialog.js
View File

@@ -46,6 +46,7 @@ export class FusionHelpdeskDialog extends Component {
ticketId: null,
ticketUrl: "",
attached: 0,
failed: 0,
// ---- My Tickets ----
isAdmin: false,
scope: "mine", // 'mine' | 'all'
@@ -110,6 +111,7 @@ export class FusionHelpdeskDialog extends Component {
this.state.isAdmin = !!res.is_admin;
}
} catch (err) {
console.error("fusion_helpdesk: my_tickets failed", err);
this.state.listError = (err && err.message) || _t("Network error.");
} finally {
this.state.loadingList = false;
@@ -143,6 +145,7 @@ export class FusionHelpdeskDialog extends Component {
row.has_unread = false;
}
} catch (err) {
console.error("fusion_helpdesk: open ticket failed", err);
this.state.threadError = (err && err.message) || _t("Network error.");
} finally {
this.state.loadingThread = false;
@@ -173,6 +176,7 @@ export class FusionHelpdeskDialog extends Component {
this.notification.add(_t("Reply sent."), { type: "success" });
}
} catch (err) {
console.error("fusion_helpdesk: send reply failed", err);
this.state.threadError = (err && err.message) || _t("Network error.");
} finally {
this.state.sendingReply = false;
@@ -358,12 +362,14 @@ export class FusionHelpdeskDialog extends Component {
this.state.ticketId = res.ticket_id;
this.state.ticketUrl = res.ticket_url;
this.state.attached = res.attached || 0;
this.state.failed = res.failed || 0;
this.state.subject = "";
this.state.description = "";
this.state.errorCode = "";
this.state.attachments = [];
}
} catch (err) {
console.error("fusion_helpdesk: submit failed", err);
this.state.error = (err && err.message) || _t("Network error.");
} finally {
this.state.submitting = false;

5
fusion_helpdesk/static/src/xml/fusion_helpdesk_dialog.xml
View File

@@ -110,6 +110,11 @@
created<t t-if="state.attached"> with <t t-esc="state.attached"/> attachment(s)</t>.
You'll get replies by email, and can follow up under <b>My Tickets</b>.
</div>
<div t-if="state.success and state.failed" class="alert alert-warning mt-2">
<i class="fa fa-exclamation-triangle me-1"/>
<t t-esc="state.failed"/> attachment(s) could not be uploaded.
Open the ticket from <b>My Tickets</b> and add them there.
</div>
</div>
<!-- ===== LIST ===== -->

14
fusion_helpdesk/tests/test_utils.py
View File

@@ -68,6 +68,20 @@ class TestScopeDomain(TransactionCase):
self.assertEqual(len(label_terms), 1)
self.assertNotEqual(label_terms[0][2], '')
def test_wildcard_email_cannot_widen_scope(self):
# IDOR guard: a self-set email of '%' must NOT become a match-all
# =ilike term — the wildcard has to be escaped to a literal.
dom = build_scope_domain(label='ENTECH', email='%', is_admin=False)
email_terms = [t for t in dom if t[0] == 'partner_email']
self.assertEqual(len(email_terms), 1)
self.assertEqual(email_terms[0][2], '\\%',
"'%' must be escaped so ILIKE matches it literally")
def test_underscore_in_real_email_is_escaped_but_preserved(self):
dom = build_scope_domain(label='ENTECH', email='john_doe@x.com', is_admin=False)
email_terms = [t for t in dom if t[0] == 'partner_email']
self.assertEqual(email_terms[0][2], 'john\\_doe@x.com')
@tagged('post_install', '-at_install', 'fusion_helpdesk')
class TestMessageFilterAndUnread(TransactionCase):

17
fusion_helpdesk/utils.py
View File

@@ -15,6 +15,20 @@ filter and the unread maths without a live central Odoo to talk to.
_NO_MATCH = '__none__'
def escape_like(value):
"""Escape SQL LIKE/ILIKE wildcards so a user-supplied value can never
widen an `=ilike` match to other rows.
`res.users.email` is self-writeable and unvalidated, so without this a
user could set their email to ``%`` and have ``partner_email =ilike '%'``
match EVERY ticket in their deployment (a cross-user IDOR). Escaping the
backslash first, then ``%`` and ``_``, makes those characters match
literally. Real emails containing ``_`` (e.g. ``john_doe@x.com``) keep
working — the underscore is matched as a literal, which is what we want.
"""
return (value or '').replace('\\', '\\\\').replace('%', '\\%').replace('_', '\\_')
def build_ticket_vals(kind, subject, body_html, team_id, client_label,
reporter_name, reporter_email, company_name):
"""Construct the `helpdesk.ticket` create vals for a forwarded report.
@@ -53,7 +67,8 @@ def build_scope_domain(label, email, is_admin):
"""
domain = [('x_fc_client_label', '=', label or _NO_MATCH)]
if not is_admin:
domain.append(('partner_email', '=ilike', email or _NO_MATCH))
safe_email = escape_like(email)
domain.append(('partner_email', '=ilike', safe_email or _NO_MATCH))
return domain