From fb345be3751aa6efd9e7f1f6edc5fffa36ca9f6a Mon Sep 17 00:00:00 2001 From: gsinghpal Date: Wed, 27 May 2026 03:53:37 -0400 Subject: [PATCH] fix(fusion_helpdesk): close IDOR + reliability gaps from code review - 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) --- fusion_helpdesk/controllers/main.py | 95 ++++++++++++++----- .../static/src/js/fusion_helpdesk_dialog.js | 6 ++ .../static/src/xml/fusion_helpdesk_dialog.xml | 5 + fusion_helpdesk/tests/test_utils.py | 14 +++ fusion_helpdesk/utils.py | 17 +++- 5 files changed, 112 insertions(+), 25 deletions(-) diff --git a/fusion_helpdesk/controllers/main.py b/fusion_helpdesk/controllers/main.py index 3fb22ab5..58612b59 100644 --- a/fusion_helpdesk/controllers/main.py +++ b/fusion_helpdesk/controllers/main.py @@ -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', diff --git a/fusion_helpdesk/static/src/js/fusion_helpdesk_dialog.js b/fusion_helpdesk/static/src/js/fusion_helpdesk_dialog.js index b4a89ad2..fecd5cc1 100644 --- a/fusion_helpdesk/static/src/js/fusion_helpdesk_dialog.js +++ b/fusion_helpdesk/static/src/js/fusion_helpdesk_dialog.js @@ -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; diff --git a/fusion_helpdesk/static/src/xml/fusion_helpdesk_dialog.xml b/fusion_helpdesk/static/src/xml/fusion_helpdesk_dialog.xml index 6aa83afb..5a45fec2 100644 --- a/fusion_helpdesk/static/src/xml/fusion_helpdesk_dialog.xml +++ b/fusion_helpdesk/static/src/xml/fusion_helpdesk_dialog.xml @@ -110,6 +110,11 @@ created with attachment(s). You'll get replies by email, and can follow up under My Tickets. +
+ + attachment(s) could not be uploaded. + Open the ticket from My Tickets and add them there. +
diff --git a/fusion_helpdesk/tests/test_utils.py b/fusion_helpdesk/tests/test_utils.py index aa47adcd..30fa6d3b 100644 --- a/fusion_helpdesk/tests/test_utils.py +++ b/fusion_helpdesk/tests/test_utils.py @@ -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): diff --git a/fusion_helpdesk/utils.py b/fusion_helpdesk/utils.py index 56a06f9a..28b81310 100644 --- a/fusion_helpdesk/utils.py +++ b/fusion_helpdesk/utils.py @@ -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