starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-12 05:20:31 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

IP address model does not check if assigned_object exists #6914

New Issue
Closed
opened 2025-12-29 19:46:37 +01:00 by adam · 7 comments
adam commented 2025-12-29 19:46:37 +01:00
Owner
Copy Link

Originally created by @amhn on GitHub (Aug 30, 2022).

Originally assigned to: @abhi1693 on GitHub.

NetBox version

v3.3.1

Python version

3.10

Steps to Reproduce

  1. Create IP address "127.0.0.1/8"
  2. Create a PATCH request to /api/ipam/ip-addresses/:
curl -X PATCH \
       -H  "accept: application/json" -H  "Authorization: Token $NETBOX_TOKEN" \
      -H "Content-Type: application/json" \
      --data '{"assigned_object_type": "virtualization.vminterface", "assigned_object_id":4711}' \
     "http://localhost:8000/api/ipam/ip-addresses/1/"

Expected Behavior

An error is returned that a vminterface with ID 4711 does not exist.

Observed Behavior

assigned_object_id and assigned_object_type are saved in the model and returned on subsequent GET requests.

{
  "id": 1,
  "url": "http://localhost:8000/api/ipam/ip-addresses/1/",
[...]
  "address": "127.0.0.1/8",
[...]
  "assigned_object_type": "virtualization.vminterface",
  "assigned_object_id": 4711,
  "assigned_object": null,
[...]
}
Originally created by @amhn on GitHub (Aug 30, 2022). Originally assigned to: @abhi1693 on GitHub. ### NetBox version v3.3.1 ### Python version 3.10 ### Steps to Reproduce 1. Create IP address "127.0.0.1/8" 2. Create a PATCH request to /api/ipam/ip-addresses/: ``` curl -X PATCH \ -H "accept: application/json" -H "Authorization: Token $NETBOX_TOKEN" \ -H "Content-Type: application/json" \ --data '{"assigned_object_type": "virtualization.vminterface", "assigned_object_id":4711}' \ "http://localhost:8000/api/ipam/ip-addresses/1/" ``` ### Expected Behavior An error is returned that a vminterface with ID 4711 does not exist. ### Observed Behavior assigned_object_id and assigned_object_type are saved in the model and returned on subsequent GET requests. ``` { "id": 1, "url": "http://localhost:8000/api/ipam/ip-addresses/1/", [...] "address": "127.0.0.1/8", [...] "assigned_object_type": "virtualization.vminterface", "assigned_object_id": 4711, "assigned_object": null, [...] } ```
adam added the type: bugstatus: accepted labels 2025-12-29 19:46:37 +01:00
adam closed this issue 2025-12-29 19:46:37 +01:00
adam commented 2025-12-29 19:46:38 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Aug 31, 2022):

I'm not sure we want to classify this as a bug, given that Django doesn't validate GenericForeignKey assignment out of the box. Consider what happens when assigning an invalid object ID directly on the instance:

>>> from netaddr import IPNetwork
>>> ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999)
>>> ip.full_clean()
>>> ip.save()
>>> ip = IPAddress.objects.get(pk=ip.pk)
>>> repr(ip.assigned_object)
'None'

Validation passes, and the assigned_object attribute returns None as expected. This would seem to be the case with all generic foreign keys and not just the assigned_object field on IPAddress.

This probably needs some deeper discussion to figure out how (and whether) to handle this.

@jeremystretch commented on GitHub (Aug 31, 2022): I'm not sure we want to classify this as a bug, given that Django doesn't validate GenericForeignKey assignment out of the box. Consider what happens when assigning an invalid object ID directly on the instance: ``` >>> from netaddr import IPNetwork >>> ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999) >>> ip.full_clean() >>> ip.save() >>> ip = IPAddress.objects.get(pk=ip.pk) >>> repr(ip.assigned_object) 'None' ``` Validation passes, and the `assigned_object` attribute returns None as expected. This would seem to be the case with _all_ generic foreign keys and not just the `assigned_object` field on IPAddress. This probably needs some deeper discussion to figure out how (and whether) to handle this.
adam commented 2025-12-29 19:46:38 +01:00
Author
Owner
Copy Link

@amhn commented on GitHub (Aug 31, 2022):

