Fixes some potentially unsafe variables not being wrapped correctly #114 #543

cdtomkins · 2025-04-11T15:51:08Z

Closes #114

What's Changed

Summary

inventory.py and gql_inventory.py both had issues where Jinja 2 delimeters were being unexpectedly processed rather than passed through, which led to unpredictable and undesirable behaviour.

In both cases, these have been modified so that both the hostname and other variables are passed to Ansible's wrap_var documentation as described in the official documentation

Supporting tests have also been added to ensure that these fixes are not regressed and that the lookup modules, which are not vulnerable to the same issue, remain so.

The lookup modules are not vulnerable due to this built-in protection, as per docs at https://docs.ansible.com/ansible/latest/plugins/lookup.html -

"By default, lookup return values are marked as unsafe for security reasons. If you trust the outside source for your lookup accesses, pass allow_unsafe=True to allow Jinja2 templates to evaluate lookup values."

Before with `{{}}` in vars (`inventory.py`)

ansible-playbook -i test_inventory.yml test_playbook.yml

PLAY [all] ********************************************************************************************************************************************************************************************************************************************************************************************************
[WARNING]: Found variable using reserved name: tags

TASK [TEST0] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "hostvars[inventory_hostname]": {
        "ansible_check_mode": false,
        "ansible_config_file": "/path/repos/nautobot-ansible_cdtomkins_fork/ansible.cfg",
        "ansible_diff_mode": false,
        "ansible_facts": {},
        "ansible_forks": 5,
        "ansible_inventory_sources": [
            "/path/repos/nautobot-ansible_cdtomkins_fork/test_inventory.yml"
        ],
        "ansible_playbook_python": "/path/repos/nautobot-ansible_cdtomkins_fork/.venv/bin/python",
        "ansible_run_tags": [
            "all"
        ],
        "ansible_skip_tags": [],
        "ansible_verbosity": 0,
        "ansible_version": {
            "full": "2.18.4",
            "major": 2,
            "minor": 18,
            "revision": 4,
            "string": "2.18.4"
        },
        "config_context": [
            {
                "normal_string": "just_a_normal_string",
                "string_containing_jinja2tags": "{{ a_string_containing_jinja2tags }}"
            }
        ],
        "custom_fields": {},
        "device_roles": [
            "ChrisTestRole"
        ],
        "device_types": [
            "ChrisTestModel"
        ],
        "group_names": [
            "ungrouped"
        ],
        "groups": {
            "all": [
                "ChrisTestDevice"
            ],
            "ungrouped": [
                "ChrisTestDevice"
            ]
        },
        "inventory_dir": "/path/repos/nautobot-ansible_cdtomkins_fork",
        "inventory_file": "/path/repos/nautobot-ansible_cdtomkins_fork/test_inventory.yml",
        "inventory_hostname": "ChrisTestDevice",
        "inventory_hostname_short": "ChrisTestDevice",
        "is_virtual": false,
        "local_context_data": [
            {
                "normal_string": "just_a_normal_string",
                "string_containing_jinja2tags": "{{ a_string_containing_jinja2tags }}"
            }
        ],
        "location": "ChrisTestLocation",
        "locations": [
            "ChrisTestLocation"
        ],
        "manufacturers": [
            "ChrisTestManufacturer"
        ],
        "omit": "__omit_place_holder__f101072677a45faeb05e85d0405195027318f9ac",
        "playbook_dir": "/path/repos/nautobot-ansible_cdtomkins_fork",
        "services": [],
        "status": {
            "color": "4caf50",
            "created": "2025-04-11T00:00:00Z",
            "custom_fields": {},
            "description": "Unit is active",
            "display": "Active",
            "id": "c390958e-4883-47e5-b1a8-4aa0d12a9539",
            "last_updated": "2025-04-11T14:04:55.025087Z",
            "name": "Active",
            "natural_slug": "active_c390",
            "notes_url": "http://localhost:8000/api/extras/statuses/c390958e-4883-47e5-b1a8-4aa0d12a9539/notes/",
            "object_type": "extras.status",
            "url": "http://localhost:8000/api/extras/statuses/c390958e-4883-47e5-b1a8-4aa0d12a9539/"
        },
        "tags": []
    }
}

