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

@@ -0,0 +1,2 @@
+minor_changes:
+- action plugins strictly accept valid parameters and report invalid parameters

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()
 

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:

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"}
 

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:

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:

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:

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

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):

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

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

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:

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'"

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'"

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: