Re: [Django] #12772: Allow loading template tags by fully qualified python module path
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Changes (by jacob):
* stage: Design decision needed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:19>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
---------------------------------+------------------------------------
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* status: new => assigned
* owner: nobody => patrys
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:20>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by patrys):
Pull request: https://github.com/django/django/pull/1133
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:21>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by vdboor):
I'm a bit worried about this change. What happens when someone manages to
upload a `.py` file?
With this patch, will it be possible to load that code?
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:22>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by patrys):
I assume the media folder is not a proper Python package so no, not unless
you also manage to put an `__init__.py` on all the levels above the media
folder and add it to `sys.path`.
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:23>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by patrys):
Ping :)
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:24>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by timgraham):
I am not entirely convinced either way about the security concerns. For
example, the documentation says we have `settings.ALLOWED_INCLUDE_ROOTS`
because "This is a security measure, so that template authors can’t access
files that they shouldn’t be accessing. It seems this opens up the same
sort of issue where template authors can load arbitrary Python packages
which shouldn't (but may) have side effects. It would be helpful to run
this by the mailing list and see if a consensus emerges.
After that (assuming this isn't rejected), the patch needs to be updated
to apply cleanly to master and then the Trac flags update so the patch
appears in the review queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:25>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by carljm):
There's a significant difference between `settings.ALLOWED_INCLUDE_ROOTS`
and arbitrary imports. If we didn't have the former, a template author
could access literally any file on the filesystem, as text. The latter
only allows accessing files that are on `sys.path` within a valid Python
package, and even then only in a limited way - by interpreting them as
Python code. The former is much worse as a security risk.
That said, it has never been entirely clear (to me, anyway) whether
"completely untrusted template author" is a use-case that Django intends
to support out of the box. I doubt the template language currently meets
that bar very well, so I think we'd have some work to do if the answer is
"yes". But I do agree that if the answer to that is "yes", this patch
should probably not go in.
Apart from the security concerns: while I fully agree that the flattened
app-label namespace is silly and problematic, it feels strange to me to
have Python import semantics reflected even more directly into the
template engine. I prefer the Jinja2 approach, where the things that you
can import from templates are other templates containing macros; if you
want Python functions directly in the context, you have to put them there
explicitly in Python code, you don't "import" Python modules from
templates. The current DTL approach is less explicit than the Jinja2
approach, but at least with the current DTL approach the Python developer
still has to explicitly nominate which modules should be available to the
templates.
So I think I hover somewhere around +/-0 on this patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:26>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by timgraham):
Created a thread to try to get some consensus on this:
https://groups.google.com/d/topic/django-developers/4KEr_86Q3nA/discussion
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:27>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* cc: marc.tamlyn@… (added)
Comment:
While I approve of magic removal, I'm not convinced I like the idea of
importing by python path.
A possible solution to both any security concerns and namespace collisions
would be to have `TEMPLATE_TAG_LOADERS` - one with the automatic behaviour
which could be removed if you want to, and one which is configured via a
mapping of library names to dotted paths to python files.
So current (default) behaviour:
{{{TEMPLATE_TAG_LOADERS =
('django.template.tag_loaders.AppDirectoriesLoader',)}}}
Magic removal version:
{{{
TEMPLATE_TAG_LOADERS = ('django.template.tag_loaders.ConfiguredLoader',)
TEMPLATE_TAG_LIBRARIES = {
'admin.urls': 'django.contrib.admin.templatetags.admin_urls',
# ... other built in libraries
'my_tags': 'my_project.my_custom_templatetags',
}
}}}
These settings would actually not be yet more top level settings, but more
configuration settings within Aymeric's template engines branch, which
would make them a bit more palatable.
Not 100% sure this is a good idea, but I think it is worth considering.
Most people would continue to use the magical version we have, but it
would now be explicit, overridable magic.
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:28>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by timgraham):
See also #2539 for discussion on the problem of namespace collisions.
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:29>
Django
---------------------------------+------------------------------------
Reporter: patrys | Owner: patrys
Type: New feature | Status: assigned
Component: Template system | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by collinanderson):
Isn't there already a `{% load mylibrary from myapp %}` syntax?
I do feel like there's a ton of boilerplate for creating template tags. It
would be nice to cut down on this:
{{{
mkdir templatetags
touch templatetags/__init__.py
vi templatetags/thing_to_name.py
from django import template
register = template.Library()
@register.simple_tag
}}}
Could we allow any python module to register template tags, and then it's
up to you to be sure that gets imported before you use your template? So,
any python module could do something like this?
register = template.Library('name_of_library')
Though, it still doesn't solve name collisions.
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:30>
Django
---------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* owner: Patryk Zawadzki => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/12772#comment:31>