TASK [TEST1] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "msg": "just_a_normal_string"
}

TASK [TEST2] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "msg": "{{ a_string_containing_jinja2tags }}"
}

TASK [TEST3] ******************************************************************************************************************************************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. [{'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}]: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_playbook.yml': line 14, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ hostvars[inventory_hostname].config_context[0].string_containing_jinja2tags }}\"\n    - name: TEST3\n      ^ here\n"}
...ignoring

TASK [TEST4] ******************************************************************************************************************************************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. [{'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}]: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_playbook.yml': line 17, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ config_context[0].normal_string }}\"\n    - name: TEST4\n      ^ here\n"}
...ignoring

TASK [TEST5] ******************************************************************************************************************************************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. [{'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}]: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_playbook.yml': line 20, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ config_context[0].string_containing_jinja2tags }}\"\n    - name: TEST5\n      ^ here\n"}
...ignoring

TASK [TEST6] ******************************************************************************************************************************************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. [{'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}]: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_playbook.yml': line 23, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ lookup('list', [config_context[0].normal_string]) }}\"\n    - name: TEST6\n      ^ here\n"}
...ignoring

TASK [TEST7] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "msg": [
        "just_a_normal_string"
    ]
}

TASK [TEST8] ******************************************************************************************************************************************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_playbook.yml': line 29, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ lookup('list', [hostvars[inventory_hostname].config_context[0].normal_string]) }}\"\n    - name: TEST8\n      ^ here\n"}
...ignoring

PLAY RECAP ********************************************************************************************************************************************************************************************************************************************************************************************************
ChrisTestDevice            : ok=9    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=5

After with hostname changed to include `{{}}` as well (`inventory.py`)

ansible-playbook -i test_inventory.yml test_playbook.yml

PLAY [all] ********************************************************************************************************************************************************************************************************************************************************************************************************
[WARNING]: Found variable using reserved name: tags

TASK [TEST0] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "hostvars[inventory_hostname]": {
        "ansible_check_mode": false,
        "ansible_config_file": "/path/repos/nautobot-ansible_cdtomkins_fork/ansible.cfg",
        "ansible_diff_mode": false,
        "ansible_facts": {},
        "ansible_forks": 5,
        "ansible_inventory_sources": [
            "/path/repos/nautobot-ansible_cdtomkins_fork/test_inventory.yml"
        ],
        "ansible_playbook_python": "/path/repos/nautobot-ansible_cdtomkins_fork/.venv/bin/python",
        "ansible_run_tags": [
            "all"
        ],
        "ansible_skip_tags": [],
        "ansible_verbosity": 0,
        "ansible_version": {
            "full": "2.18.4",
            "major": 2,
            "minor": 18,
            "revision": 4,
            "string": "2.18.4"
        },
        "config_context": [
            {
                "normal_string": "just_a_normal_string",
                "string_containing_jinja2tags": "{{ a_string_containing_jinja2tags }}"
            }
        ],
        "custom_fields": {},
        "device_roles": [
            "ChrisTestRole"
        ],
        "device_types": [
            "ChrisTestModel"
        ],
        "group_names": [
            "ungrouped"
        ],
        "groups": {
            "all": [
                "ChrisTest{{Device}}"
            ],
            "ungrouped": [
                "ChrisTest{{Device}}"
            ]
        },
        "inventory_dir": "/path/repos/nautobot-ansible_cdtomkins_fork",
        "inventory_file": "/path/repos/nautobot-ansible_cdtomkins_fork/test_inventory.yml",
        "inventory_hostname": "ChrisTest{{Device}}",
        "inventory_hostname_short": "ChrisTest{{Device}}",
        "is_virtual": false,
        "local_context_data": [
            {
                "normal_string": "just_a_normal_string",
                "string_containing_jinja2tags": "{{ a_string_containing_jinja2tags }}"
            }
        ],
        "location": "{{Chris}}TestLocation",
        "locations": [
            "{{Chris}}TestLocation"
        ],
        "manufacturers": [
            "ChrisTestManufacturer"
        ],
        "omit": "__omit_place_holder__1950c8477708c8aaf59f5684a99ce1573b712130",
        "playbook_dir": "/path/repos/nautobot-ansible_cdtomkins_fork",
        "services": [],
        "status": {
            "color": "4caf50",
            "created": "2025-04-11T00:00:00Z",
            "custom_fields": {},
            "description": "Unit is active",
            "display": "Active",
            "id": "c390958e-4883-47e5-b1a8-4aa0d12a9539",
            "last_updated": "2025-04-11T14:04:55.025087Z",
            "name": "Active",
            "natural_slug": "active_c390",
            "notes_url": "http://localhost:8000/api/extras/statuses/c390958e-4883-47e5-b1a8-4aa0d12a9539/notes/",
            "object_type": "extras.status",
            "url": "http://localhost:8000/api/extras/statuses/c390958e-4883-47e5-b1a8-4aa0d12a9539/"
        },
        "tags": []
    }
}

