From bf9ed0263a16413c06cdba3066d60eaed9873770 Mon Sep 17 00:00:00 2001
From: Dag Wieers <dag@wieers.com>
Date: Thu, 30 Aug 2018 15:40:36 +0200
Subject: [PATCH] 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
---
 .../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 +++++++++++++-
 .../targets/wait_for_connection/tasks/main.yml     | 12 ++++++++++++
 test/integration/targets/win_reboot/tasks/main.yml | 12 ++++++++++++
 .../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 00000000000..6047e5748e0
--- /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 4af91a480ac..c9a732d7798 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 7c958443b62..4028201130d 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 bbb8d9212b9..472feedf91d 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 9a9da51d755..8d3450c88d1 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 58749d8470c..0958ad80e97 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 f021f793adb..a347ad9ddf4 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 bd9f1f60dd0..9abdd63b45e 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 6ccb12fd76a..f9fe8b30148 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 3badee968e5..0e860a86d79 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 7ffcf044c89..be015389e15 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 8429a09a8b4..59062b7f887 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 07bf56f6ded..613209e84cd 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 c827e6bfc35..6a465b89212 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 2495e1e2819..13f3142205d 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: