fix(security): address code review and security findings

- Remove plugin_routing self-redirect from runtime.yml
- Strip reserved keys (offset/limit) from device_filters to prevent
  pagination override injection
- Replace bare except Exception with specific ApiError/AnsibleError catches
- Replace silent except/pass in site hierarchy with warning on failure
- Add warnings for skipped devices with no usable hostname
- Add warnings for failed tag member fetches instead of silent continue
- Include error details in tag list fetch failure warning
- Validate cached data keys before use, refetch on corruption
- Add safe fallback with warning in _device_to_dict conversion
- Skip sites with missing id/name with warning
- Use vault reference in EXAMPLES instead of cleartext password
- Switch from count-based pagination to fetch-until-empty strategy
  to avoid count/filter mismatch

Author: Test User

meta/runtime.yml

@@ -1,6 +1,2 @@
 ---
 requires_ansible: '>=2.16.0'
-plugin_routing:
-  inventory:
-    catalystcenter:
-      redirect: cisco.catalystcenter.catalystcenter

plugins/inventory/catalystcenter.py

@@ -104,6 +104,7 @@
         description:
             - Additional keyword arguments passed to the Catalyst Center C(get_device_list) API call.
             - Use this to pre-filter devices by family, role, management IP, etc.
+            - Keys C(offset) and C(limit) are reserved for pagination and will be stripped if provided.
         type: dict
         default: {}
     api_page_size:
@@ -145,11 +146,11 @@
 """
 
 EXAMPLES = r"""
-# Minimal configuration
+# Minimal configuration (use ansible-vault for the password in production)
 plugin: cisco.catalystcenter.catalystcenter
 host: catalyst.example.com
 username: admin
-password: secret
+password: "{{ vault_cc_password }}"
 validate_certs: false
 
 # With environment variables (CATALYSTCENTER_HOST, CATALYSTCENTER_USERNAME, etc.)
@@ -215,6 +216,7 @@
 except ImportError:
     HAS_CATALYSTCENTERSDK = False
 
+_PAGINATION_RESERVED_KEYS = frozenset(("offset", "limit"))
 
 _NETWORK_OS_MAP = {
     "ios": {
@@ -255,6 +257,8 @@
     ord("."): "_",
 }
 
+_EXPECTED_DATA_KEYS = frozenset(("devices", "sites", "device_site_map", "tags"))
+
 
 class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable):
     """Cisco Catalyst Center dynamic inventory plugin."""
@@ -289,6 +293,17 @@ def parse(self, inventory, loader, path, cache=True):
             except KeyError:
                 cache_needs_update = True
 
+        if data is not None:
+            missing = _EXPECTED_DATA_KEYS - set(data.keys())
+            if missing:
+                self.display.warning(
+                    "Cached inventory data is missing keys {0}, refetching".format(
+                        sorted(missing)
+                    )
+                )
+                data = None
+                cache_needs_update = True
+
         if data is None:
             data = self._fetch_all_data()
 
@@ -319,7 +334,7 @@ def _get_client(self):
                 verify=self.get_option("validate_certs"),
                 debug=self.get_option("debug"),
             )
