From bf9ed0263a16413c06cdba3066d60eaed9873770 Mon Sep 17 00:00:00 2001 From: Dag Wieers Date: Thu, 30 Aug 2018 15:40:36 +0200 Subject: Ensure action plugins accept only valid args (#44779) * Ensure action plugins accept only valid args This fixes #25424 This also fixes #44773 * Add missing parameters, use private _VALID_ARGS --- changelogs/fragments/plugins-accept-only-valid-args.yaml | 2 ++ lib/ansible/plugins/action/__init__.py | 10 ++++++++++ lib/ansible/plugins/action/assert.py | 1 + lib/ansible/plugins/action/debug.py | 6 +----- lib/ansible/plugins/action/fail.py | 1 + lib/ansible/plugins/action/group_by.py | 1 + lib/ansible/plugins/action/pause.py | 7 +------ lib/ansible/plugins/action/reboot.py | 1 + lib/ansible/plugins/action/set_stats.py | 1 + lib/ansible/plugins/action/wait_for_connection.py | 1 + lib/ansible/plugins/action/win_reboot.py | 4 ++++ test/integration/targets/reboot/tasks/main.yml | 14 +++++++++++++- .../integration/targets/wait_for_connection/tasks/main.yml | 12 ++++++++++++ test/integration/targets/win_reboot/tasks/main.yml | 12 ++++++++++++ .../targets/yum_repository/tasks/yum_repository_centos.yml | 1 - 15 files changed, 61 insertions(+), 13 deletions(-) create mode 100644 changelogs/fragments/plugins-accept-only-valid-args.yaml diff --git a/changelogs/fragments/plugins-accept-only-valid-args.yaml b/changelogs/fragments/plugins-accept-only-valid-args.yaml new file mode 100644 index 0000000000..6047e5748e --- /dev/null +++ b/changelogs/fragments/plugins-accept-only-valid-args.yaml @@ -0,0 +1,2 @@ +minor_changes: +- action plugins strictly accept valid parameters and report invalid parameters diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 4af91a480a..c9a732d779 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -45,6 +45,9 @@ class ActionBase(with_metaclass(ABCMeta, object)): action in use. ''' + # A set of valid arguments + _VALID_ARGS = frozenset([]) + def __init__(self, task, connection, play_context, loader, templar, shared_loader_obj): self._task = task self._connection = connection @@ -95,6 +98,13 @@ class ActionBase(with_metaclass(ABCMeta, object)): elif self._task.async_val and self._play_context.check_mode: raise AnsibleActionFail('check mode and async cannot be used on same task.') + # Error if invalid argument is passed + if self._VALID_ARGS: + task_opts = frozenset(self._task.args.keys()) + bad_opts = task_opts.difference(self._VALID_ARGS) + if bad_opts: + raise AnsibleActionFail('Invalid options for %s: %s' % (self._task.action, ','.join(list(bad_opts)))) + if self._connection._shell.tmpdir is None and self._early_needs_tmp_path(): self._make_tmp_path() diff --git a/lib/ansible/plugins/action/assert.py b/lib/ansible/plugins/action/assert.py index 7c958443b6..4028201130 100644 --- a/lib/ansible/plugins/action/assert.py +++ b/lib/ansible/plugins/action/assert.py @@ -27,6 +27,7 @@ class ActionModule(ActionBase): ''' Fail with custom message ''' TRANSFERS_FILES = False + _VALID_ARGS = frozenset(('fail_msg', 'msg', 'that')) def run(self, tmp=None, task_vars=None): if task_vars is None: diff --git a/lib/ansible/plugins/action/debug.py b/lib/ansible/plugins/action/debug.py index bbb8d9212b..472feedf91 100644 --- a/lib/ansible/plugins/action/debug.py +++ b/lib/ansible/plugins/action/debug.py @@ -28,16 +28,12 @@ class ActionModule(ActionBase): ''' Print statements during execution ''' TRANSFERS_FILES = False - VALID_ARGS = frozenset(('msg', 'var', 'verbosity')) + _VALID_ARGS = frozenset(('msg', 'var', 'verbosity')) def run(self, tmp=None, task_vars=None): if task_vars is None: task_vars = dict() - for arg in self._task.args: - if arg not in self.VALID_ARGS: - return {"failed": True, "msg": "'%s' is not a valid option in debug" % arg} - if 'msg' in self._task.args and 'var' in self._task.args: return {"failed": True, "msg": "'msg' and 'var' are incompatible options"} diff --git a/lib/ansible/plugins/action/fail.py b/lib/ansible/plugins/action/fail.py index 9a9da51d75..8d3450c88d 100644 --- a/lib/ansible/plugins/action/fail.py +++ b/lib/ansible/plugins/action/fail.py @@ -25,6 +25,7 @@ class ActionModule(ActionBase): ''' Fail with custom message ''' TRANSFERS_FILES = False + _VALID_ARGS = frozenset(('msg',)) def run(self, tmp=None, task_vars=None): if task_vars is None: diff --git a/lib/ansible/plugins/action/group_by.py b/lib/ansible/plugins/action/group_by.py index 58749d8470..0958ad80e9 100644 --- a/lib/ansible/plugins/action/group_by.py +++ b/lib/ansible/plugins/action/group_by.py @@ -26,6 +26,7 @@ class ActionModule(ActionBase): # We need to be able to modify the inventory TRANSFERS_FILES = False + _VALID_ARGS = frozenset(('key', 'parents')) def run(self, tmp=None, task_vars=None): if task_vars is None: diff --git a/lib/ansible/plugins/action/pause.py b/lib/ansible/plugins/action/pause.py index f021f793ad..a347ad9ddf 100644 --- a/lib/ansible/plugins/action/pause.py +++ b/lib/ansible/plugins/action/pause.py @@ -73,8 +73,8 @@ def clear_line(stdout): class ActionModule(ActionBase): ''' pauses execution for a length or time, or until input is received ''' - PAUSE_TYPES = ['seconds', 'minutes', 'prompt', 'echo', ''] BYPASS_HOST_LOOP = True + _VALID_ARGS = frozenset(('echo', 'minutes', 'prompt', 'seconds')) def run(self, tmp=None, task_vars=None): ''' run the pause action module ''' @@ -100,11 +100,6 @@ class ActionModule(ActionBase): echo=echo )) - if not set(self._task.args.keys()) <= set(self.PAUSE_TYPES): - result['failed'] = True - result['msg'] = "Invalid argument given. Must be one of: %s" % ", ".join(self.PAUSE_TYPES) - return result - # Should keystrokes be echoed to stdout? if 'echo' in self._task.args: try: diff --git a/lib/ansible/plugins/action/reboot.py b/lib/ansible/plugins/action/reboot.py index bd9f1f60dd..9abdd63b45 100644 --- a/lib/ansible/plugins/action/reboot.py +++ b/lib/ansible/plugins/action/reboot.py @@ -28,6 +28,7 @@ class TimedOutException(Exception): class ActionModule(ActionBase): TRANSFERS_FILES = False + _VALID_ARGS = frozenset(('connect_timeout', 'msg', 'post_reboot_delay', 'pre_reboot_delay', 'test_command')) DEFAULT_REBOOT_TIMEOUT = 600 DEFAULT_CONNECT_TIMEOUT = None diff --git a/lib/ansible/plugins/action/set_stats.py b/lib/ansible/plugins/action/set_stats.py index 6ccb12fd76..f9fe8b3014 100644 --- a/lib/ansible/plugins/action/set_stats.py +++ b/lib/ansible/plugins/action/set_stats.py @@ -27,6 +27,7 @@ from ansible.utils.vars import isidentifier class ActionModule(ActionBase): TRANSFERS_FILES = False + _VALID_ARGS = frozenset(('aggregate', 'data', 'per_host')) # TODO: document this in non-empty set_stats.py module def run(self, tmp=None, task_vars=None): diff --git a/lib/ansible/plugins/action/wait_for_connection.py b/lib/ansible/plugins/action/wait_for_connection.py index 3badee968e..0e860a86d7 100644 --- a/lib/ansible/plugins/action/wait_for_connection.py +++ b/lib/ansible/plugins/action/wait_for_connection.py @@ -37,6 +37,7 @@ class TimedOutException(Exception): class ActionModule(ActionBase): TRANSFERS_FILES = False + _VALID_ARGS = frozenset(('connect_timeout', 'delay', 'sleep', 'timeout')) DEFAULT_CONNECT_TIMEOUT = 5 DEFAULT_DELAY = 0 diff --git a/lib/ansible/plugins/action/win_reboot.py b/lib/ansible/plugins/action/win_reboot.py index 7ffcf044c8..be015389e1 100644 --- a/lib/ansible/plugins/action/win_reboot.py +++ b/lib/ansible/plugins/action/win_reboot.py @@ -24,6 +24,10 @@ class TimedOutException(Exception): class ActionModule(RebootActionModule, ActionBase): TRANSFERS_FILES = False + _VALID_ARGS = frozenset(( + 'connect_timeout', 'connect_timeout_sec', 'msg', 'post_reboot_delay', 'post_reboot_delay_sec', 'pre_reboot_delay', 'pre_reboot_delay_sec', + 'reboot_timeout', 'reboot_timeout_sec', 'shutdown_timeout', 'shutdown_timeout_sec', 'test_command', + )) DEFAULT_CONNECT_TIMEOUT = 5 DEFAULT_PRE_REBOOT_DELAY = 2 diff --git a/test/integration/targets/reboot/tasks/main.yml b/test/integration/targets/reboot/tasks/main.yml index 8429a09a8b..59062b7f88 100644 --- a/test/integration/targets/reboot/tasks/main.yml +++ b/test/integration/targets/reboot/tasks/main.yml @@ -34,13 +34,25 @@ command: who -b register: after_boot_time - - name: Enusure system was actually rebooted + - name: Ensure system was actually rebooted assert: that: - reboot_result is changed - reboot_result.elapsed > 10 - before_boot_time.stdout != after_boot_time.stdout + - name: Use invalid parameter + reboot: + foo: bar + ignore_errors: yes + register: invalid_parameter + + - name: Ensure task fails with error + assert: + that: + - invalid_parameter is failed + - "invalid_parameter.msg == 'Invalid options for reboot: foo'" + always: - name: Cleanup temp file file: diff --git a/test/integration/targets/wait_for_connection/tasks/main.yml b/test/integration/targets/wait_for_connection/tasks/main.yml index 07bf56f6de..613209e84c 100644 --- a/test/integration/targets/wait_for_connection/tasks/main.yml +++ b/test/integration/targets/wait_for_connection/tasks/main.yml @@ -3,3 +3,15 @@ connect_timeout: 5 sleep: 1 timeout: 10 + +- name: Use invalid parameter + wait_for_connection: + foo: bar + ignore_errors: yes + register: invalid_parameter + +- name: Ensure task fails with error + assert: + that: + - invalid_parameter is failed + - "invalid_parameter.msg == 'Invalid options for wait_for_connection: foo'" diff --git a/test/integration/targets/win_reboot/tasks/main.yml b/test/integration/targets/win_reboot/tasks/main.yml index c827e6bfc3..6a465b8921 100644 --- a/test/integration/targets/win_reboot/tasks/main.yml +++ b/test/integration/targets/win_reboot/tasks/main.yml @@ -76,3 +76,15 @@ win_user: name: '{{standard_user}}' state: absent + +- name: Use invalid parameter + reboot: + foo: bar + ignore_errors: yes + register: invalid_parameter + +- name: Ensure task fails with error + assert: + that: + - invalid_parameter is failed + - "invalid_parameter.msg == 'Invalid options for reboot: foo'" diff --git a/test/integration/targets/yum_repository/tasks/yum_repository_centos.yml b/test/integration/targets/yum_repository/tasks/yum_repository_centos.yml index 2495e1e281..13f3142205 100644 --- a/test/integration/targets/yum_repository/tasks/yum_repository_centos.yml +++ b/test/integration/targets/yum_repository/tasks/yum_repository_centos.yml @@ -171,7 +171,6 @@ - "'RPM-GPG2-KEY-EPEL' in repofile" - "'aaa bbb' in repofile" - "'ccc ddd' in repofile" - value: - name: Cleanup list test repo yum_repository: -- cgit v1.2.1