With the following in IPAddress.clean() in file netbox/netbox/models/ip.py

        # Check if assigned object exists
        if (self.assigned_object_id != None or self.assigned_object_type != None):
            print("Validation")
            for cls, objtype in ((Interface, 'dcim | interface'), (VMInterface, 'virtualization | interface'),):
                print(objtype, str(self.assigned_object_type))
                print(type(objtype), type(self.assigned_object_type))
                if objtype == str(self.assigned_object_type):
                    print(objtype)
                    try:
                        assigned_object = cls.objects.get(
                            pk=self.assigned_object_id)
                    except VMInterface.DoesNotExist:
                        raise ValidationError({
                            'assigned_object': f"Assigned Object does not exist."
                        })

This becomes:

>>> ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999)
>>> ip.full_clean()
Validation
dcim | interface dcim | interface
<class 'str'> <class 'django.contrib.contenttypes.models.ContentType'>
dcim | interface
Traceback (most recent call last):
  File "/usr/lib/python3.10/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
  File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/base.py", line 1390, in full_clean
    self.clean()
  File "/home/andy/python/netbox/netbox/ipam/models/ip.py", line 954, in clean
    assigned_object = cls.objects.get(
  File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/query.py", line 496, in get
    raise self.model.DoesNotExist(
dcim.models.device_components.Interface.DoesNotExist: Interface matching query does not exist.
>>>

Not sure if that is the way you want to go. But it is a possibilty.

If you want to go forward with this, I can clean it up, add FHRPGroup and create a PR

@amhn commented on GitHub (Aug 31, 2022): With the following in IPAddress.clean() in file netbox/netbox/models/ip.py ``` # Check if assigned object exists if (self.assigned_object_id != None or self.assigned_object_type != None): print("Validation") for cls, objtype in ((Interface, 'dcim | interface'), (VMInterface, 'virtualization | interface'),): print(objtype, str(self.assigned_object_type)) print(type(objtype), type(self.assigned_object_type)) if objtype == str(self.assigned_object_type): print(objtype) try: assigned_object = cls.objects.get( pk=self.assigned_object_id) except VMInterface.DoesNotExist: raise ValidationError({ 'assigned_object': f"Assigned Object does not exist." }) ``` This becomes: ``` >>> ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999) >>> ip.full_clean() Validation dcim | interface dcim | interface <class 'str'> <class 'django.contrib.contenttypes.models.ContentType'> dcim | interface Traceback (most recent call last): File "/usr/lib/python3.10/code.py", line 90, in runcode exec(code, self.locals) File "<console>", line 1, in <module> File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/base.py", line 1390, in full_clean self.clean() File "/home/andy/python/netbox/netbox/ipam/models/ip.py", line 954, in clean assigned_object = cls.objects.get( File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/manager.py", line 85, in manager_method return getattr(self.get_queryset(), name)(*args, **kwargs) File "/home/andy/.local/lib/python3.10/site-packages/django/db/models/query.py", line 496, in get raise self.model.DoesNotExist( dcim.models.device_components.Interface.DoesNotExist: Interface matching query does not exist. >>> ``` Not sure if that is the way you want to go. But it is a possibilty. If you want to go forward with this, I can clean it up, add FHRPGroup and create a PR
adam commented 2025-12-29 19:46:38 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Sep 1, 2022):

My two primary concerns with heading down this path are:

  1. Avoiding introducing significant overhead in the form of additional database queries that check for an object's existence.
  2. Implementing the validation in such a way that it is naturally and consistently replicated for all GFK fields.
@jeremystretch commented on GitHub (Sep 1, 2022): My two primary concerns with heading down this path are: 1. Avoiding introducing significant overhead in the form of additional database queries that check for an object's existence. 2. Implementing the validation in such a way that it is naturally and consistently replicated for all GFK fields.
adam commented 2025-12-29 19:46:38 +01:00
Author
Owner
Copy Link

@amhn commented on GitHub (Sep 3, 2022):

These are valid concerns.

Regarding 1.: I looked further into it. And it seems on the Django shell the following is enough, because the assigned_object is pre_populated:

        # Check if assigned object exists
        if (self.assigned_object_id is not None or self.assigned_object_type is not None) and self.assigned_object is None:
            raise ValidationError({
                'assigned_object': f"Assigned Object does not exist."
            })

However this does not work with the Rest-API. For this I had to extend the validate method in ValidatedModelSerializer to populate the assigned_object field. Changes are in 1c74f3373f

diff --git a/netbox/netbox/api/serializers/base.py b/netbox/netbox/api/serializers/base.py
index f1aea0e2b..d0aeaace9 100644
--- a/netbox/netbox/api/serializers/base.py
+++ b/netbox/netbox/api/serializers/base.py
@@ -1,3 +1,6 @@
+from contextlib import suppress
+from django.contrib.contenttypes.fields import GenericForeignKey
+from django.core.exceptions import ObjectDoesNotExist
 from django.db.models import ManyToManyField
 from rest_framework import serializers
 
@@ -38,6 +41,18 @@ class ValidatedModelSerializer(BaseModelSerializer):
             instance = self.instance
             for k, v in attrs.items():
                 setattr(instance, k, v)
+
+        # Update GenericForeignKey fields if either foreign_key or content_type has changed
+        for field in self.Meta.model._meta.get_fields():
+            if isinstance(field, GenericForeignKey) and getattr(instance, field.name, None) is None:
+                if field.ct_field in attrs.keys() or field.fk_field in attrs.keys():
+                    ct = attrs.get(field.ct_field, getattr(instance, field.ct_field))
+                    fk = attrs.get(field.fk_field, getattr(instance, field.fk_field))
+                    if ct is not None and fk is not None:
+                        with suppress(ObjectDoesNotExist):
+                            new_field = ct.model_class().objects.get(pk=fk)
+                            setattr(instance, field.name, new_field)
+
         instance.full_clean()
 
         return data

An additional advantage of this change: In the answer to a PATCH request, assigned_object is now populated, if an object is assigned to an uanssigned IPAdress. In the develop branch null is returned for assigned_object in this case.

This does not fully address your second concern, but the code that has to be added per field is a lot less than my first idea,

Related but not part of this issue:
Looking at the docs I found the following notice https://docs.djangoproject.com/en/4.0/ref/contrib/contenttypes/#django.contrib.contenttypes.fields.GenericForeignKey:

Unlike for the [ForeignKey], a database index is not automatically created on the [GenericForeignKey],
so it’s recommended that you use [Meta.indexes]  to add your own multiple column index. This behavior [may change]
in the future.

Is it intentional that the mentioned index is not created for (assigned_object_id, assigned_object_type)? Shoud I create an issue to check all GFK fields for indexes?

@amhn commented on GitHub (Sep 3, 2022): These are valid concerns. Regarding 1.: I looked further into it. And it seems on the Django shell the following is enough, because the assigned_object is pre_populated: ```python # Check if assigned object exists if (self.assigned_object_id is not None or self.assigned_object_type is not None) and self.assigned_object is None: raise ValidationError({ 'assigned_object': f"Assigned Object does not exist." }) ``` However this does not work with the Rest-API. For this I had to extend the validate method in ValidatedModelSerializer to populate the assigned_object field. Changes are in https://github.com/amhn/netbox/commit/1c74f3373faf11ae9e5ae32d8a2296b54e42e3fc ```python diff --git a/netbox/netbox/api/serializers/base.py b/netbox/netbox/api/serializers/base.py index f1aea0e2b..d0aeaace9 100644 --- a/netbox/netbox/api/serializers/base.py +++ b/netbox/netbox/api/serializers/base.py @@ -1,3 +1,6 @@ +from contextlib import suppress +from django.contrib.contenttypes.fields import GenericForeignKey +from django.core.exceptions import ObjectDoesNotExist from django.db.models import ManyToManyField from rest_framework import serializers @@ -38,6 +41,18 @@ class ValidatedModelSerializer(BaseModelSerializer): instance = self.instance for k, v in attrs.items(): setattr(instance, k, v) + + # Update GenericForeignKey fields if either foreign_key or content_type has changed + for field in self.Meta.model._meta.get_fields(): + if isinstance(field, GenericForeignKey) and getattr(instance, field.name, None) is None: + if field.ct_field in attrs.keys() or field.fk_field in attrs.keys(): + ct = attrs.get(field.ct_field, getattr(instance, field.ct_field)) + fk = attrs.get(field.fk_field, getattr(instance, field.fk_field)) + if ct is not None and fk is not None: + with suppress(ObjectDoesNotExist): + new_field = ct.model_class().objects.get(pk=fk) + setattr(instance, field.name, new_field) + instance.full_clean() return data ``` An additional advantage of this change: In the answer to a PATCH request, assigned_object is now populated, if an object is assigned to an uanssigned IPAdress. In the develop branch null is returned for assigned_object in this case. This does not fully address your second concern, but the code that has to be added per field is a lot less than my first idea, Related but not part of this issue: Looking at the docs I found the following notice [https://docs.djangoproject.com/en/4.0/ref/contrib/contenttypes/#django.contrib.contenttypes.fields.GenericForeignKey](https://docs.djangoproject.com/en/4.0/ref/contrib/contenttypes/#django.contrib.contenttypes.fields.GenericForeignKey): ``` Unlike for the [ForeignKey], a database index is not automatically created on the [GenericForeignKey], so it’s recommended that you use [Meta.indexes] to add your own multiple column index. This behavior [may change] in the future. ``` Is it intentional that the mentioned index is not created for (assigned_object_id, assigned_object_type)? Shoud I create an issue to check all GFK fields for indexes?
adam commented 2025-12-29 19:46:39 +01:00
Author
Owner
Copy Link

@github-actions[bot] commented on GitHub (Nov 3, 2022):

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Do not attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our contributing guide.

@github-actions[bot] commented on GitHub (Nov 3, 2022): This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. **Do not** attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our [contributing guide](https://github.com/netbox-community/netbox/blob/develop/CONTRIBUTING.md).
adam commented 2025-12-29 19:46:39 +01:00
Author
Owner
Copy Link

@abhi1693 commented on GitHub (Jan 14, 2023):

How about this as a solution? Since, everything inherits from NetBoxModel a simple extension to clean method will solve this issue on the model level. This definitely works for the issue that Jerry stated at https://github.com/netbox-community/netbox/issues/10221#issuecomment-1233285651

diff --git a/netbox/netbox/models/__init__.py b/netbox/netbox/models/__init__.py
index a4c8e0ec2..be21b978a 100644
--- a/netbox/netbox/models/__init__.py
+++ b/netbox/netbox/models/__init__.py
@@ -1,4 +1,6 @@
 from django.conf import settings
+from django.contrib.contenttypes.fields import GenericForeignKey
+from django.contrib.contenttypes.models import ContentType
 from django.core.validators import ValidationError
 from django.db import models
 from mptt.models import MPTTModel, TreeForeignKey
@@ -58,6 +60,23 @@ class NetBoxModel(CloningMixin, NetBoxFeatureSet, models.Model):
     class Meta:
         abstract = True
 
+    def clean(self):
+        """
+        Validate the model for GenericForeignKey fields to ensure that the content type and object ID exist.
+        """
+        super().clean()
+
+        for field in self._meta.get_fields():
+            if isinstance(field, GenericForeignKey):
+                if getattr(self, field.ct_field) and getattr(self, field.fk_field):
+                    klass = getattr(self, field.ct_field).model_class()
+                    try:
+                        klass.objects.get(pk=getattr(self, field.fk_field))
+                    except klass.DoesNotExist:
+                        raise ValidationError({
+                            field.fk_field: f"Invalid {getattr(self, field.fk_field)}: object does not exist on {getattr(self, field.ct_field)}."
+                        })
+
 
 class PrimaryModel(NetBoxModel):
     """

Result

>>> from dcim.models import Interface
... from django.contrib.contenttypes.models import ContentType
... from ipam.models import IPAddress
... from netaddr import IPNetwork
... ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999)
... ip.full_clean()
... 
... 
Traceback (most recent call last):
  File "/snap/pycharm-professional/316/plugins/python/helpers/pydev/pydevconsole.py", line 364, in runcode
    coro = func()
  File "<input>", line 6, in <module>
  File "/home/asaharan/PycharmProjects/netbox/venv/lib/python3.10/site-packages/django/db/models/base.py", line 1477, in full_clean
    raise ValidationError(errors)
django.core.exceptions.ValidationError: {'assigned_object_id': ['Invalid 999999: object does not exist on dcim | interface.']}

Using API, result:

curl -X PATCH \                                             
       -H  "accept: application/json" -H  "Authorization: Token $NETBOX_TOKEN" \ 
      -H "Content-Type: application/json" \
      --data '{"assigned_object_type": "virtualization.vminterface", "assigned_object_id":4711}' \
     "http://localhost:8000/api/ipam/ip-addresses/1/"
{"assigned_object_id":["Invalid 4711: object does not exist on virtualization | interface."]}

@jeremystretch If you think this is doable, I can submit a PR for this

@abhi1693 commented on GitHub (Jan 14, 2023): How about this as a solution? Since, everything inherits from `NetBoxModel` a simple extension to `clean` method will solve this issue on the model level. This definitely works for the issue that Jerry stated at https://github.com/netbox-community/netbox/issues/10221#issuecomment-1233285651 ``` diff --git a/netbox/netbox/models/__init__.py b/netbox/netbox/models/__init__.py index a4c8e0ec2..be21b978a 100644 --- a/netbox/netbox/models/__init__.py +++ b/netbox/netbox/models/__init__.py @@ -1,4 +1,6 @@ from django.conf import settings +from django.contrib.contenttypes.fields import GenericForeignKey +from django.contrib.contenttypes.models import ContentType from django.core.validators import ValidationError from django.db import models from mptt.models import MPTTModel, TreeForeignKey @@ -58,6 +60,23 @@ class NetBoxModel(CloningMixin, NetBoxFeatureSet, models.Model): class Meta: abstract = True + def clean(self): + """ + Validate the model for GenericForeignKey fields to ensure that the content type and object ID exist. + """ + super().clean() + + for field in self._meta.get_fields(): + if isinstance(field, GenericForeignKey): + if getattr(self, field.ct_field) and getattr(self, field.fk_field): + klass = getattr(self, field.ct_field).model_class() + try: + klass.objects.get(pk=getattr(self, field.fk_field)) + except klass.DoesNotExist: + raise ValidationError({ + field.fk_field: f"Invalid {getattr(self, field.fk_field)}: object does not exist on {getattr(self, field.ct_field)}." + }) + class PrimaryModel(NetBoxModel): """ ``` Result ``` >>> from dcim.models import Interface ... from django.contrib.contenttypes.models import ContentType ... from ipam.models import IPAddress ... from netaddr import IPNetwork ... ip = IPAddress(address=IPNetwork('192.0.2.1/24'), assigned_object_type=ContentType.objects.get_for_model(Interface), assigned_object_id=999999) ... ip.full_clean() ... ... Traceback (most recent call last): File "/snap/pycharm-professional/316/plugins/python/helpers/pydev/pydevconsole.py", line 364, in runcode coro = func() File "<input>", line 6, in <module> File "/home/asaharan/PycharmProjects/netbox/venv/lib/python3.10/site-packages/django/db/models/base.py", line 1477, in full_clean raise ValidationError(errors) django.core.exceptions.ValidationError: {'assigned_object_id': ['Invalid 999999: object does not exist on dcim | interface.']} ``` Using API, result: ``` curl -X PATCH \ -H "accept: application/json" -H "Authorization: Token $NETBOX_TOKEN" \ -H "Content-Type: application/json" \ --data '{"assigned_object_type": "virtualization.vminterface", "assigned_object_id":4711}' \ "http://localhost:8000/api/ipam/ip-addresses/1/" {"assigned_object_id":["Invalid 4711: object does not exist on virtualization | interface."]} ``` @jeremystretch If you think this is doable, I can submit a PR for this
adam commented 2025-12-29 19:46:40 +01:00
Author
Owner
Copy Link

@candlerb commented on GitHub (Mar 1, 2023):

            if getattr(self, field.ct_field) and getattr(self, field.fk_field):

That doesn't address the inconsistency which can occur if ct_field is not None but fk_field is None, or vice versa - see #11866.

I think an additional check for that case would be straightforward, but you'd have to beware leaving the user in limbo if the database is in this inconsistent state, because there is no way to fix it via the UI.

Maybe if either ct_field or fk_field is None, the other should be set to None automatically?

@candlerb commented on GitHub (Mar 1, 2023): > if getattr(self, field.ct_field) and getattr(self, field.fk_field): That doesn't address the inconsistency which can occur if ct_field is not None but fk_field is None, or vice versa - see #11866. I think an additional check for that case would be straightforward, but you'd have to beware leaving the user in limbo if the database is in this inconsistent state, because there is no way to fix it via the UI. Maybe if either ct_field or fk_field is None, the other should be set to None automatically?
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label status: accepted type: bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#6914
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?