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/