TASK [TEST1] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "just_a_normal_string"
}

TASK [TEST2] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "{{ a_string_containing_jinja2tags }}"
}

TASK [TEST3] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "just_a_normal_string"
}

TASK [TEST4] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "{{ a_string_containing_jinja2tags }}"
}

TASK [TEST5] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "just_a_normal_string"
    ]
}

TASK [TEST6] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "{{ a_string_containing_jinja2tags }}"
    ]
}

TASK [TEST7] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "just_a_normal_string"
    ]
}

TASK [TEST8] ******************************************************************************************************************************************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "{{ a_string_containing_jinja2tags }}"
    ]
}

PLAY RECAP ********************************************************************************************************************************************************************************************************************************************************************************************************
ChrisTest{{Device}}        : ok=9    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Before with `{{}}` in vars (`gql_inventory.py`)

ansible-playbook -i test_gql_inventory.yml test_gql_playbook.yml 
 [ERROR]: Mapping ansible_host requires primary_ip6.host or primary_ip4.host as part of the query.

PLAY [all] *******************************************************************************************************************************************************************************
[WARNING]: Found variable using reserved name: name

TASK [TEST0] *****************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "hostvars[inventory_hostname]": {
        "ansible_check_mode": false,
        "ansible_config_file": "/path/repos/nautobot-ansible_cdtomkins_fork/ansible.cfg",
        "ansible_diff_mode": false,
        "ansible_facts": {},
        "ansible_forks": 5,
        "ansible_host": "ChrisTestDevice",
        "ansible_inventory_sources": [
            "/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_inventory.yml"
        ],
        "ansible_playbook_python": "/path/repos/nautobot-ansible_cdtomkins_fork/.venv/bin/python",
        "ansible_run_tags": [
            "all"
        ],
        "ansible_skip_tags": [],
        "ansible_verbosity": 0,
        "ansible_version": {
            "full": "2.18.4",
            "major": 2,
            "minor": 18,
            "revision": 4,
            "string": "2.18.4"
        },
        "config_context": {
            "normal_string": "just_a_normal_string",
            "string_containing_jinja2tags": "{{ a_string_containing_jinja2tags }}"
        },
        "group_names": [
            "ungrouped"
        ],
        "groups": {
            "all": [
                "ChrisTestDevice"
            ],
            "ungrouped": [
                "ChrisTestDevice"
            ]
        },
        "inventory_dir": "/path/repos/nautobot-ansible_cdtomkins_fork",
        "inventory_file": "/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_inventory.yml",
        "inventory_hostname": "ChrisTestDevice",
        "inventory_hostname_short": "ChrisTestDevice",
        "location": {
            "contact_name": "",
            "description": "",
            "name": "{{Chris}}TestLocation",
            "parent": null
        },
        "name": "ChrisTestDevice",
        "omit": "__omit_place_holder__ee007a6e7516527e8806e3c822fe8c565de57858",
        "playbook_dir": "/path/repos/nautobot-ansible_cdtomkins_fork"
    }
}

TASK [TEST1] *****************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "msg": "just_a_normal_string"
}

TASK [TEST2] *****************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "msg": "{{ a_string_containing_jinja2tags }}"
}

TASK [TEST3] *****************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. {'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_playbook.yml': line 14, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ hostvars[inventory_hostname].config_context.string_containing_jinja2tags }}\"\n    - name: TEST3\n      ^ here\n"}
...ignoring

TASK [TEST4] *****************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. {'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_playbook.yml': line 17, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ config_context.normal_string }}\"\n    - name: TEST4\n      ^ here\n"}
...ignoring

