From 7f4722c9fce95cf0e7a6bb185189cd70f97ee401 Mon Sep 17 00:00:00 2001 From: Benoit Le Goff Date: Tue, 18 Aug 2026 16:12:37 +0200 Subject: [PATCH] [FIX] endpoint: validate registry sync An unknown URL converter prevents Odoo from building the global routing map. Validate active endpoints before they can be synchronized to the registry. Also check endpoint snippets for safe_eval syntax and unavailable global variables. Keep invalid endpoints editable and allow inactive records to sync so stale registry rules can be removed. --- endpoint/__manifest__.py | 2 +- endpoint/models/endpoint_mixin.py | 61 +++++++++++++++ endpoint/tests/test_endpoint.py | 77 +++++++++++++++++++ endpoint_route_handler/__manifest__.py | 2 +- .../models/endpoint_route_handler.py | 26 +++++++ .../models/endpoint_route_sync_mixin.py | 38 +++++++++ endpoint_route_handler/tests/test_endpoint.py | 22 +++++- 7 files changed, 225 insertions(+), 3 deletions(-) diff --git a/endpoint/__manifest__.py b/endpoint/__manifest__.py index 0fde8c00..035b2377 100644 --- a/endpoint/__manifest__.py +++ b/endpoint/__manifest__.py @@ -4,7 +4,7 @@ { "name": "Endpoint", "summary": """Provide custom endpoint machinery.""", - "version": "19.0.1.2.0", + "version": "19.0.1.2.1", "license": "LGPL-3", "development_status": "Beta", "author": "Camptocamp,Odoo Community Association (OCA)", diff --git a/endpoint/models/endpoint_mixin.py b/endpoint/models/endpoint_mixin.py index b9c4a619..4790d415 100644 --- a/endpoint/models/endpoint_mixin.py +++ b/endpoint/models/endpoint_mixin.py @@ -3,6 +3,7 @@ # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). import json +import symtable import textwrap import jsonschema @@ -43,6 +44,10 @@ ["new", "compare_digest"], ) +# ``safe_eval`` injects these built-ins at runtime. Keep validation aligned with +# Odoo instead of maintaining a second, potentially outdated list here. +_SAFE_EVAL_BUILTINS = frozenset(safe_eval._BUILTINS) + @disable_rpc() # Block ALL RPC calls class EndpointMixin(models.AbstractModel): @@ -98,6 +103,62 @@ def _validate_exec__code(self): ) ) + def _registry_sync_errors(self): + errors = super()._registry_sync_errors() + snippet = self.code_snippet or "" + syntax_error = safe_eval.test_python_expr(snippet, mode="exec") + if syntax_error: + errors.append( + self.env._("Invalid code snippet: %(error)s", error=syntax_error) + ) + return errors + + unavailable_names = self._code_snippet_unavailable_names(snippet) + if unavailable_names: + errors.append( + self.env._( + "The code snippet uses unavailable variable(s): %(names)s. " + "Available system variables are: %(available_names)s.", + names=", ".join(unavailable_names), + available_names=", ".join( + sorted(self._code_snippet_system_variable_names()) + ), + ) + ) + return errors + + def _code_snippet_system_variable_names(self): + # Derive names from the actual evaluation context so this validation + # stays accurate when another system variable is added. + return set(self._get_code_snippet_eval_context(request=None)) + + def _code_snippet_unavailable_names(self, snippet): + """Find global names that safe_eval will not provide at runtime.""" + symbol_table = symtable.symtable(snippet, "", "exec") + referenced_globals = set() + + # ``symtable`` distinguishes global lookups from local names in nested + # functions and comprehensions, avoiding warnings for valid variables. + def collect_globals(table): + for symbol in table.get_symbols(): + if symbol.is_referenced() and symbol.is_global(): + referenced_globals.add(symbol.get_name()) + for child in table.get_children(): + collect_globals(child) + + collect_globals(symbol_table) + assigned_names = { + symbol.get_name() + for symbol in symbol_table.get_symbols() + if symbol.is_assigned() or symbol.is_imported() + } + available_names = ( + self._code_snippet_system_variable_names() + | _SAFE_EVAL_BUILTINS + | assigned_names + ) + return sorted(referenced_globals - available_names) + def _get_request_content_schema_applicable_for_types(self): """Content types for which ``request_content_schema`` applies.""" return ["application/json", "application/xml"] diff --git a/endpoint/tests/test_endpoint.py b/endpoint/tests/test_endpoint.py index 49030bec..b2cb8c3b 100644 --- a/endpoint/tests/test_endpoint.py +++ b/endpoint/tests/test_endpoint.py @@ -246,6 +246,83 @@ def test_registry_sync(self): partial_func.func.__name__, "_handle_registry_sync_post_commit" ) + def test_invalid_route_can_be_saved_but_not_synchronized(self): + endpoint = self.env["endpoint.endpoint"].create( + { + "name": "Invalid route", + "route": "/invalid/", + "exec_mode": "code", + "code_snippet": 'result = {"payload": "ok"}', + "request_method": "GET", + "auth_type": "user_endpoint", + } + ) + registry = endpoint._endpoint_registry + key = endpoint._endpoint_registry_unique_key() + self.assertEqual(registry._get_rule(key), None) + + with ( + self.assertRaisesRegex(exceptions.UserError, "converter 'date'"), + self.env.cr.savepoint(), + ): + endpoint.registry_sync = True + self.assertEqual(registry._get_rule(key), None) + + endpoint.route = "/valid/" + endpoint._handle_registry_sync() + self.assertEqual(registry._get_rule(key).route, "/valid/") + + def test_inactive_invalid_route_can_be_removed_from_registry(self): + endpoint = self.env["endpoint.endpoint"].create( + { + "name": "Route to remove", + "route": "/route/to/remove", + "exec_mode": "code", + "code_snippet": 'result = {"payload": "ok"}', + "request_method": "GET", + "auth_type": "user_endpoint", + } + ) + registry = endpoint._endpoint_registry + key = endpoint._endpoint_registry_unique_key() + endpoint._handle_registry_sync() + self.assertIsNotNone(registry._get_rule(key)) + + endpoint.write({"route": "/invalid/", "active": False}) + endpoint._handle_registry_sync() + self.assertEqual(registry._get_rule(key), None) + + def test_invalid_code_can_be_saved_but_not_synchronized(self): + endpoint = self.env["endpoint.endpoint"].create( + { + "name": "Invalid code", + "route": "/invalid/code", + "exec_mode": "code", + "code_snippet": "result = date(request.params.get('since'))", + "request_method": "GET", + "auth_type": "user_endpoint", + } + ) + with self.assertRaisesRegex(exceptions.UserError, "unavailable.*date"): + endpoint._handle_registry_sync() + + endpoint.code_snippet = "result = {" + with self.assertRaisesRegex(exceptions.UserError, "Invalid code snippet"): + endpoint._handle_registry_sync() + + def test_locally_defined_code_variables_are_available(self): + self.endpoint.code_snippet = textwrap.dedent( + """ + value = request.params.get("value") + result = {"payload": value} + """ + ) + self.assertEqual(self.endpoint._registry_sync_errors(), []) + + def test_safe_eval_builtins_are_available(self): + self.endpoint.code_snippet = "result = {'count': len(request.params)}" + self.assertEqual(self.endpoint._registry_sync_errors(), []) + def test_duplicate(self): endpoint = self.endpoint.copy() self.assertTrue(endpoint.route.endswith("/COPY_FIXME")) diff --git a/endpoint_route_handler/__manifest__.py b/endpoint_route_handler/__manifest__.py index 5a5c359d..be100103 100644 --- a/endpoint_route_handler/__manifest__.py +++ b/endpoint_route_handler/__manifest__.py @@ -4,7 +4,7 @@ { "name": "Endpoint route handler", "summary": """Provide mixin and tool to generate custom endpoints on the fly.""", - "version": "19.0.1.2.0", + "version": "19.0.1.2.1", "license": "LGPL-3", "development_status": "Beta", "author": "Camptocamp,Odoo Community Association (OCA)", diff --git a/endpoint_route_handler/models/endpoint_route_handler.py b/endpoint_route_handler/models/endpoint_route_handler.py index 5e82521a..3a24b7fa 100644 --- a/endpoint_route_handler/models/endpoint_route_handler.py +++ b/endpoint_route_handler/models/endpoint_route_handler.py @@ -4,6 +4,8 @@ import logging +import werkzeug + from odoo import api, exceptions, fields, models ENDPOINT_ROUTE_CONSUMER_MODELS = { @@ -187,6 +189,28 @@ def _check_route(self): ) ) + def _registry_sync_errors(self): + errors = super()._registry_sync_errors() + try: + # Binding the rule performs the same converter lookup as Odoo's + # routing map without modifying the live endpoint registry. + routing_map = werkzeug.routing.Map( + strict_slashes=False, + converters=self.env["ir.http"]._get_converters(), + ) + rule = werkzeug.routing.Rule(self.route) + rule.merge_slashes = False + routing_map.add(rule) + except (LookupError, TypeError, ValueError) as error: + errors.append( + self.env._( + "Invalid route %(route)s: %(error)s", + route=self.route, + error=error, + ) + ) + return errors + @api.constrains("request_method", "request_content_type") def _check_request_method(self): for rec in self: @@ -207,6 +231,8 @@ def _endpoint_registry_unique_key(self): # TODO: consider if useful or not for single records def _register_single_controller(self, options=None, key=None, init=False): """Shortcut to register one single controller.""" + # Programmatic callers can bypass registry_sync and must be protected too. + self._validate_registry_sync(active_only=False) rule = self._make_controller_rule(options=options, key=key) self._endpoint_registry.update_rules([rule], init=init) self.env.registry.clear_cache("routing") diff --git a/endpoint_route_handler/models/endpoint_route_sync_mixin.py b/endpoint_route_handler/models/endpoint_route_sync_mixin.py index f9d861c1..1a9882c8 100644 --- a/endpoint_route_handler/models/endpoint_route_sync_mixin.py +++ b/endpoint_route_handler/models/endpoint_route_sync_mixin.py @@ -6,6 +6,7 @@ from functools import partial from odoo import api, fields, models +from odoo.exceptions import UserError from ..registry import EndpointRegistry @@ -43,6 +44,9 @@ def write(self, vals): if any([x in vals for x in self._routing_impacting_fields() + ("active",)]): # Mark as out of sync vals["registry_sync"] = False + if vals.get("registry_sync"): + # Validate only when publishing so invalid drafts remain editable. + self._validate_registry_sync() res = super().write(vals) if vals.get("registry_sync"): # NOTE: this is not done on create to allow bulk reload of the envs @@ -51,6 +55,38 @@ def write(self, vals): self._add_after_commit_hook(self.ids) return res + def _registry_sync_errors(self): + """Return errors preventing this record from being registered.""" + self.ensure_one() + return [] + + def _validate_registry_sync(self, active_only=True): + """Protect the global routing map from invalid endpoint records. + + Inactive records can be skipped because synchronization only removes + their existing rules from the registry. + """ + invalid_records = [] + records = self.filtered("active") if active_only else self + for record in records: + errors = record._registry_sync_errors() + if errors: + invalid_records.append( + self.env._( + "%(name)s:\n- %(errors)s", + name=record.display_name, + errors="\n- ".join(errors), + ) + ) + if invalid_records: + raise UserError( + self.env._( + "The registry cannot be synchronized because some active " + "records are invalid:\n\n%(errors)s", + errors="\n\n".join(invalid_records), + ) + ) + @api.model def _add_after_commit_hook(self, record_ids): self.env.cr.postcommit.add( @@ -91,6 +127,8 @@ def unlink(self): def _register_controllers(self, init=False, options=None, clear_cache=True): if not self: return + # Startup and programmatic callers do not necessarily pass through write(). + self._validate_registry_sync(active_only=False) rules = self._prepare_endpoint_rules(options=options) self._endpoint_registry.update_rules(rules, init=init) if clear_cache: diff --git a/endpoint_route_handler/tests/test_endpoint.py b/endpoint_route_handler/tests/test_endpoint.py index 60086503..0cf8a901 100644 --- a/endpoint_route_handler/tests/test_endpoint.py +++ b/endpoint_route_handler/tests/test_endpoint.py @@ -3,7 +3,7 @@ # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). from contextlib import contextmanager -from odoo import api, modules +from odoo import api, exceptions, modules from odoo.tests import common from odoo.tools import mute_logger @@ -50,6 +50,26 @@ def test_as_tool_base_data(self): new_route.route += "/new" self.assertNotEqual(new_route.endpoint_hash, first_hash) + def test_invalid_route_cannot_be_registered(self): + for route, error in ( + ("/my/test/", "converter 'date' does not exist"), + ("/my/test/