-        except Exception as e:
+        except (ApiError, ConnectionError, OSError) as e:
             raise AnsibleError(
                 "Failed to connect to Catalyst Center at {0}:{1}: {2}".format(
                     host, port, to_native(e)
@@ -351,33 +366,39 @@ def _fetch_devices(self, client):
         device_filters = self.get_option("device_filters")
         include_aps = self.get_option("include_aps")
 
-        try:
-            count_resp = client.devices.get_device_count()
-            device_count = count_resp.response
-        except (ApiError, Exception) as e:
-            raise AnsibleParserError(
-                "Failed to get device count: {0}".format(to_native(e))
+        safe_filters = {
+            k: v for k, v in device_filters.items()
+            if k not in _PAGINATION_RESERVED_KEYS
+        }
+        if len(safe_filters) != len(device_filters):
+            self.display.warning(
+                "device_filters contained reserved keys {0} which were "
+                "stripped to avoid breaking pagination".format(
+                    sorted(set(device_filters) & _PAGINATION_RESERVED_KEYS)
+                )
             )
 
-        pages = max(1, math.ceil(device_count / page_size))
         all_devices = []
+        offset = 1
 
-        for page in range(pages):
-            offset = page * page_size + 1
+        while True:
             try:
                 resp = client.devices.get_device_list(
                     offset=offset,
                     limit=page_size,
-                    **device_filters
+                    **safe_filters
                 )
                 page_devices = resp.response if resp.response else []
-            except (ApiError, Exception) as e:
+            except ApiError as e:
                 raise AnsibleParserError(
                     "Failed to get device list (offset={0}): {1}".format(
                         offset, to_native(e)
                     )
                 )
 
+            if not page_devices:
+                break
+
             for device in page_devices:
                 device_dict = self._device_to_dict(device)
 
@@ -388,6 +409,11 @@ def _fetch_devices(self, client):
 
                 all_devices.append(device_dict)
 
+            if len(page_devices) < page_size:
+                break
+
+            offset += page_size
+
         return all_devices
 
     def _device_to_dict(self, device):
@@ -402,18 +428,37 @@ def _device_to_dict(self, device):
                 if not k.startswith("_")
             }
         except TypeError:
-            return dict(device)
+            self.display.warning(
+                "Unexpected device object type ({0}), "
+                "attempting dict() conversion".format(type(device).__name__)
+            )
+            try:
+                return dict(device)
+            except (TypeError, ValueError) as e:
+                raise AnsibleParserError(
+                    "Cannot convert device to dict: {0}".format(to_native(e))
+                )
 
     def _fetch_site_topology(self, client):
         try:
             resp = client.topology.get_site_topology()
             sites = resp.response.sites if resp.response and resp.response.sites else []
-        except (ApiError, Exception) as e:
+        except ApiError as e:
             raise AnsibleParserError(
                 "Failed to get site topology: {0}".format(to_native(e))
             )
 
-        return [self._site_to_dict(s) for s in sites]
+        result = []
+        for s in sites:
+            site_dict = self._site_to_dict(s)
+            if not site_dict.get("id") or not site_dict.get("name"):
+                self.display.warning(
+                    "Skipping site with missing id or name: {0}".format(site_dict)
+                )
+                continue
+            result.append(site_dict)
+
+        return result
 
     def _site_to_dict(self, site):
         if hasattr(site, "to_dict"):
@@ -434,7 +479,7 @@ def _fetch_device_site_map(self, client):
         try:
             resp = client.topology.get_physical_topology()
             nodes = resp.response.nodes if resp.response and resp.response.nodes else []
-        except (ApiError, Exception) as e:
+        except ApiError as e:
             raise AnsibleParserError(
                 "Failed to get physical topology: {0}".format(to_native(e))
             )
@@ -463,11 +508,16 @@ def _fetch_device_tags(self, client, device_ids):
         if not device_ids:
             return tags
 
+        device_id_set = set(device_ids)
+
         try:
             resp = client.tag.get_tag()
             all_tags = resp.response if resp.response else []
-        except (ApiError, Exception):
-            self.display.warning("Failed to fetch tags, skipping tag-based grouping")
+        except ApiError as e:
+            self.display.warning(
+                "Failed to fetch tags from Catalyst Center, "
+                "skipping tag-based grouping: {0}".format(to_native(e))
+            )
             return tags
 
         for tag_obj in all_tags:
@@ -485,15 +535,21 @@ def _fetch_device_tags(self, client, device_ids):
                     id=tag_id, member_type="networkdevice"
                 )
                 members = members_resp.response if members_resp.response else []
-            except (ApiError, Exception):
+            except ApiError as e:
+                self.display.warning(
+                    "Failed to fetch members for tag '{0}' (id={1}), "
+                    "this tag will be missing from inventory: {2}".format(
+                        tag_name, tag_id, to_native(e)
+                    )
+                )
                 continue
 
             for member in members:
                 member_dict = member if isinstance(member, dict) else (
                     member.to_dict() if hasattr(member, "to_dict") else vars(member)
                 )
                 member_id = member_dict.get("instanceUuid", "")
-                if member_id in device_ids:
+                if member_id in device_id_set:
                     tags.setdefault(member_id, []).append(tag_name)
 
         return tags
@@ -505,13 +561,15 @@ def _populate(self, data):
         tags = data.get("tags", {})
 
         strict = self.get_option("strict")
+        skipped_count = 0
 
         if self.get_option("group_by_site"):
             self._build_site_groups(sites)
 
         for device in devices:
             hostname = self._get_hostname(device)
             if not hostname:
+                skipped_count += 1
                 continue
 
             self.inventory.add_host(hostname)
@@ -571,6 +629,13 @@ def _populate(self, data):
                 self.get_option("keyed_groups"), host_vars, hostname, strict=strict
             )
 
+        if skipped_count:
+            self.display.warning(
+                "Skipped {0} device(s) with no usable hostname. "
+                "Consider using hostname_source: managementIpAddress "
+                "or hostname_source: id".format(skipped_count)
+            )
+
     def _get_hostname(self, device):
         source = self.get_option("hostname_source")
         hostname = device.get(source, "")
@@ -631,8 +696,11 @@ def _build_site_groups(self, sites):
                 if parent_group and parent_group != child_group:
                     try:
                         self.inventory.add_child(parent_group, child_group)
-                    except Exception:
-                        pass
+                    except AnsibleError as e:
+                        self.display.warning(
+                            "Could not nest site group '{0}' under '{1}': "
+                            "{2}".format(child_group, parent_group, to_native(e))
+                        )
 
     def _normalize_site_name(self, site):
         name = site.get("name", "unknown")