TASK [TEST5] *****************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. {'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_playbook.yml': line 20, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ config_context.string_containing_jinja2tags }}\"\n    - name: TEST5\n      ^ here\n"}
...ignoring

TASK [TEST6] *****************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. {'normal_string': 'just_a_normal_string', 'string_containing_jinja2tags': '{{ a_string_containing_jinja2tags }}'}: 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_playbook.yml': line 23, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ lookup('list', [config_context.normal_string]) }}\"\n    - name: TEST6\n      ^ here\n"}
...ignoring

TASK [TEST7] *****************************************************************************************************************************************************************************
ok: [ChrisTestDevice] => {
    "msg": [
        "just_a_normal_string"
    ]
}

TASK [TEST8] *****************************************************************************************************************************************************************************
fatal: [ChrisTestDevice]: FAILED! => {"msg": "The task includes an option with an undefined variable.. 'a_string_containing_jinja2tags' is undefined\n\nThe error appears to be in '/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_playbook.yml': line 29, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        msg: \"{{ lookup('list', [hostvars[inventory_hostname].config_context.normal_string]) }}\"\n    - name: TEST8\n      ^ here\n"}
...ignoring

PLAY RECAP *******************************************************************************************************************************************************************************
ChrisTestDevice            : ok=9    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=5

After with hostname changed to include `{{}}` as well (`gql_inventory.py`)

ansible-playbook -i test_gql_inventory.yml test_gql_playbook.yml 
 [ERROR]: Mapping ansible_host requires primary_ip6.host or primary_ip4.host as part of the query.

PLAY [all] *******************************************************************************************************************************************************************************
[WARNING]: Found variable using reserved name: name

TASK [TEST0] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "hostvars[inventory_hostname]": {
        "ansible_check_mode": false,
        "ansible_config_file": "/path/repos/nautobot-ansible_cdtomkins_fork/ansible.cfg",
        "ansible_diff_mode": false,
        "ansible_facts": {},
        "ansible_forks": 5,
        "ansible_host": "ChrisTest{{Device}}",
        "ansible_inventory_sources": [
            "/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_inventory.yml"
        ],
        "ansible_playbook_python": "/path/repos/nautobot-ansible_cdtomkins_fork/.venv/bin/python",
        "ansible_run_tags": [
            "all"
        ],
        "ansible_skip_tags": [],
        "ansible_verbosity": 0,
        "ansible_version": {
            "full": "2.18.4",
            "major": 2,
            "minor": 18,
            "revision": 4,
            "string": "2.18.4"
        },
        "config_context": {
            "normal_string": "just_a_normal_string",
            "string_containing_jinja2tags": "{{ a_string_containing_jinja2tags }}"
        },
        "group_names": [
            "ungrouped"
        ],
        "groups": {
            "all": [
                "ChrisTest{{Device}}"
            ],
            "ungrouped": [
                "ChrisTest{{Device}}"
            ]
        },
        "inventory_dir": "/path/repos/nautobot-ansible_cdtomkins_fork",
        "inventory_file": "/path/repos/nautobot-ansible_cdtomkins_fork/test_gql_inventory.yml",
        "inventory_hostname": "ChrisTest{{Device}}",
        "inventory_hostname_short": "ChrisTest{{Device}}",
        "location": {
            "contact_name": "",
            "description": "",
            "name": "{{Chris}}TestLocation",
            "parent": null
        },
        "name": "ChrisTest{{Device}}",
        "omit": "__omit_place_holder__0063501d2a5dd8398413f93522b68ee8f8e03513",
        "playbook_dir": "/path/repos/nautobot-ansible_cdtomkins_fork"
    }
}

TASK [TEST1] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "just_a_normal_string"
}

TASK [TEST2] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "{{ a_string_containing_jinja2tags }}"
}

TASK [TEST3] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "just_a_normal_string"
}

TASK [TEST4] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": "{{ a_string_containing_jinja2tags }}"
}

TASK [TEST5] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "just_a_normal_string"
    ]
}

TASK [TEST6] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "{{ a_string_containing_jinja2tags }}"
    ]
}

TASK [TEST7] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "just_a_normal_string"
    ]
}

TASK [TEST8] *****************************************************************************************************************************************************************************
ok: [ChrisTest{{Device}}] => {
    "msg": [
        "{{ a_string_containing_jinja2tags }}"
    ]
}

PLAY RECAP *******************************************************************************************************************************************************************************
ChrisTest{{Device}}        : ok=9    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Explanation of Change(s)
Added change log fragment(s) (for more information see the documentation)
Unit, Integration Tests

plugins/inventory/inventory.py

cdtomkins · 2025-04-14T14:56:42Z

@joewesch Could you please review this and see what you think:

The main functionality passes my test case fine, can you think of anything I might be missing?
check_needs_wrapping() is duplicated - would you rather I moved it out to a shared library? If so, where?
I still need to do unit tests - any special requests?

joewesch

I agree that we should probably move check_needs_wrapping to a central utils folder since it doesn't seem to be required to be a method. It will make it much easier to unit test that way as well.

edit to add: module_utils.utils is a common location for all plugin types - not just modules - so that would probably be my vote on where to put it.

changes/114.fixed

plugins/module_utils/utils.py

tests/integration/entrypoint.sh

Co-authored-by: Josh VanDeraa <josh@josh-v.com>

cdtomkins · 2025-05-13T14:35:00Z

Just need to figure out where to put the tests for inventory and gql_inventory, and this should be good to review/merge.

cdtomkins · 2025-07-03T18:03:17Z

Just need to figure out where to put the tests for inventory and gql_inventory, and this should be good to review/merge.

These are done now. Due to the fact that ansible-test does not support dynamic inventory, they are alongside the other integration tests, but use ansible-playbook. This is noted in entrypoint.sh.

cdtomkins · 2025-07-04T09:58:13Z

@joewesch I believe this is ready to go! 🚀

joewesch

Everything LGTM so far. I'm going to pull down your branch and run some more tests locally before approving.

joewesch · 2025-07-07T15:29:44Z

tests/integration/inventory-regression/gql_inventory_plugin_playbook.yml

@@ -0,0 +1,56 @@
+- hosts: all


Seeing debug instead of assert might be confusing for anyone in the future reviewing this code. Let's add some comments at the top of this file like you did for the unsafe_vars.yml file that explain what these tests are doing would help with that.

Good call - done; see what you think.

joewesch · 2025-07-08T12:08:30Z

The tests were passing yesterday, and the only addition has been some comments. I believe the tests are now failing due to a new module field in v2.4.11 called module_family which is causing the inventory test to fail. I'm going to merge this as-is and fix the tests in another PR as it does not pertain to this.

cdtomkins self-assigned this Apr 11, 2025

cdtomkins changed the title ~~Handled config context case~~ Ensure that some potentially unsafe variables are wrapped correctly. Closes #114 Apr 11, 2025

cdtomkins changed the title ~~Ensure that some potentially unsafe variables are wrapped correctly. Closes #114~~ Ensure that some potentially unsafe variables are wrapped correctly #114 Apr 11, 2025

cdtomkins changed the title ~~Ensure that some potentially unsafe variables are wrapped correctly #114~~ Fixes some potentially unsafe variables not being wrapped correctly #114 Apr 14, 2025

jvanderaa reviewed Apr 14, 2025

View reviewed changes

plugins/inventory/inventory.py Outdated Show resolved Hide resolved

joewesch reviewed Apr 15, 2025

View reviewed changes

changes/114.fixed Outdated Show resolved Hide resolved

cdtomkins force-pushed the handle_unsafe_variables_114 branch from 3bd1b97 to 6bd3fd1 Compare April 17, 2025 13:13

cdtomkins force-pushed the handle_unsafe_variables_114 branch 2 times, most recently from ccdb9d8 to d616d66 Compare May 9, 2025 17:26

cdtomkins added 5 commits May 12, 2025 16:23

Handled config context case

c69d13e

All variables are now handled safely including hostname

6cb8aac

GQL

a9fd265

Unit tests for check_needs_wrapping

01528ab

Extend integration timeout waiting for Nautobot

aba5491

cdtomkins force-pushed the handle_unsafe_variables_114 branch from 1ef06e3 to 04312c1 Compare May 12, 2025 15:23

Improved lookup tests by moving inventory

2fcbb64

