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

Pass an iterator instead of a queryset when rendering export templates #7876

New Issue
Closed
opened 2025-12-29 20:29:16 +01:00 by adam · 9 comments
adam commented 2025-12-29 20:29:16 +01:00
Owner
Copy Link

Originally created by @jeremystretch on GitHub (Apr 7, 2023).

NetBox version

v3.4.7

Feature type

Change to existing functionality

Proposed functionality

When an export template is rendered from a list of objects, the view's QuerySet is passed as context data. This FR proposes instead passing an iterator derived from that QuerySet.

Use case

While powerful and flexible, a QuerySet also includes several methods, such as delete(), that could be mistakenly invoked or intentionally abused within a template. (For this reason, we recommend restricting the creation of export template to trusted administrators.) The intent here is to remove as much risk as possible without unduly sacrificing functionality.

Passing an iterator in place of a QuerySet instance enables efficient iteration through a list of objects without exposing any methods that could potentially mutate the data. However, it also restricts the use of many innocuous methods, such as filter() and count(), that may be useful when crafting an export template. I've opened this FR primarily to gauge whether this trade-off is considered reasonable.

An alternative approach would be to explicitly disable the use of specific QuerySet methods within the Jinja2 environment, however this would require a thorough audit of all current methods and may be difficult to maintain over time.

(Thanks to @x64x6a for raising this as a concern!)

Database changes

No response

External dependencies

No response

Originally created by @jeremystretch on GitHub (Apr 7, 2023). ### NetBox version v3.4.7 ### Feature type Change to existing functionality ### Proposed functionality When an export template is rendered from a list of objects, the view's QuerySet is passed as context data. This FR proposes instead passing an [iterator](https://docs.djangoproject.com/en/4.2/ref/models/querysets/#iterator) derived from that QuerySet. ### Use case While powerful and flexible, a QuerySet also includes several methods, such as `delete()`, that could be mistakenly invoked or intentionally abused within a template. (For this reason, we recommend restricting the creation of export template to trusted administrators.) The intent here is to remove as much risk as possible without unduly sacrificing functionality. Passing an iterator in place of a QuerySet instance enables efficient iteration through a list of objects without exposing any methods that could potentially mutate the data. However, it also restricts the use of many innocuous methods, such as `filter()` and `count()`, that may be useful when crafting an export template. I've opened this FR primarily to gauge whether this trade-off is considered reasonable. An alternative approach would be to explicitly disable the use of specific QuerySet methods within the Jinja2 environment, however this would require a thorough audit of _all_ current methods and may be difficult to maintain over time. (Thanks to @x64x6a for raising this as a concern!) ### Database changes _No response_ ### External dependencies _No response_
adam added the type: featurestatus: under review labels 2025-12-29 20:29:16 +01:00
adam closed this issue 2025-12-29 20:29:17 +01:00
adam commented 2025-12-29 20:29:21 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Apr 7, 2023):

Could this perhaps be an option when creating the template? "Pass as iterator or queryset"

@DanSheps commented on GitHub (Apr 7, 2023): Could this perhaps be an option when creating the template? "Pass as iterator or queryset"
adam commented 2025-12-29 20:29:22 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Apr 7, 2023):

No, it'll need to be addressed one way or the other. I'm not interested in maintaining both approaches.

@jeremystretch commented on GitHub (Apr 7, 2023): No, it'll need to be addressed one way or the other. I'm not interested in maintaining both approaches.
adam commented 2025-12-29 20:29:22 +01:00
Author
Owner
Copy Link

@kkthxbye-code commented on GitHub (Apr 17, 2023):

I don't think passing an iterator does enough to prevent the stated goal of preventing:

such as delete(), that could be mistakenly invoked or intentionally abused within a template.

While iterating the passed iterator these dangerous methods can still be called directly on the individual objects. Especially the goal of preventing intentional abuse seems to not be solved by passing an iterator. Calling delete on the entire queryset would indeed be prevented, but I don't think it is worth it as it prevents further filtering if the entire queryset which seems very limiting as all objects must be iterated and the filtering done with template code.

Lastly I'm not sure that solving it only for export templates makes sense, as at least some of the issues mentioned apply to everywhere we allow jinja2 user-supplied code.