cdtomkins force-pushed the handle_unsafe_variables_114 branch from 7388efd to 2fcbb64 Compare May 12, 2025 16:37

jvanderaa reviewed May 13, 2025

View reviewed changes

plugins/module_utils/utils.py Outdated Show resolved Hide resolved

jvanderaa reviewed May 13, 2025

View reviewed changes

tests/integration/entrypoint.sh Show resolved Hide resolved

cdtomkins and others added 3 commits May 13, 2025 14:24

Update plugins/module_utils/utils.py

4ba8854

Co-authored-by: Josh VanDeraa <josh@josh-v.com>

Putting this back as the linter insists.

271cb32

Shortening to please pep8

52c3dc4

nautobot deleted a comment from jvanderaa May 13, 2025

cdtomkins added 2 commits June 27, 2025 13:36

Fix count expected

32fa0fa

Lookup and inventory tests

12fc750

cdtomkins and others added 2 commits July 3, 2025 19:08

Merge branch 'develop' into handle_unsafe_variables_114

523e78a

Remove temp entrypoint break for testing and lint

efc05d1

cdtomkins force-pushed the handle_unsafe_variables_114 branch from f55879e to efc05d1 Compare July 4, 2025 09:30

cdtomkins marked this pull request as ready for review July 4, 2025 09:58

cdtomkins requested a review from jmcgill298 as a code owner July 4, 2025 09:58

joewesch reviewed Jul 7, 2025

View reviewed changes

Added descriptive comments to new integration tests

5631e15

joewesch approved these changes Jul 8, 2025

View reviewed changes

joewesch merged commit e5065d1 into nautobot:develop Jul 8, 2025
7 of 9 checks passed

cdtomkins deleted the handle_unsafe_variables_114 branch July 8, 2025 13:01

This was referenced Jul 22, 2025

release 2.5.13 breaks inventory host vars #601

Closed

Adding allow_unsafe option to inventory plugins #605

Merged

cdtomkins mentioned this pull request Aug 22, 2025

Add ansible-core 2.19 to CI tests #606

Open

cdtomkins mentioned this pull request Dec 5, 2025

Update and test matrices (add ansible-core 2.19/2.20, remove 2.17), fix --allow-unsafe handling, and make inventory tests more conditional #667

Draft

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes some potentially unsafe variables not being wrapped correctly #114 #543

Fixes some potentially unsafe variables not being wrapped correctly #114 #543

Uh oh!

cdtomkins commented Apr 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

cdtomkins commented Apr 14, 2025 •

edited

Loading

Uh oh!

joewesch left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdtomkins commented May 13, 2025

Uh oh!

cdtomkins commented Jul 3, 2025

Uh oh!

cdtomkins commented Jul 4, 2025

Uh oh!

joewesch left a comment

Uh oh!

joewesch Jul 7, 2025

Uh oh!

cdtomkins Jul 8, 2025

Uh oh!

joewesch commented Jul 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fixes some potentially unsafe variables not being wrapped correctly #114 #543

Fixes some potentially unsafe variables not being wrapped correctly #114 #543

Uh oh!

Conversation

cdtomkins commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #114

What's Changed

Summary

Before with {{}} in vars (inventory.py)

After with hostname changed to include {{}} as well (inventory.py)

Before with {{}} in vars (gql_inventory.py)

After with hostname changed to include {{}} as well (gql_inventory.py)

Uh oh!

Uh oh!

cdtomkins commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joewesch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdtomkins commented May 13, 2025

Uh oh!

cdtomkins commented Jul 3, 2025

Uh oh!

cdtomkins commented Jul 4, 2025

Uh oh!

joewesch left a comment

Choose a reason for hiding this comment

Uh oh!

joewesch Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

cdtomkins Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

joewesch commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cdtomkins commented Apr 11, 2025 •

edited

Loading

Before with `{{}}` in vars (`inventory.py`)

After with hostname changed to include `{{}}` as well (`inventory.py`)

Before with `{{}}` in vars (`gql_inventory.py`)

After with hostname changed to include `{{}}` as well (`gql_inventory.py`)

cdtomkins commented Apr 14, 2025 •

edited

Loading

joewesch left a comment •

edited

Loading

joewesch commented Jul 8, 2025 •

edited

Loading