I do think that it might be worth considering to have a general "Security Considerations" page in the docs where potential dangerous features could be explained more in depth, with suggestions about how to limit exposure.

@kkthxbye-code commented on GitHub (Apr 17, 2023): I don't think passing an iterator does enough to prevent the stated goal of preventing: > such as delete(), that could be mistakenly invoked or intentionally abused within a template. While iterating the passed iterator these dangerous methods can still be called directly on the individual objects. Especially the goal of preventing intentional abuse seems to not be solved by passing an iterator. Calling delete on the entire queryset would indeed be prevented, but I don't think it is worth it as it prevents further filtering if the entire queryset which seems very limiting as all objects must be iterated and the filtering done with template code. Lastly I'm not sure that solving it only for export templates makes sense, as at least some of the issues mentioned apply to everywhere we allow jinja2 user-supplied code. I do think that it might be worth considering to have a general "Security Considerations" page in the docs where potential dangerous features could be explained more in depth, with suggestions about how to limit exposure.
adam commented 2025-12-29 20:29:22 +01:00
Author
Owner
Copy Link

@tyler-8 commented on GitHub (Apr 17, 2023):

If I'm understanding iterators correctly, using iterator would prevent the usage of prefetch_related, select_related, and filter in the Export Template itself. if that is the case, then this would significantly reduce the usefulness of Export Templates in my opinion.

Many of my templates use filter to cross data relationships with more complexity than the GUI filters allow (filtering a circuit list by attributes of the sites with terminations. or vice versa, for example).

Additionally - select_related and prefetch_related are necessary when, even if filtering the data, the result list to export is many thousands (or tens of thousands) items long and you still want multiple related object fields. The webserver (nginx/apache/etc) when using sane defaults for worker timeout (<=60) will often hit that limit. A simple *_related query significantly reduces the export time (often to <5s).

@tyler-8 commented on GitHub (Apr 17, 2023): If I'm understanding iterators correctly, using `iterator` would prevent the usage of `prefetch_related`, `select_related`, and `filter` in the Export Template itself. if that is the case, then this would significantly reduce the usefulness of Export Templates in my opinion. Many of my templates use `filter` to cross data relationships with more complexity than the GUI filters allow (filtering a circuit list by attributes of the sites with terminations. or vice versa, for example). Additionally - `select_related` and `prefetch_related` are necessary when, even if filtering the data, the result list to export is many thousands (or tens of thousands) items long and you still want multiple related object fields. The webserver (nginx/apache/etc) when using sane defaults for worker timeout (<=60) will often hit that limit. A simple `*_related` query significantly reduces the export time (often to <5s).
adam commented 2025-12-29 20:29:23 +01:00
Author
Owner
Copy Link

@renatoalmeidaoliveira commented on GitHub (Apr 17, 2023):

Its possible to build a template_manager that returns a cuatom manager but returns all objects as only view?

@renatoalmeidaoliveira commented on GitHub (Apr 17, 2023): Its possible to build a `template_manager` that returns a cuatom manager but returns all objects as only view?
adam commented 2025-12-29 20:29:23 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Apr 25, 2023):

I have to agree with @tyler-8; I don't think it's really feasibly to pass the iterator as it precludes too much valuable functionality.

Its possible to build a template_manager that returns a custom manager but returns all objects as only view?

@renatoalmeidaoliveira you may be onto something there. I'm not sure about a separate manager, but I've been poking around Django's base QuerySet class and found that all write operations set its _for_write attribute to true. We could potentially add a custom method to effectively disable setting _for_write to true. Here's what I've come up with so far:

class RestrictedQuerySet(QuerySet):
    read_only = False

    def __setattr__(self, key, value):
        if key == '_for_write' and self.read_only:
            raise Exception('Queryset is read-only!')
        if key == 'read_only' and not value and self.read_only:
            raise Exception('Cannot unmark a read-only queryset')
        return super().__setattr__(key, value)

    def _clone(self):
        c = super()._clone()
        c.read_only = self.read_only
        return c

Setting readonly = True on the queryset triggers an exception whenever a write-enabled method is called:

>>> qs = Site.objects.filter(pk=1)
>>> qs.update(description='foo')
1
>>> qs.read_only = True
>>> qs.update(description='bar')
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/opt/netbox/venv/lib/python3.8/site-packages/django/db/models/query.py", line 1171, in update
    self._for_write = True
  File "/opt/netbox/netbox/utilities/querysets.py", line 38, in __setattr__
    raise Exception('Queryset is read-only!')
Exception: Queryset is read-only!
>>> qs.read_only = False
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/opt/netbox/netbox/utilities/querysets.py", line 40, in __setattr__
    raise Exception('Cannot unmark a read-only queryset')
Exception: Cannot unmark a read-only queryset

Needs some more investigation, but this may prove to be a viable solution.

@jeremystretch commented on GitHub (Apr 25, 2023): I have to agree with @tyler-8; I don't think it's really feasibly to pass the iterator as it precludes too much valuable functionality. > Its possible to build a template_manager that returns a custom manager but returns all objects as only view? @renatoalmeidaoliveira you may be onto something there. I'm not sure about a separate manager, but I've been poking around Django's base QuerySet class and found that all write operations set its `_for_write` attribute to true. We could potentially add a custom method to effectively disable setting `_for_write` to true. Here's what I've come up with so far: ```python class RestrictedQuerySet(QuerySet): read_only = False def __setattr__(self, key, value): if key == '_for_write' and self.read_only: raise Exception('Queryset is read-only!') if key == 'read_only' and not value and self.read_only: raise Exception('Cannot unmark a read-only queryset') return super().__setattr__(key, value) def _clone(self): c = super()._clone() c.read_only = self.read_only return c ``` Setting `readonly = True` on the queryset triggers an exception whenever a write-enabled method is called: ``` >>> qs = Site.objects.filter(pk=1) >>> qs.update(description='foo') 1 >>> qs.read_only = True >>> qs.update(description='bar') Traceback (most recent call last): File "<console>", line 1, in <module> File "/opt/netbox/venv/lib/python3.8/site-packages/django/db/models/query.py", line 1171, in update self._for_write = True File "/opt/netbox/netbox/utilities/querysets.py", line 38, in __setattr__ raise Exception('Queryset is read-only!') Exception: Queryset is read-only! >>> qs.read_only = False Traceback (most recent call last): File "<console>", line 1, in <module> File "/opt/netbox/netbox/utilities/querysets.py", line 40, in __setattr__ raise Exception('Cannot unmark a read-only queryset') Exception: Cannot unmark a read-only queryset ``` Needs some more investigation, but this may prove to be a viable solution.
adam commented 2025-12-29 20:29:23 +01:00
Author
Owner
Copy Link

@renatoalmeidaoliveira commented on GitHub (Apr 26, 2023):

I guess one drawback of using this kind of implementation is that it doesn't solve the object access, the user gonna be unable to run write operations in the queryset but after iterating in the QS it might be able to edit the model.

@renatoalmeidaoliveira commented on GitHub (Apr 26, 2023): I guess one drawback of using this kind of implementation is that it doesn't solve the object access, the user gonna be unable to run write operations in the queryset but after iterating in the QS it might be able to edit the model.
adam commented 2025-12-29 20:29:24 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Apr 26, 2023):

I guess one drawback of using this kind of implementation is that it doesn't solve the object access

I think we have some options there as well, particularly Jinja2's unsafe() decorator. We should be able to wrap model methods like save() and delete() with it to prohibit their execution inside a template. (Thanks to @x64x6a for suggesting that in a separate discussion!)

@jeremystretch commented on GitHub (Apr 26, 2023): > I guess one drawback of using this kind of implementation is that it doesn't solve the object access I think we have some options there as well, particularly Jinja2's [`unsafe()` decorator](https://jinja.palletsprojects.com/en/3.0.x/sandbox/#jinja2.sandbox.unsafe). We should be able to wrap model methods like `save()` and `delete()` with it to prohibit their execution inside a template. (Thanks to @x64x6a for suggesting that in a separate discussion!)
adam commented 2025-12-29 20:29:24 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 14, 2023):

I'm closing this proposal in favor of #12898, which captures the alternative solution proposed above.

@jeremystretch commented on GitHub (Jun 14, 2023): I'm closing this proposal in favor of #12898, which captures the alternative solution proposed above.
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: under review type: feature
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#7876
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?